Add CDN cache headers and a byte-bounded negative cache

[claude]

Requested by John Kleinschmidt · Slack thread

Before

• Responses carried no Cache-Control headers, so Cloudflare's edge cache and downstream clients could not cache anything: every symbol lookup — including lookups for symbols that don't exist — hit the origin dyno and the upstream store.
• The in-memory "missing symbol" cache was capped at 10,000 entries with no TTL, far too small for flood traffic, and it also stored an entry for every hit (as false), wasting slots on answers the cache's absence would give anyway.
• An upstream error against a client that had already disconnected could throw while writing the 500 response, producing uncaughtException noise in the logs.

After

• Negative caching, sized for floods: known-missing symbols are cached in memory under a 32 MiB byte budget (roughly 70k entries) with a 1-hour TTL, and only genuine misses are stored. Repeat lookups for missing symbols are answered as 404 without touching upstream.
• Edge-cacheable responses: negative-cache 404s and 403→404 rewrites are served with Cache-Control: public, max-age=3600; successful 200s get a long immutable Cache-Control when upstream sends none (upstream's own header is preserved when present); the 302 redirect path (triggered by a symbolicator user-agent or the x-electron-symbol-redirect header) sends Cache-Control: no-store plus Cloudflare-CDN-Cache-Control: public, max-age=3600, so browsers and generic shared caches never store the redirect while Cloudflare — the only cache whose key separates the redirect cohorts from ordinary traffic (electron/infra#313) — still caches it at the edge for an hour.
• Quieter error handling: the proxy error handler no longer attempts to write a 500 into a response that is already destroyed, finished, or has sent headers.

How

• The negative cache uses lru-cache@6's length calculator to make max a total-byte budget, charging key.length + 384 per entry — the overhead constant was measured with node --expose-gc against real heap usage, since charging the key length alone undercounts by roughly 5x.
• Cache-Control headers for proxied responses are set in the existing writeHead wrapper in the proxyReq hook; the redirect and negative-cache paths set theirs directly.
• Tests cover the 404/200/redirect header behavior and preservation of upstream Cache-Control.

Scope note: earlier revisions of this PR added upstream concurrency limiting, request dedup, and disconnect cancellation; those were dropped once Cloudflare (WAF + rate limit + edge cache) took over origin protection — see the review thread for the history.

[claude]

Thanks @jkleinsc — both findings confirmed and fixed in 552d9f0.

Concurrency cap bypass (same-path waiters): Reproduced with a probe against the previous commit: MAX_UPSTREAM_CONCURRENCY=2, one in-flight leader plus six dedup waiters on the same path. When the leader completed successfully, all six waiters proxied — the upstream saw 6 simultaneous requests (7 total) against a cap of 2, exactly as described, because each waiter overwrote the same inFlightRequests key so Map.size stayed at 1, and the first close deleted the shared entry while the rest were still in flight.

Fix: the cap is now enforced against a dedicated activeUpstreamRequests counter — incremented only when a proxy actually starts, decremented exactly once via a settle guard shared by the close and error handlers. Woken waiters go back through proxyToUpstream, which re-checks the counter and sheds with 503 + Retry-After when at capacity. The Map is kept for dedup only; an entry is registered only if the key is absent and removed only by the request that registered it, so concurrent same-path proxies can't drop each other's entries. Same probe after the fix: max 2 simultaneous, 3 upstream requests total (leader + 2 waiters), remaining 4 waiters shed with 503. Added as a regression test (same-path dedup waiters cannot bypass the upstream concurrency cap).

Negative cache footprint: Confirmed the cache inserted an entry for every upstream response — false for hits and errors (dead weight; get returning undefined behaves identically) — with a 500k entry-count bound. Measured a full cache of 500k representative rewritten paths (~96 bytes each) at ~190 MiB of V8 heap, in line with the reported ~193 MiB.

Fix: entries are now stored only for genuine misses (the 403→404 rewrite), and the cache is bounded by bytes instead of entry count — lru-cache@6 treats max as a total-length budget when a length calculator is provided, so it's now max: 32 MiB with length = the key's string length (the boolean value is negligible). Since rewritten paths run ~60–100 bytes, that's roughly 300–500k entries worst case but bounded in bytes either way; the 1h TTL is unchanged.

Full suite passes: 36/36 (including the new regression test).

---

Generated by Claude Code

[claude]

Confirmed both findings, thanks — fixed in 5dcfbc5.

Canceled dedup waiter leaks an upstream slot. Reproduced with your exact recipe (MAX_UPSTREAM_CONCURRENCY=1, leader held upstream, one same-path waiter whose client socket is destroyed mid-wait, leader completed, then a distinct path probed): the probe got a 503 while only the leader had ever reached upstream, and every subsequent probe kept 503ing — the slot was gone until restart. Root cause as you described: the woken waiter's response close fires on client disconnect before proxying, so the listeners registered in proxyToUpstream never fire and activeUpstreamRequests is never decremented. Fix:

• A clientGone(req, res) guard (req.destroyed / res.destroyed / res.writableEnded / dead socket) now runs both when a waiter wakes and at the top of proxyToUpstream, so requests whose client already hung up are dropped before the counter is touched — this covers fresh requests too.
• Belt and braces for the race between that check and listener registration: after incrementing and registering close/error, if the client is gone, settle() is invoked by hand. settle is idempotent (single settled flag shared by the listeners and the manual call), so a double decrement is impossible.
• Added a regression test (a dedup waiter whose client disconnects mid-wait does not leak an upstream slot) that replays your recipe and asserts the distinct-path probe is proxied (200) with exactly leader + probe reaching upstream. It fails against the previous commit and passes now. While wiring it up I found the leaked proxied connection also kept the fake upstream's server.close() from resolving, hanging the runner's after-hooks — the test helper now calls closeAllConnections() on close so the bug fails the test instead of wedging it.

Byte budget vs. real heap. Reproduced and measured with node --expose-gc, representative 96-char keys, filled to steady-state eviction: the old length: key.length config admitted 349,525 entries / 32 MiB accounted / ~160 MiB heapUsed (~482 real bytes/entry — same shape as your 325k / ~131 MiB, a bit worse on this Node build). The overhead beyond the key string is therefore ~384 bytes per entry (lru-cache linked-list node + Map entry + V8 string/object headers). The cache now charges key.length + 384 against the same 32 MiB budget; measured full-cache result: 69,905 entries / 32 MiB accounted / ~34.1 MiB heapUsed. The measured basis is documented in a comment next to MISSING_CACHE_ENTRY_OVERHEAD_BYTES.

Full suite: 37/37 passing.

---

Generated by Claude Code

[claude]

Confirmed, thanks @jkleinsc — fixed in 38877cc.

Repro. Your exact scenario against the previous head (5dcfbc5), with a fake upstream that counts simultaneous active requests and records aborts (a response close before writableEnded): MAX_UPSTREAM_CONCURRENCY=1, request A reaches the held upstream, A's client disconnects, then a distinct request B is sent.

	before (5dcfbc5)	after (38877cc)
upstream active after A's client disconnects	1 (A kept running)	0 (A canceled)
aborts observed by upstream	0	1
max simultaneous upstream requests once B arrives	2	1

Exactly as you described: the slot counter and dedup promise settled on the downstream close, but http-proxy never cancels the upstream request on its own — its req.on('aborted') hook (web-incoming.js) doesn't fire on modern Node for a request whose body was already fully received, i.e. every GET — so B was admitted while A's upstream fetch was still running.

Fix — lifecycle tied to the actual upstream request (your suggested direction, shipped as the full cancel, not the hold-only fallback):

• The shared proxyReq hook captures the outgoing request and maps it back to the right downstream request via a WeakMap keyed by the incoming req (no cleanup bookkeeping; entries die with the request).
• Downstream close/error now explicitly destroys the captured proxyReq, so the upstream work is actually canceled — the fake upstream observes the abort (table above). The destroy is guarded (destroyed check + try/catch) so it's a no-op on an already-finished request.
• The slot and dedup promise settle on the upstream request's close, which fires both when the proxied response has been fully read (normal completion) and after a destroy/error — both paths route to the same idempotent settle. The dedup promise therefore stays pending until the upstream request has ended or been aborted, so woken waiters can't land in a still-occupied slot.
• If no proxyReq was ever captured for a response (never reached proxy.web, or http-proxy skips the proxyReq event for Expect: 100-continue requests), downstream close settles immediately as before, so that edge can't leak a slot; a late capture then destroys the upstream request on arrival.
• On client abort, http-proxy surfaces the teardown as econnreset (unlistened, harmless) or error; the error handler now declines to write the 500 when the response is already destroyed/finished, instead of throwing into uncaughtException.

Client-gone pre-checks, 503 shedding, miss caching, and the Cache-Control behavior are untouched. Added a regression test for the scenario above (client disconnect mid-proxy aborts upstream and frees the slot only after): it asserts upstream observes the abort and never sees 2 simultaneous active requests; it fails against 5dcfbc5 (times out waiting for the abort) and passes now.

Full suite: 38/38 passing (3 consecutive runs).

---

Generated by Claude Code

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/index.ts:203-208 — The sampled request-log sanitizer escapes quotes in the User-Agent before escaping pre-existing backslashes, so a UA like x\"malicious=value sanitizes to x\\"malicious=value — two backslashes followed by a bare quote. A consumer applying standard backslash-escape decoding would read that as an escaped backslash followed by an unescaped closing quote, letting the value break out of the quoted ua="..." field, contradicting the function's own comment. Fix by escaping backslashes before quotes: .replace(/\\/g, "\\\\").replace(/"/g, "\\\"").

  Extended reasoning...

  sampleRequestLog in src/index.ts (lines 203-208) sanitizes the User-Agent header with .replace(/[\r\n]+/g, ' ').replace(/"/g, '\\"') before embedding it in a log line as ua="...". This strips newlines (good — no cross-line log injection) and escapes literal quote characters, but it never escapes literal backslashes that were already present in the raw header, and the escaping is applied in the wrong order relative to how backslash-escaped strings are normally decoded.

  Concretely, for a User-Agent whose raw bytes are x\"malicious=value (a literal backslash immediately followed by a literal quote), the sanitizer only touches the quote: it inserts a backslash in front of it, producing x\\"malicious=value — i.e. the original backslash, plus a newly-inserted backslash, plus the quote. Embedded in the log line this reads as ua="x\\"malicious=value". Any consumer that parses this with standard C/JSON-style backslash-escape semantics (where \\ decodes to one literal backslash and \" decodes to an escaped quote) will consume the two backslashes as a single escaped-backslash token, then treat the very next " as the real closing delimiter of the field. That terminates the ua value early as x\ and spills malicious=value" onto the line as unquoted trailing text — the User-Agent value has broken out of its quoted field.

  This directly contradicts the header comment directly above the function, which states 'the ua="..." field always stays on one line and cannot be broken out of.' The newline-stripping half of that claim holds, but the quote-containment half does not, because the two .replace() calls don't compose safely: escaping the delimiter (quote) without first escaping the escape character (backslash) means any pre-existing backslash adjacent to a quote silently gets swallowed by whatever unescaping the consumer applies.

  Step-by-step proof:

  1. Attacker sends a request with header User-Agent: x\"malicious=value.
  2. sampleRequestLog runs .replace(/[\r\n]+/g, ' ') — no newlines present, no change: x\"malicious=value.
  3. It then runs .replace(/"/g, '\\"'), which finds the one " and prepends a backslash to it, yielding x\\"malicious=value (backslash, backslash, quote, then the rest).
  4. The line is logged as: request-sample method=GET path=/foo disposition=proxied ua="x\\"malicious=value".
  5. A downstream parser applying standard backslash-unescape rules reads \\ as one literal backslash, then hits the bare " and treats it as the field terminator. It extracts ua = "x\" and is left with malicious=value" as unexpected trailing content on the line, rather than the full intended value.

  The correct fix is to escape backslashes before quotes, e.g. userAgent.replace(/[\r\n]+/g, ' ').replace(/\\/g, '\\\\').replace(/"/g, '\\"'), which is the standard order for composing these two escapes safely.

  Impact is real but modest: this is a newly-introduced function guarded by REQUEST_LOG_SAMPLE_RATE (~1/1000 requests), the newline strip already prevents forging an entirely new log line, and there's no known strict machine parser currently consuming these greppable lines with backslash-escape semantics — so today the practical consequence is limited to potential field-boundary confusion on an occasional sampled line, not log-line forgery or a crash. It's a one-line, low-risk fix worth making since it falsifies a stated invariant in the code's own comment, but it doesn't block merge.

• 🔴 src/index.ts:160-176 — A client GET with an Expect: 100-continue header permanently suppresses http-proxy's proxyReq event (it's only emitted when !proxyReq.getHeader('expect')), so lifecycle.proxyReq never gets set. If that client disconnects mid-flight, onDownstreamGone's else branch calls settle() directly instead of aborting the upstream request — freeing the concurrency slot and dedup entry while the real request to TARGET_HOST keeps running uncounted and uncancelled. Repeated with concurrent Expect:100-continue GETs, this lets an attacker bypass MAX_UPSTREAM_CONCURRENCY entirely, the core protection this PR adds.

  Extended reasoning...

  The bug

  http-proxy's web-incoming.js only emits the proxyReq event from inside proxyReq.on('socket', ...), guarded by if (server && !proxyReq.getHeader('expect')) (confirmed at /opt/node-tools/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.js:132-133). common.setupOutgoing copies req.headers verbatim onto the outgoing request (outgoing.headers = extend({}, req.headers)), so a client-supplied Expect: 100-continue header on a GET is forwarded upstream unchanged and permanently suppresses that event for the life of the request — not delayed, never fired.

  proxy.on('proxyReq', ...) in src/index.ts (lines 149-176) is the only place lifecycle.proxyReq is assigned (line 166) and the only place proxyReq.on('close', lifecycle.settle) is registered (line 172). If the event never fires, lifecycle.proxyReq stays null for the entire lifetime of the request.

  The triggering path

  1. proxyToUpstream increments activeUpstreamRequests, creates the lifecycle object with proxyReq: null, registers res.on('close', onDownstreamGone) / res.on('error', onDownstreamGone), then calls proxy.web(req, res, ...).
  2. Because the request carries Expect: 100-continue, the proxyReq event is never emitted, so lifecycle.proxyReq remains null even after the outgoing request has actually been dispatched to TARGET_HOST.
  3. The client disconnects while the upstream fetch is still in flight. res's 'close' event fires immediately, invoking onDownstreamGone.
  4. Inside onDownstreamGone, lifecycle.proxyReq is null, so it takes the else branch and calls settle() directly — the code's own comment says "Nothing to cancel; settle now so the slot cannot leak," which is true for the "never reached proxy.web" case but false here: there is a live, uncounted request against origin.
  5. settle() decrements activeUpstreamRequests and deletes the dedup map entry immediately. Nothing calls abortUpstreamRequest because no proxyReq was ever captured — the real outbound TCP connection to TARGET_HOST keeps running to completion, fully outside the counter's visibility.

  Why nothing else saves this

  This PR's own prior fix (commit 38877cc) already established that http-proxy's req.on('aborted') never fires on modern Node for a GET whose body was fully received — that's exactly why explicit proxyReq.destroy() plumbing was added. That plumbing depends entirely on proxyReq having been captured via the 'proxyReq' event, which is precisely what Expect: 100-continue suppresses. So this bug sits in the one gap that fix didn't anticipate.

  Impact and proof

  An attacker sends N concurrent GETs to distinct paths (or paths guaranteed to miss the negative cache), each with Expect: 100-continue, and disconnects the client socket immediately after the upstream fetch is dispatched (e.g. right after upstream.requests.length increments in a probe, or heuristically after a short delay in the wild). For each one:

  • activeUpstreamRequests goes 0 → 1 → 0 within milliseconds (increment on dispatch, decrement on the disconnect-triggered settle()).
  • The real HTTPS request to TARGET_HOST is still in flight, uncounted.

  Repeating this in a tight loop keeps activeUpstreamRequests near zero — so the >= UPSTREAM_CONCURRENCY_LIMIT shed check never trips — while real outbound connections to origin accumulate without bound (default Node https agent has maxSockets: Infinity). This is a direct, deterministic, attacker-triggerable bypass of MAX_UPSTREAM_CONCURRENCY, which is the specific defense this PR was written to add against the flood described in the PR description.

  Suggested fix

  Strip the Expect header before calling proxy.web (e.g. delete req.headers.expect in proxyToUpstream before dispatch), which makes the proxyReq event fire normally for every request and lets the existing lifecycle/abort logic work unmodified. Alternatively, capture the outgoing request through a different mechanism that does not depend on the proxyReq event.

[claude]

Both findings in the automated review above (the sampled request-log sanitizer escape order and the Expect: 100-continue / MAX_UPSTREAM_CONCURRENCY bypass) refer to code that was removed in 9f57a4e when this PR was slimmed down to CDN cache headers plus the byte-bounded negative cache. Neither sampleRequestLog nor the upstream concurrency/lifecycle machinery exists on the current branch head, so these findings do not apply to the current diff.

---

Generated by Claude Code

[claude]

Confirmed, @jkleinsc — fixed in 9c6ed74 here plus a companion commit on electron/infra#313.

The finding holds. The redirect branch triggers on either User-Agent: symbolicator/* or x-electron-symbol-redirect: 1, but Cloudflare's custom cache key only carried the UA marker — so an ordinary-UA request with the redirect header would cache its public, max-age=3600 302 into the ordinary cohort, and normal clients requesting that path could then be served the redirect instead of the proxied symbol. And since the 302 carried no Vary, any standards-compliant shared cache between us and a client could reuse it across clients regardless of Cloudflare's key configuration.

The fix, both halves of your suggested direction:

1. Origin (this PR, 9c6ed74): the 302 now sends Cache-Control: no-store plus Cloudflare-CDN-Cache-Control: public, max-age=3600. The latter is Cloudflare-specific — per Cloudflare's CDN-Cache-Control docs, Cloudflare prefers Cloudflare-CDN-Cache-Control over Cache-Control when making caching decisions and does not forward it downstream, because it's "a header only used to control Cloudflare". Net effect: browsers and generic shared caches see no-store and never store the redirect; only Cloudflare — the one cache with the cohort-aware key — still caches it for an hour. The redirect test now asserts both headers; full suite 33/33 green.
2. Cache key (electron/infra#313): the ruleset's custom key gains check_presence = ["x-electron-symbol-redirect"] alongside the existing user-agent contains symbolicator/ marker, so both redirect triggers partition the cache. That closes the poisoning window even for the ruleset's 302 edge-TTL fallback path (Cloudflare's status-code TTLs ignore origin Cache-Control, so the key must be correct independently of the headers).

On Vary: it became unnecessary rather than being added — generic caches now see no-store, so there is nothing for them to reuse across clients, and Cloudflare's cohort separation comes from the cache key rather than Vary (which Cloudflare doesn't honor for non-image content anyway).

---

Generated by Claude Code

[claude]

🔴 The negative-cache key is derived from path+query only, never HTTP method, but the server proxies every method identically — so a single non-GET request (e.g. a stray POST/PUT, or a CORS-preflight OPTIONS) that Azure Blob rejects with 403 for a genuinely-existing symbol gets recorded as a miss and rewritten to 404. Previously that pollution was confined to one dyno's in-memory cache; this PR adds Cache-Control: public, max-age=3600 to that rewritten 404 (src/index.ts:107-109), so Cloudflare now caches and broadcasts the false-negative to every edge PoP for an hour, making a real symbol unavailable to all GET clients with no way to invalidate early. Consider gating the negative-cache write/read (and the 403->404 rewrite) on request.method === '\''GET'\''.

Extended reasoning...

The bug. missingSymbolCache is keyed purely by proxyReq.path / cacheKey (path + query), and the handler in http.createServer has no method dispatch at all — every request other than /health and the symbolicator redirect falls through to proxy.web(req, res, ...) regardless of verb. In the proxyReq hook's writeHead wrapper (src/index.ts:101-109), any upstream 403 — irrespective of the method that produced it — is unconditionally recorded via missingSymbolCache.set(proxyReq.path, true) and rewritten to 404.

Why a non-GET 403 is realistic. Azure Blob Storage (the backing store referenced throughout the comments) returns 403, not 404, for anonymous non-GET operations against a path whose blob genuinely exists: write verbs (POST/PUT/DELETE) against a read-only public container are unauthorized, and a CORS-preflight OPTIONS without a matching storage-account CORS rule also comes back 403. Both happen independent of whether the target blob is actually present. Given the proxy advertises Access-Control-Allow-Origin: *, a scanner, bot, or even a legitimate cross-origin browser fetch that triggers a preflight can hit an existing symbol path with a non-GET request and get a 403 from upstream.

What this PR changes. Before this PR, that method-agnostic 403-to-404 rewrite already happened, but the resulting 404 carried no Cache-Control header, so the pollution was bounded to whatever the single dyno's in-memory LRU held (with no TTL enforcement issue for HTTP caching, since nothing downstream would cache it). This PR adds response.setHeader('Cache-Control', MISSING_CACHE_CONTROL) in that exact 403 branch (now public, max-age=3600), and the cache-hit path at the bottom of the handler (missingSymbolCache.get(cacheKey)) also now emits that header on the cached 404. Since Cloudflare fronts this origin (per the PR description, WAF + rate limit + edge cache), a 404 with a public Cache-Control will be cached and served at every edge PoP — this turns a previously per-dyno, memory-only false negative into a TTL-guaranteed, globally broadcast outage for a real symbol.

Step-by-step proof.

1. A symbol genuinely exists at /electron/foo.pdb/ABCDEF/foo.pdb on the upstream store.
2. A scanner (or a CORS preflight, or a stray non-GET client) sends OPTIONS or POST to that exact path.
3. The proxy has no method check, so it forwards the request upstream via proxy.web.
4. Azure Blob Storage responds 403 (anonymous non-GET against a real, read-only blob is unauthorized / CORS-rejected), independent of blob existence.
5. The writeHead wrapper sees args[0] == 403, calls missingSymbolCache.set(proxyReq.path, true), rewrites to 404, and — new in this PR — sets Cache-Control: public, max-age=3600.
6. Cloudflare, sitting in front of the origin, caches this 404 response for the path at the edge.
7. Every subsequent legitimate GET for that exact symbol path, from any client anywhere, is now served the cached 404 by Cloudflare — the origin is never even reached again — for up to one hour, with no mechanism to purge it early.

Why nothing today prevents this. There is no method check anywhere in src/index.ts; the cache key construction (incomingPathToProxyPath) only normalizes case/aliasing of the path and never touches req.method; and the CORS headers set elsewhere (Access-Control-Allow-Methods: GET) are advisory to browsers only — they do not stop the server itself from proxying non-GET requests upstream or from trusting the 403 it gets back.

Fix. Gate the negative-cache write (and ideally the 403->404 rewrite itself, or the whole proxy) on request.method === 'GET' — e.g. only call missingSymbolCache.set(...) when request.method === 'GET', and/or short-circuit non-GET/non-HEAD requests with a 405 before they ever reach proxy.web. This is a small, low-risk change that removes the amplification entirely.

I'm flagging this as normal severity rather than nit specifically because this PR is what makes the failure mode global and durable: pre-PR it was a bounded, single-dyno, best-effort in-memory artifact; post-PR it is a Cloudflare-wide, hour-long, unrecoverable-without-a-manual-purge outage for a real symbol, triggered by a single stray request. That's a concrete availability regression a real user (or CI job pulling symbols) could hit, not just a cosmetic gap.

[claude]

🟡 The missingSymbolCache hit path (src/index.ts:156-158) returns a 404 directly without going through proxy.web, so it never gets the Access-Control-Allow-Origin/Access-Control-Allow-Methods headers that are set only inside proxy.on('proxyReq', ...). This CORS gap pre-dates the PR, but the PR now marks these 404s public, max-age=3600 and edge-cacheable, so a CORS-less 404 could populate Cloudflare's edge cache and be served to browser-based CORS clients for up to an hour. Consider setting the two CORS headers on the cache-hit path as well.

Extended reasoning...

The bug: CORS headers (Access-Control-Allow-Origin: * and Access-Control-Allow-Methods: GET) are set in exactly one place in this file: inside proxy.on('proxyReq', (proxyReq, request, response, options) => { ... }) at src/index.ts:95-96. That handler only fires when proxy.web(req, res, ...) is actually invoked. The negative-cache hit path in the main http.createServer handler:

if (missingSymbolCache.get(cacheKey)) {
  return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end();
}

returns directly and never calls proxy.web, so it never runs proxyReq and never sets the CORS headers. Compare this to a live 403->404 conversion, which happens inside the writeHead wrapper installed by the proxyReq handler — that path does carry the CORS headers because they were already set earlier in the same handler, before the wrapped writeHead is even called.

Why this predates the PR but is worsened by it: this asymmetry between the two 404-producing code paths already existed before this PR — a cache hit has always returned a CORS-less 404 while a fresh 403 rewrite has always carried CORS headers. What the PR changes is caching behavior: it adds Cache-Control: public, max-age=3600 to the cache-hit 404 (line 157), which is a signal that tells Cloudflare's edge it may cache and serve this exact response to future requests without touching the origin at all.

Concrete walk-through of the failure mode:

1. Client A requests /electron/foo.pdb/ABC123/foo.pdb for a symbol that doesn't exist. This is a cache miss, so it goes through proxy.web -> proxyReq -> upstream returns 403 -> rewritten to 404 with CORS headers set, and the origin process's missingSymbolCache now has an entry for this path (TTL 1h).
2. A little later, but before Cloudflare's edge has cached anything for that URL at the specific PoP handling the next request (different Cloudflare PoP, or the edge cache simply hasn't been populated yet for that exact path), a second request for the same path lands on the same origin dyno (or one with a warm process, e.g. across a load-balanced pool that isn't sticky, or on the same dyno if there's only one). This request hits the missingSymbolCache.get(cacheKey) branch and returns a 404 with Cache-Control: public, max-age=3600 but no Access-Control-Allow-Origin header.
3. Cloudflare's edge cache, honoring that Cache-Control, stores this CORS-less 404 as the canonical cached response for the path for up to an hour.
4. Any browser-based client (e.g. a web-based crash-symbolication UI or dev tool doing a fetch() with CORS enforcement) that requests the same path within that hour gets the edge-cached CORS-less 404. Since there's no Access-Control-Allow-Origin header, the browser's CORS check fails and the fetch() promise rejects with a network/CORS error instead of resolving to a normal 404 response.

Why existing code doesn't prevent it: the CORS headers are hard-coded into the proxyReq event handler specifically, with no equivalent guard or shared helper for cache-hit responses. There is nothing stopping the negative-cache branch from being the one that "wins" and gets cached at the edge, since which of the two code paths executes for a given request is purely a function of in-memory cache state at that origin process, uncorrelated with Cloudflare's edge cache state for that same URL.

Impact: the primary consumers of this service are native symbol-server clients (symsrv.dll, Sentry's symbolicator) that don't perform CORS preflight/enforcement, so most traffic is unaffected — the response is a 404 either way and those clients only care about the status code. The practical exposure is limited to any browser-based tooling that queries this endpoint directly, but for those the failure mode is materially worse than a normal 404 (silent CORS rejection vs. a resolvable 404 response), and once poisoned into the edge cache the bad response persists for up to an hour with a public max-age.

Fix: set Access-Control-Allow-Origin: * and Access-Control-Allow-Methods: GET on the cache-hit response too, e.g.:

if (missingSymbolCache.get(cacheKey)) {
  return res.writeHead(404, {
    'Cache-Control': MISSING_CACHE_CONTROL,
    'Access-Control-Allow-Origin': '*',
    'Access-Control-Allow-Methods': 'GET',
  }).end();
}

This is a small, low-risk addition since the same static header values are already applied unconditionally elsewhere in the file, showing the intent is for all responses (not just proxied ones) to be CORS-permissive.