Skip to content

fix(auth): answer a refresh burst with one successor token - #2007

Closed
henfircreo wants to merge 1 commit into
vstorm-co:mainfrom
Creo-Digital-AS:fix/refresh-burst-reissue
Closed

henfircreo wants to merge 1 commit into
vstorm-co:mainfrom
Creo-Digital-AS:fix/refresh-burst-reissue

Conversation

@henfircreo

Copy link
Copy Markdown
Contributor

Problem

This follows up the refresh-token grace window that shipped in 0.0.515 (REFRESH_REUSE_GRACE_SECONDS, 0102_session_rotated_at). Inside the window, a spent token rotated the session again. In a burst of refreshes on one cookie (every tab's timer firing after a laptop wakes), the first request rotates the row and the second rotates it again inside the grace window. The third then matches no previous hash and gets a 401, and its response clears the cookie the other two had just set. The session stays active and the browser loses it. In production we still saw sign-outs after the grace window landed.

Second, a refused refresh left the console signed in: every request answered 401, and the chat socket reconnected on a dead token until the person did a full reload.

Change

  • One successor per spent token. successor_refresh_token derives the next refresh token from the spent one: jti is an HMAC (keyed with SECRET_KEY) of the spent token's hash, exp is the row's own expiry, and the payload has no iat. The same spent token therefore always produces the same successor, byte for byte. Within the window, SessionService.reissue_within_grace answers with the token the row already holds instead of rotating again. Every request in the burst gets the same token, so the cookie ends up the same whichever response lands last. The row never stores a credential.
  • The grace answer is refused when the row has rotated past that successor (a later refresh on the new token) or the account's credential version has moved. Reuse detection outside the window is unchanged, and so are upstream's guards against a rotation stamped in the future.
  • A 401 from the refresh logs the console out (api-client.ts), so AuthGuard sends the person to sign in. A rate limit, a 5xx or an ended impersonation is still not treated as the end of the session.
  • The chat socket's /auth/me recovery tolerates an empty answer.
  • Docs (configuration, security in en/pl/de/es) and the CHANGELOG describe the reissue.

Verification

  • tests/test_session_refresh_grace.py: the successor is deterministic, differs between spent tokens and carries the given expiry and version. The reissue is refused once the row has moved on or the credential version has changed.
  • tests/integration/test_session_revocation.py, through the real route: a lost response gets back the token it missed; a burst of three gets 200 and the same token on every request; once the successor has rotated, the spent token gets a 401; and a password change closes the window.
  • api-client.test.ts: a 401 refresh logs out, and a 429 or 5xx does not.
  • Locally: these backend tests, tests/api/test_auth.py (51 passed), and api-client/use-chat/auth-lock vitest (185 passed). ruff is clean, and check_docs_i18n.py reports every translation current.

Limitations

The tradeoff is the same as the existing window: a stolen refresh token replayed within REFRESH_REUSE_GRACE_SECONDS of the victim's own refresh is answered rather than detected. The difference is that it now receives the token the victim also holds, rather than a fresh rotation.

The reuse grace window rotated the session again on every grace
refresh, so in a burst of three refreshes on one cookie the third
matched no previous hash, got a 401, and its response cleared the
cookie the other two had just set. The session stayed active and the
browser lost it.

- The successor refresh token is now derived from the spent one (jti is
  an HMAC of its hash, exp is the row's expiry), so a grace refresh is
  answered with the token the row already holds instead of rotating
  again. Every request in the burst gets the same token.
- A 401 from the refresh logs the console out, so AuthGuard sends the
  person to sign in instead of leaving a page where every request fails.
- The chat socket's /auth/me recovery tolerates an empty answer.
@DEENUU1

DEENUU1 commented Oct 6, 2026

Copy link
Copy Markdown
Member

Thanks @henfircreo — merged as #2018. Maintainer edits can't push to a branch on an organization-owned fork, so your commit was carried unchanged on a vstorm-co branch with main merged in; it ships in 0.0.521.

@DEENUU1 DEENUU1 mentioned this pull request Oct 6, 2026
DEENUU1 added a commit that referenced this pull request Oct 6, 2026
### Summary

Release 0.0.521: version, lock and changelog.

### Changes

- `backend/pyproject.toml`, `frontend/package.json` and
`backend/uv.lock` move to 0.0.521.
- `CHANGELOG.md`: the `[Unreleased]` block becomes `[0.0.521] -
2026-10-06`, with a fresh empty `[Unreleased]` above it.
- Ships #2018 (from #2007):
- a burst of refreshes on one cookie is answered with the one successor
token the session holds, so the cookie converges instead of being
cleared;
- a refused refresh logs the console out and sends the person to sign
in.
DEENUU1 added a commit that referenced this pull request Oct 6, 2026
…2017)

### Summary

Replaces #2008 by @henfircreo: the Workspaces page reads hosts
concurrently instead of one after another. This branch holds only that
commit, cherry-picked onto `main` with its authorship kept, plus the
follow-ups the review on #2008 asked for. The fork's other changes
(refresh-token reissue, 1800 s timeout ceiling, Dockerfile, SQL script)
are not here; the auth one is #2007.

### Changes

- `250d254c8` (henfircreo): listings and thumbnails read side by side,
the thumbnail budget shared out in listing order before any fetch, a 10
s browse timeout per host call, connection resolution behind a lock.
- Review follow-ups:
- The fan-outs run in an `asyncio.TaskGroup`, so a call that raises
cancels the rest instead of leaving them running against the request's
session.
- Host `ls` and `read_bytes` calls run on a process-wide eight-thread
`ThreadPoolExecutor` of their own (`_HOST_CALLS`), not the default
executor `to_thread` uses, which `bcrypt` and DNS share. A per-request
semaphore was tried first; Codex pointed out it bounds one page load
only, so a few concurrent loads against a dead host could still take
every default thread. The pool bounds the process, and a dead host slows
only other host calls.
- `_archive` always passes a timeout, defaulting to the library's
`DEFAULT_TIMEOUT_SECONDS`.
- New tests: the connection lock (one resolve per connection, never two
at once) and the process-wide bound, with two page loads at once on
their own threads. Both fail when the fix is reverted. The
budget-ordering test now uses an `Event` so the first-listed host really
answers last, and it is exempt from the security marker (fetch budget,
not spend).
- The CHANGELOG no longer says a host is "reported after ten seconds"
without qualification: the timeout applies per call, so a slow host can
still take longer over a deep walk.

### Verification

- `tests/test_sandbox_workspace.py`,
`tests/api/test_workspace_browser_routes.py`,
`tests/test_security_marker.py`: 239 passed.
`app/services/sandbox_workspace.py` at 100% coverage.
- `ruff format`, `ruff check`, `ty check` on the changed files: clean.
- Not run locally: full `make test` and `make check`; CI covers those.

Closes #2008

---------

Co-authored-by: henfircreo <henning.firman@creodigital.no>
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.

2 participants