From 3bc670a4a8f6e90c1d97de0542c5d6e8573012e7 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Sat, 8 Aug 2026 17:29:32 +0200 Subject: [PATCH] fix(security): stop caching an empty CSP derivation Derived origins did nothing for hosted production projects -- the projects the feature exists for. Verified against a probe project whose page references two external image origins: preview served both in `img-src`, production served neither, from the same release and the same source. `getAllSourceFiles` answers `[]` 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 the first request derived nothing -- and that emptiness was written to the per-content-version cache, which is never revisited. The warm file list that arrived a moment later was never read, and the release served the bare floor for the life of the pod. Preview only appeared to work because its file list happened to be warm by the time it was asked. An empty read is not an answer. Files read with no origins found is immutable for a content version and still cached; nothing read is a race and is retried. This reverses a deliberate earlier decision, pinned by a test, that cached the empty result so a broken adapter would not be re-read per request. That saving could not distinguish a broken adapter from a cold one, and the cold case is the common one -- it was paid for with the feature not working at all. The cost coming back is small by construction: the empty path is a cache lookup that schedules a warmup rather than a source read, and concurrent callers still collapse onto one attempt through the in-flight map. --- src/security/http/derived-csp-cache.test.ts | 94 ++++++++++++++++++++- src/security/http/derived-csp-cache.ts | 19 ++++- 2 files changed, 109 insertions(+), 4 deletions(-) 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;