NextJS middleware#640
Conversation
|
Thanks James! @tyler-reitz please keep this PR open. This is a prototype Next.js middleware for Firebase Auth to allow sync-ing auth state between Server Components and Client Components. |
tyler-reitz
left a comment
There was a problem hiding this comment.
Thanks for this PR, @jamesdaniels. A few things to address before merge:
Blockers
jose in devDependencies (package.json line 26)
jose is imported at runtime but listed under devDependencies. It won't be installed in production. Based on the README install command (npm install firebase-cookie-middleware jose lru-cache), the intent looks like peerDependencies, but that should be explicit. lru-cache is in dependencies (correct); jose should be too, or explicitly peered.
Next.js 16 compatibility (README, index.ts line 614)
Next.js 16 renamed middleware.ts to proxy.ts and deprecated export const middleware in favor of export const proxy. It also dropped Edge Runtime support for the proxy layer, which now runs on Node.js only. The README's headline feature ("100% Edge Runtime Compatible") and all quickstart examples are incorrect for Next.js 16 users. The devDependency pins to ^15.2.4, so this was never tested against 16. This needs either an explicit Next.js 15-only scope in the docs, or an update to support 16.
API key logged on refresh failure (line 481)
refreshUrl.searchParams.set("key", options.apiKey); // line 465
console.error(refreshUrl.toString(), refreshResponse.status, ...); // line 481The API key is a query param on refreshUrl and gets logged verbatim on any non-OK refresh response. In production with centralized log aggregation this exposes the key to anyone with log read access. Fix: log refreshUrl.origin + refreshUrl.pathname instead.
tenantId documented as function but typed as string (README line 125, index.ts line 506)
The README documents tenantId: string | ((req: NextRequest) => string | Promise<string>) but Config only has tenantId?: string. normalizeConfig passes it through without resolving. A user following the docs gets a TypeScript compile error. Either implement the function form or remove it from the docs.
Empty error message (line 580)
if (!normalizedConfigurations.length) throw new Error("");An empty error string gives callers nothing to debug. Should describe what is missing and how to fix it.
Should fix (surfaced by automated security review, flagging for verification)
JWKS eviction lock not released on network error (lines 218-225)
If getFirebaseAuthJwks(cache, true) at line 221 throws, the lock in cache appears to be left set for the full 30-second TTL with no release path. There is no finally block. If that is correct, during that 30-second window every request that gets ERR_JWKS_NO_MATCHING_KEY would take the else branch and re-throw, rejecting users with a newly-rotated key until the TTL expires. A try/finally that deletes the lock on failure would address this if it is indeed a gap.
Unhandled TypeError from new URL(finalTargetParam) (line 347)
Line 344 guards against null/empty finalTarget, but a non-URL string causes new URL() to throw with no surrounding try/catch at this call site. This would surface as a 500 rather than a 400. Worth verifying whether Next.js catches this upstream.
defaultMemoryCache and serverless (line 59)
The default in-memory LRU is per-process and empty on every cold start. Worth a note in the docs clarifying that the JWKS eviction lock and JWT caching only work as intended when a shared external cache (Redis, Vercel KV, etc.) is provided. The README currently frames custom caching as optional for performance, which may undersell when it is actually needed.
Minor
@firebase/utilinternal import (line 13):getDefaultAppConfigis from an internal Firebase package with no public stability guarantees. If there is no public-API equivalent, a comment acknowledging the fragility would help.exp=0falsy check (line 200):if (jwtPayload.exp && ...)treatsexp=0as falsy and skips verification. Should bejwtPayload.exp !== undefined. Low real-world impact since Firebase never issuesexp=0, but the guard is logically wrong.isEmulatorRefreshTokenuses internal Firebase property (line 157): Relies on_AuthEmulatorRefreshToken, which is undocumented. Worth a comment acknowledging it could break if the emulator changes its refresh token format.- TODO committed (line 542):
// TODO don't override string valueshould be resolved or removed before merge. - Self-referential comment (line 17): The comment links to this PR itself; remove before merge.
createTokentest helper duplicated (multiple test files): Defined with slight divergences acrossmiddleware_verify,middleware_cache, andmiddleware_refresh. A shared test utility file would keep them consistent.
Design questions for the team
- Is this intended to be a standalone npm package (
firebase-cookie-middleware) or areactfire/nextjsexport? The PR does not modify the root build config, so the publication path is unclear. - Should
josebe bundled (adds around 50KB) or peer-depended? Given thatjosehas had breaking changes across major versions, a tight peer dep range may be safer than bundling. - Does the
tenantIdfunction form need to be implemented before merge, or is it deferred?
| "@vitest/coverage-v8": "^4.1.9", | ||
| "esbuild": "^0.25.1", | ||
| "firebase": "^11.5.0", | ||
| "jose": "^6.0.10", |
There was a problem hiding this comment.
jose is imported at runtime throughout index.ts but listed here as a devDependency — it will not be installed in production. Move to dependencies, or add to peerDependencies to match the README install instructions (npm install firebase-cookie-middleware jose lru-cache).
| return logout(); | ||
| } | ||
| if (!refreshResponse.ok) { | ||
| console.error(refreshUrl.toString(), refreshResponse.status, refreshResponse.statusText); |
There was a problem hiding this comment.
This logs refreshUrl.toString() which includes ?key=<apiKey> set at line 465. The Firebase API key is exposed in plaintext in server logs on any refresh failure. Log refreshUrl.origin + refreshUrl.pathname instead.
|
|
||
| export type Config = { | ||
| emulator?: boolean | string; | ||
| tenantId?: string; |
There was a problem hiding this comment.
The README documents tenantId as accepting string | ((req: NextRequest) => string | Promise<string>) but the type here only allows string. A user following the docs gets a TypeScript compile error. Either implement the function form or update the README to match.
| cache: defaultMemoryCache, | ||
| }); | ||
| } | ||
| if (!normalizedConfigurations.length) throw new Error(""); |
There was a problem hiding this comment.
new Error("") gives callers nothing to debug. Should describe what is missing, e.g. "No Firebase configuration found. Pass a Config to composeMiddleware or initialize a Firebase app first."
| ); | ||
| }; | ||
|
|
||
| export const middleware = composeMiddleware(() => {}); |
There was a problem hiding this comment.
Next.js 16 deprecated export const middleware in favor of export const proxy (and middleware.ts → proxy.ts). It also dropped Edge Runtime support for the proxy layer, which now runs on Node.js only. This export name will be silently ignored by Next.js 16, meaning no auth middleware runs. Needs either explicit Next.js 15-only scoping in the docs, or an update to support 16.
Description
Code sample