From 8e7a825f51954853076ec2c959aca3fca25a74c2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:17:09 -0700 Subject: [PATCH] fix(rate-limit): close an unauthenticated DoS, a permanently-stuck limiter, and two outbound backoff defects (#9469, #9493, #9494) The inbound webhook bucket was keyed on installation.id read from the body BEFORE signature verification, with no IP component -- so anyone could pin a chosen installation's 10/60s window by POSTing {"installation":{"id":}} ten times a minute. GitHub's real deliveries then 429, and GitHub does not auto-redeliver, so the engine went deaf for that installation at a cost of one unauthenticated request every six seconds. The prior threat note correctly observed that a spoofed id grants no ACCESS; it missed that sharing the bucket IS the attack. Folding the connecting IP into the key keeps #4891's tenant separation (the installation id still splits brokered tenants behind one egress) while ensuring an attacker and GitHub never share a bucket. It also restores headroom #4891 removed by accident: GitHub delivers from many egress IPs, so a CI storm spreads across buckets again instead of contending for a single window. The Redis limiter's INCR-then-conditional-EXPIRE is two round trips: a death, dropped connection, or a failed EXPIRE between them left the key with no TTL, so the window never reset and every request on that key was denied until manual deletion -- with the -1 PTTL fallback papering over it by promising "retry in 60s" forever. One Lua script now makes the increment and its expiry atomic, and repairs an already-TTL-less key so anything stranded by the old path self-heals on next contact. Outbound, getAppInstallation recorded its App-JWT response headers under installation:{id}, but the JWT has its own 5000/hr bucket -- so refresh-installation-health wrote a healthy remaining count over a genuinely exhausted installation's newest observation every 30 minutes, re-admitting parked jobs to burn retries against 403s and re-park. It now records under a distinct app-jwt key. And a secondary-limit Retry-After (typically 60s) was truncated to the 8s inline cap and retried up to three times inside the window GitHub asked us to stay out of, which its own docs warn can extend the block; the response now surfaces so the queue's existing jittered Retry-After handling applies. The inline ladder is jittered too, so concurrent workers no longer retry in lockstep. --- src/auth/rate-limit.ts | 18 ++++- src/github/app.ts | 8 +- src/github/client.ts | 57 ++++++++++++-- src/selfhost/redis-ratelimit.ts | 37 ++++++++- test/unit/auth.test.ts | 27 ++++--- test/unit/github-app.test.ts | 34 ++++++++- test/unit/selfhost-redis-ratelimit.test.ts | 88 ++++++++++++++++------ 7 files changed, 219 insertions(+), 50 deletions(-) diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index 8903e44242..b134c990ea 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -204,7 +204,7 @@ async function actorHint(c: Context<{ Bindings: Env }>): Promise { async function rateLimitIdentity(c: Context<{ Bindings: Env }>): Promise { const ipIdentity = `ip:${await hashToken(clientIp(c))}`; - const installationIdentity = await installationRateLimitIdentity(c); + const installationIdentity = await installationRateLimitIdentity(c, ipIdentity); if (installationIdentity) return installationIdentity; if (isPreAuthRateLimitPath(c.req.path)) return ipIdentity; @@ -222,11 +222,23 @@ async function rateLimitIdentity(c: Context<{ Bindings: Env }>): Promise const INSTALLATION_KEYED_WEBHOOK_PATHS = new Set(["/v1/github/webhook", "/v1/orb/webhook"]); const INSTALLATION_KEYED_ORB_BEARER_PATHS = new Set(["/v1/orb/token", "/v1/orb/relay/register", "/v1/orb/relay/pull"]); -async function installationRateLimitIdentity(c: Context<{ Bindings: Env }>): Promise { +async function installationRateLimitIdentity(c: Context<{ Bindings: Env }>, ipIdentity: string): Promise { const path = c.req.path; if (INSTALLATION_KEYED_WEBHOOK_PATHS.has(path)) { const installationId = await peekWebhookInstallationId(c); - return installationId === null ? null : `installation:${installationId}`; + // #9469: the installation id here is read from the body BEFORE signature verification, so it is + // ATTACKER-CONTROLLED. Keying on it alone let anyone pin a chosen installation's bucket by POSTing + // `{"installation":{"id":}}` 10 times a minute -- GitHub's genuine deliveries then 429, and + // GitHub does not auto-redeliver, so the engine went deaf for that installation at a cost of one + // unauthenticated request every six seconds. The prior threat note correctly observed that a spoofed id + // grants no ACCESS, but sharing the bucket IS the attack. + // + // Folding in the connecting IP means an attacker and GitHub can never occupy the same bucket, while + // #4891's requirement still holds: many brokered tenants behind ONE shared egress no longer collide, + // because the installation id still separates them. It also restores headroom that #4891 removed by + // accident -- GitHub delivers from many egress IPs, so a CI storm now spreads across per-IP buckets + // again instead of contending for a single 10/60s window. + return installationId === null ? null : `installation:${installationId}:${ipIdentity}`; } if (path === "/v1/orb/relay") { // Single-tenant per deployment: the bound enrollment secret (never the request body) IS this deployment's diff --git a/src/github/app.ts b/src/github/app.ts index 53420b44ee..5c859c9366 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -9,6 +9,7 @@ import { clearGitHubResponseCacheForTest, forcedSelfhostMode, githubHeaders, + githubRateLimitAdmissionKeyForAppJwt, githubRateLimitAdmissionKeyForInstallation, makeInstallationOctokit, timeoutFetch, @@ -43,9 +44,11 @@ export { } from "../review/check-names"; export type { CachedGitHubResponse, GitHubResponseCache } from "./client"; export { + exceedsInlineRetryBudget, isCacheableGithubUrl, isRateLimitedResponse, rateLimitRetryMs, + retryAfterMs, setGitHubResponseCache, } from "./client"; export { @@ -403,7 +406,10 @@ export async function getAppInstallation( installationId: number, ): Promise> { const jwt = await createAppJwt(env); - const admissionKey = githubRateLimitAdmissionKeyForInstallation(installationId); + // #9494: this call authenticates with the App JWT, whose rate-limit bucket is separate from the + // installation's -- so its headers must NOT be recorded under `installation:{id}`. See + // githubRateLimitAdmissionKeyForAppJwt's own doc for the 30-minute false-clear this produced. + const admissionKey = githubRateLimitAdmissionKeyForAppJwt(); const path = `/app/installations/${installationId}`; // Deliberately NOT passed as timeoutFetch's githubRateLimitAdmissionKey option here: that option also drives the // GET response-cache key (responseCacheKey, ./client), and `installation:{id}` collides across different calling diff --git a/src/github/client.ts b/src/github/client.ts index 17ae5f608f..c08ee76498 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -121,6 +121,18 @@ export function githubRateLimitAdmissionKeyForPublicToken(): GitHubRateLimitAdmi return "public-token"; } +/** + * #9494: the App JWT has its OWN 5000/hr bucket, entirely separate from any installation token's. Recording a + * JWT-authenticated response's headers under `installation:{id}` (as getAppInstallation did) periodically wrote + * a HEALTHY remaining count over a genuinely exhausted installation's newest observation -- and the admission + * readers take only the newest REST row per key. `refresh-installation-health` runs unconditionally every 30 + * minutes, so an exhausted installation's parked jobs were re-admitted on that cadence, burned their retries + * against 403s, and re-parked, for the whole exhaustion window. + */ +export function githubRateLimitAdmissionKeyForAppJwt(): GitHubRateLimitAdmissionKey { + return "app-jwt"; +} + /** The SINGLE token→admission-key resolver, so every GitHub read attributes consistently and a token can never * travel without its matching key: the public bucket for the shared public token, the installation bucket for an * installation token with a known installation id, else undefined (unattributed). Callers pass whichever token @@ -420,16 +432,43 @@ export async function isRateLimitedResponse(response: Response): Promise GITHUB_RATE_LIMIT_MAX_DELAY_MS; +} + /** How long to wait before the next rate-limit retry: honor a valid Retry-After (seconds), else exponential * backoff — each capped so a review can never stall on one call. A sustained PRIMARY limit (reset up to an hour - * out) simply exhausts the few inline retries and the queue retries the job later. Exported for tests. */ + * out) simply exhausts the few inline retries and the queue retries the job later. Exported for tests. + * + * #9494: jittered. Up to QUEUE_CONCURRENCY workers can trip the same limit within milliseconds of each other; + * a fixed 500/1000/2000 ladder had them all retry in lockstep, concentrating every attempt into the same + * instants. The jitter is deterministic in `attempt` only insofar as it is bounded -- the randomness is what + * spreads the herd, and a rate-limit retry has no replay/determinism contract to preserve. */ export function rateLimitRetryMs(response: Response, attempt: number): number { - const retryAfterHeader = response.headers.get("retry-after"); - if (retryAfterHeader != null) { - const retryAfter = Number(retryAfterHeader); - if (Number.isFinite(retryAfter) && retryAfter >= 0) return Math.min(retryAfter * 1000, GITHUB_RATE_LIMIT_MAX_DELAY_MS); - } - return Math.min(500 * 2 ** attempt, GITHUB_RATE_LIMIT_MAX_DELAY_MS); + const instructed = retryAfterMs(response); + if (instructed !== null) return Math.min(instructed, GITHUB_RATE_LIMIT_MAX_DELAY_MS); + const base = Math.min(500 * 2 ** attempt, GITHUB_RATE_LIMIT_MAX_DELAY_MS); + return Math.min(base + Math.floor(Math.random() * (base / 2)), GITHUB_RATE_LIMIT_MAX_DELAY_MS); } function responseFromCached(hit: CachedGitHubResponse, replayKind: "hit" | "coalesced"): Response { @@ -483,6 +522,10 @@ async function fetchWithGitHubRetry(input: RequestInfo | URL, init?: GitHubTimeo attempt >= GITHUB_RATE_LIMIT_MAX_RETRIES ? "exhausted" : "scheduled", ); if (attempt >= GITHUB_RATE_LIMIT_MAX_RETRIES) break; + // #9494: GitHub asked for longer than we are willing to sleep inline -- surface the response instead of + // retrying inside the window it told us to avoid. The queue's own rate-limit path honors the full + // Retry-After with jitter and defers sibling jobs for the same target. + if (exceedsInlineRetryBudget(response)) break; await sleep(rateLimitRetryMs(response, attempt)); } return response; diff --git a/src/selfhost/redis-ratelimit.ts b/src/selfhost/redis-ratelimit.ts index fd0e8cf8c9..3838f9610f 100644 --- a/src/selfhost/redis-ratelimit.ts +++ b/src/selfhost/redis-ratelimit.ts @@ -10,6 +10,37 @@ interface RateLimitBody { windowSeconds?: number; } +/** + * #9493: INCR-then-conditional-EXPIRE is two round trips, so a process death, a dropped connection, or an + * EXPIRE that simply errored between them left the key with **no TTL**. The fixed window then never resets: + * the counter climbs forever and, once past the limit, every request on that key is denied until someone + * deletes it by hand. The worst key is `strict:/v1/github/webhook:installation:` — a stuck key means + * every webhook delivery for that installation is 429'd indefinitely, and GitHub does not auto-redeliver. + * The old code even papered over the symptom: a `PTTL` of -1 fell back to a full window, so the 429 kept + * promising "retry in 60s" forever. + * + * One script makes the increment and its expiry atomic, and repairs a key that somehow still has no TTL + * (`t < 0`) instead of leaving it immortal — so a key stranded by the old code self-heals on next contact + * rather than needing manual intervention. Returns `[count, ttlMs]`. + */ +const INCREMENT_WINDOW_SCRIPT = ` +local count = redis.call('INCR', KEYS[1]) +if count == 1 then + redis.call('PEXPIRE', KEYS[1], ARGV[1]) +end +local ttl = redis.call('PTTL', KEYS[1]) +if ttl < 0 then + redis.call('PEXPIRE', KEYS[1], ARGV[1]) + ttl = tonumber(ARGV[1]) +end +return { count, ttl } +`; + +async function incrementWindow(redis: Redis, key: string, windowSeconds: number): Promise<[number, number]> { + const reply = (await redis.eval(INCREMENT_WINDOW_SCRIPT, 1, key, String(windowSeconds * 1000))) as [number, number]; + return [Number(reply[0]), Number(reply[1])]; +} + export function createRedisRateLimiter(redis: Redis): DurableObjectNamespace { const stub = { // A DO stub's fetch is called fetch-style: `.fetch(url, init)`. On Workers the runtime builds the Request; @@ -21,9 +52,9 @@ export function createRedisRateLimiter(redis: Redis): DurableObjectNamespace { return Response.json({ error: "invalid_rate_limit_request" }, { status: 400 }); } const k = `ratelimit:${body.key}`; - const count = await redis.incr(k); - if (count === 1) await redis.expire(k, body.windowSeconds); // start the window on first hit - const ttlMs = await redis.pttl(k); + const [count, ttlMs] = await incrementWindow(redis, k, body.windowSeconds); + // PTTL is now guaranteed positive by the script's own repair arm, but keep the fallback: a future + // transport/serialization change that yields a non-numeric reply must not produce a NaN resetAt. const resetMs = ttlMs > 0 ? ttlMs : body.windowSeconds * 1000; const allowed = count <= body.limit; const decision = { diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index 5dffdfdddf..b6391521a2 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -373,11 +373,12 @@ describe("private-beta auth and rate limiting", () => { expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/token:ip:/); }); - it("keys /v1/github/webhook and /v1/orb/webhook by the payload's installation.id, not IP (#4891) -- a shared hosted egress path can't collide two tenants' buckets", async () => { + it("keys /v1/github/webhook and /v1/orb/webhook by installation.id AND the connecting IP (#4891, #9469)", async () => { const observedKeys: string[] = []; const env = rateLimitTestEnv({}, observedKeys); - // Two DIFFERENT installations from the SAME connecting IP (a shared hosted egress) get independent buckets. + // #4891's requirement, unchanged: two DIFFERENT installations from the SAME connecting IP (a shared hosted + // egress) must get independent buckets, so one tenant's traffic can never exhaust another's. await expect( enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 111 } }), "strict"), ).resolves.toBeNull(); @@ -385,10 +386,15 @@ describe("private-beta auth and rate limiting", () => { enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 222 } }), "strict"), ).resolves.toBeNull(); expect(observedKeys).toHaveLength(2); - expect(observedKeys[0]).toBe("strict:/v1/github/webhook:installation:111"); - expect(observedKeys[1]).toBe("strict:/v1/github/webhook:installation:222"); - - // The SAME installation from two DIFFERENT IPs shares one bucket. + expect(observedKeys[0]).not.toBe(observedKeys[1]); + expect(observedKeys[0]).toContain("installation:111"); + expect(observedKeys[1]).toContain("installation:222"); + + // #9469: the SAME installation from two DIFFERENT IPs must NOT share a bucket. installation.id is read from + // the body BEFORE signature verification, so it is attacker-controlled -- when it alone keyed the bucket, + // anyone could pin a victim installation's 10/60s window with unsigned requests and GitHub's real + // deliveries would 429 (and GitHub does not auto-redeliver). Folding in the connecting IP means an attacker + // and GitHub can never occupy the same bucket. observedKeys.length = 0; await expect( enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 333 } }), "strict"), @@ -396,14 +402,17 @@ describe("private-beta auth and rate limiting", () => { await expect( enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "198.51.100.50" }, { installation: { id: 333 } }), "strict"), ).resolves.toBeNull(); - expect(observedKeys[0]).toBe(observedKeys[1]); + expect(observedKeys[0]).not.toBe(observedKeys[1]); + // Both still carry the installation identity -- the tenant separation #4891 added is not lost. + expect(observedKeys[0]).toContain("installation:333"); + expect(observedKeys[1]).toContain("installation:333"); - // /v1/orb/webhook shares the same installation-keying logic. + // /v1/orb/webhook shares the same keying logic. observedKeys.length = 0; await expect( enforceRateLimit(fakeContext(env, "/v1/orb/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 444 } }), "strict"), ).resolves.toBeNull(); - expect(observedKeys[0]).toBe("strict:/v1/orb/webhook:installation:444"); + expect(observedKeys[0]).toMatch(/^strict:\/v1\/orb\/webhook:installation:444:ip:/); }); it("falls back to IP-keying /v1/github/webhook when installation.id can't be resolved (#4891)", async () => { diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 7bd5df90b3..b2708e748c 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -19,6 +19,7 @@ import { isGitHubBadCredentialsError, isGitHubRateLimitedError, isRateLimitedResponse, + exceedsInlineRetryBudget, rateLimitRetryMs, setGitHubResponseCache, setInstallationTokenStore, @@ -2634,8 +2635,13 @@ describe("GitHub check runs", () => { // Unlike a bare fetch, this is now visible to shouldWaitForGitHubRateLimit's admission reads -- before #4506, // this call passed no rate-limit-admission option at all, so it never reached recordGitHubRateLimitObservation. + // #9494: recorded under `app-jwt`, NOT `installation:123`. This call authenticates with the App JWT, whose + // 5000/hr bucket is separate from the installation's -- filing its healthy remaining count under the + // installation key overwrote a genuinely exhausted installation's newest observation every 30 minutes + // (refresh-installation-health's cadence), re-admitting its parked jobs to burn retries against 403s. + // #4506's requirement (the call is VISIBLE to admission reads) is unchanged; only its attribution is fixed. expect(await listLatestGitHubRateLimitObservations(env)).toEqual( - expect.arrayContaining([expect.objectContaining({ path: "/app/installations/123", statusCode: 200, remaining: 4321, admissionKey: "installation:123" })]), + expect.arrayContaining([expect.objectContaining({ path: "/app/installations/123", statusCode: 200, remaining: 4321, admissionKey: "app-jwt" })]), ); }); @@ -3023,9 +3029,29 @@ describe("GitHub rate-limit handling (#ratelimit-resilience)", () => { expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "9999" } }), 0)).toBe(8000); expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "0" } }), 0)).toBe(0); }); - it("falls back to exponential backoff when Retry-After is absent or invalid", () => { - expect(rateLimitRetryMs(new Response("", {}), 2)).toBe(2000); // 500 * 2^2, no header - expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "soon" } }), 0)).toBe(500); // invalid → 500 * 2^0 + it("falls back to jittered exponential backoff when Retry-After is absent or invalid (#9494)", () => { + // Jitter spreads the herd: up to QUEUE_CONCURRENCY workers can trip the same limit within milliseconds, + // and a fixed ladder had them all retry in lockstep. The base is unchanged; the added spread is [0, base/2). + const noHeader = rateLimitRetryMs(new Response("", {}), 2); // base 500 * 2^2 + expect(noHeader).toBeGreaterThanOrEqual(2000); + expect(noHeader).toBeLessThan(3000); + const invalidHeader = rateLimitRetryMs(new Response("", { headers: { "retry-after": "soon" } }), 0); // base 500 * 2^0 + expect(invalidHeader).toBeGreaterThanOrEqual(500); + expect(invalidHeader).toBeLessThan(750); + }); + + it("honors a Retry-After within the inline budget exactly, without jitter (#9494)", () => { + // A server-instructed wait is authoritative -- jitter applies only to our own invented backoff. + expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "3" } }), 0)).toBe(3000); + }); + + it("reports a Retry-After beyond the inline budget so the caller stops retrying inline (#9494)", () => { + // A secondary-limit Retry-After is typically 60s against an 8s inline cap. Retrying inside the window + // GitHub asked us to avoid can extend the block; the queue honors the full wait with jitter instead. + expect(exceedsInlineRetryBudget(new Response("", { headers: { "retry-after": "60" } }))).toBe(true); + expect(exceedsInlineRetryBudget(new Response("", { headers: { "retry-after": "3" } }))).toBe(false); + expect(exceedsInlineRetryBudget(new Response("", {}))).toBe(false); + expect(exceedsInlineRetryBudget(new Response("", { headers: { "retry-after": "soon" } }))).toBe(false); }); }); diff --git a/test/unit/selfhost-redis-ratelimit.test.ts b/test/unit/selfhost-redis-ratelimit.test.ts index 62131ad83e..7b13896951 100644 --- a/test/unit/selfhost-redis-ratelimit.test.ts +++ b/test/unit/selfhost-redis-ratelimit.test.ts @@ -2,20 +2,35 @@ import type { Redis } from "ioredis"; import { describe, expect, it } from "vitest"; import { createRedisRateLimiter } from "../../src/selfhost/redis-ratelimit"; -/** Minimal in-memory stand-in for the ioredis methods the limiter uses. */ -function fakeRedis(): Redis { - const store = new Map(); +/** + * Minimal in-memory stand-in for the ioredis surface the limiter uses. Since #9493 the limiter drives a single + * Lua script rather than INCR + conditional EXPIRE, so this models the two pieces of state the script actually + * reads and writes: the counter and its TTL (-1 for "exists, no TTL", which is the stuck-key shape the script + * repairs). `seedNoTtl` lets a test construct a key stranded by the OLD code path. + */ +function fakeRedis(seed?: { key: string; count: number; ttlMs: number }): Redis { + const counts = new Map(); + const ttls = new Map(); + if (seed) { + counts.set(seed.key, seed.count); + ttls.set(seed.key, seed.ttlMs); + } return { - async incr(k: string) { - const v = (store.get(k) ?? 0) + 1; - store.set(k, v); - return v; + // Mirrors INCREMENT_WINDOW_SCRIPT's semantics, including the ttl < 0 repair arm. + async eval(_script: string, _numKeys: number, key: string, windowMs: string) { + const count = (counts.get(key) ?? 0) + 1; + counts.set(key, count); + if (count === 1) ttls.set(key, Number(windowMs)); + let ttl = ttls.get(key) ?? -1; + if (ttl < 0) { + ttls.set(key, Number(windowMs)); + ttl = Number(windowMs); + } + return [count, ttl]; }, - async expire() { - return 1; - }, - async pttl() { - return 30_000; + // Exposed only so assertions can inspect the resulting TTL state. + async pttl(k: string) { + return ttls.get(k) ?? -2; }, } as unknown as Redis; } @@ -44,21 +59,48 @@ describe("createRedisRateLimiter (#977)", () => { expect(res.status).toBe(400); }); - it("accepts a Request object and handles a missing TTL", async () => { - const noTtl = { - async incr() { - return 1; - }, - async expire() { - return 1; - }, - async pttl() { - return -1; // no expiry set → resetMs falls back to windowSeconds + it("accepts a Request object and handles a non-numeric TTL reply defensively", async () => { + // The script guarantees a positive TTL, but a transport/serialization change that yields a non-numeric + // reply must still produce a sane resetAt rather than NaN -- that fallback arm stays covered. + const oddReply = { + async eval() { + return [1, "not-a-number"]; }, } as unknown as Redis; - const ns = createRedisRateLimiter(noTtl); + const ns = createRedisRateLimiter(oddReply); const req = new Request("https://rl/check", { method: "POST", body: JSON.stringify({ key: "k", limit: 5, windowSeconds: 60 }) }); const res = await ns.get(ns.idFromName("k")).fetch(req); // pass a Request (not url+init) expect(res.status).toBe(200); + const body = (await res.json()) as { resetAt: string }; + expect(Number.isNaN(Date.parse(body.resetAt))).toBe(false); + }); + + // #9493 regression: INCR then a conditional EXPIRE is two round trips. A death, dropped connection, or an + // EXPIRE that merely errored between them left the key with NO TTL, so the fixed window never reset: the + // counter climbed forever and, once past the limit, every request on that key was denied until someone + // deleted it by hand. The worst key is strict:/v1/github/webhook:installation: -- a stuck key means every + // webhook delivery for that installation 429s indefinitely, and GitHub does not auto-redeliver. + it("#9493 sets the window TTL atomically with the first increment", async () => { + const redis = fakeRedis(); + const ns = createRedisRateLimiter(redis); + await ns.get(ns.idFromName("k")).fetch("https://rl/check", { method: "POST", body: JSON.stringify({ key: "k", limit: 5, windowSeconds: 60 }) }); + // A TTL exists immediately after the very first hit -- there is no window in which the key is immortal. + expect(await redis.pttl("ratelimit:k")).toBe(60_000); + }); + + it("#9493 repairs a key that already has no TTL instead of leaving it immortal", async () => { + // A key stranded by the OLD code path: a live counter, already over the limit, with no expiry. + const redis = fakeRedis({ key: "ratelimit:stuck", count: 999, ttlMs: -1 }); + const ns = createRedisRateLimiter(redis); + const res = await ns + .get(ns.idFromName("stuck")) + .fetch("https://rl/check", { method: "POST", body: JSON.stringify({ key: "stuck", limit: 5, windowSeconds: 60 }) }); + + // Still denied on this request (the counter is genuinely over the limit) ... + expect(res.status).toBe(429); + // ... but the key now expires, so the bucket recovers on its own rather than needing manual deletion. + expect(await redis.pttl("ratelimit:stuck")).toBe(60_000); + const body = (await res.json()) as { retryAfterSeconds: number }; + expect(body.retryAfterSeconds).toBeLessThanOrEqual(60); }); });