fix: KEEP-240 add Origin header check on cookie-authed mutating routes#1048
fix: KEEP-240 add Origin header check on cookie-authed mutating routes#1048
Conversation
better-auth's default SameSite=Lax cookies are the primary CSRF defence, but its built-in originCheckMiddleware only guards /api/auth/**. The 124 application API routes that authenticate via session cookies have no explicit origin check, leaving a sibling-subdomain CSRF gap because the *.keeperhub.com wildcard in trustedOrigins is treated as same-site by browsers. Adds two layers: - Root middleware.ts enforces Origin (falling back to Referer) against trustedOrigins on POST/PATCH/PUT/DELETE under /api/** when the request carries a cookie. Exempts /api/auth/** (better-auth handles its own origin check), webhooks (Stripe + workflow), MCP workflow calls, cron, and OAuth client endpoints, all of which legitimately accept cross-origin POSTs and don't depend on session cookies. - getDualAuthContext, resolveOrganizationId, and resolveCreatorContext re-run the same check after session auth resolves, as defence-in-depth for any route the middleware matcher might miss. The trusted-origin list is extracted to lib/trusted-origins.ts so the edge middleware (which can't import lib/auth.ts due to Node-only deps) shares a single source of truth with better-auth. OAuth Bearer and API-key callers are not gated; cookieless cross-origin calls remain unaffected. Blocked attempts log info-level for monitoring. 48 new and updated unit tests cover the matcher, exempt paths, method and origin/referer fallback logic.
Reordering checkSessionOrigin above auth.api.getSession in all three helpers avoids a wasted DB hit when the request will be rejected anyway. Bearer/API-key callers still short-circuit before the check, so they remain unaffected.
…eCreatorContext The defence-in-depth check is wired identically into all three helpers but only getDualAuthContext was tested. Adds happy-path and rejection tests for the other two so a regression in one helper is caught independently.
The previous "path traversal" name implied a security guarantee the function isn't actually making — it just rejects anything that isn't a bare origin, because the regex anchors and the wildcard doesn't span slashes. Renamed and added a non-traversal path case to make the intent explicit. Real callers go through normaliseOrigin first.
headers.has("cookie") returns true for an empty value, which would
trigger a spurious 403 if a proxy strips the cookie value. Switch to
a falsy check on get("cookie") so empty and missing are equivalent.
Next.js 16 deprecated the `middleware` file convention in favour of `proxy`; the build emits a deprecation warning otherwise. Renames the file and the exported function, and updates the doc comments and the test import path. Behaviour and matcher are unchanged — Next recognises both filenames identically until removal.
Review feedback addressedSix follow-up commits on top of the original:
54/54 unit tests pass; lint and type-check clean on new code. Notes (review items 1 and 6)Server Actions: Live smoke test: I attempted |
PR Environment DeployedYour PR environment has been deployed! Environment Details:
Components:
The environment will be automatically cleaned up when this PR is closed or merged. |
The Dockerfile lists every root-level source file explicitly (granular COPYs for BuildKit layer caching, see line 28). The new proxy.ts was not added when the file was introduced, so the deployed image had no CSRF middleware and only Layer B (in getDualAuthContext) was active in production. Smoke testing the PR deploy showed cross-origin POSTs to auth.api.getSession callers (e.g. /api/api-keys) reaching the handler. Adding the COPY makes Layer A active in builds.
PR Environment DeployedYour PR environment has been deployed! Environment Details:
Components:
The environment will be automatically cleaned up when this PR is closed or merged. |
Smoke test on deploy-pr-1048: PASS after Dockerfile fix
The bolded rows are the ones that would have been vulnerable without Layer A — routes that authenticate with The What we foundPre-fix, on the deploy:
Root cause: the Dockerfile uses granular per-directory COPYs and never picked up the new root-level Note on cookie semantics
CleanupAll test API keys deleted (verified empty list). Local cookie file wiped. |
…present The previous "any non-empty Cookie header" check caused false 403s for Bearer/API-key callers behind Cloudflare Access — CF_AppSession and CF_Authorization cookies are present on every request through CF Access, which the proxy treated as session-bearing. Tighten the check to look specifically for the better-auth session token cookie name in the Cookie header: better-auth.session_token= (dev) __Secure-better-auth.session_token= (prod, with secure prefix) Cookieless callers and callers carrying only unrelated cookies (CF Access tokens, analytics, sidebar-collapsed, etc.) bypass the origin check. Real CSRF still requires the victim's session cookie to be sent, which triggers the substring match and the origin check. Adds a pinning test that imports getCookies from better-auth/cookies and asserts the substring still matches what better-auth generates, so a future better-auth upgrade that renames the cookie fails CI rather than silently disabling the gate.
PR Environment DeployedYour PR environment has been deployed! Environment Details:
Components:
The environment will be automatically cleaned up when this PR is closed or merged. |
Cookie tightening fix verified on PR deployCommit Smoke matrix on the redeployed PR environment:
66/66 unit tests pass (12 new) |
|
Hi @suisuss, LGTM. Layered design is sound and the better-auth cookie-name pinning test is a great touch. A few notes:
|
…ndant trusted origin Addresses review notes 1 and 2: - Replace SESSION_COOKIE_NAME_SUBSTRING (substring match) with SESSION_COOKIE_RE, a boundary-anchored regex that rejects the substring appearing inside a cookie value or as a suffix of an unrelated cookie name. Cleanup, not a security fix. - Drop https://app-staging.keeperhub.com from TRUSTED_ORIGINS; it is already covered by the https://*.keeperhub.com pattern. - Update pinning test to assert the regex matches the cookie name better-auth generates, and add new tests for the boundary cases the substring check missed.
PR Environment DeployedYour PR environment has been deployed! Environment Details:
Components:
The environment will be automatically cleaned up when this PR is closed or merged. |
Review feedback addressed (notes 1, 2, 4) + follow-up filed (note 3)Pushed
69/69 unit tests pass (+3 new boundary cases). Lint and type-check clean on changed files. Re-smoke on
|
| Scenario | Expected | Result |
|---|---|---|
| A. CSRF blocks | ||
POST /api/api-keys evil Origin |
403 | 403 |
POST /api/api-keys evil Referer only |
403 | 403 |
POST /api/api-keys no Origin/Referer |
403 | 403 |
PATCH /api/user evil Origin |
403 | 403 |
DELETE /api/api-keys/{id} evil Origin |
403 | 403 |
| B. Trusted origin pass-through | ||
POST /api/api-keys trusted Origin |
not-403 | 200 (key created and deleted) |
POST /api/api-keys trusted Referer only |
not-403 | 200 (key created and deleted) |
| C. Cookie tightening (regex now anchored) | ||
| POST evil + only CF cookies (no session) | not-403 | 401 (handler-level) |
| POST evil + no cookies | not-403 | 302 (handler-level) |
POST evil + decoy=better-auth.session_token=trapvalue |
not-403 (anchor rejects substring inside a cookie value) | 302 (handler-level) -- the OLD substring check would have returned 403 here |
| D. Exempt paths | ||
POST /api/auth/sign-out evil |
bypass proxy | 403 from better-auth |
POST /api/billing/webhooks/stripe evil |
bypass proxy | 404 (handler-level) |
| E. GET (unguarded) | ||
GET /api/user/wallet evil origin |
200 | 200 |
13/13. Cookies wiped, test keys deleted (verified via list).
🧹 PR Environment Cleaned UpThe PR environment has been successfully deleted. Deleted Resources:
All resources have been cleaned up and will no longer incur costs. |
Note on row E (GET unguarded) and audit of GET side effectsA reviewer might reasonably ask why The proxy intentionally skips
The smoke test bypasses (1) by manually attaching the session cookie via That argument breaks if any GET handler actually mutates state. I audited all GET handlers under
58+ GET handlers scanned. The two with side effects are both batch/cleanup endpoints gated by Bearer / internal-service auth (NOT session cookies). All user-facing GET routes are reads only, so the SameSite + CORS argument holds for them. Conclusion: skip-GET in the proxy is the correct design and matches better-auth's own behaviour. No further changes needed for this PR. |
2 participants
Summary
/api/**routes by enforcingOriginagainsttrustedOriginson state-changing methods.middleware.ts(primary enforcement) with an in-handler check insidegetDualAuthContext/resolveOrganizationId/resolveCreatorContext(defence-in-depth).SameSite=Lax(verified in source), but itsoriginCheckMiddlewareonly guards/api/auth/**; the other 124 API routes had no origin check.Behaviour
Middleware runs on
/api/:path*and:GET/HEAD/OPTIONS./api/auth/,/api/billing/webhooks/,/api/cron/,/api/oauth/,/api/workflows/[id]/webhook,/api/mcp/workflows/[slug]/call.cookieheader is present (Bearer/API-key callers unaffected).Origin, falls back toReferer. Missing or untrusted -> 403{ error: "Invalid origin" }.getDualAuthContext(and the org/creator variants) repeat the check after session auth resolves, so a misconfigured matcher cannot silently bypass the protection. OAuth and API-key callers are not gated.trustedOriginsis extracted tolib/trusted-origins.tsso the edge middleware (cannot importlib/auth.tsdue to Node-only deps) shares one source of truth with better-auth.Blocked attempts log info-level for the first weeks of monitoring.
Test plan
pnpm vitest run tests/unit/trusted-origins.test.ts tests/unit/dual-auth-context.test.ts tests/unit/middleware.test.ts-- 48 passingpnpm check-- new code clean (pre-existinglib/auth.ts:334,366useBlockStatementsleft untouched per surgical-changes rule)pnpm type-check-- cleanapp-pr-1048deploy6065c85(2026-05-03): 13/13 scenarios pass. Full matrix in PR comments.Notes for reviewer
lib/trusted-origins.tsis a small standalone implementation (~30 lines) rather than reusing better-auth's internalwildcardMatch, to avoid coupling to a non-public export.https://*.keeperhub.commatches better-auth's existing matcher:*matches arbitrary depth subdomains. No tightening of the trusted-origin set in this PR.https://app.keeperhub.com/../evilbecauseURLnormalises out the..before comparison.