diff --git a/src/github/app.ts b/src/github/app.ts index 2e447eca14..a42514ce1b 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -192,6 +192,21 @@ export async function withInstallationTokenRetry( } } +/** POST the App-installations access-token endpoint with a given JWT. Extracted so mintInstallationToken can + * issue the same request twice — once with the cached JWT, once with a freshly-signed one on a 401 (#2453). */ +function requestInstallationTokenWithJwt( + jwt: string, + installationId: number, +): Promise { + return timeoutFetch( + `https://api.github.com/app/installations/${installationId}/access_tokens`, + { + method: "POST", + headers: githubHeaders(`Bearer ${jwt}`), + }, + ); +} + /** Mint a fresh installation token (broker or local App-JWT) and cache it. `cached` is the expired/absent prior * entry, consulted only for the brokered stale-token grace. Extracted from createInstallationToken so that * function can single-flight concurrent cold-cache callers onto one mint (see inFlightMints). */ @@ -242,13 +257,35 @@ async function mintInstallationToken( } } const jwt = await createAppJwt(env); - const response = await timeoutFetch( - `https://api.github.com/app/installations/${installationId}/access_tokens`, - { - method: "POST", - headers: githubHeaders(`Bearer ${jwt}`), - }, - ); + let response = await requestInstallationTokenWithJwt(jwt, installationId); + if (response.status === 401) { + // The cached App JWT itself was rejected (a transient GitHub-side validation hiccup, a clock-skew edge case, + // or a brief App suspend/reinstate) while env.GITHUB_APP_PRIVATE_KEY is unchanged. Unlike installation tokens + // (evicted + retried once by withInstallationTokenRetry), the App JWT had no eviction path at all: every mint + // attempt across EVERY installation on the instance kept reusing the SAME poisoned cache entry for up to + // APP_JWT_REUSE_MS (8 minutes), stalling merges/comments/check-runs/approvals fleet-wide (#2453). Evict + retry + // once with a freshly-signed JWT, mirroring withInstallationTokenRetry's identical bounded-once pattern. + console.warn( + JSON.stringify({ + level: "warn", + event: "github_app_jwt_rejected", + appId: env.GITHUB_APP_ID, + status: response.status, + }), + ); + expireCachedAppJwt(env.GITHUB_APP_ID); + const freshJwt = await createAppJwt(env); + response = await requestInstallationTokenWithJwt(freshJwt, installationId); + if (response.status === 401) { + // The freshly-signed retry JWT was ALSO rejected. createAppJwt caches optimistically -- before this POST + // proves the JWT valid -- so without this the just-rejected JWT would sit in the cache and keep poisoning + // every mint fleet-wide for up to APP_JWT_REUSE_MS, exactly the bug this whole retry exists to fix + // (flagged by the gate's own review of #2453). Evict again; the throw below still surfaces this failure to + // the caller, but the NEXT mint attempt (this one or any other installation's) gets a fresh JWT instead of + // replaying the poisoned one. + expireCachedAppJwt(env.GITHUB_APP_ID); + } + } if (!response.ok) { const body = await response.text(); throw new Error( @@ -378,6 +415,13 @@ export async function getRepositoryCollaboratorPermission( const APP_JWT_REUSE_MS = 8 * 60_000; const appJwtCache = new Map(); +/** Evict the cached App JWT for `appId` so the NEXT createAppJwt call re-signs, instead of continuing to serve a + * JWT GitHub just rejected for up to APP_JWT_REUSE_MS more (#2453). Mirrors expireCachedInstallationToken's + * identical eviction-on-rejection pattern for installation tokens. */ +function expireCachedAppJwt(appId: string): void { + appJwtCache.delete(appId); +} + async function createAppJwt(env: Env): Promise { if (!env.GITHUB_APP_PRIVATE_KEY) { throw new Error("GitHub App credentials are not configured."); diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index dcd6c70d50..41ff6fdff4 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -190,6 +190,87 @@ describe("GitHub check runs", () => { expect(mints).toBe(1); }); + it("REGRESSION (#2453): evicts a rejected App JWT and retries the mint once instead of failing outright", async () => { + // Unlike installation tokens (evicted + retried once by withInstallationTokenRetry), the App JWT itself had + // no eviction path before #2453: mintInstallationToken threw straight through on the first non-ok response, + // with no retry attempt at all, leaving the poisoned JWT cached for up to APP_JWT_REUSE_MS (8 min) and + // failing every installation-token mint on the instance in the meantime. Before this fix, mintCalls would be + // 1 and the overall call would reject; after it, exactly one retry recovers the mint. + const privateKey = await generatePrivateKeyPem(); + let mintCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) { + mintCalls += 1; + if (mintCalls === 1) return Response.json({ message: "Bad credentials" }, { status: 401 }); + return Response.json({ token: "fresh-installation-token", expires_at: new Date(Date.now() + 60 * 60_000).toISOString() }); + } + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect(createInstallationToken(env, 777)).resolves.toBe("fresh-installation-token"); + expect(mintCalls).toBe(2); // exactly one bounded retry, not zero (old behavior) or unbounded + }); + + it("REGRESSION (#2453): does not infinite-loop when the retried App JWT is ALSO rejected", async () => { + const privateKey = await generatePrivateKeyPem(); + let mintCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) { + mintCalls += 1; + return Response.json({ message: "Bad credentials" }, { status: 401 }); + } + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect(createInstallationToken(env, 777)).rejects.toThrow(/Failed to create GitHub installation token \(401\)/); + expect(mintCalls).toBe(2); // bounded to exactly one retry, never an unbounded loop + }); + + it("REGRESSION (#2453, second pass — flagged by the gate's own review): evicts the App JWT again when the RETRIED JWT is ALSO rejected, so the NEXT mint attempt does not replay the poisoned JWT", async () => { + // createAppJwt caches optimistically (before the POST proves the JWT valid). Without a second eviction, the + // just-rejected retry JWT from the FIRST createInstallationToken call would sit in the cache and get replayed + // by a SECOND, independent createInstallationToken call — still failing every mint fleet-wide for up to + // APP_JWT_REUSE_MS, exactly the bug the eviction-on-401 fix exists to prevent. Fake timers advance the clock a + // few seconds between the two top-level calls (still far inside the 8-minute reuse window) so a genuinely + // fresh sign produces a different iat and a different Authorization header — RS256 signs an identical JWT for + // an identical iat/exp within the same second, so comparing headers without advancing the clock would be + // flaky (a false pass could occur even without eviction, purely from a same-second coincidence). + vi.useFakeTimers(); + try { + const privateKey = await generatePrivateKeyPem(); + const authHeaders: string[] = []; + let mintCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) { + mintCalls += 1; + authHeaders.push(new Headers(init?.headers).get("authorization") ?? ""); + if (mintCalls <= 2) return Response.json({ message: "Bad credentials" }, { status: 401 }); + return Response.json({ token: "recovered-token", expires_at: new Date(Date.now() + 60 * 60_000).toISOString() }); + } + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect(createInstallationToken(env, 777)).rejects.toThrow(/Failed to create GitHub installation token \(401\)/); + expect(mintCalls).toBe(2); + const retryHeader = authHeaders[1]; + + await vi.advanceTimersByTimeAsync(5_000); // still well inside APP_JWT_REUSE_MS (8 min) — isolates eviction, not TTL expiry + await expect(createInstallationToken(env, 777)).resolves.toBe("recovered-token"); + expect(mintCalls).toBe(3); + // If the retry-rejected JWT had NOT been evicted, this third call would replay the cached (still within its + // reuse window) poisoned JWT, producing the SAME Authorization header as the retry above. + expect(authHeaders[2]).not.toBe(retryHeader); + } finally { + vi.useRealTimers(); + } + }); + it("expires a rejected cached installation token and retries check-run publication once", async () => { const privateKey = await generatePrivateKeyPem(); let mints = 0;