Skip to content

fix(shared-kernel): cookie revocations were silently dropped on every error response - #380

Open
vianneybacoup wants to merge 2 commits into
fix/sso-cli-flow-coveragefrom
fix/clear-cookies-on-error-responses
Open

fix(shared-kernel): cookie revocations were silently dropped on every error response#380
vianneybacoup wants to merge 2 commits into
fix/sso-cli-flow-coveragefrom
fix/clear-cookies-on-error-responses

Conversation

@vianneybacoup

Copy link
Copy Markdown
Collaborator

Stacked on #379. Base is fix/sso-cli-flow-coveragemerge #379 first, this PR then retargets to main on its own.

The bug

Raised by Copilot on #379, verified and found to be wider than reported.

main.py:280 rebuilds a fresh JSONResponse from exc.headers alone. Anything staged on the injected Response is discarded the moment a route raises. So a route that does response.delete_cookie(...) and then raise HTTPException(...) sends nothing at all:

refresh status: 400 {'detail': 'Invalid or expired refresh token'}
Set-Cookie on failure: []
logged_in STILL in jar: true

Three sites were affected, across two routes:

Site Lost Impact
refresh_access_token_routes.py:99-101 access_token, refresh_token, logged_in on a dead session the real one
sso_callback_route.py state rejection sso_state benign
sso_callback_route.py after the state check sso_state, whenever the code exchange then fails state stays replayable ≤10 min

auth_logout_routes.py is unaffected — it never raises.

Why this is the server's job

access_token and refresh_token are httpOnly. No frontend code can clear them; a Set-Cookie with Max-Age=0 from the server is the only mechanism that exists. The route already knew this and did the right thing — the deletions just never left the process.

Nothing surfaced because the SPA compensates: customClient.ts catches the 401, fails the refresh, tears down its store and redirects to /login, and logged_in self-expires with the access token. That compensation is UX behaviour, not revocation, and a non-browser client has none of it.

The fix

revoke_cookie(request, response, key, ...) in shared_kernel/adapters/primary/ records the deletion on the request as well as staging it on the Response; both handlers in main.py replay what was recorded. One seam, so the next route that clears a cookie before raising is covered by construction rather than by remembering.

Not taken: attaching Set-Cookie to exc.headers, as Copilot suggested. exc.headers is a dict, so it cannot carry the three Set-Cookie values the refresh route needs, and it would leave the trap armed for every future route.

Verification

Both new assertions were watched red first, and killed by mutation — stubbing the replay to a no-op fails them with assert 'access_token=' in '' and assert 'sso_state=' in ''.

uv run pytest tests/ -q
760 passed, 3 skipped

ruff format / ruff check src/ tests/ clean.

Note for whoever owns the CLI

This makes the server send the revocation, but it only cleans a client that keeps a persistent cookie jar — the same fix they are already making for the SSO state. A client that stores tokens itself must purge on a 400 regardless: the server owns the credential's lifecycle, the client owns its own storage.

🤖 Generated with Claude Code

@github-actions github-actions Bot added the bug Something isn't working label Sep 9, 2026
@vianneybacoup
vianneybacoup added this pull request to stack #381 September 9, 2026 09:47
vianneybacoup and others added 2 commits September 9, 2026 12:56
A route that stages `response.delete_cookie(...)` and then raises loses it:
the global handlers in main.py build a fresh JSONResponse from `exc.headers`
alone, so nothing staged on the injected Response ever reaches the client.

`refresh_access_token_routes.py` was doing exactly that — on an invalid or
revoked refresh token it cleared access_token, refresh_token and logged_in,
and none of the three deletions was ever sent. Verified before the fix:
`Set-Cookie: []` on the 400, with `logged_in=true` left in the jar.

That cleanup is not optional politeness. access_token and refresh_token are
httpOnly, so the server is the only actor able to clear them; no frontend
code can. Today the SPA compensates by tearing down its own state and
redirecting, which is why nothing surfaced — but a non-browser client has no
such layer, and the compensation is invisible from the server's side.

`revoke_cookie` records the deletion on the request alongside staging it on
the Response, and both handlers replay what was recorded. One seam, so the
next route that clears a cookie before raising is covered by construction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both `delete_cookie` calls in the SSO callback sat in front of a `raise`, so
neither ever reached the client. The consequence is smaller than the refresh
one but real: once the state check passes, the state is meant to be spent,
and a failing code exchange left it replayable for the rest of its 10-minute
window.

Route them through `revoke_cookie` so the deletion survives the raise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vianneybacoup
vianneybacoup force-pushed the fix/clear-cookies-on-error-responses branch from e4b9cad to c546c11 Compare September 9, 2026 10:58
@vianneybacoup
vianneybacoup deployed to helm-integration September 9, 2026 11:00 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant