fix(server): a partial fragment is never shared-cacheable - #1141
Conversation
Closes #1140 A reduced X-Webjs-Have body inherited the page's Cache-Control, so on a page that opted into public caching it went out as `public, s-maxage=600` and its safety rested entirely on the CDN honouring `Vary: X-Webjs-Have`. Cloudflare honours only Accept-Encoding. Measured on webjs.dev: 71,759 bytes for the document versus 47,375 for the fragment, identical Cache-Control on both, so a CDN with HTML caching on could store the chrome-less fragment under the page URL and serve it to whoever navigated there next. Latent rather than live only because Cloudflare does not cache HTML by default. It goes live the moment an app does the ordinary thing and turns HTML caching on, which is exactly what metadata.cacheControl invites. The failure is silent, cache-resident, and served to many visitors. Fragments are now `private`: no shared cache may store one, the browser that asked for it still may, and the freshness the page declared is preserved. Vary stays as belt-and-braces rather than as the guarantee. Full documents are untouched, which the test asserts byte for byte, because their edge-cacheability is the point of the header. The frame-subtree path gets the same call as an invariant guard. It is a no-op today (that response is built without page metadata, so it is already no-store), and its test asserts the contract rather than that coincidence. Side effect worth knowing: `private` drops fragments out of the conditional-GET funnel, whose isCacheable excludes private. Deliberate, since the router revalidates its own fetches and dedupes via its snapshot cache, so a fragment ETag bought nothing.
Review found the first pass traded one bug for a worse one. Making fragments `private` dropped them out of the conditional-GET path, whose isCacheable excluded private outright. The router fetches partials with cache: no-cache (#1131) precisely so a revalidation is a cheap 304, so every prefetch and soft navigation started re-downloading the whole 47KB fragment. My stated rationale had the causality backwards: #1131 is exactly WHY the validator mattered. private is now included in the validator path. The old worry was a cross-session 304 on per-user content, and it cannot happen: the ETag hashes THIS response's body, so two users with different bodies get different ETags and neither can match the other's, while two users with identical bodies are asking about identical bytes. private already forbids the one real hazard, a shared cache storing the response. Verified end to end: the fragment is private AND 304s on replay. Also found by review: the page-action 422 re-render inherited the page's public cacheControl while carrying the submitter's own field values and errors. A non-200 now never inherits it. The directive parser is quoted-string aware (`no-cache="Set-Cookie, X-Foo"` was being torn apart) and matches on the parsed directive NAME, so `private="x-user"` is recognised rather than duplicated and `s-maxage = 600` is actually stripped. Dropped the frame-path call: that response carries no page metadata, so it was dead code with nothing to assert against. The property is locked by a test that fails if the frame response ever starts inheriting. Docs: the reference paragraph contradicted itself within two sentences, and AGENTS.md plus the client-router docs page still presented Vary as the guarantee.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
This traded one bug for a worse one and the review caught it. Making fragments private dropped them out of the conditional-GET path, and the router fetches partials with cache: no-cache specifically so a revalidation is a cheap 304, so every prefetch and soft nav started re-downloading the whole 47KB fragment. My rationale had the causality backwards: #1131 is exactly why the validator mattered, not a reason it did not.
The right fix was upstream, in conditional-get.js. private was excluded from the validator path on the theory that per-user content must never get a cross-session 304, and that cannot happen: the ETag hashes this response's body, so two users with different bodies get different ETags and neither can match the other's, while two users with identical bodies are asking about identical bytes. private already forbids the one real hazard, a shared cache storing the response. Fragments are now private AND validated, verified end to end (private, max-age=60 with a working 304 replay).
Two more real ones. The page-action 422 re-render inherited the page's public cacheControl while carrying the submitter's own field values and errors, so a non-200 never inherits it now. And the directive parser was not quoted-string aware, so no-cache="Set-Cookie, X-Foo" got torn apart and private="x-user" was duplicated rather than recognised; it matches on the parsed directive name now.
Dropped the frame-path call as dead code: that response carries no page metadata, so it was already no-store and there was nothing to assert against. The property is locked by a test that fails if it ever starts inheriting instead.
All three guards counterfactualled individually, and the wider server suites (SSR, dev, cache, headers) are 480 green.
…ivate Round two, and two of the finds were my own leftovers. The non-200 guard I added to htmlResponse was dead: no caller passes it metadata, so the value was already the no-store default and reverting the line changed nothing. That is the identical shape round one had me remove from the frame path, shipped again nine lines away. Removed; the guard that matters is on streamingHtmlResponse, which the 422 test drives. privateFragment treated `private="x-user"` as already unshareable and returned untouched. Per RFC 9111 the qualified form marks only the NAMED header fields private and leaves the response storable by a shared cache, so it is the one input where the invariant did not hold. Only a bare `private` short-circuits now. The rest is comments and docs the isCacheable change falsified, in four places I had not opened: the module header of conditional-get.js itself (still teaching the theory its own function now refutes), a comment nine lines from one I edited in ssr.js, the stale rationale for actions.js hand-rolling its own 304, the docs deployment page, and the built-ins skill reference, which is the surface every agent reads. Also the test gap that matters most: nothing drove a reduced fragment through the real handler and asserted it carries a validator and 304s. The halves were asserted separately, which is exactly how the regression got through the first time. Reverting either half now reds this test.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Two of this round's finds were my own leftovers, which is the useful part.
The non-200 guard I added to htmlResponse was dead code: no caller passes it metadata, so reverting the line changed nothing. That is the identical shape round one had me remove from the frame path, and I shipped it again nine lines away in the same commit. Removed.
privateFragment also treated private="x-user" as already unshareable. Per RFC 9111 the qualified form marks only the named header fields private and leaves the response storable by a shared cache, so it was the single input where the PR's invariant did not hold. Only a bare private short-circuits now.
The rest were comments and docs the isCacheable change falsified in four places I had not opened, including the module header of conditional-get.js itself, which was still teaching the theory its own function now refutes, and the built-ins skill reference, which is what every agent reads.
The test gap is the one worth naming. Nothing drove a reduced fragment through the real handler and asserted it carries a validator and 304s; the two halves were asserted separately, which is exactly how the re-download regression got through the first time. That test exists now and reverting either half reds it.
Round three. No correctness bug left in the code, but two of my own fixes had no test at all: mutating the qualified-private check or the quote-aware splitter left every suite green. Both have fixtures now. The splitter test needed a second pass to be worth anything. With a space after the comma inside the quoted argument, a naive split and re-join happens to rebuild the original byte for byte, so the test passed either way. The no-space form is what discriminates, since the naive path reassembles it with different inner spacing. The rest is the doc sweep I keep under-scoping. The isCacheable change falsified statements in six more places, including packages/server's own AGENTS.md (twice), the header of the very test file that now disproves it, three sentences on the deployment docs page that disagreed with the one sentence between them, and a blog post still teaching the cross-session-replay hazard as real. That phrase now appears nowhere in the repo. Also fixed the privateFragment JSDoc, which still argued a fragment ETag buys nothing, and a comment left dangling by the round-two rewrite.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
No correctness bug left in the code this round, but two of my own fixes had no test: mutating either the qualified-private check or the quote-aware splitter left every suite green. Both have fixtures now.
The splitter test needed a second pass to be worth anything, which is the interesting bit. With a space after the comma inside the quoted argument, a naive split and re-join happens to rebuild the original byte for byte, so the first version of the test passed with the fix reverted. The no-space form is what actually discriminates.
The rest is the doc sweep I keep under-scoping. Changing isCacheable falsified statements in six more places: packages/server's own AGENTS.md twice, the header of the very test file that now disproves it, three sentences on the deployment docs page that disagreed with the one sentence between them, and a blog post still teaching the cross-session-replay hazard as real. That phrase now appears nowhere in the repo, which is the check I should have run in round one rather than fixing surfaces one round at a time.
Round four found two real holes in my own helper, both the same posture the PR exists to remove: relying on a cache behaving well. A qualified `private="x-user"` was dropped from neither list, so the output carried the directive twice. A cache resolving a repeat by last occurrence reads the qualified form, which per RFC 9111 leaves the response shared-storable. `private` joins the strip list now, since a bare one is prepended anyway. `if (!cc) return` failed OPEN: a response with no Cache-Control is heuristically storable by a shared cache, so "no header" is exactly when the downgrade matters most. It sets `private` now. Unreachable today because every caller sets the header, which is precisely how a future change there would have reintroduced a shareable fragment unnoticed, so the helper is exported and unit-tested rather than left to a vacuous test that returns early when it cannot import it. Also the last four stale surfaces: the funnel's own call site in dev.js, routing-and-pages.md (which ships verbatim into every scaffolded app, so it re-teaches the wrong rule most reliably), the website dogfood test that asserted the deleted rule and would have failed a legitimately private page, and a blog paragraph left tautological by my earlier edit. The 8 website gallery failures in this worktree are environmental: the generated components/ui registry mirror is absent, and they fail identically with this change stashed.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Two real holes in my own helper this round, both the exact posture this PR exists to remove: relying on a cache behaving well.
A qualified private="x-user" was dropped from neither list, so the output carried the directive twice, and a cache resolving a repeat by last occurrence reads the qualified form, which leaves the response shared-storable. And if (!cc) return failed OPEN, when a response with no Cache-Control is heuristically storable by a shared cache, so that is exactly the input where the downgrade matters most.
The second one is worth a note on process. It is unreachable today because every caller sets the header, and my first attempt to cover it was a test that returned early when it could not import the private function, i.e. green and worthless. The helper is exported and genuinely unit-tested now.
Also the last four stale surfaces, including routing-and-pages.md, which ships verbatim into every scaffolded app and so re-teaches the wrong rule more reliably than anything else, and the website dogfood test that asserted the deleted rule and would have failed a legitimately private page.
The 8 website gallery failures in this worktree are environmental (the generated components/ui mirror is absent) and fail identically with this change stashed.
Round five found my justification inverted. I wrote that excluding `private` "cost the router's partial responses their 304s", implying a pre-existing bug. It did not: before this PR a fragment INHERITED the page's public header, so it already passed isCacheable and already carried a validator. The validator loss is introduced by this PR's own `private` downgrade, and widening isCacheable is what makes that downgrade safe to ship. A maintainer reading the old wording would think reverting it restores a real bug, when it would only undo a self-inflicted one, and would think an app-wide widening was mandatory rather than chosen. Also corrected three unconditional ETag claims: a fragment keeps its validator only on a page that opted into caching, since a default page's fragment is no-store and there is nothing to validate either way. Plus the last two surfaces: renderChain still asserted Vary sufficiency three lines above the code that produces the reduced body, and the package module map had no mention of privateFragment, so a newly exported helper with a subtle contract was invisible in the file agents are told to read to find what lives where.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
The find that mattered here was not a bug in the code but a lie in the comment explaining it. I justified widening isCacheable by saying the exclusion had cost the router's partial responses their 304s. It never did: before this PR a fragment inherited the page's public header, so it already passed the check and already had a validator. The validator loss is introduced by my own private downgrade, and the widening is what makes that downgrade safe. Someone reading the old wording would believe reverting it restores a pre-existing bug and that an app-wide widening was mandatory rather than a choice I made.
Also corrected three ETag claims that were stated unconditionally but only hold for a page that opted into caching, since a default page's fragment is no-store with nothing to validate. Plus renderChain still asserting Vary sufficiency three lines above the code that produces the reduced body, and the package module map having no mention of the new exported helper.
Confirmed as not a problem: exporting privateFragment does not widen the public API. The package index re-exports only ssrPage and ssrNotFound, and the exports map has no ./src/* subpath, so it is unreachable from outside the repo.
Round six, and the docs one is the finding that actually mattered. I wrote "a public cacheControl is safe to use behind a CDN" and justified it with the absence of Set-Cookie. That reasoning covers headers and says nothing about the response BODY, which is the dominant leak, and unlike `revalidate` (which auto-excludes a render that reads cookies, a session or auth) `cacheControl` is emitted verbatim with no such defense. So the page told a reader it was safe to mark a signed-in view `public, s-maxage=600`, which is the CDN-resident cross-user leak this whole PR is about. It now leads with the visitor-identical rule, names the missing protection, and points at `private` for per-user pages. The privateFragment JSDoc also overstated the bug: it said a reduced body AND a frame subtree inherit the page's Cache-Control, but the frame response is built without page metadata and never did. Left as written it reads as though frames flow through the helper, so someone adding a new partial-response shape would assume they are covered. It now says the reduced path is the only caller and a new shape must call it explicitly. And the PR body still described the frame-path call an earlier round removed.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
The docs find is the one that mattered, and it is the worst thing this branch has produced.
I wrote that a public cacheControl is safe behind a CDN, and justified it with the absence of Set-Cookie. That reasoning covers headers and says nothing about the response body, which is the dominant leak. Worse, unlike revalidate, which auto-excludes a render that reads cookies or a session or auth, cacheControl is emitted verbatim with no such defense. So the page was telling a reader it was safe to mark a signed-in view public with a shared TTL, which is exactly the CDN-resident cross-user leak this PR exists to prevent, on the same docs page. It now leads with the visitor-identical rule, names the missing protection, and points at private.
The helper's JSDoc also overstated the bug's scope, claiming a frame subtree inherits the page's Cache-Control when it never did. Left alone, someone adding a new partial-response shape would read that framing and assume the helper already covers them; it now says the reduced path is the only caller and a new shape has to call it.
Plus the PR body still described a frame-path call an earlier round removed.
…ort form Round seven. Four surfaces condensed the safety claim to "one user's validator can never match another's", which is simply false: two users whose private responses have identical bodies get identical ETags, so one does match the other. The repo already proves it, in the test asserting an ETag is stable for identical content across requests. The full argument has two clauses and needs both: different bodies give different ETags so they cannot match, and identical bodies are asking about identical bytes, where a 304 discloses nothing. I had written it correctly in the isCacheable JSDoc and the test comment, then dropped the second clause everywhere I restated it, which turned the justification for a security-relevant widening into something a careful reader would rightly reject. Also the call site of the new fix was the last place still presenting `Vary` as the protection, with no mention of privateFragment two lines below it. It now says which of the two markings is the guarantee and which is belt-and-braces, since that order of trust is the entire point.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Sharp find on the safety claim. Four surfaces condensed it to "one user's validator can never match another's", which is false: two users whose private responses have identical bodies get identical ETags, so one does match the other. The repo proves it, in the test asserting an ETag is stable for identical content across requests.
The real argument has two clauses and needs both. Different bodies give different ETags so they cannot match; identical bodies are asking about identical bytes, where a 304 discloses nothing. I wrote it correctly in the isCacheable JSDoc and the test comment, then dropped the second clause every time I restated it elsewhere, which left the justification for a security-relevant widening in a form a careful reader should reject.
The other one closes the loop: the call site of the new fix was the last place still presenting Vary as the protection, with no mention of privateFragment two lines below it. It now names which marking is the guarantee and which is belt-and-braces.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Clean. Eight guards mutation-tested individually and each is killed by exactly the test written for it, with no vacuous ones left among the new tests. Also confirmed a few things I had been asserting without checking: privateFragment is not re-exported from the package index, a webjs.headers rule cannot clobber the fragment's Cache-Control, the only other consumer of isCacheable already short-circuits with the same matcher so its behaviour is unchanged, and the dynamic-access helpers really do cover cookies, auth and session, which is what backs the new caching-docs caveat. The Bun contract is byte-identical to Node, so there was no cross-runtime divergence to add a test for.
Nothing left to find here.
|
Cross-check against Next.js: same bug class, and their fix is the heavier of the two Verified against the Next.js source rather than from memory, since this PR's justification leans on the claim that the threat is real. It is, verbatim. Three points of comparison worth recording:
So the shipped fix here is the LIGHTER of the two industry answers: standards-based, no URL surface changes, fragments simply never shared. If WebJs ever wants edge-cached fragments, or meets a CDN config that ignores origin headers outright, the Next-style hashed cache key is the known upgrade path, and it composes with (rather than replaces) what this PR ships. |
1 participant
Closes #1140
A reduced
X-Webjs-Havefragment inherited the page'sCache-Control, so on a page that opted into public caching it went out aspublic, s-maxage=600and its safety rested entirely on the CDN honouringVary: X-Webjs-Have. Cloudflare honours onlyAccept-Encoding.Measured on the live origin,
/docs/ai-first:So a CDN with HTML caching enabled could store the 47KB fragment under the page URL and serve it to whoever navigated there next: a page rendered without its header, nav, or footer. Silent, cache-resident, and served to many visitors for the whole
s-maxagewindow.Why this was worth fixing rather than configuring around
It is latent today only because Cloudflare does not cache HTML by default, which is why webjs.dev sits at
cf-cache-status: DYNAMIC. It goes live the moment an app does the ordinary thing and turns HTML caching on, which is precisely whatmetadata.cacheControlinvites a user to do.That framing is the reason this is a framework change and not a dashboard tweak on webjs.dev. Hand-crafting a Cache Rule for one site would have made the dogfood app stop representing what a real user runs, and left every other WebJs app on Cloudflare exposed.
The fix
Fragments are served
private: no shared cache may store one, the browser that asked for it still may, and the freshness the page declared is preserved (private, max-age=60, stale-while-revalidate=86400).Varystays, as belt-and-braces for caches that do honour it rather than as the guarantee.Full documents are untouched. Their edge-cacheability is the whole point of the header, and the test asserts the full-document value byte for byte so a blanket downgrade cannot creep in.
The
<webjs-frame>subtree path deliberately does NOT get the same call. That response is built without page metadata, so it is alreadyno-storeand a call there would be dead code with nothing to assert against. The property is locked by a test instead, which fails if that response ever starts inheriting the page header.Keeping the validator
Marking fragments
privatewould, on its own, have cost them their ETag:conditional-get.jsexcludedprivatefrom the validator path outright. That matters because the router fetches partials withcache: 'no-cache'(#1131) precisely so a revalidation is a cheap 304, so the downgrade would have turned every prefetch and soft navigation into a full re-download of the fragment.So this PR also stops
isCacheableexcludingprivate. That exclusion was never justified on its own terms: a 304 is returned only when the client's ownIf-None-Matchmatches an ETag hashed from THIS response's body, so two users with different bodies get different ETags and neither can match the other's, while two users with identical bodies are asking about identical bytes.privatealready forbids the one real hazard, a shared cache storing the response. To be precise about the causality, since an earlier draft of this body got it backwards: the validator loss is introduced by this PR's own downgrade, not by a pre-existing bug, and the widening is what makes the downgrade safe to ship.On a page that never opted into caching the fragment is
no-storeand there is nothing to validate either way; the ETag discussion applies to pages that setmetadata.cacheControl.Test plan
test/ssr/ssr.test.js(five cases): a reduced response on a page declaringpublic, s-maxage=600comes backprivate, with nopublicand nos-maxage, keepingmax-age=60and itsVary, and the same page's full document keeps its declared header byte for byte; a frame-subtree equivalent; a qualifiedprivate="x-user"is downgraded rather than passed through or emitted twice; a quoted directive argument containing a comma survives byte for byte; a non-200 never inherits the page header.packages/server/test/dev/conditional-get.test.js(three cases): aprivatepage gets an ETag and 304s; a reduced fragment driven through the REAL handler isprivateAND 304s on replay (the composition, since asserting the halves separately is how the re-download regression got through once);privateFragmentfails closed on an absent header.test/ssr/*+packages/server/test/cache/*+ the html-cache suite: 271 pass, 0 fail.createRequestHandleragainst the real website app before writing the tests: fullpublic, max-age=60, s-maxage=600, swr, fragmentprivate, max-age=60, swr, 24KB smaller,Varyintact.Docs
references/client-router-and-streaming.mdnow states the guarantee does not depend onVary.website/app/docs/cache/page.tsgains the answer to the question this bug would otherwise raise: a publiccacheControlis safe behind a CDN, because the response is cookieless and the router's partial responses are handled for you.