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
27 changes: 25 additions & 2 deletions src/github/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ export function isForeignAppInstallation(
export function clearInstallationTokenCacheForTest(): void {
installationTokenCache.clear();
externalTokenStore = null;
appJwtCache.clear();
clearGitHubResponseCacheForTest();
}

Expand Down Expand Up @@ -357,19 +358,41 @@ export async function getRepositoryCollaboratorPermission(
return payload.permission ?? null;
}

// The App JWT is valid ~9 min (iat backdated 60s, exp +540s). Re-signing (RS256) it on EVERY call is wasteful CPU
// AND defeats response caching of App-level reads (/app/installations/{id}): the rotating JWT changes the
// auth-scoped response-cache key on every call, so the metadata cache class never hits for its heaviest caller
// (refresh-installation-health / the per-repo backfill). Reuse a minted JWT for a margin of its validity so
// repeated App-JWT reads share ONE signature and ONE stable cache key. A Map keyed by App id — so a process that
// alternates between App identities keeps a JWT per App instead of evicting one for another — with the private key
// held in the entry so a same-App CREDENTIAL ROTATION invalidates immediately and never serves a JWT signed by the
// now-revoked old key (a stale-key JWT would fail every App-level read once the old key is revoked). (#1940)
const APP_JWT_REUSE_MS = 8 * 60_000;
const appJwtCache = new Map<string, { privateKey: string; jwt: string; expiresAtMs: number }>();

async function createAppJwt(env: Env): Promise<string> {
if (!env.GITHUB_APP_PRIVATE_KEY) {
throw new Error("GitHub App credentials are not configured.");
}
const now = Math.floor(Date.now() / 1000);
return signRs256Jwt(
const nowMs = Date.now();
const cached = appJwtCache.get(env.GITHUB_APP_ID);
if (cached && cached.privateKey === env.GITHUB_APP_PRIVATE_KEY && cached.expiresAtMs > nowMs) {
return cached.jwt;
}
const now = Math.floor(nowMs / 1000);
const jwt = await signRs256Jwt(
{
iss: env.GITHUB_APP_ID,
iat: now - 60,
exp: now + 540,
},
env.GITHUB_APP_PRIVATE_KEY,
);
appJwtCache.set(env.GITHUB_APP_ID, {
privateKey: env.GITHUB_APP_PRIVATE_KEY,
jwt,
expiresAtMs: nowMs + APP_JWT_REUSE_MS,
});
return jwt;
}

export async function createOrUpdateCheckRun(
Expand Down
67 changes: 67 additions & 0 deletions test/unit/github-app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1993,6 +1993,73 @@ describe("self-host Redis token store + GitHub GET response cache", () => {
expect([...store.keys()].some((key) => key.includes("Bearer "))).toBe(false);
});

it("reuses the App JWT within its window so metadata reads keep cache-hitting despite rotation (#1940)", async () => {
const privateKey = await generatePrivateKeyPem();
const rotatedKey = await generatePrivateKeyPem();
vi.useFakeTimers();
try {
const store = new Map<string, { status: number; body: string; contentType: string }>();
setGitHubResponseCache({ get: async (u) => store.get(u) ?? null, set: async (u, v) => void store.set(u, v) });
let fetches = 0;
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
if (input.toString().endsWith("/app/installations/42")) {
fetches += 1;
return Response.json({ id: 42, account: { login: "JSONbored" } });
}
return new Response("not found", { status: 404 });
});
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey });
await getAppInstallation(env, 42); // cache empty → mint JWT → network fetch
vi.advanceTimersByTime(90_000); // 90s later: a freshly-minted JWT would rotate (new iat) and MISS the cache
await getAppInstallation(env, 42);
expect(fetches).toBe(1); // JWT reused → stable auth-scoped key → served from the response cache

// A same-App private-KEY rotation must re-mint immediately — never serve a JWT signed by the old, now-revoked
// key (which would fail every App-level read once GitHub rejects the old key). Still inside the reuse window.
const rotated = createTestEnv({ GITHUB_APP_PRIVATE_KEY: rotatedKey }); // same App id, new key
await getAppInstallation(rotated, 42);
expect(fetches).toBe(2); // key changed → cache invalid → re-mint → new auth key → cache miss → network fetch

vi.advanceTimersByTime(9 * 60_000); // past the reuse window → the JWT is re-minted
await getAppInstallation(rotated, 42);
expect(fetches).toBe(3);

// A different App id never reuses another App's cached JWT.
const otherApp = createTestEnv({ GITHUB_APP_PRIVATE_KEY: rotatedKey, GITHUB_APP_ID: "999999" });
await getAppInstallation(otherApp, 42);
expect(fetches).toBe(4);
} finally {
vi.useRealTimers();
}
});

it("keeps a per-App JWT so alternating App identities do not evict each other (#1940)", async () => {
const keyA = await generatePrivateKeyPem();
const keyB = await generatePrivateKeyPem();
vi.useFakeTimers();
try {
const store = new Map<string, { status: number; body: string; contentType: string }>();
setGitHubResponseCache({ get: async (u) => store.get(u) ?? null, set: async (u, v) => void store.set(u, v) });
let fetches = 0;
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
if (input.toString().endsWith("/app/installations/7")) {
fetches += 1;
return Response.json({ id: 7, account: { login: "JSONbored" } });
}
return new Response("not found", { status: 404 });
});
const appA = createTestEnv({ GITHUB_APP_PRIVATE_KEY: keyA }); // App 3824093
const appB = createTestEnv({ GITHUB_APP_PRIVATE_KEY: keyB, GITHUB_APP_ID: "555" });
await getAppInstallation(appA, 7); // mint A → fetch (caches /7 under A's JWT)
await getAppInstallation(appB, 7); // mint B → fetch
vi.advanceTimersByTime(30_000); // a re-mint here would rotate the JWT (new iat) and miss the cache
await getAppInstallation(appA, 7); // A still cached in the Map → reuse A's JWT → response-cache HIT → no fetch
expect(fetches).toBe(2); // A was NOT evicted by B — a single-entry cache would re-mint A → cache miss → fetch
} finally {
vi.useRealTimers();
}
});

it("does not cache a non-200 GitHub GET", async () => {
const privateKey = await generatePrivateKeyPem();
const store = new Map<
Expand Down
Loading