From a5a449284fcee92480b472e7c27badd36ce25398 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 03:33:09 -0700 Subject: [PATCH] fix(github): key the response cache by installation identity, not the raw token Closes #2538. The self-host GitHub response cache derived its Redis key from a hash of the literal Authorization header, so every installation-token rotation (roughly hourly by design, plus on auth failure, plus on redeploy if the token isn't persisted) invalidated the entire cached-response namespace for that installation across every cache class at once, even though the underlying resources hadn't changed and every entry was still within its own TTL. Key by the same stable per-installation/public-token identity already used for rate-limit admission scoping instead, via the existing githubRateLimitAdmissionKey plumbing. 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. Also fixes the one caller authenticating with the shared public token instead of an installation token (resolveUpstreamCommitSha) to pass its own distinctly- scoped key, so it gets the same benefit without colliding with any installation-scoped entry. --- src/github/client.ts | 28 ++++++-- src/upstream/commit.ts | 5 +- test/unit/github-client.test.ts | 102 ++++++++++++++++++++++++++++++ test/unit/upstream-commit.test.ts | 17 +++++ 4 files changed, 146 insertions(+), 6 deletions(-) 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 }));