diff --git a/src/github/app.ts b/src/github/app.ts index 881a8c9f90..07ea4df59f 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -69,11 +69,15 @@ export function setGitHubResponseCache( responseCache = cache; } -/** Only cache GETs to the GitHub REST API, and never the token-minting or rate-limit endpoints (volatile / - * per-call). Everything else (PR/file/user/org reads) is safe to dedup for a short window. Exported for tests. */ +/** Only cache safe GETs to the GitHub REST API. Never cache token-minting, rate-limit, or + * authorization/permission endpoints whose response must reflect the live caller context. Exported for tests. */ export function isCacheableGithubUrl(url: string): boolean { if (!url.startsWith("https://api.github.com/")) return false; - return !url.includes("/access_tokens") && !url.includes("/rate_limit"); + if (url.includes("/access_tokens") || url.includes("/rate_limit")) + return false; + return !/\/repos\/[^/]+\/[^/]+\/collaborators\/[^/]+\/permission(?:$|[?#])/.test( + url, + ); } async function timeoutFetch( diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index b1f28c133f..a879f6b447 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -1359,7 +1359,7 @@ describe("self-host Redis token store + GitHub GET response cache", () => { expect(store.has(321)).toBe(true); // written to the external store, not the in-isolate Map }); - it("isCacheableGithubUrl: caches GitHub GETs but not token-mint / rate-limit / non-GitHub URLs", () => { + it("isCacheableGithubUrl: caches safe GitHub GETs but not sensitive endpoints", () => { expect( isCacheableGithubUrl("https://api.github.com/repos/o/r/pulls/1"), ).toBe(true); @@ -1371,9 +1371,53 @@ describe("self-host Redis token store + GitHub GET response cache", () => { expect(isCacheableGithubUrl("https://api.github.com/rate_limit")).toBe( false, ); + expect( + isCacheableGithubUrl( + "https://api.github.com/repos/o/r/collaborators/maintainer/permission", + ), + ).toBe(false); + expect( + isCacheableGithubUrl( + "https://api.github.com/repos/o/r/collaborators/maintainer/permission?ref=live", + ), + ).toBe(false); expect(isCacheableGithubUrl("https://example.com/x")).toBe(false); }); + it("does not serve repository collaborator permissions from the shared response cache", async () => { + const privateKey = await generatePrivateKeyPem(); + 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 permissionFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) + return Response.json({ token: "installation-token" }); + if (url.endsWith("/repos/o/r/collaborators/maintainer/permission")) { + permissionFetches += 1; + return Response.json({ permission: "write" }); + } + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect( + getRepositoryCollaboratorPermission(env, 123, "o/r", "maintainer"), + ).resolves.toBe("write"); + await expect( + getRepositoryCollaboratorPermission(env, 123, "o/r", "maintainer"), + ).resolves.toBe("write"); + + expect(permissionFetches).toBe(2); + expect(store.size).toBe(0); + }); + it("serves a cached GitHub GET on the second call and skips the network", async () => { const privateKey = await generatePrivateKeyPem(); const store = new Map<