fix(web): attach CSRF token to client mutations, and close a lowercase-PATCH CSRF bypass - #167
Conversation
No authenticated write in the dashboard could persist. middleware.ts calls requiresCsrfValidation() on every mutating /api/* request and rejects with 403 unless BOTH the csrf cookie and an X-CSRF-Token header are present. The cookie is set on every response; the header was sent by nothing. csrfHeaders() existed in lib/csrf-client.ts documented for exactly this purpose and was unused across the repo -- 19 client files issue mutating fetches and not one attached it. Affected: settings (profile, org, team, templates), agents, tickets (create, notes, view actions), docs, loom import, broadcasts, accounts, onboarding, qa-chat, sync force-sync, sync mappings. Only setup, login and invite-accept escaped, because their endpoints sit in PUBLIC_PATHS / CSRF_EXEMPT_PREFIXES. This also explains why the mappings hardening in #95 and #160 was validating a write path the UI could never reach. Fixed with a wrapper rather than by spreading csrfHeaders() at each of the 36 call sites. The bug exists precisely because attaching the header was opt-in and every author opted out; a wrapper makes the correct behaviour the default, and puts it somewhere testable. apiFetch is deliberately thin -- it does not parse bodies, throw on non-2xx, or retry -- so each migration is a one-line change and callers keep the Response. Details worth knowing: - GET/HEAD pass straight through untouched; the middleware does not check them, so attaching a token nothing reads would be noise. - A caller-supplied X-CSRF-Token is not clobbered. - Content-Type defaults to JSON only when a body is present AND the caller has not set one, so FormData uploads keep the browser-generated multipart boundary. - No token is sent when the cookie is absent, rather than an empty header. GET-only files (dashboard, docs listing, messaging, faq, my-tasks, account-owner-select) are left on plain fetch -- migrating them would widen the diff without changing behaviour. Tests: 10 added, red-green verified. Neutering the wrapper to the old bare-fetch behaviour fails 6 of them. NOT verified: that these writes now succeed end to end. This change proves the request carries the token and clears the middleware; several of those routes may have their own problems that the 403 has been masking, and confirming that needs a running app and database. Verified: typecheck 10/10, tests 10/10, build 10/10.
Round-1 CR findings on the wrapper itself. Content-Type was forced onto any non-FormData body. Only FormData was carved out, so URLSearchParams, Blob, ArrayBuffer and a null body were all stamped application/json -- misdescribing the payload and breaking server-side parsing. Every BodyInit except a plain string already carries an encoding fetch derives correctly (multipart boundary for FormData, form-urlencoded for URLSearchParams, the Blob's own type, none for buffers), so the default now applies only to a non-empty string body. Nothing in the repo sends those types today, but this wrapper is now the default path for every API call, so the first caller that does would have been silently misparsed. Corrected three overstated comments. They claimed "no authenticated write in the dashboard could persist" -- false: CSRF_EXEMPT_PREFIXES exempts /api/setup, /api/auth, /api/webhooks and /api/health, which is exactly why setup, login and invite-accept kept working. Also dropped the hardcoded "19 client files" (already inaccurate at 23 and rot-prone), removed a comment that justified production behaviour by what the tests do, and cut the duplicated Usage block from csrf-client.ts that documented another module's signature. The middleware requires cookie AND header AND equality, not header alone. The test file overwrote document.cookie with a data property, destroying jsdom's prototype accessor for every later test in the file. The original descriptor is now captured and restored in afterEach. apiFetch's input type widened from string to RequestInfo | URL to match fetch; the narrower type would have rejected a URL or Request caller. Documented why MUTATING_METHODS is duplicated rather than imported from csrf.ts (that module pulls in next/server, which must not reach the client bundle) with an explicit keep-in-sync note. Tests: 15 now, up from 10 -- added URLSearchParams/Blob/ArrayBuffer, null body, and lowercase-method coverage. Call-site enumeration: - apiFetch: 36 call sites across 19 files, all `apiFetch(path, init)` string invocations; widening the input type is additive and breaks none. - MUTATING_METHODS (api-fetch.ts): module-local, no external references. - csrfHeaders: consumed by api-fetch.ts only; docstring change is comment-only. Verified: typecheck 10/10, tests 10/10, build 10/10.
Round-2 CR found six issues in a fifty-line wrapper, every one introduced by
round-1's own additions rather than by the original change. Patching each edge
would have added more surface, so the wrapper is narrowed instead: attach the
CSRF token on same-origin mutating requests, and nothing else.
Reverted the RequestInfo | URL widening. That was taken in round 1 as a
type-fidelity improvement and it reintroduced the exact bug this PR fixes: a
Request carries its own method, which the wrapper reads only from init, so
apiFetch(new Request(url, {method:'POST'})) took the non-mutating branch and
shipped a real POST with no token. Four of seven reviewers flagged it. input
is a string again; every call site passes one.
Enforced same-origin instead of only documenting it. An absolute cross-origin
URL previously had the live CSRF token attached and sent to that host. The
token is now attached only when the resolved origin matches location.origin.
Dropped the Content-Type default entirely. It mislabelled every non-JSON
string body (text, CSV, XML) and had already needed one fix for
URLSearchParams/Blob/ArrayBuffer. Checked every mutating call site first: only
five omit an explicit Content-Type and all five are bodyless (one POST /run,
four DELETEs), so removing the default changes no request in the codebase.
Exported MUTATING_METHODS from csrf.ts and added a parity test. The comment
warned that drift between the two copies would silently drop CSRF coverage,
and nothing enforced it; now a mismatch fails.
Corrected the comments again -- third round in a row they were the defect.
They credited CSRF_EXEMPT_PREFIXES for invite-accept, which actually survives
via PUBLIC_PATHS in the middleware (trimming the exempt list would have
silently broken that flow), and claimed the jsdom accessor is destroyed and
restored on the prototype when the own property merely shadows it and the
delete is the repair. Explanatory prose is now cut to statements that were
checked.
Tests: 17, up from 15. Added cross-origin and absolute-same-origin, HEAD and
OPTIONS, the MUTATING_METHODS parity assertion, init pass-through
(input/body/signal/credentials/cache), and a Headers instance with a
lowercase header name. Red-green verified on both new guards: removing the
same-origin check fails the cross-origin test, and dropping DELETE from the
server set fails the parity test.
Call-site enumeration:
- apiFetch input type: narrowed string | URL | Request -> string. grep of all
36 call sites confirms every one passes a string literal, template literal,
or a string variable; none passes URL or Request. Typecheck green.
- Content-Type default removed: 32 mutating call sites parsed; 27 set the
header explicitly, the other 5 send no body. No caller relied on it.
- MUTATING_METHODS: newly exported from csrf.ts (additive); its two internal
uses are unchanged. Imported only by the parity test.
Verified: typecheck 10/10, tests 10/10, build 10/10.
…igin Round-3 CR findings. Two are security fixes; the third removes the comments that have been this branch's recurring defect. CSRF validation was skipped entirely for lowercase PATCH. requiresCsrfValidation compared request.method against a set of uppercase names, and the Fetch spec normalises only DELETE/GET/HEAD/OPTIONS/POST/PUT -- PATCH is excluded -- so `method: 'patch'` arrived lowercase, the lookup missed, and the function returned false. Six API routes implement PATCH (tickets/[id], agents/[id], accounts/[id], team/[id], broadcasts/[id], docs/articles/[id]). The method is now uppercased before lookup. Four of seven reviewers found this independently. Cross-origin exploitation is constrained by CORS preflight, but the double-submit defence was defeated and nothing logged the miss. isSameOrigin failed open. With `location` undefined it returned true, so every absolute third-party URL counted as same-origin -- inverting the guarantee the comment claimed. It now returns false when the origin cannot be determined, so an unverified destination never receives the token. The parity test was too weak to catch either. It compared set membership, which stayed equal while the casing bug was live. It now also asserts requiresCsrfValidation returns true for every method in both cases, which is the property that actually matters. Comments cut back to claims that were checked. They have been wrong in four consecutive rounds -- including round 2's own correction, which said the four exempt-prefix routes survive "for a different reason" than /api/team/invite/accept when all five are in PUBLIC_PATHS and survive identically. The explanatory history and cross-file causal narration are gone; what remains is the rejection rule, the PUBLIC_PATHS exception, and why the method set is duplicated. Tests: 25, up from 17. Red-green verified on both fixes -- reverting the server-side uppercase fails 4 tests, reverting the origin fail-safe fails 1. Call-site enumeration: - requiresCsrfValidation: single caller (middleware.ts:37). Uppercasing only widens what it matches; no path that previously required CSRF stops doing so. - MUTATING_METHODS (csrf.ts): two internal uses plus the parity test. Unchanged contents. - isSameOrigin: module-local, called once from apiFetch. Deferred to a security follow-up, all pre-existing in csrf.ts: setCsrfCookie adopts an inbound csrf cookie verbatim, so a subdomain- or XSS-planted value becomes a valid double-submit pair (cookie tossing); CSRF_EXEMPT_PREFIXES matches without a path boundary; the token is never rotated on login or logout; safeEqual length-checks UTF-16 while indexing UTF-8 bytes; and a crypto.subtle.timingSafeEqual branch that does not exist. Verified: typecheck 10/10, tests 10/10, build 10/10.
CPK-7932 Merge PR #167 — CSRF token on client mutations + lowercase-PATCH bypass
CopilotKit/outpost#167 — Merge whenever — no dependency on the #187 split. Zero file overlap with #190/#191; shares exactly one file with #168. Security fix and it has been open since 2026-08-07. The lowercase- No roadmap issue tracks this one — the fix predates the labelling pass. Worth linking it to one, or noting in the merge commit that it stands alone. |
|
Branch updated from Also verified locally before pushing the update — merged against current Blocked on review only ( |
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Two fixes here and the smaller one is the more serious.
The lowercase-PATCH bypass is a real vulnerability, correctly diagnosed. The Fetch spec normalises only DELETE/GET/HEAD/OPTIONS/POST/PUT, so method: 'patch' reaches the middleware lowercase, MUTATING_METHODS.has('patch') is false, and requiresCsrfValidation returns false — CSRF validation skipped entirely on every PATCH route. Uppercasing before the lookup is the right fix and it belongs on the server side, where it is, rather than being papered over by well-behaved clients.
apiFetch is the right shape for the other half. The opt-in csrfHeaders() helper that every caller forgot is a design that was always going to fail this way; making the token the default and leaving the helper for raw access is the correct inversion. Three details I liked:
- It does exactly one thing. No Content-Type guessing, no body parsing, no throwing on non-2xx, no retry. That is why it is safe to drop into ~20 call sites mechanically.
isSameOriginreturns false when it cannot determine the answer — nolocation, or an unparseable URL — so an unverified origin never receives the token. Failing that direction costs a 403; failing the other direction leaks it.- Not clobbering a header the caller set deliberately, and taking
input: stringrather thanRequestInfo | URLbecause aRequestcarries its own method that this function would not read. Accepting aRequestwould have silently skipped the header on exactly the calls that need it — good that it is a type error instead of a comment.
The duplicated MUTATING_METHODS is handled honestly. The copy is unavoidable (csrf.ts pulls in next/server and cannot reach the client bundle), and rather than leaving a comment hoping someone notices, the server-side set is exported purely so api-fetch.test.ts can assert the two have not drifted. A method added to one and not the other silently drops CSRF coverage — that is exactly the failure this PR is fixing, so pinning it with a test is the right instinct.
I spot-checked the call-site conversions in settings/team/page.tsx and tickets-view.tsx. Mechanical and correct — methods, headers and bodies all preserved. Converting the GETs too is a harmless no-op (apiFetch returns plain fetch for non-mutating methods) and is worth it for consistency: a reader never has to ask why this one call uses raw fetch.
Approving.
The module header claimed "Comparison uses `crypto.timingSafeEqual`" and
`safeEqual` claimed it used Web Crypto's `subtle.timingSafeEqual` when present.
Neither is true: `crypto.subtle.timingSafeEqual` is a Cloudflare Workers
extension, not a Web Crypto API, so the probe never succeeded on Node or Edge
and the manual XOR loop has always been the only path. A security module
documenting a constant-time guarantee it does not provide is worse than one
documenting the weaker guarantee it does.
Removes the dead probe rather than leaving it as an invitation to re-add, and
records why not to. No behavior change: the branch never executed.
Also corrects three further false claims in the same file, all found in the
same review:
- "sets a `csrf` cookie ... on every response" — paths returning before
`setCsrfCookie` get none, and the token is adopted rather than re-randomised.
- "Attach (or refresh)" on setCsrfCookie — the token is never rotated, not on
login and not on privilege change.
- the `safeEqual` length guard compares UTF-16 string length while the loop
compares UTF-8 bytes; documents that this fails closed (`x ^ undefined` is
`x ^ 0`, so mismatch stays non-zero) and is unreachable for UUID tokens.
The two substantive weaknesses named in the new header — an unbound, adopted
token and boundary-free prefix exemptions across two lists — are tracked as #225
rather than fixed here.
Every assertion in this file read `fetchMock.mock.calls[0]`, so the suite only
ever checked what was SENT. Two consequences, both verified by breaking the
wrapper on purpose:
- A wrapper that returns `new Response()` instead of fetch's own, or that
awaits fetch only for its side effect, passed all 25 tests. Now covered on
both return paths (mutating, and the non-mutating short-circuit).
- A wrapper that issues the request twice was invisible, because the duplicate
lands in calls[1]. That includes the case that matters most here: a retry
that re-sends a mutation, or one that retries cross-origin while still
carrying the token. Now pinned with toHaveBeenCalledTimes(1) on the
token-attachment and cross-origin cases, plus an assertion that NO call
carried the token cross-origin.
Red-green verified: returning a fresh Response fails the identity tests; adding
a duplicate fetch call fails 7, including all four token-attachment cases.
|
@jerelvelarde — ran a 17-agent review over this diff (agent-assisted, so it does not substitute for your approving review; GitHub won't take mine anyway since I'm the author). Summary of what it found, and where your attention is actually worth spending. Recommendation: merge itThis PR's own code came out clean on correctness. It also fixes a live outage: every non-exempt dashboard write was 403ing because Two fixes applied from the review (new commits)
Where review effort paysThe Worth knowing while you read: 4 mutating What this PR does NOT fix, now trackedCSRF remains bypassable after this merges. Not a regression — pre-existing, and out of scope for a 372-LOC bugfix — but you should know before approving:
#225 is scoped as structural fixes rather than patches, because three bypasses of one chain (lowercase Deferred, not lost~120 findings were pre-existing unchecked Full reports are on disk if you want any specific finding's detail. |
|
Merged as
Two fixes landed inside this PR from the same review: |
2 participants
Fixes dashboard saves, which have been returning 403 for every authenticated user, and closes a CSRF bypass found while reviewing that fix.
The bug
middleware.tsrejects a mutating/api/*request unless it carries both thecsrfcookie and a matchingX-CSRF-Tokenheader. The cookie is set on every response; the header was sent by nothing.csrfHeaders()existed inlib/csrf-client.tsdocumented for exactly this and was unused across the repo — 19 client files issue mutating fetches and not one attached it.Affected: settings (profile, org, team, templates), agents, tickets (create, notes, actions), docs, loom import, broadcasts, accounts, onboarding, qa-chat, sync force-sync, sync mappings. Routes in
PUBLIC_PATHSwere unaffected, which is why setup, login and invite-accept kept working.This also explains why the mappings hardening in #95/#160 was validating a write path the UI could never reach.
The fix
apps/web/src/lib/api-fetch.ts—apiFetch, a 44-line wrapper that attaches the token on same-origin mutating requests and does nothing else. 36 call sites across 19 files migrated.Chosen over spreading
csrfHeaders()at each site because the bug exists precisely because attaching the header was opt-in and every author opted out. A wrapper makes correct the default and puts it somewhere testable.Deliberately narrow, after review found each convenience cost more than it bought:
Content-Typedefault. It mislabelledURLSearchParams,Bloband plain-text bodies. Verified before removing: of 32 mutating call sites, 27 set the header explicitly and the other 5 send no body — so no request in the codebase changes.input: string, notRequestInfo | URL. ARequestcarries its own method, which the wrapper reads frominit— accepting one silently skipped the header and reintroduced this very 403. All 36 call sites pass strings.Security fix found during review
CSRF validation was skipped entirely for lowercase
PATCH.requiresCsrfValidationcomparedrequest.methodagainst uppercase names, and the Fetch spec normalises onlyDELETE/GET/HEAD/OPTIONS/POST/PUT—PATCHis excluded — somethod: 'patch'arrived lowercase, the lookup missed, and the function returnedfalse. Six routes implement PATCH (tickets/[id],agents/[id],accounts/[id],team/[id],broadcasts/[id],docs/articles/[id]). Cross-origin exploitation is constrained by CORS preflight, but the double-submit defence was defeated and nothing logged the miss.Also:
isSameOriginreturnedtruewhenlocationwas undefined, so an absolute third-party URL counted as same-origin — the inverse of the stated guarantee. It now fails safe.Verification
pnpm typecheck10/10 ·pnpm test10/10 ·pnpm build10/10 · 25 wrapper tests.Red-green verified on both security fixes: reverting the server-side uppercase fails 4 tests; reverting the origin fail-safe fails 1. The
MUTATING_METHODSparity test asserts the client set matches the server's and thatrequiresCsrfValidationaccepts every method in both cases — set equality alone stayed green while the casing bug was live.Not verified: that these writes now succeed end to end. This proves the request carries the token and clears the middleware. It does not prove the handlers work — these routes have been unreachable long enough that a second wave of failures is plausible, and confirming needs a running app and database.
Review
Three cr-loop rounds, 13 must-fix findings, all in this branch's own code. Round 2 caught that round 1's type widening had reintroduced the original bug; round 3 caught the PATCH bypass. Comments were the recurring defect — wrong in four consecutive rounds, including round 2's correction of round 1's error — so they are now cut to claims that were checked.
Follow-ups (not in this PR)
Security,
csrf.ts— worth its own focused review:setCsrfCookieadopts an inboundcsrfcookie verbatim, so a subdomain- or XSS-planted value becomes a valid double-submit pair (cookie tossing);CSRF_EXEMPT_PREFIXESmatches without a path boundary, so a future/api/authorizeor/api/setup-wizardinherits the exemption; the token is never rotated on login or logout;safeEquallength-checks UTF-16 while indexing UTF-8 bytes; and acrypto.subtle.timingSafeEqualbranch that does not exist is guarded by a comment claiming it is live.middleware.ts:13— auth bypass.pathname.includes('.')returns beforegetTokenand before the CSRF check, so any request path containing a dot skips authentication entirely. The matcher already excludes_next/static,_next/imageandfavicon.ico, so the check appears redundant. Pre-existing and unrelated to this change, but it undermines the property this PR establishes. Flagged, not fixed here.Dashboard error handling — a broad class surfaced by review: missing
res.okchecks, swallowed errors, unreverted optimistic updates, uncleared timeouts, and stale-response races across settings, sync, accounts, onboarding, tickets and qa-chat. Several are the same shape as the bug fixed in668f238.