Skip to content

Commit cf3e76a

Browse files
tyler-reitztyler-reitz
committed
fix: address review feedback on Next.js middleware PR
- 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
1 parent 11f38d4 commit cf3e76a

8 files changed

Lines changed: 67 additions & 566 deletions

‎src/nextjs/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ Creates a composable Next.js middleware handler that verifies cookies and manage
122122
| :--- | :--- | :--- |
123123
| **`options`** | `FirebaseOptions` | Standard Firebase configuration object (`apiKey`, `projectId`, `authDomain`). |
124124
| **`appName`** | `string` | Name of the Firebase app instance. Defaults to `"[DEFAULT]"`. |
125-
| **`tenantId`** | `string \| ((req: NextRequest) => string \| Promise<string>)` | Google Cloud Identity Platform tenant ID, or a function resolving the tenant dynamically per request. |
125+
| **`tenantId`** | `string` | Google Cloud Identity Platform tenant ID. |
126126
| **`emulator`** | `boolean \| string` | Whether to use the Firebase Auth Emulator. If `true`, reads host from `FIREBASE_AUTH_EMULATOR_HOST`. |
127127
| **`cache`** | `CacheProvider` | Optional custom caching store (e.g. Memorystore, Redis, Vercel KV, Cloudflare KV) for token validation and JWKS caching. Defaults to an in-memory `LRUCache`. |
128128

‎src/nextjs/index.ts‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,10 @@ import {
1010
decodeProtectedHeader,
1111
JSONWebKeySet,
1212
} from "jose";
13+
// getDefaultAppConfig is an internal @firebase/util export with no public stability guarantee.
1314
import { getDefaultAppConfig } from "@firebase/util";
1415
import type { FirebaseOptions } from "firebase/app";
1516

16-
// FYI this code is supposed to be part of the Firebase SDKs soon
17-
// https://github.com/FirebaseExtended/reactfire/pull/640
1817

1918
export type FirebaseJWTPayload = JWTPayload & {
2019
email?: string;
@@ -34,6 +33,7 @@ export interface CacheProvider {
3433
options?: { ex?: number; ttl?: number; nx?: boolean },
3534
): Promise<any> | any;
3635
setex?(key: string, seconds: number, value: any): Promise<any> | any;
36+
del?(key: string): Promise<any> | any;
3737
}
3838

3939
export class MemoryCacheProvider implements CacheProvider {
@@ -54,6 +54,10 @@ export class MemoryCacheProvider implements CacheProvider {
5454
setex(key: string, seconds: number, value: any): any {
5555
return this.set(key, value, { ex: seconds });
5656
}
57+
58+
del(key: string): any {
59+
this.cache.delete(key);
60+
}
5761
}
5862

5963
const defaultMemoryCache = new MemoryCacheProvider();
@@ -100,6 +104,16 @@ async function cacheSetNx(
100104
}
101105
}
102106

107+
async function cacheDelete(cache: CacheProvider, key: string): Promise<void> {
108+
try {
109+
if (typeof cache.del === "function") {
110+
await cache.del(key);
111+
}
112+
} catch (e) {
113+
console.error(`Cache delete failed for key "${key}":`, e);
114+
}
115+
}
116+
103117
const MAX_MAX_AGE = 34560000; // 400 days, https://developer.chrome.com/blog/cookie-max-age-expires
104118

105119
const FIREBASE_AUTH_JWKS_URL =
@@ -149,6 +163,8 @@ async function getFirebaseAuthJwks(
149163
}
150164
}
151165

