diff --git a/src/github/client.ts b/src/github/client.ts index a4922816a9..931cae410d 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -60,7 +60,9 @@ type EnvLookup = Record; export type GitHubTimeoutFetchInit = RequestInit & { /** Opt in to using this response's REST bucket headers for self-host queue admission control. */ githubRateLimitAdmission?: boolean; - /** Stable actor key for admission control. Installation-token reads should use the installation id. */ + /** Stable actor key for admission control AND (independent of githubRateLimitAdmission) the response-cache key — + * present whenever it, so a cacheable GET's cache key stays stable across token rotation instead of being + * derived from the raw token. Installation-token reads should use the installation id. */ githubRateLimitAdmissionKey?: string; /** Consulted ONLY when this GET is about to make a NETWORK read (a cache hit is always served first, for free). * Return true to skip the network read — timeoutFetch then resolves to a synthetic 503 so a best-effort caller @@ -236,11 +238,19 @@ async function sha256Short(value: string): Promise { return Array.from(new Uint8Array(digest), (byte) => byte.toString(16).padStart(2, "0")).join("").slice(0, 16); } -async function responseCacheKey(url: string, headers: Headers): Promise { - const authHash = await sha256Short(headers.get("authorization") || ""); +// Prefer the SAME stable per-installation/public-token identity already used for rate-limit admission scoping +// (githubRateLimitAdmissionKeyForToken) over hashing the raw Authorization header. An installation token rotates +// roughly hourly by design (plus on auth failure, plus on every redeploy if not persisted); hashing it means EVERY +// rotation invalidates the ENTIRE cached-response namespace for that installation across all cache classes at once +// — not just entries that are actually stale — even though every entry is still within its own TTL. Keying by the +// stable identity instead means a token rotation no longer touches the cache at all. A caller that doesn't thread +// an admission key falls back to the previous token-hash behavior: still correctly isolated, just without the +// cross-rotation benefit (mirrors the App-JWT-reuse fix for the same class of problem, #1940). (#2538) +async function responseCacheKey(url: string, headers: Headers, admissionKey: GitHubRateLimitAdmissionKey | null): Promise { + const authIdentity = admissionKey ? `key:${admissionKey}` : `auth:${await sha256Short(headers.get("authorization") || "")}`; const accept = encodeURIComponent(headers.get("accept") || ""); const apiVersion = encodeURIComponent(headers.get("x-github-api-version") || ""); - return `v2:${authHash}:${accept}:${apiVersion}:${url}`; + return `v3:${authIdentity}:${accept}:${apiVersion}:${url}`; } type VolatileSingleFlightScope = { requestKey: string; authorization: string }; @@ -286,6 +296,14 @@ function rateLimitAdmissionKey(init: GitHubTimeoutFetchInit | undefined): GitHub return key ? key : null; } +// Deliberately NOT gated on githubRateLimitAdmission (unlike rateLimitAdmissionKey above): a caller may know its +// stable identity and want it used for cache keying without opting into local rate-limit observation for this +// particular call. A blank/whitespace-only key is treated as absent, same as rateLimitAdmissionKey. (#2538) +function cacheKeyAdmissionIdentity(init: GitHubTimeoutFetchInit | undefined): GitHubRateLimitAdmissionKey | null { + const key = init?.githubRateLimitAdmissionKey?.trim(); + return key ? key : null; +} + function requestInitForFetch(init: GitHubTimeoutFetchInit | undefined): RequestInit | undefined { if (!init || (!("githubRateLimitAdmission" in init) && !("githubRateLimitAdmissionKey" in init))) return init; const { githubRateLimitAdmission: _omitted, githubRateLimitAdmissionKey: _omittedKey, ...rest } = init; @@ -493,7 +511,7 @@ export async function timeoutFetch(input: RequestInfo | URL, init?: GitHubTimeou return fetchWithGitHubRetry(input, init); } - const cacheKey = await responseCacheKey(url, headers); + const cacheKey = await responseCacheKey(url, headers, cacheKeyAdmissionIdentity(init)); let hit: CachedGitHubResponse | null = null; try { hit = await responseCache!.get(cacheKey); diff --git a/src/upstream/commit.ts b/src/upstream/commit.ts index d2d9da0c0a..5d7d7d00f9 100644 --- a/src/upstream/commit.ts +++ b/src/upstream/commit.ts @@ -1,4 +1,4 @@ -import { timeoutFetch } from "../github/client"; +import { githubRateLimitAdmissionKeyForPublicToken, timeoutFetch } from "../github/client"; import { shouldWaitForGitHubRateLimit } from "../github/rate-limit"; function upstreamCommitHeaders(token: string | undefined): Record { @@ -38,6 +38,9 @@ export async function resolveUpstreamCommitSha( // read is skipped (→ synthetic non-OK → null → caller falls back to the mutable ref) when the REST budget is // at/below the low-water floor. This callback runs ONLY on a cache miss, so it never suppresses a cache hit. githubSkipNetworkWhen: () => shouldWaitForGitHubRateLimit(env).then(Boolean), + // Give this bare-commit read the same stable, distinctly-scoped cache-key identity every installation-token + // read gets, so the shared public token rotating doesn't reset this cacheable "commit" class either (#2538). + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForPublicToken(), }, ); if (!response.ok) return null; diff --git a/test/unit/github-client.test.ts b/test/unit/github-client.test.ts index b37c37338c..41388af188 100644 --- a/test/unit/github-client.test.ts +++ b/test/unit/github-client.test.ts @@ -443,6 +443,108 @@ describe("timeoutFetch", () => { expect([...store.keys()].filter((key) => key.includes(url))).toHaveLength(2); }); + it("keys a cacheable GET by the stable rate-limit admission identity, surviving a token rotation (#2538)", async () => { + const store = installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json({ fetches: getFetches }); + }); + + const url = "https://api.github.com/repos/o/r"; + const admissionKey = githubRateLimitAdmissionKeyForInstallation(42); + const beforeRotation = await timeoutFetch(url, { + headers: { authorization: "Bearer old-token", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: admissionKey, + }); + // A minted replacement installation token for the SAME installation -- rotation must not evict the entry + // beforeRotation just set, since it's still within its own TTL. + const afterRotation = await timeoutFetch(url, { + headers: { authorization: "Bearer new-token-after-rotation", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: admissionKey, + }); + + expect(await beforeRotation.json()).toEqual({ fetches: 1 }); + expect(await afterRotation.json()).toEqual({ fetches: 1 }); + expect(afterRotation.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBe("hit"); + expect(getFetches).toBe(1); + expect([...store.keys()].filter((key) => key.includes(url))).toHaveLength(1); + expect([...store.keys()].some((key) => key.includes("old-token") || key.includes("new-token-after-rotation"))).toBe(false); + }); + + it("keeps two installations' admission-keyed cache entries isolated even if they briefly share raw token bytes (#2538)", async () => { + const store = installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json({ fetches: getFetches }); + }); + + const url = "https://api.github.com/repos/o/r"; + const first = await timeoutFetch(url, { + headers: { authorization: "Bearer shared-token", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(1), + }); + const second = await timeoutFetch(url, { + headers: { authorization: "Bearer shared-token", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(2), + }); + + expect(await first.json()).toEqual({ fetches: 1 }); + expect(await second.json()).toEqual({ fetches: 2 }); + expect(getFetches).toBe(2); + expect([...store.keys()].filter((key) => key.includes(url))).toHaveLength(2); + }); + + it("scopes the public-token admission key distinctly from an installation-scoped key for the same URL (#2538)", async () => { + installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json({ fetches: getFetches }); + }); + + // The bare-commit "commit" cache class -- the exact shape resolveUpstreamCommitSha reads with the shared + // public token, distinct from an installation-token read of the same URL. + const url = "https://api.github.com/repos/o/r/commits/main"; + const viaInstallation = await timeoutFetch(url, { + headers: { authorization: "Bearer tok", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(7), + }); + const viaPublicToken = await timeoutFetch(url, { + headers: { authorization: "Bearer tok", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForPublicToken(), + }); + + expect(await viaInstallation.json()).toEqual({ fetches: 1 }); + expect(await viaPublicToken.json()).toEqual({ fetches: 2 }); + expect(getFetches).toBe(2); + }); + + it("treats a blank admission key as absent, keeping the auth-hash fallback for keying (#2538)", async () => { + installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json({ fetches: getFetches }); + }); + + const url = "https://api.github.com/repos/o/r"; + const first = await timeoutFetch(url, { + headers: { authorization: "Bearer token-a", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: " ", + }); + const second = await timeoutFetch(url, { + headers: { authorization: "Bearer token-b", accept: "application/vnd.github+json" }, + githubRateLimitAdmissionKey: " ", + }); + + expect(await first.json()).toEqual({ fetches: 1 }); + // A different token with only a blank admission key falls back to auth-hash keying -- still a MISS. + expect(await second.json()).toEqual({ fetches: 2 }); + expect(getFetches).toBe(2); + }); + it("replays pagination and validator headers while dropping rate-limit headers", async () => { installMemoryResponseCache(); let getFetches = 0; diff --git a/test/unit/upstream-commit.test.ts b/test/unit/upstream-commit.test.ts index 45647e8595..0c39151e2b 100644 --- a/test/unit/upstream-commit.test.ts +++ b/test/unit/upstream-commit.test.ts @@ -81,6 +81,23 @@ describe("resolveUpstreamCommitSha — one shared, cached, budget-gated upstream expect(fetchSpy).not.toHaveBeenCalled(); }); + it("keys its cache entry by the stable public-token admission identity, surviving a GITHUB_PUBLIC_TOKEN change (#2538)", async () => { + rateLimitMock.shouldWaitForGitHubRateLimit.mockResolvedValue(undefined); + const store = new Map(); + setGitHubResponseCache({ get: async (k) => store.get(k) ?? null, set: async (k, v) => void store.set(k, v) }); + let fetches = 0; + vi.stubGlobal("fetch", async () => { + fetches += 1; + return Response.json({ sha: "stable-across-rotation" }); + }); + expect(await resolveUpstreamCommitSha(env, config)).toBe("stable-across-rotation"); + // Simulate an operator rotating the shared public token (e.g. a redeploy with a new value) -- the entry the + // first call just set is still within its TTL and must still be served, not re-fetched. + const rotatedEnv = { GITHUB_PUBLIC_TOKEN: "pub-tok-rotated" } as unknown as Env; + expect(await resolveUpstreamCommitSha(rotatedEnv, config)).toBe("stable-across-rotation"); + expect(fetches).toBe(1); + }); + it("fails open (null) on a non-OK status, a missing/empty sha, or a thrown fetch", async () => { rateLimitMock.shouldWaitForGitHubRateLimit.mockResolvedValue(undefined); vi.stubGlobal("fetch", async () => new Response("nope", { status: 404 }));