fix(users): harden the session_version cache - #311
Merged
Merged
Conversation
`_TOKEN_LIFETIME_SECONDS` is one number for the whole process — `current_user` resolves its strategy from the shared backend, so it cannot vary per request — and it has to be the widest window any sign-in asks for. Every other credential inherited it. An ordinary sign-in wrote an `sm_auth` cookie whose `Max-Age` said fourteen days while the row behind it was accepted for thirty, and `Max-Age` is browser-enforced only: a cookie lifted off disk is replayed without one. That is exactly the hole `SESSION_EXPIRES_AT_KEY` closed for the session cookie. `/api/users/auth/token` had the same shape in reverse — it returned `expires_in=900` for a row good for a month, so a client that honoured it re-authenticated every fifteen minutes while the token it discarded stayed live. `UserAccessToken` now carries its own `expires_at`, stamped from what that sign-in actually asked for (`cookie_max_age_seconds`, `remember_me_max_age_seconds`, or `bearer_token_lifetime_seconds`), plus the `session_version` it was minted under. `ExpiringDatabaseStrategy` stamps both and enforces them, and `_resolve_bearer` applies the same two clauses in SQL — so the two bearer readers agree rather than drifting. The process-wide constant stays as the read *ceiling*: narrowing it would reject the longer rows a remembered sign-in legitimately wrote. The stamped counter is the revocation `_resolve_bearer` never had. A password change bumped `session_version` and stranded every session while every bearer token minted before it kept working — including any an attacker who knew the old password had already collected. `change_my_password` now also deletes the access tokens and revokes the refresh tokens, matching the bearer half of `revoke-all`: the counter alone strands them, but leaving the rows lets a stolen token resolve until its own deadline and lets refresh tokens mint replacements. `token_refresh` re-reads the account rather than trusting `rt.user_id`, since a refresh token outlives the access tokens it mints. The migration backfills both columns with the values existing rows were actually minted under — thirty days from `created_at`, and the owning account's counter — so upgrading signs nobody out and leaves no NULL for the read path to fail open on. Last, the legacy session fail-open. `session_has_expired` accepted a session with no deadline, with nothing tracking when that could safely be closed. The provider now stamps one on first sight (`ensure_session_expiry`), which can only tighten: the signer independently enforces the signature window from the session's original mint time, so a stamp written now cannot outlast the ceiling the session already had. Nobody is signed out, and with the only caller stamping first, `session_has_expired` fails closed on absence. Closes #292
Three edges around the 30-second revocation cache. The trade-off itself stands;
these are the parts of it that were not deliberate.
`read_session_version` did `if user_id in cache: return cache[user_id]`. That is
a TTLCache, so the entry can expire between the two, and the caller has no
`except KeyError` — landing in the window turned a revocation check into a 500
on an authenticated request. One `.get(sentinel)` cannot be torn that way, and
the sentinel keeps a cached `None` ("no such row") distinguishable from a miss.
`forget_session_version` ran inline in the handler, before the request-scoped
session commits. Moved onto `session.on_commit()`, the hook #268 added. Clearing
before the row is durable is the wrong direction on failure: a rolled-back bump
leaves the cache empty and the counter unchanged, so the next read repopulates
the *old* value and quietly re-admits everything the bump was meant to strand —
with the cache looking freshly invalidated. The callback runs inside the response
cycle, so the browser that pressed the button is still never told it worked while
this worker keeps letting the old sessions in.
The cross-process lag is now an operator choice rather than a constant.
`SM_USERS_SESSION_VERSION_TTL_SECONDS` sets the window, and 0 disables caching —
one indexed read per request, no window at all — for a deployment that will not
accept another worker honouring a revoked session for any length of time.
Not done: the shared invalidation channel the issue suggests. Redis belongs to
the `background_tasks` plugin and the framework `EventBus` is explicitly
in-process, so a cross-worker notification needs a framework-level transport
that `users` cannot reach without either duplicating Redis config it does not own
or breaking the framework/plugin boundary SM009 enforces. That is its own change;
the TTL knob is the honest interim.
Closes #294
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying simple-module-python with
|
| Latest commit: |
8df22a0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e652e9a2.simple-module-python.pages.dev |
| Branch Preview URL: | https://fix-session-version-cache.simple-module-python.pages.dev |
Both conflicts are this branch's version against the squash-merged #292, which this work was stacked on: the RequestSession annotation and the post-commit invalidation supersede the inline forget_session_version calls that landed with the token-expiry change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #294
The problem
The 30-second in-process cache in front of
User.session_versionis a deliberate trade. Three things around it were not.KeyError.read_session_versiondidif user_id in _SESSION_VERSIONS: return _SESSION_VERSIONS[user_id]. That is aTTLCache— the entry can expire between the membership test and the subscript, and the caller has noexcept KeyError. Sub-microsecond window, 500 on an authenticated request when it lands.forget_session_versionran inline in the handler. The issue calls a failed commit "harmless"; it is the wrong direction. A rolled-back bump leaves the cache empty and the counter unchanged, so the next read repopulates the old value and re-admits everything the bump was meant to strand — with the cache looking freshly invalidated.The fix
inthen[].get(_MISS)— a cachedNonestays distinguishable from a missdb.on_commit(...)— the hook #268 added, still inside the response cycle30SM_USERS_SESSION_VERSION_TTL_SECONDS;0disables caching entirelyNot done, and why
The shared invalidation channel the issue suggests (Redis pub/sub so every worker drops its entry at once). Redis belongs to the
background_tasksplugin —usersdoes not own that connection or its config — and the frameworkEventBusis explicitly in-process ("Async in-process event bus backed by pyee"). Wiring one fromusersmeans either duplicating Redis configuration it does not own, or reaching across the framework/plugin boundarySM009enforces as an error. That is a framework-level transport and its own change.The TTL knob is the honest interim: a deployment that will not accept any window in which one worker has not seen another's revocation can set it to
0and pay one indexed read per request — the exact cost the cache was introduced to avoid, now a decision rather than a default. Worth its own issue.Verification
uv run pytest -q(full suite) — 2821 passed, 2 skipped, 60 deselecteduv run pytest modules/users— 462 + 15 new.gettoin/[]fails the race test; revertingon_committo the inline call fails the ordering test. Both restored, all 15 pass.committo raise tripsuser_manager.update's own commit first, before either invalidation runs. It instead records whether the entry is still cached each time a commit begins, and asserts on the last observation. That does discriminate (verified above).ruff format --check/ruff check/ty check modules/users/check_file_size.py— all pass