Skip to content

fix(users): harden the session_version cache - #311

Merged
antosubash merged 4 commits into
mainfrom
fix/session-version-cache
Sep 5, 2026
Merged

antosubash merged 4 commits into
mainfrom
fix/session-version-cache

Conversation

@antosubash

Copy link
Copy Markdown
Owner

Closes #294

Stacked on #309 — based on fix/bearer-token-expiry, not main, because both touch the session-version path in provider.py and self_account.py. Retarget to main after #309 merges; GitHub will do that automatically.

The problem

The 30-second in-process cache in front of User.session_version is a deliberate trade. Three things around it were not.

  • TOCTOU KeyError. read_session_version did if user_id in _SESSION_VERSIONS: return _SESSION_VERSIONS[user_id]. That is a TTLCache — the entry can expire between the membership test and the subscript, and the caller has no except KeyError. Sub-microsecond window, 500 on an authenticated request when it lands.
  • Invalidation fired before the commit. forget_session_version ran 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.
  • Cross-process revocation lag of up to 30 s, hard-coded.

The fix

before after
cache read in then [] one .get(_MISS) — a cached None stays distinguishable from a miss
invalidation inline, pre-commit db.on_commit(...) — the hook #268 added, still inside the response cycle
staleness window constant 30 SM_USERS_SESSION_VERSION_TTL_SECONDS; 0 disables caching entirely

Not 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_tasks plugin — users does not own that connection or its config — and the framework EventBus is explicitly in-process ("Async in-process event bus backed by pyee"). Wiring one from users means either duplicating Redis configuration it does not own, or reaching across the framework/plugin boundary SM009 enforces 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 0 and 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 deselected
  • uv run pytest modules/users462 + 15 new
  • Fail-before proof, per fix: reverting the .get to in/[] fails the race test; reverting on_commit to the inline call fails the ordering test. Both restored, all 15 pass.
  • The ordering test is worth a look: fault injection cannot show this bug — patching commit to raise trips user_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

`_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
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review Completed 2026-09-04T22:07:52.494550Z f1347da PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 4, 2026

Copy link
Copy Markdown

Deploying simple-module-python with  Cloudflare Pages  Cloudflare Pages

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

View logs

@antosubash
antosubash changed the base branch from fix/bearer-token-expiry to main September 5, 2026 05:24
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.
@antosubash
antosubash merged commit 0cae5d6 into main Sep 5, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden the session_version cache: TOCTOU read, pre-commit invalidation, cross-process lag

1 participant