feat(nextjs): add firebase-cookie-middleware with security hardening#739
feat(nextjs): add firebase-cookie-middleware with security hardening#739tyler-reitz wants to merge 11 commits into
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
- Move jose from devDependencies to dependencies (runtime dep) - Strip API key from refresh error log (log origin+pathname only) - Fix exp=0 falsy guard (use !== undefined) - Release JWKS eviction lock on fetch error (add cacheDelete + try/finally) - Return 400 for malformed finalTarget URL instead of throwing TypeError - Fill in empty error message in composeMiddleware config guard - Align tenantId docs to string-only (function form deferred) - Add del() to CacheProvider interface and MemoryCacheProvider - Annotate @firebase/util and _AuthEmulatorRefreshToken as internal/fragile - Remove self-referential PR comment and resolved TODO - Centralize createToken test helper in middleware_test_utils.ts
Introduces the firebase-cookie-middleware package (v0.0.1) for Next.js 14/15/16, taking over from PR FirebaseExtended#640 (James Daniels). Security fixes applied during takeover review: - Emulator explicit opt-in only (env var not auto-detected) - JWT signature failure sets refreshable=false to prevent bypass - Verify refreshed token before accepting it - SSRF: loopback validation on emulator host - CSRF: Origin check on POST/DELETE to /__cookies__ - CHIPS: SameSite=None on partitioned cookies - JWKS eviction lock released in finally block - Falsy-zero TTL: require ttlSeconds > 0 before caching - Proxy targets validated; return 400 instead of throwing - Optional chaining on JWT payload access - Removed duplex: 'half' workaround (unnecessary in Node.js) Next.js 16: adds proxy export (proxy.ts replaces middleware.ts in Next.js 16 projects). proxy.ts runs on Node.js runtime only; middleware.ts remains available for Edge Runtime (deprecated). Also fixes pre-existing TypeScript strict-mode errors surfaced by upgrading moduleResolution from node to bundler. Ref: FirebaseExtended#640 Original work by: James Daniels (jamesdaniels@google.com)
40b253c to
37050fa
Compare
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
The nextjs package is a self-contained sub-package with its own tsconfig and test runner. Including it in the root tsc/vitest runs causes failures because next is not installed as a root dependency.
Use **/ prefix so the patterns match at any directory depth, covering both src/nextjs/ and package/src/nextjs/ (the CI test job unpacks the npm tarball alongside the checkout, creating both paths). Also broaden node_modules exclusion to catch nested directories like functions/node_modules/.
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
…e throw with logout Three uncaught exceptions that crash middleware with 500: - proxy path: response.json() fired before response.ok check, returns 502 on non-JSON - refresh path: refreshResponse.json() now catches non-JSON bodies and calls logout() - refresh path: missing id_token now calls logout() instead of throwing Also remove duplicate JSDoc paragraph and fix two em-dashes in comments.
Local code review (in lieu of Gemini bot)The Gemini review bot has failed on every attempt for this PR (5+ runs). The diff is 2,500+ lines of all-new code, which appears to exceed the bot's input limit. I ran a local multi-angle review as a substitute. Three findings were fixed in the latest commit (bb006e9); the rest are documented below for your consideration. Fixed in bb006e91. Proxy path: 2. Refresh path: 3. Refresh path: Remaining findings (for your review)Security:
Operational correctness:
Minor correctness:
Forward compatibility:
Overall the package is solid. The three crash-on-error cases are fixed; the remaining items are defense-in-depth or edge-case correctness issues appropriate for follow-up issues. |
020570a to
bb006e9
Compare
Adds 'import' condition alongside 'module' so TypeScript moduleResolution bundler resolves the package types correctly. Switches tsconfig from noEmit to emitDeclarationOnly so tsc generates index.d.ts in dist/.
armando-navarro
left a comment
There was a problem hiding this comment.
I ran the suite (71/71 green), confirmed the sub-package is fully isolated from the reactfire build/tsc/publish, and read the middleware closely. One thing I found looks worth fixing before this lands, plus a couple of smaller items.
Refresh path is missing the main-path guard against unsigned tokens
The description lists "verify-after-refresh" as a fix, so I went looking at how the refreshed token is validated. My reading is that the refresh path doesn't carry the same alg:"none" guard the main path has: an unsigned token in the refresh response is accepted where the identical token would be rejected if it arrived as the ID-token cookie. To be clear up front, I don't think this is remotely exploitable today (reasoning below); it reads to me as a defense-in-depth gap in the stated fix rather than a live hole.
The asymmetry:
verifyFirebaseIdTokentreats analg:"none"header as an emulator credential and returns the payload without a signature check (index.ts:219-230). That's by design, since there's no signature to verify. The safety check is meant to be the caller inspecting the returnedisEmulatedCredentialflag.- The main entry path does inspect it:
if (isEmulatedCredential && !options.emulatorHost) return logout(). - The refresh path discards it:
const [verifiedNewPayload] = await verifyFirebaseIdToken(...)and then accepts the payload, with noemulatorHostre-check.
I reproduced it locally with a throwaway test: mock jose.jwtVerify to always reject (so anything accepted must have skipped signature checking), and have the refresh endpoint return an alg:"none" token with well-formed claims. The refresh path accepted it as an authenticated user; the identical token as the ID cookie was rejected.
To be honest about exploitability: reaching this in production sends the refresh to the hardcoded securetoken.googleapis.com, which you don't control, so over intact HTTPS to Google it isn't remotely exploitable. It only bites if the refresh response is attacker-influenced (TLS interception, a compromised upstream, or a later refactor that varies the refresh target), which is the same "compromised token endpoint" case the verify-after-refresh fix is aimed at, so it seemed in-scope to me rather than a follow-up. Unless I'm misreading the intended boundary, mirroring the main-path guard would close it:
const [verifiedNewPayload, newIsEmulated] = await verifyFirebaseIdToken(newIdToken, ...);
if (!verifiedNewPayload || (newIsEmulated && !options.emulatorHost)) return logout();
A regression test for an alg:"none" token arriving via refresh would be worth adding alongside, since middleware_refresh.node.test.ts currently only covers signed tokens and emulator-refresh-token detection, so this path isn't exercised.
Smaller items
- README
SameSite(README.md:218): the Cookie Architecture section says the ID-token cookie isSameSite=Lax, but production setsSameSite=None(line 333);Laxis only the Safari-localhost fallback, and CHIPSPartitionedrequiresNoneanyway. The code'sNoneis deliberate and correct for partitioned cookies, so this is purely a docs fix, but worth getting right because it matters to the security model: withSameSite=Nonethe auth cookies are sent cross-site, so theOrigincheck in the/__cookies__handler (362-367) is the actual CSRF defense, not SameSite. A reader trusting the README would assume a SameSite protection that isn't there. - JWKS TTL (index.ts:159-164): following up on your own thread note about the unclamped
max-age. It only bites a customCacheProviderthat reads the cached JWKS back: the default cache's reads short-circuit to the remote JWKS set (line 144), so even though the rotation refetch path does write afirebase:jwksentry, the default cache never reads it. Add theERR_JWKS_NO_MATCHING_KEYrefetch self-healing on rotation and it's minor overall. One thing on the suggestedMath.min(maxAge, MAX_MAX_AGE):MAX_MAX_AGEis the 400-day cookie bound, which is a much larger magnitude than a JWKS TTL, so if you do clamp it, a JWKS-appropriate ceiling is probably what you want. Your call whether it's worth it.
Everything else I poked at (emulator loopback pinning, the proxy target allow-list, the explicit emulator:true opt-in, the per-project cache-key safety) held up well. I may be missing something about how you intend the refresh boundary to work, so if the alg:"none" case is handled somewhere I overlooked, point me at it and I'll take another pass.
| } | ||
| // Full signature + claims verification on the refreshed token. Caching is | ||
| // handled inside verifyFirebaseIdToken, so no separate cacheSetEx needed. | ||
| const [verifiedNewPayload] = await verifyFirebaseIdToken( |
There was a problem hiding this comment.
This drops isEmulatedCredential from the tuple, so the refresh path has no equivalent of the main-path guard at line 501 (isEmulatedCredential && !options.emulatorHost → logout). An alg:"none" token in the refresh response is accepted here without a signature check, where the same token as a cookie would be rejected. Not exploitable over intact HTTPS to Google, but it's a gap in the "full verification" the comment above promises. Mirroring line 501 would close it:
const [verifiedNewPayload, newIsEmulated] = await verifyFirebaseIdToken(newIdToken, options.projectId, options.tenantId, cache);
if (!verifiedNewPayload || (newIsEmulated && !options.emulatorHost)) {
console.error("Refreshed token failed verification");
return logout();
}
Summary
This PR introduces
firebase-cookie-middleware(v0.0.1), a production-ready Next.js middleware package for Firebase Auth cookie persistence. It takes over from #640 (original work by James Daniels, @jamesdaniels), which is being closed in favor of this PR to establish clear ownership.All logic from #640 has been carried forward. The following security issues were identified and addressed during the takeover review:
FIREBASE_AUTH_EMULATOR_HOSTis no longer auto-detected; requiresemulator: truein config to prevent unsigned token acceptance if the env var leaks into productionrefreshable=falseon signature failure: prevents refresh attempts when the original token has an invalid signature (not just expiry)localhost,127.0.0.1,::1)Originheader checked onPOST/DELETEto/__cookies__; cross-origin requests rejected with 403SameSite=Noneapplied correctly on partitioned cookies (was incorrectlySameSite=Laxin production)finallyblock to prevent deadlock on verification errorttlSeconds > 0required before caching;0no longer treated as "cache forever"400instead of an uncaught throwduplex: 'half': workaround was unnecessary and caused Node.js type errorsNext.js 16 compatibility: adds a
proxynamed export forproxy.ts(Node.js runtime). Themiddlewareexport remains formiddleware.tsusers (Edge Runtime, v14/15 and v16 deprecated path). Logic is identical between the two exports.Also fixes pre-existing TypeScript strict-mode errors surfaced by upgrading
moduleResolutionfromnodetobundler.Test plan
npm testinsrc/nextjs/)tsc --noEmitclean at repo root and insrc/nextjs/emulator: true+FIREBASE_AUTH_EMULATOR_HOSTsetproxy.tsexport in a Next.js 16 appAttribution
Original implementation by James Daniels (jamesdaniels@google.com), PR #640. Takeover and security hardening by Tyler Dixon.
Closes #640