diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index e3ee5c266..6ce21e10d 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -66,13 +66,19 @@ export async function enforceRateLimit(c: Context<{ Bindings: Env }>, routeClass body: JSON.stringify({ key, ...config }), }); } catch (error) { - // Fail OPEN (#5000): this middleware runs on every route ahead of the handler's own try/catch, and no - // app.onError is registered anywhere -- an uncaught Durable Object hiccup (eviction, migration, a - // rolling-deploy blip) previously escaped as Hono's bare, unstructured 500 for whatever route the caller - // happened to be hitting, indistinguishable from a real application bug in that route. The rate limiter - // exists to protect the app, not crash the request it's supposed to be gating. + // Fail closed with a structured response (#5000): limiter outages should not escape as Hono's + // bare 500, but they also must not disable the global abuse-control middleware for strict or + // expensive routes while authentication and route handlers continue. console.error(JSON.stringify({ level: "error", event: "rate_limit_check_failed", routeClass, message: error instanceof Error ? error.message : String(error) })); - return null; + return c.json( + { + error: "rate_limiter_unavailable", + routeClass, + retryAfterSeconds: 5, + }, + 503, + { "retry-after": "5" }, + ); } const decision = (await decisionResponse.json().catch(() => ({}))) as Partial; if (decisionResponse.status !== 429) { @@ -81,8 +87,8 @@ export async function enforceRateLimit(c: Context<{ Bindings: Env }>, routeClass if (decision.resetAt) c.res.headers.set("x-ratelimit-reset", decision.resetAt); return null; } - // Best-effort: the 429 itself must still reach the caller even if this audit write fails (#5000, same - // fail-open reasoning as the DO call above). + // Best-effort: the 429 itself must still reach the caller even if this audit write fails (#5000); + // audit durability should not turn an already-denied request into a framework 500. await recordAuditEvent(c.env, { eventType: "rate_limit.denied", actor: await actorHint(c), diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index d22ea0c72..5f566a781 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -479,7 +479,7 @@ describe("private-beta auth and rate limiting", () => { expect(JSON.parse(tokenAudit?.metadata_json ?? "{}")).toMatchObject({ retryAfterSeconds: 17 }); }); - it("REGRESSION (#5000): fails OPEN when the rate-limit Durable Object itself throws, instead of crashing the request with a bare framework 500", async () => { + it("REGRESSION (#5000): fails closed with a structured 503 when the rate-limit Durable Object itself throws", async () => { const throwingLimiter = { idFromName(name: string) { return name; @@ -495,13 +495,16 @@ describe("private-beta auth and rate limiting", () => { const env = createTestEnv({ RATE_LIMITER: throwingLimiter as unknown as DurableObjectNamespace }); const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); - await expect(enforceRateLimit(fakeContext(env, "/v1/orb/token", { "cf-connecting-ip": "203.0.113.9" }), "strict")).resolves.toBeNull(); + const response = await enforceRateLimit(fakeContext(env, "/v1/orb/token", { "cf-connecting-ip": "203.0.113.9" }), "strict"); + expect(response?.status).toBe(503); + expect(response?.headers.get("retry-after")).toBe("5"); + await expect(response?.json()).resolves.toMatchObject({ error: "rate_limiter_unavailable", routeClass: "strict", retryAfterSeconds: 5 }); expect(errors.mock.calls.some(([line]) => typeof line === "string" && line.includes("rate_limit_check_failed") && line.includes("\"routeClass\":\"strict\""))).toBe(true); errors.mockRestore(); }); - it("REGRESSION (#5000): still fails open when the Durable Object rejects with a non-Error value", async () => { + it("REGRESSION (#5000): still fails closed when the Durable Object rejects with a non-Error value", async () => { const throwingLimiter = { idFromName(name: string) { return name; @@ -509,7 +512,7 @@ describe("private-beta auth and rate limiting", () => { get() { return { async fetch() { - // Deliberately a non-Error rejection -- exercises the `error instanceof Error` false branch in the fail-open catch below. + // Deliberately a non-Error rejection -- exercises the `error instanceof Error` false branch in the structured fail-closed catch below. throw "not an Error instance"; }, }; @@ -518,8 +521,10 @@ describe("private-beta auth and rate limiting", () => { const env = createTestEnv({ RATE_LIMITER: throwingLimiter as unknown as DurableObjectNamespace }); const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); - await expect(enforceRateLimit(fakeContext(env, "/v1/orb/token", { "cf-connecting-ip": "203.0.113.10" }), "strict")).resolves.toBeNull(); + const response = await enforceRateLimit(fakeContext(env, "/v1/orb/token", { "cf-connecting-ip": "203.0.113.10" }), "strict"); + expect(response?.status).toBe(503); + await expect(response?.json()).resolves.toMatchObject({ error: "rate_limiter_unavailable", routeClass: "strict", retryAfterSeconds: 5 }); expect(errors.mock.calls.some(([line]) => typeof line === "string" && line.includes("rate_limit_check_failed") && line.includes("not an Error instance"))).toBe(true); errors.mockRestore(); });