diff --git a/src/security/http/derived-csp-cache.test.ts b/src/security/http/derived-csp-cache.test.ts index f921f87823..dbf2220379 100644 --- a/src/security/http/derived-csp-cache.test.ts +++ b/src/security/http/derived-csp-cache.test.ts @@ -8,6 +8,86 @@ const IMG = ``; afterEach(() => __clearDerivedCspCacheForTests()); describe("security/http/derived-csp-cache", () => { + it("retries after a source read that came back empty", async () => { + // `getAllSourceFiles` returns [] while its own file list is cold and warms + // it asynchronously. Remembering that emptiness pinned a release to the + // bare floor for the life of the pod: every pod is cold on the first + // request after a release, so hosted production projects derived nothing + // at all, and the warm list that arrived a moment later was never read. + let call = 0; + const loadSourceFiles = () => { + call += 1; + return Promise.resolve( + call === 1 + ? [] + : [{ path: "pages/index.tsx", content: '' }], + ); + }; + + const cold = await getDerivedCspOrigins({ + projectScope: "acme", + contentVersion: "rel-1@0", + loadSourceFiles, + }); + assertEquals(cold["img-src"], undefined, "a cold read yields nothing"); + + const warm = await getDerivedCspOrigins({ + projectScope: "acme", + contentVersion: "rel-1@0", + loadSourceFiles, + }); + assertEquals( + warm["img-src"], + ["https://cdn.example.com"], + "same key must re-derive once readable", + ); + assertEquals(call, 2); + }); + + it("does not retry once files were read, even if they yield no origins", async () => { + // The other half: a release whose source genuinely references no external + // origin is immutable for that content version, so it is cached and the + // source is not read again. + let call = 0; + const loadSourceFiles = () => { + call += 1; + return Promise.resolve([{ path: "pages/index.tsx", content: "export default () => null;" }]); + }; + + for (let i = 0; i < 3; i += 1) { + await getDerivedCspOrigins({ + projectScope: "acme", + contentVersion: "rel-2@0", + loadSourceFiles, + }); + } + assertEquals(call, 1, "an answered derivation is computed once"); + }); + + it("retries after the source read throws", async () => { + let call = 0; + const loadSourceFiles = () => { + call += 1; + if (call === 1) return Promise.reject(new Error("adapter not ready")); + return Promise.resolve([{ + path: "a.tsx", + content: '', + }]); + }; + + await getDerivedCspOrigins({ + projectScope: "acme", + contentVersion: "rel-3@0", + loadSourceFiles, + }); + const warm = await getDerivedCspOrigins({ + projectScope: "acme", + contentVersion: "rel-3@0", + loadSourceFiles, + }); + assertEquals(warm["img-src"], ["https://cdn.example.com"]); + }); + it("derives once per content version", async () => { // Derivation reads every file a release pins. Doing that per response would // be absurd; doing it once per immutable content version is exactly right. @@ -128,7 +208,17 @@ describe("security/http/derived-csp-cache", () => { assertEquals(absent, {}); }); - it("caches the empty result too, so a broken adapter is not retried per request", async () => { + it("retries a failed read rather than caching the failure", async () => { + // This deliberately reverses an earlier decision. Caching the empty result + // did avoid re-reading for a broken adapter, but it could not tell a broken + // adapter from a cold one, and the cold case is the common one: every pod + // is cold for a content version on the first request after a release, and + // `getAllSourceFiles` answers [] until its own file list warms up. The + // saving was paid for by the feature not working in production at all. + // + // The cost this reintroduces is small by construction: the empty path is a + // cache lookup that schedules a warmup, not a source read, and concurrent + // callers still collapse onto one attempt through the in-flight map. let loads = 0; const lookup = { projectScope: "proj", @@ -140,7 +230,7 @@ describe("security/http/derived-csp-cache", () => { }; await getDerivedCspOrigins(lookup); await getDerivedCspOrigins(lookup); - assertEquals(loads, 1); + assertEquals(loads, 2); }); it("stays bounded as content versions accumulate", async () => { diff --git a/src/security/http/derived-csp-cache.ts b/src/security/http/derived-csp-cache.ts index a286797be7..290a19aeda 100644 --- a/src/security/http/derived-csp-cache.ts +++ b/src/security/http/derived-csp-cache.ts @@ -108,10 +108,25 @@ async function deriveOnce( projectScope: lookup.projectScope, error: error instanceof Error ? error.message : String(error), }); - return remember(key, EMPTY); + // Deliberately not remembered. See below. + return EMPTY; } - if (!files || files.length === 0) return remember(key, EMPTY); + // An empty read is not an answer, and caching it is the difference between + // this feature working and doing nothing at all. + // + // `getAllSourceFiles` returns [] whenever its own file list is cold, warming + // it asynchronously afterwards. Every pod is cold for a content version on + // the first request after a release, so remembering that emptiness pinned the + // release to the bare floor for the life of the pod -- the warm file list + // that arrived a moment later was never consulted again. Hosted production + // projects, the ones this exists for, saw derivation do nothing at all, while + // preview appeared to work whenever its file list happened to be warm. + // + // So distinguish the two cases: files read and no origins found is immutable + // for the content version and worth caching, while nothing read is a race and + // must be retried. + if (!files || files.length === 0) return EMPTY; const derived = deriveCspOriginsFromSource(files); const count = derived["img-src"]?.length ?? 0;