fix(auth): fail closed on rate limiter errors

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<RateLimitDecision>;
   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),

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,21 +495,24 @@ 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;
       },
       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();
   });