Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 51 additions & 7 deletions src/github/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,21 @@ export async function withInstallationTokenRetry<T>(
}
}

/** 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<Response> {
return timeoutFetch(
`https://github.com/ghapi/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). */
Expand Down Expand Up @@ -242,13 +257,35 @@ async function mintInstallationToken(
}
}
const jwt = await createAppJwt(env);
const response = await timeoutFetch(
`https://github.com/ghapi/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(
Expand Down Expand Up @@ -378,6 +415,13 @@ export async function getRepositoryCollaboratorPermission(
const APP_JWT_REUSE_MS = 8 * 60_000;
const appJwtCache = new Map<string, { privateKey: string; jwt: string; expiresAtMs: number }>();

/** 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<string> {
if (!env.GITHUB_APP_PRIVATE_KEY) {
throw new Error("GitHub App credentials are not configured.");
Expand Down
81 changes: 81 additions & 0 deletions test/unit/github-app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading