diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index c360062fb9..e3ee5c2668 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -58,11 +58,22 @@ export async function enforceRateLimit(c: Context<{ Bindings: Env }>, routeClass if (!c.env.RATE_LIMITER) return null; const config = CONFIG[routeClass]; const key = await rateLimitKey(c, routeClass); - const id = c.env.RATE_LIMITER.idFromName(key); - const decisionResponse = await c.env.RATE_LIMITER.get(id).fetch("https://rate-limit/check", { - method: "POST", - body: JSON.stringify({ key, ...config }), - }); + let decisionResponse: Response; + try { + const id = c.env.RATE_LIMITER.idFromName(key); + decisionResponse = await c.env.RATE_LIMITER.get(id).fetch("https://rate-limit/check", { + method: "POST", + 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. + console.error(JSON.stringify({ level: "error", event: "rate_limit_check_failed", routeClass, message: error instanceof Error ? error.message : String(error) })); + return null; + } const decision = (await decisionResponse.json().catch(() => ({}))) as Partial; if (decisionResponse.status !== 429) { c.res.headers.set("x-ratelimit-limit", String(decision.limit ?? config.limit)); @@ -70,12 +81,16 @@ 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). await recordAuditEvent(c.env, { eventType: "rate_limit.denied", actor: await actorHint(c), route: c.req.path, outcome: "denied", metadata: { routeClass, retryAfterSeconds: decision.retryAfterSeconds ?? null }, + }).catch((error) => { + console.warn(JSON.stringify({ level: "warn", event: "rate_limit_denied_audit_failed", routeClass, message: error instanceof Error ? error.message : String(error) })); }); return c.json( { diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index daebb66309..d22ea0c725 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -479,6 +479,85 @@ 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 () => { + const throwingLimiter = { + idFromName(name: string) { + return name; + }, + get() { + return { + async fetch() { + throw new Error("Durable Object reset"); + }, + }; + }, + }; + 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(); + + 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 () => { + 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. + throw "not an Error instance"; + }, + }; + }, + }; + 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(); + + 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(); + }); + + it("REGRESSION (#5000): a failed rate_limit.denied audit write does not stop the 429 itself from reaching the caller", async () => { + const deniedEnv = createTestEnv({ RATE_LIMITER: rateLimiterNamespace({ status: 429, body: { resetAt: "2026-05-25T00:04:00.000Z" } }) as unknown as DurableObjectNamespace }); + const realPrepare = deniedEnv.DB.prepare.bind(deniedEnv.DB); + deniedEnv.DB.prepare = ((sql: string) => { + if (/^insert into "?audit_events"?/i.test(sql)) throw new Error("poisoned query"); + return realPrepare(sql); + }) as typeof deniedEnv.DB.prepare; + const warnings = vi.spyOn(console, "warn").mockImplementation(() => undefined); + + const response = await enforceRateLimit(fakeContext(deniedEnv, "/v1/local/branch-analysis", { "cf-connecting-ip": "203.0.113.44" }), "expensive"); + + expect(response?.status).toBe(429); + await expect(response?.json()).resolves.toMatchObject({ error: "rate_limited", routeClass: "expensive" }); + expect(warnings.mock.calls.some(([line]) => typeof line === "string" && line.includes("rate_limit_denied_audit_failed") && line.includes("poisoned query"))).toBe(true); + warnings.mockRestore(); + }); + + it("REGRESSION (#5000): still fails open on a non-Error audit-write rejection", async () => { + const deniedEnv = createTestEnv({ RATE_LIMITER: rateLimiterNamespace({ status: 429, body: { resetAt: "2026-05-25T00:05:00.000Z" } }) as unknown as DurableObjectNamespace }); + const realPrepare = deniedEnv.DB.prepare.bind(deniedEnv.DB); + deniedEnv.DB.prepare = ((sql: string) => { + // Deliberately a non-Error throw -- exercises the `error instanceof Error` false branch in the fail-open catch below. + if (/^insert into "?audit_events"?/i.test(sql)) throw "not an Error instance"; + return realPrepare(sql); + }) as typeof deniedEnv.DB.prepare; + const warnings = vi.spyOn(console, "warn").mockImplementation(() => undefined); + + const response = await enforceRateLimit(fakeContext(deniedEnv, "/v1/local/branch-analysis", { "cf-connecting-ip": "203.0.113.45" }), "expensive"); + + expect(response?.status).toBe(429); + expect(warnings.mock.calls.some(([line]) => typeof line === "string" && line.includes("rate_limit_denied_audit_failed") && line.includes("not an Error instance"))).toBe(true); + warnings.mockRestore(); + }); + it("starts GitHub device flow and rejects malformed provider responses", async () => { const env = createTestEnv({ GITHUB_OAUTH_CLIENT_ID: "client-id" }); vi.stubGlobal("fetch", async () =>