Recover gracefully from expired CSRF tokens instead of losing form input - #43
Merged
Merged
Conversation
A form left open past the token window became unsubmittable: every CSRF failure returned a bare plain-text 403, so the only way out was to refresh and retype everything. A token whose HMAC verifies against an older time bucket is proof the holder possesses the session's CSRF secret - it is stale, not forged. Since validateOrigin already runs first and hard-rejects anything cross-origin, that case can safely be recovered from: reject the action, stash the submitted values in the existing signed flash cookie, and redirect back so the form re-renders pre-filled with a fresh token. - services/csrf: add inspectCsrfToken returning a CsrfTokenStatus, accepting a bounded range of older buckets as "expired". verifyCsrfToken becomes a shim over it, so its contract is unchanged. Expired hits deliberately skip the failure brake (nothing to guess once the HMAC verifies) and get their own far looser ceiling to bound replay. - middleware/csrf: add checkCsrf returning a discriminated result plus isRecoverableCsrfFailure. csrfProtection is now a wrapper with byte-identical behaviour, so its existing tests pass unmodified. Only expired-token is recoverable - expired-session has no secret to verify against, so it stays a hard 403 alongside origin, missing-token and forged failures. - forms and projects create: preserve submitted values and re-render with a warning. projects destroy bounces back for a deliberate second click rather than replaying a delete. - logout honours a stale token: its nav token is minted on every page render, there is nothing to preserve and no form to return to, and a sign-out button that appears broken is its own problem. - Validation failures now preserve input too, via the same machinery. - utils/state: fitFlashState trims oversized payloads while always keeping the marker, fixing a latent bug where a long message silently dropped the cookie. - utils/form-data: readFormValues, safe to call after the middleware's clone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JLN1yneXAn9mzmirbdTd1o
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.
Summary
A form left open past the token window became unsubmittable.
TIME_WINDOW_MINUTES = 15buckets tokens by time and verification accepted only the current and previous bucket, so a token's real lifetime was 15–30 minutes depending on where in the window the page rendered. Every CSRF failure returned a bare plain-text403 Invalid CSRF token— no layout, no redirect — so the only way out was to refresh and retype everything.The security argument. A token whose HMAC verifies against an older time bucket is proof the holder possesses the session's
csrf_secret. A cross-origin attacker cannot produce one. "Expired" is a weaker signal than "valid" for freshness, not for authenticity. SincevalidateOriginalready runs first and hard-rejects anything not same-origin, the recovery path is reachable only by a genuine user on our own site holding a genuine but stale token.The action is still never performed. The submitted values are stashed in the existing signed flash cookie, the request 303s back, and the form re-renders pre-filled with a fresh token — so resubmitting takes one deliberate click.
What must not move, and didn't: origin failures, missing tokens, missing sessions, forged tokens and rate-limited requests all still return the same hard 403 with the same body strings.
services/csrf: addedinspectCsrfTokenreturning aCsrfTokenStatus, accepting a bounded range of older buckets as"expired".verifyCsrfTokenis now a shim over it, so its contract is unchanged. Expired hits deliberately skip the failure brake — that counter exists to stop guessing, and there's nothing to guess once the HMAC verifies — and get their own far looser ceiling to bound replay of a captured token.middleware/csrf: addedcheckCsrfreturning a discriminated result, plusisRecoverableCsrfFailure.csrfProtectionis now a wrapper with byte-identical behaviour.forms.createandprojects.createpreserve submitted values and re-render with a warning.projects.destroybounces back for a deliberate second click rather than replaying a delete.logouthonours a stale token rather than bouncing (see judgement calls below).forms.createandprojects.creatediscarded everything on a short name/title.utils/state:fitFlashStatetrims oversized payloads while always preserving the marker field. This fixes a latent bug where a long message silently blew the ~4096-byte cookie limit and the success banner never appeared.utils/form-data: newreadFormValues, safe to call after the middleware'sreq.clone().formData().TIME_WINDOW_MINUTESstays at 15 — this fixes recovery, not lifetime. Recovery extends ~2h15m past that viaCSRF_GRACE_WINDOWS; beyond that it's still a hard 403.Judgement calls worth reviewing
Logout honours a stale token rather than bouncing (
logout.tsx:11). Its nav token is minted on every page render, so an old tab hits this constantly; unlike the create/delete flows there's nothing to preserve and no form to return to, and a sign-out button that appears broken is its own security problem. The request still passed the origin check and still proved possession of the session secret, and sign-out is idempotent. Easy to reverse if you'd rather it bounced.expired-sessionis not recoverable, though the original plan had it so. Implementing it, an existing test caught that a forged token against a secretless session was landing on the friendly path — with no secret to verify against there's no proof of authenticity. Only proven-stale tokens recover.projects.destroynow normalisespathtonew URL(req.url).pathname— it was passing the full URL. Harmless before (the middleware ignoresoptions.pathfor verification) but a trap for the next reader.Test plan
bun run check(lint + typecheck) passesmiddleware/csrf.test.tspasses completely unmodified — the primary evidence the security boundary didn't moveInvalid request originmockDeleteProjectis never called on a stale delete token"expired"— a user with several stale tabs can't lock themselves out of recoverysetSystemTimerather than a test-only parameter, keeping the production signature clean/formsin a browser (temporarily setTIME_WINDOW_MINUTES = 1to avoid the wait), plus/projectscreate + delete and the nav sign-out buttonGenerated by Claude Code