fix(web): fail closed when NEXTAUTH_SECRET is missing in production - #1229
Conversation
`proxy.ts` gated the entire login check on `AUTH_ENABLED = !!AUTH_SECRET`, which fails OPEN. A single missing/unpropagated `NEXTAUTH_SECRET` in production silently served `/dashboard` and every non-public `/api/*` route to anonymous visitors, with no error and no signal. Replace the boolean with `resolveAuthGateMode()`: | secret | NODE_ENV | opt-out | mode | |---------|------------|---------|----------------| | present | any | any | enforce | | missing | production | unset | misconfigured | | missing | production | set | disabled | | missing | dev/test | - | disabled | `misconfigured` returns 503 rather than 401/redirect: without the secret a sign-in cannot succeed either, so redirecting to /login would loop forever. Public paths (incl. /api/auth/*) stay reachable so an operator can still complete OAuth setup, and the server-to-server internal-token bypass is unchanged. The state is logged once at module init, not per request. Local dev DX is unchanged - a missing secret outside production still just leaves the gate off. Opting out in production now requires an explicit, auditable `AUTH_ALLOW_UNAUTHENTICATED=1`. Adds 10 tests (4 pure-policy, 6 integration against `proxy()`). The two fail-closed integration tests were confirmed RED before the fix, and a negative control (reverting `'misconfigured'` -> `'disabled'`) fails exactly 4 of them, proving the guards are non-vacuous. Closes #1058 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
Follow-up filed: the pre-existing flake described in Note for reviewers now has its own fix in #1230 (independent branch off |
Closes #1058
The defect
apps/web/src/proxy.tsgated the entire login check on a boolean that fails open:If
NEXTAUTH_SECRETis absent in production, the gate is skipped entirely and:/dashboardand all nested routes are served unauthenticated/api/*route is served unauthenticatedOne missing or unpropagated env var silently converts a login-gated app into a fully public one — with no error and no signal. The "activate-when-configured / safe rollout" intent is reasonable for local dev; it is not acceptable as implicit production behaviour.
The fix
Secure by default; insecure only by explicit, auditable declaration. New pure policy fn
resolveAuthGateMode()inlib/auth-paths.ts(already the Next-free, offline-testable policy home):NODE_ENVenforcemisconfigureddisableddisabledDesign notes
/loginwould infinite-loop. 503 is loud, correct, non-looping, and correctly signals an operator fault rather than a client one./api/auth/*too would make it impossible to ever complete the OAuth setup that fixes the state. Locked in by a test.x-eventrelay-internalcheck still runs first.Evidence
RED (before the fix)
The two fail-closed integration tests were written first and failed against the unmodified
proxy():The other 4 passed immediately, confirming the test harness models existing behaviour correctly (401 for API, 307 -> /login for pages, public routes open, dev unaffected).
GREEN
Negative control (guards are non-vacuous)
Reverting the single decisive line (`'misconfigured'` -> `'disabled'`) fails exactly the 4 tests that assert the fix, across both layers:
Full verification
vitest run(wholeapps/web)main: 245 / 44 — +10 new, 0 regressions)eslint src middleware.tstsc --noEmitnext buildThis is intentionally not backwards compatible for one specific case: a deployment running with
NODE_ENV=productionand noNEXTAUTH_SECRETtoday serves everything publicly; after this change it returns 503.That is the vulnerability being fixed. If a deployment is intentionally public, set:
Documented in
.env.example,apps/web/.env.example,LAUNCH_CHECKLIST.md, and themiddleware.tsdocstring (which previously carried the now-stale claim "session required when NEXTAUTH_SECRET is set").Note for reviewers
While verifying, I found a pre-existing, unrelated flaky test:
src/app/api/__tests__/billing-chat-gating.test.tstimes out intermittently on any machine whereAI_GATEWAY_API_KEYis set in the shell, becauseapp/api/chat/route.ts:140then makes a real network call to the Vercel AI Gateway.vitest.config.tsneutralizesBACKEND_URLandBILLING_COOKIE_SECRET"so tests are deterministic regardless of the developer's shell env" but misses the gateway keys.Proof it is unrelated to this PR: with these changes fully stashed, the test still fails 1-in-6 runs; with
AI_GATEWAY_API_KEYunset it passes 6/6. Filed separately rather than mixed into this security fix.