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); }); });