fix(auth): make logout revoke, and keep the JWT out of the URL - #72
Merged
Conversation
Logging out only deleted the cookie. A refresh token copied out of the browser stayed valid for its full seven days, so the endpoint did nothing at all against the one person it was meant to stop. A JWT cannot be withdrawn once issued, so each refresh token now carries the `ver` it was minted under and `GitHubUser.token_version` is bumped on logout. Cheaper than a denylist: nothing stored per token, nothing to expire. It invalidates all of the user's sessions rather than just this browser's, which for a tool holding GitHub and Claude credentials is the behaviour someone clicking logout after losing a laptop expects. Logout is authenticated now, because revocation has to know whose tokens to kill. Tokens issued before the column existed carry no `ver` claim and are rejected the same way, logging current users out once. That is the intent, not a side effect. Separately, the OAuth callback put the access token in the redirect URL, where it landed in browser history, in the Referer of whatever the page loaded next, and in every proxy log along the way -- for the credential that authenticates the entire API. The redirect carries no token now: it sets the httpOnly refresh cookie, and the frontend trades that for an access token through the `refreshToken()` helper the API client already had for silent renewal. 426 backend tests, 66 frontend.
|
helPRs session created for this PR. Skill: |
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.
The two identity findings that had been open since the first audit round.
Logout did not log anyone out
POST /auth/logoutdeleted the cookie and returned{"status": "ok"}. A refresh token copied out of the browser — which is the only scenario where logging out matters — stayed valid for its full seven days. The endpoint was a no-op against exactly the person it existed to stop.A JWT cannot be withdrawn once issued, so each refresh token now carries the
verit was minted under, andGitHubUser.token_versionis bumped on logout. Cheaper than a denylist: nothing stored per token, nothing to expire, no cleanup job.Two consequences worth agreeing on before merge:
jtidenylist would be narrower, at the cost of a table and an expiry job.verclaim — current users get logged out once on deploy. That is the intent rather than a side effect: the point is that tokens minted under the old rules stop being honoured.POST /auth/logoutis authenticated now, because revocation has to know whose tokens to invalidate.The access token was in the redirect URL
That URL lands in browser history, in the
Refererof whatever the page loads next, and in every proxy log between the two — carrying the credential that authenticates the whole API.The redirect carries no token now. It sets the httpOnly refresh cookie, and the frontend trades that for an access token by calling
POST /auth/refresh— throughrefreshToken(), which the API client already had for silent renewal and which is now exported rather than re-implemented in the callback.OAuthCallback.tsxno longer readsuseSearchParamsat all, which is the property the new test pins: the token can only come from the cookie exchange.Verification
ruff,ruff format,mypy,tsc --noEmitandeslintall cleanverclaim is rejected, a freshly issued one still works after revocation, logout is 401 without auth, logout increments the versionaccess_token=and does set the refresh cookie5b500a24f018autogenerated against a real Postgres and applied over the full chainNote
docs/self-hosting.mdneeds no change — nothing there documented the callback's query string. CLAUDE.md gained both invariants.