166+
// `_AuthEmulatorRefreshToken` is an undocumented internal Firebase emulator property.
167+
// It could break if the emulator changes its refresh token format.
152168
export function isEmulatorRefreshToken(refreshToken: string): boolean {
153169
try {
154170
const decoded = decodeURIComponent(refreshToken);
@@ -197,7 +213,7 @@ export async function verifyFirebaseIdToken(
197213

198214
if (jwtPayload) {
199215
const muchEpochWow = Math.floor(+new Date() / 1000);
200-
if (jwtPayload.exp && jwtPayload.exp > muchEpochWow) {
216+
if (jwtPayload.exp !== undefined && jwtPayload.exp > muchEpochWow) {
201217
if (isEmulatedCredential) {
202218
return [jwtPayload, isEmulatedCredential, true];
203219
} else {
@@ -217,9 +233,14 @@ export async function verifyFirebaseIdToken(
217233
if (verifyErr?.code === "ERR_JWKS_NO_MATCHING_KEY") {
218234
const acquired = await cacheSetNx(cache, "firebase:jwks_eviction_lock", 30, "1");
219235
if (acquired) {
220-
// Fetch fresh keys directly, skipping cache read
221-
jwks = await getFirebaseAuthJwks(cache, true);
222-
await jwtVerify(idToken, jwks);
236+
try {
237+
// Fetch fresh keys directly, skipping cache read
238+
jwks = await getFirebaseAuthJwks(cache, true);
239+
await jwtVerify(idToken, jwks);
240+
} catch (fetchErr) {
241+
await cacheDelete(cache, "firebase:jwks_eviction_lock");
242+
throw fetchErr;
243+
}
223244
} else {
224245
throw verifyErr;
225246
}
@@ -344,7 +365,12 @@ export async function runMiddleware(
344365
if (!finalTargetParam) {
345366
return [new NextResponse("Missing finalTarget parameter", { status: 400 })];
346367
}
347-
const url = new URL(finalTargetParam);
368+
let url: URL;
369+
try {
370+
url = new URL(finalTargetParam);
371+
} catch {
372+
return [new NextResponse("Invalid finalTarget parameter", { status: 400 })];
373+
}
348374

349375
let body: ReadableStream<any> | string | null = request.body;
350376

@@ -478,7 +504,7 @@ export async function runMiddleware(
478504
return logout();
479505
}
480506
if (!refreshResponse.ok) {
481-
console.error(refreshUrl.toString(), refreshResponse.status, refreshResponse.statusText);
507+
console.error(refreshUrl.origin + refreshUrl.pathname, refreshResponse.status, refreshResponse.statusText);
482508
return logout();
483509
}
484510
const json = (await refreshResponse.json()) as TokenResponse;
@@ -539,7 +565,6 @@ export const normalizeConfig = (
539565
tenantId: config.tenantId,
540566
cache: config.cache || defaultMemoryCache,
541567
};
542-
// TODO don't override string value
543568
if (typeof config.emulator === "string") {
544569
normalizedConfig.emulatorHost = config.emulator;
545570
} else if (config.emulator !== false) {
@@ -577,7 +602,7 @@ export const composeMiddleware =
577602
cache: defaultMemoryCache,
578603
});
579604
}
580-
if (!normalizedConfigurations.length) throw new Error("");
605+
if (!normalizedConfigurations.length) throw new Error("composeMiddleware requires at least one configuration. Pass a Config object or array of Config objects.");
581606

582607
let defaultUser: FirebaseJWTPayload | undefined = undefined;
583608
const allUsers: Record<string, FirebaseJWTPayload> = {};

‎src/nextjs/middleware_cache.node.test.ts‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
22
import { verifyFirebaseIdToken, runMiddleware, CacheProvider } from "./index";
33
import { NextRequest } from "next/server";
44
import * as jose from "jose";
5+
import { createToken } from "./middleware_test_utils";
56

67
vi.mock("jose", async (importOriginal) => {
78
const actual = await importOriginal<typeof jose>();
@@ -28,12 +29,6 @@ describe("CacheProvider plugin paths", () => {
2829
sub: "user123",
2930
};
3031

31-
const createToken = (header: any, payload: any) => {
32-
const h = Buffer.from(JSON.stringify(header)).toString("base64");
33-
const p = Buffer.from(JSON.stringify(payload)).toString("base64");
34-
return `${h}.${p}.signature`;
35-
};
36-
3732
let mockCacheGet: ReturnType<typeof vi.fn>;
3833
let mockCacheSet: ReturnType<typeof vi.fn>;
3934
let mockCacheSetex: ReturnType<typeof vi.fn>;
@@ -57,7 +52,7 @@ describe("CacheProvider plugin paths", () => {
5752
return null;
5853
});
5954

60-
const token = createToken(validHeader, validPayload);
55+
const token = createToken(validPayload);
6156
const [payload, isEmulated] = await verifyFirebaseIdToken(
6257
token,
6358
mockProjectId,
@@ -81,7 +76,7 @@ describe("CacheProvider plugin paths", () => {
8176
protectedHeader: validHeader,
8277
} as any);
8378

84-
const token = createToken(validHeader, validPayload);
79+
const token = createToken(validPayload);
8580
await verifyFirebaseIdToken(token, mockProjectId, mockTenantId, mockCache);
8681

8782
expect(jose.createLocalJWKSet).toHaveBeenCalledWith({ keys: ["mock-key"] });
@@ -102,7 +97,7 @@ describe("CacheProvider plugin paths", () => {
10297
protectedHeader: validHeader,
10398
} as any);
10499

105-
const token = createToken(validHeader, validPayload);
100+
const token = createToken(validPayload);
106101
await verifyFirebaseIdToken(token, mockProjectId, mockTenantId, mockCache);
107102

108103
expect(mockCacheSetex).toHaveBeenCalledWith("firebase:jwks", 7200, { keys: ["fetched-key"] });
@@ -125,7 +120,7 @@ describe("CacheProvider plugin paths", () => {
125120
.mockRejectedValueOnce(err)
126121
.mockResolvedValueOnce({ payload: validPayload, protectedHeader: validHeader } as any);
127122

128-
const token = createToken(validHeader, validPayload);
123+
const token = createToken(validPayload);
129124
const [payload] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId, mockCache);
130125

131126
expect(mockCacheSet).toHaveBeenCalledWith("firebase:jwks_eviction_lock", "1", {
@@ -139,12 +134,12 @@ describe("CacheProvider plugin paths", () => {
139134
mockCacheGet.mockResolvedValue(null);
140135

141136
const expiredPayload = { ...validPayload, exp: Math.floor(Date.now() / 1000) - 3600 };
142-
const idToken = createToken(validHeader, expiredPayload);
137+
const idToken = createToken(expiredPayload);
143138
const req = new NextRequest("https://localhost:3000/dashboard");
144139
req.cookies.set("__HOST-FIREBASE_app", idToken);
145140
req.cookies.set("__HOST-FIREBASEID_app", "valid-refresh");
146141

147-
const newIdToken = createToken(validHeader, validPayload);
142+
const newIdToken = createToken(validPayload);
148143
fetchMock.mockResolvedValue({
149144
ok: true,
150145
json: async () => ({

‎src/nextjs/middleware_refresh.node.test.ts‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
22
import { runMiddleware } from "./index";
33
import { NextRequest, NextResponse } from "next/server";
44
import * as jose from "jose";
5+
import { createToken } from "./middleware_test_utils";
56

67
// Mock jose
78
vi.mock("jose", async (importOriginal) => {
@@ -31,14 +32,6 @@ describe("runMiddleware Token Refresh", () => {
3132
authDomain: "test.firebaseapp.com",
3233
};
3334

34-
// Helper to create a dummy JWT
35-
const createToken = (payload: any) => {
36-
const header = { alg: "RS256", typ: "JWT" };
37-
const h = Buffer.from(JSON.stringify(header)).toString("base64");
38-
const p = Buffer.from(JSON.stringify(payload)).toString("base64");
39-
return `${h}.${p}.signature`;
40-
};
41-
4235
const validPayload = {
4336
iss: `https://securetoken.google.com/${mockProjectId}`,
4437
aud: mockProjectId,

‎src/nextjs/middleware_test_utils.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
const DEFAULT_HEADER = { alg: "RS256", typ: "JWT" };
2+
3+
export function createToken(payload: any, header: any = DEFAULT_HEADER): string {
4+
const h = Buffer.from(JSON.stringify(header)).toString("base64");
5+
const p = Buffer.from(JSON.stringify(payload)).toString("base64");
6+
return `${h}.${p}.signature`;
7+
}

‎src/nextjs/middleware_verify.node.test.ts‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
22
import { verifyFirebaseIdToken } from "./index";
33
import * as jose from "jose";
4+
import { createToken } from "./middleware_test_utils";
45

56
// Mock jose
67
vi.mock("jose", async (importOriginal) => {
@@ -16,13 +17,6 @@ describe("verifyFirebaseIdToken", () => {
1617
const mockProjectId = "test-project";
1718
const mockTenantId = "test-tenant";
1819

19-
// Helper to create a dummy JWT string (header.payload.signature)
20-
const createToken = (header: any, payload: any) => {
21-
const h = Buffer.from(JSON.stringify(header)).toString("base64");
22-
const p = Buffer.from(JSON.stringify(payload)).toString("base64");
23-
return `${h}.${p}.signature`;
24-
};
25-
2620
const validHeader = { alg: "RS256", typ: "JWT" };
2721
const validPayload = {
2822
iss: `https://securetoken.google.com/${mockProjectId}`,
@@ -43,7 +37,7 @@ describe("verifyFirebaseIdToken", () => {
4337

4438
it("should return payload for a valid token", async () => {
4539
const payload = { ...validPayload, jti: "unique-1" };
46-
const token = createToken(validHeader, payload);
40+
const token = createToken(payload);
4741
const [resultPayload, isEmulated] = await verifyFirebaseIdToken(
4842
token,
4943
mockProjectId,
@@ -62,7 +56,7 @@ describe("verifyFirebaseIdToken", () => {
6256
exp: Math.floor(Date.now() / 1000) - 3600,
6357
jti: "unique-2",
6458
};
65-
const token = createToken(validHeader, expiredPayload);
59+
const token = createToken(expiredPayload);
6660

6761
const [payload, isEmulated] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
6862

@@ -74,7 +68,7 @@ describe("verifyFirebaseIdToken", () => {
7468
it("should return undefined if verification fails", async () => {
7569
vi.mocked(jose.jwtVerify).mockRejectedValue(new Error("Invalid signature"));
7670
const payload = { ...validPayload, jti: "unique-3" };
77-
const token = createToken(validHeader, payload);
71+
const token = createToken(payload);
7872

7973
const [resultPayload, isEmulated] = await verifyFirebaseIdToken(
8074
token,
@@ -88,7 +82,7 @@ describe("verifyFirebaseIdToken", () => {
8882

8983
it("should return undefined for invalid claims (wrong aud)", async () => {
9084
const invalidPayload = { ...validPayload, aud: "wrong-project", jti: "unique-4" };
91-
const token = createToken(validHeader, invalidPayload);
85+
const token = createToken(invalidPayload);
9286

9387
const [payload] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
9488

@@ -97,7 +91,7 @@ describe("verifyFirebaseIdToken", () => {
9791

9892
it("should return undefined for invalid claims (wrong iss)", async () => {
9993
const invalidPayload = { ...validPayload, iss: "https://wrong.issuer.com", jti: "unique-5" };
100-
const token = createToken(validHeader, invalidPayload);
94+
const token = createToken(invalidPayload);
10195

10296
const [payload] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
10397

@@ -110,7 +104,7 @@ describe("verifyFirebaseIdToken", () => {
110104
firebase: { tenant: "wrong-tenant" },
111105
jti: "unique-6",
112106
};
113-
const token = createToken(validHeader, invalidPayload);
107+
const token = createToken(invalidPayload);
114108

115109
const [payload] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
116110

@@ -130,7 +124,7 @@ describe("verifyFirebaseIdToken", () => {
130124
customRole: true,
131125
jti: "tenant-mismatch-dev",
132126
};
133-
const token = createToken(validHeader, tokenTenantPayload);
127+
const token = createToken(tokenTenantPayload);
134128

135129
const [payload] = await verifyFirebaseIdToken(token, mockProjectId, "different-tenant");
136130
expect(payload).toBeUndefined();
@@ -144,7 +138,7 @@ describe("verifyFirebaseIdToken", () => {
144138
customRole: true,
145139
jti: "tenant-mismatch-prod",
146140
};
147-
const token = createToken(validHeader, tokenTenantPayload);
141+
const token = createToken(tokenTenantPayload);
148142

149143
const [payload] = await verifyFirebaseIdToken(token, mockProjectId, "different-tenant");
150144
expect(payload).toBeUndefined();
@@ -154,7 +148,7 @@ describe("verifyFirebaseIdToken", () => {
154148
it("should handle emulator tokens (alg: none)", async () => {
155149
const emulatorHeader = { alg: "none", typ: "JWT" };
156150
const emulatorPayload = { ...validPayload, jti: "unique-7" }; // Valid claims still required
157-
const token = createToken(emulatorHeader, emulatorPayload);
151+
const token = createToken(emulatorPayload, emulatorHeader);
158152

159153
const [payload, isEmulated] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
160154

@@ -166,7 +160,7 @@ describe("verifyFirebaseIdToken", () => {
166160

167161
it("should use cache for subsequent valid requests", async () => {
168162
const payload = { ...validPayload, jti: "unique-cache-test" };
169-
const token = createToken(validHeader, payload);
163+
const token = createToken(payload);
170164

171165
// First call
172166
await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
@@ -193,7 +187,7 @@ describe("verifyFirebaseIdToken", () => {
193187
it("preserves arbitrary custom claims in development", async () => {
194188
vi.stubEnv("NODE_ENV", "development");
195189
const payload = { ...validPayload, customRole: true, jti: "dev-keep" };
196-
const token = createToken(validHeader, payload);
190+
const token = createToken(payload);
197191
vi.mocked(jose.jwtVerify).mockResolvedValue({ payload, protectedHeader: validHeader } as any);
198192

199193
const [result] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
@@ -205,7 +199,7 @@ describe("verifyFirebaseIdToken", () => {
205199
it("preserves arbitrary custom claims in production", async () => {
206200
vi.stubEnv("NODE_ENV", "production");
207201
const payload = { ...validPayload, customRole: true, jti: "prod-keep" };
208-
const token = createToken(validHeader, payload);
202+
const token = createToken(payload);
209203
vi.mocked(jose.jwtVerify).mockResolvedValue({ payload, protectedHeader: validHeader } as any);
210204

211205
const [result] = await verifyFirebaseIdToken(token, mockProjectId, mockTenantId);
@@ -218,7 +212,7 @@ describe("verifyFirebaseIdToken", () => {
218212
it("preserves arbitrary custom claims from cached payload in production", async () => {
219213
vi.stubEnv("NODE_ENV", "production");
220214
const payload = { ...validPayload, customRole: true, jti: "cache-keep-unique" };
221-
const token = createToken(validHeader, payload);
215+
const token = createToken(payload);
222216
vi.mocked(jose.jwtVerify).mockResolvedValue({ payload, protectedHeader: validHeader } as any);
223217

224218
// First call populates the LRU cache

0 commit comments

Comments
 (0)