Unify Vary and add Cache-Control on RDF variants (#315) - #316
Conversation
Browser reloads of mashlib-rendered RDF resources sometimes showed the
raw Turtle/JSON-LD body instead of the mashlib data-browser view.
Root cause: three different code paths set different Vary values for
variants of the same URL:
- mashlib HTML wrapper: "Accept"
- getVaryHeader (Turtle/JSON-LD via conneg): "Accept, Origin"
- getResponseHeaders (default): "Accept, Authorization, Origin"
Chromium/Brave's HTTP cache gets confused by Vary mismatches across
variants and can serve the cached Turtle body on top-level navigation —
the browser then renders it as text. Hard refresh bypasses the cache,
which is why it always worked.
Fix:
- getVaryHeader is the single source of truth. It always emits
"Accept, Authorization, Origin" (when mashlib or conneg is on)
or "Authorization, Origin" otherwise. Authorization is correct
because WAC lets responses vary by authenticated user.
- getResponseHeaders / getAllHeaders / getNotFoundHeaders accept
mashlibEnabled and route through getVaryHeader.
- All headers['Vary'] = 'Accept' overrides in handlers are replaced
with the centralized helper.
- RDF data variants now carry Cache-Control:
"private, no-cache, must-revalidate". ETag stays, so revalidation
is a cheap 304. This also closes a real (if narrow) security gap
where a cached response from one auth state could leak into
another. The mashlib HTML wrapper keeps no-store.
There was a problem hiding this comment.
Pull request overview
Unifies caching-related response headers across RDF content-negotiation variants to prevent browsers from caching/serving the wrong representation (notably on soft reloads), and adds regression tests for the behavior.
Changes:
- Centralizes
Varyheader logic viagetVaryHeader(connegEnabled, mashlibEnabled)and routes multiple header builders/handlers through it. - Adds
Cache-Control: private, no-cache, must-revalidateto RDF data variants while keeping the mashlib HTML wrapperno-store. - Introduces a new regression test ensuring consistent
Vary, correctCache-Control, and presence ofETagon RDF variants.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vary-cache-headers.test.js | Adds regression coverage for consistent Vary and cache revalidation headers across variants. |
| src/rdf/conneg.js | Updates getVaryHeader() to be the single source of truth and include Authorization consistently. |
| src/ldp/headers.js | Plumbs mashlibEnabled through header utilities and uses centralized getVaryHeader(). |
| src/handlers/resource.js | Replaces ad-hoc Vary overrides with getVaryHeader() and adds revalidating Cache-Control to RDF data responses. |
| src/handlers/container.js | Aligns Vary calculation with centralized helper and mashlib enablement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const unique = new Set(varys.map((v) => v.vary)); | ||
| assert.strictEqual(unique.size, 1, | ||
| `expected identical Vary across variants, got: ${JSON.stringify(varys)}`); | ||
| const vary = [...unique][0]; |
There was a problem hiding this comment.
The regression test can throw a TypeError if res.headers.get('vary') returns null/undefined (since assert.match expects a string). Consider asserting the header exists first (or coerce to an empty string) so failures are reported as assertion failures with a clear message.
| const vary = [...unique][0]; | |
| const vary = [...unique][0]; | |
| assert.ok(vary, `expected Vary header across variants, got: ${JSON.stringify(varys)}`); |
| // Cache-Control for RDF data responses: let clients keep the body but force | ||
| // revalidation via ETag on every use. This prevents stale bodies from leaking | ||
| // across auth-state changes (WAC) and closes the mashlib render-race window | ||
| // where a cached data variant was served on top-level navigation (#315). | ||
| const RDF_CACHE_CONTROL = 'private, no-cache, must-revalidate'; | ||
|
|
There was a problem hiding this comment.
RDF_CACHE_CONTROL is applied to most RDF data responses, but the index.html container code path that extracts a JSON-LD data island and returns Turtle/JSON-LD (handleGet() when stats.isDirectory and indexExists) still returns those RDF variants without setting this Cache-Control. That leaves a caching/revalidation gap for those variants; consider setting Cache-Control: private, no-cache, must-revalidate (and keeping the ETag) for both the Turtle and JSON-LD responses from that branch as well.
- Cover the index.html data-island conneg path (container with index.html + embedded <script type=\"application/ld+json\">) which also returns RDF variants and was missing Cache-Control. Adds a regression test. - Harden the Vary-consistency test with a defensive ok() check so a missing header produces a clean assertion failure rather than a TypeError on assert.match.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const cc = res.headers.get('cache-control'); | ||
| assert.ok(cc, `expected Cache-Control on ${accept} variant`); | ||
| assert.match(cc, /no-cache|no-store/, `Cache-Control "${cc}" must prevent stale reuse (Accept: ${accept})`); | ||
| // ETag is preserved so revalidation is cheap (304). | ||
| assert.ok(res.headers.get('etag'), `expected ETag on ${accept} variant`); | ||
| } |
There was a problem hiding this comment.
The Cache-Control assertion here is too permissive (it passes for any header containing "no-cache" even if the required directives like "private" and "must-revalidate" are missing). Since this test is meant to prevent the auth-state leakage described in #315, it should assert the full directive set (or at least require both "private" and "must-revalidate" in addition to "no-cache").
| const cc = res.headers.get('cache-control'); | ||
| assert.ok(cc, `expected Cache-Control on ${accept} island variant, got headers: ${[...res.headers].map(([k, v]) => `${k}=${v}`).join(', ')}`); | ||
| assert.match(cc, /no-cache|no-store/, | ||
| `Cache-Control "${cc}" must prevent stale reuse (Accept: ${accept})`); | ||
| } |
There was a problem hiding this comment.
Same as above: this test only checks for "no-cache"/"no-store", so it won’t catch regressions where "private" or "must-revalidate" are dropped. Consider asserting the complete expected Cache-Control policy for these RDF data-island variants as well.
| const varys = []; | ||
| for (const accept of accepts) { | ||
| const res = await request('/varytest/public/card.jsonld', { headers: { Accept: accept } }); | ||
| varys.push({ accept, vary: res.headers.get('vary') }); | ||
| } | ||
| // All three variants must carry the same Vary — inconsistent Vary is | ||
| // what confused browser caches into serving the wrong variant. | ||
| const unique = new Set(varys.map((v) => v.vary)); | ||
| assert.strictEqual(unique.size, 1, | ||
| `expected identical Vary across variants, got: ${JSON.stringify(varys)}`); | ||
| const vary = [...unique][0]; | ||
| assert.ok(vary, `expected Vary header across variants, got: ${JSON.stringify(varys)}`); |
There was a problem hiding this comment.
Minor naming nit: varys isn’t a standard pluralization and makes the intent a bit harder to read. Consider renaming to something like varyHeaders/varyValues (and uniqueVaryValues) to improve clarity.
| const varys = []; | |
| for (const accept of accepts) { | |
| const res = await request('/varytest/public/card.jsonld', { headers: { Accept: accept } }); | |
| varys.push({ accept, vary: res.headers.get('vary') }); | |
| } | |
| // All three variants must carry the same Vary — inconsistent Vary is | |
| // what confused browser caches into serving the wrong variant. | |
| const unique = new Set(varys.map((v) => v.vary)); | |
| assert.strictEqual(unique.size, 1, | |
| `expected identical Vary across variants, got: ${JSON.stringify(varys)}`); | |
| const vary = [...unique][0]; | |
| assert.ok(vary, `expected Vary header across variants, got: ${JSON.stringify(varys)}`); | |
| const varyValues = []; | |
| for (const accept of accepts) { | |
| const res = await request('/varytest/public/card.jsonld', { headers: { Accept: accept } }); | |
| varyValues.push({ accept, vary: res.headers.get('vary') }); | |
| } | |
| // All three variants must carry the same Vary — inconsistent Vary is | |
| // what confused browser caches into serving the wrong variant. | |
| const uniqueVaryValues = new Set(varyValues.map((v) => v.vary)); | |
| assert.strictEqual(uniqueVaryValues.size, 1, | |
| `expected identical Vary across variants, got: ${JSON.stringify(varyValues)}`); | |
| const vary = [...uniqueVaryValues][0]; | |
| assert.ok(vary, `expected Vary header across variants, got: ${JSON.stringify(varyValues)}`); |
| const headers = getAllHeaders({ | ||
| isContainer: true, | ||
| etag: stats.etag, | ||
| contentType: 'text/html', | ||
| origin, | ||
| resourceUrl, | ||
| connegEnabled | ||
| }); | ||
| headers['Vary'] = 'Accept'; | ||
| headers['Vary'] = getVaryHeader(connegEnabled, request.mashlibEnabled); | ||
| headers['X-Frame-Options'] = 'DENY'; |
There was a problem hiding this comment.
getAllHeaders() now accepts mashlibEnabled and already routes Vary through getVaryHeader(). Instead of populating headers and then overriding headers['Vary'] afterwards, pass mashlibEnabled: request.mashlibEnabled into getAllHeaders() here (and in the other call sites) to avoid repetition and reduce the chance of future inconsistencies.
| const headers = getAllHeaders({ | ||
| isContainer: isCreatingContainer, | ||
| origin, | ||
| connegEnabled | ||
| }); | ||
| headers['Location'] = resourceUrl; | ||
| headers['Vary'] = getVaryHeader(connegEnabled); | ||
| headers['Vary'] = getVaryHeader(connegEnabled, request.mashlibEnabled); | ||
|
|
There was a problem hiding this comment.
Same pattern as elsewhere: since getAllHeaders() supports mashlibEnabled, pass mashlibEnabled: request.mashlibEnabled into the call rather than setting headers['Vary'] manually afterwards. This keeps header construction centralized and avoids duplicated logic.
- Pass mashlibEnabled into getAllHeaders at every call site in
resource.js/container.js and drop the now-redundant headers['Vary']
overrides. Header construction is centralized in getResponseHeaders
via getVaryHeader — a future edit that forgets the override can no
longer desync Vary across variants. Removes the getVaryHeader
import from both handlers since it's unused now.
- Tighten Cache-Control assertions in the regression tests to pin the
full directive set ("private, no-cache, must-revalidate") instead
of matching on "no-cache|no-store", so dropping "private" or
"must-revalidate" fails the test.
- Rename "varys" → "varyValues" for clarity.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| origin, | ||
| resourceUrl, | ||
| connegEnabled | ||
| }); | ||
| headers['Cache-Control'] = RDF_CACHE_CONTROL; |
There was a problem hiding this comment.
With Cache-Control: private, no-cache, must-revalidate added for these container RDF variants, clients will revalidate using If-None-Match, but handleGet currently performs the 304 check earlier against the container directory stats.etag (not indexStats.etag). For containers served via index.html (and RDF extracted from it), that means revalidation will often return 200 instead of a cheap 304 even when the representation hasn’t changed. Consider moving the If-None-Match check to after you determine the effective ETag for the selected variant (e.g., use indexStats.etag for index.html-backed representations).
| resourceUrl, | ||
| connegEnabled | ||
| connegEnabled, | ||
| mashlibEnabled: request.mashlibEnabled | ||
| }); | ||
| headers['Cache-Control'] = RDF_CACHE_CONTROL; |
There was a problem hiding this comment.
For container listings (no index.html), the ETag used here is stats.etag, which is derived from the directory mtime/size. Directory mtime typically does not change when an existing child resource is modified, but the JSON-LD listing includes per-entry dcterms:modified/stat:size, so the representation can change without the ETag changing. With must-revalidate, clients may get 304 and keep a stale container listing. Consider computing a representation-specific ETag based on the listing content/entries (or otherwise ensure the container ETag changes when any child metadata reflected in the listing changes).
* Phase 1: embed JSON-LD data island in mashlib HTML wrapper (#7) The mashlib HTML wrapper now carries the originating resource's JSON-LD bytes as a `<script type="application/ld+json" id="dataisland" data-uri="…">` block. Phase 1 is strictly additive: - mashlib still XHR-fetches as before (no behaviour change yet); - browsers ignore the script body since its MIME isn't JS; - but anything that knows to look for `application/ld+json` islands — search engines (Rich Results), archival crawlers, scrapers, static-site exporters, future LWS-aware tooling — now finds the data without a second HTTP request. This sets up Phase 2, where a small inline shim will patch mashlib's `fetcher` to read the island instead of refetching, eliminating the double-fetch that motivated the recent Vary/Cache work (#316, #325/#326). Format: JSON-LD (not Turtle), since: - JSS stores JSON-LD natively → zero server-side conversion; - `<script type="application/ld+json">` is the standardised way to embed structured data on the web; - aligns with our LWS / CID work — an HTML-only LWS verifier can parse the embedded JSON-LD and find the CID `service[]` entry without conneg. Sites: - src/mashlib/index.js: `generateDatabrowserHtml(url, cdn, {embedJsonLd})` emits the island when a payload is supplied. New `DATA_ISLAND_MAX_BYTES = 256 * 1024` size cap silently drops the island for oversize resources; the wrapper still works because mashlib falls back to its XHR path. - src/handlers/resource.js: passes the JSON-LD bytes when the stored content type is `application/ld+json` (resources) or whenever a container listing is generated. Security: - `</script>` substrings inside the JSON-LD body are escaped to `<\/script>` so a user-PUTted resource can't close the script tag prematurely and inject HTML. - `<!--` substrings are escaped to neutralise comment-escape tricks. - The `data-uri` attribute is HTML-entity-encoded (&, ", <, >) so attribute-quote injection isn't possible either. 9 new tests in test/data-island.test.js cover: emission shape, the back-compat omission case, the size cap, both `</script>` and `<!--` escapes, attribute encoding, and live HTTP integration for both resources and container listings — plus a negative test that the mashlib XHR path (Accept: application/ld+json) still gets RDF rather than the wrapper. Phase 1 of #7. 546/546 green. * Address Copilot round-1 on #343 Six points, all real: - src/mashlib/index.js: escapeForScriptBlock now encodes EVERY `<` byte as the JSON Unicode escape `<`. The previous narrow regex only caught literal `</script>` and `<!--`, but HTML parsers terminate a <script> element on the prefix `</script` regardless of what follows — `</script >`, `</script\n>`, `</SCRIPT>` and friends all close it. After this transform the body cannot contain a literal `<` at all, so no end-tag (or comment, or CDATA) can possibly start. JSON-LD semantics are preserved because JSON parsers decode `<` back to `<` natively. - src/mashlib/index.js: header comment rewritten to accurately describe the new strategy (the previous comment had been internally inconsistent and described a different approach). - src/mashlib/index.js: generateModuleDatabrowserHtml now also accepts opts.embedJsonLd and emits the data island. Previously module-mode mashlib deployments missed the feature entirely. - src/handlers/resource.js: both wrapper paths (CDN/local + module) now pass the JSON-LD content through. Module mode wraps consume the same opts shape. - src/handlers/resource.js: cap-aware short-circuit — when stats.size > DATA_ISLAND_MAX_BYTES the handler skips storage.read entirely. Previously a 10MB JSON-LD resource would load into memory on every HTML navigation only to have the island silently dropped by generateDatabrowserHtml. - test/data-island.test.js: replaced the single-variant </script> test with a parametrised loop covering exact/whitespace/ newline/uppercase/mixed-case end-tag forms plus <!--. All assert the same invariant: NO literal `<` survives in the script body. Added a module-mode emission test. Full suite: 551/551 (5 new tests). * Address Copilot round-2 on #343 (clarity / dedupe) Three points: - src/mashlib/index.js: escapeForScriptBlock comment now says literally "the six-character JSON escape sequence \\u003c" instead of "<", which had ambiguously rendered the literal escape as just `<` and made the security rationale read backwards. - src/mashlib/index.js: dataIsland's URI escaping now reuses the existing escapeHtml() helper from this file instead of an inline duplicate. Function declarations hoist, so the call site can precede the helper definition without reordering. - test/data-island.test.js: parametrised escape-test header comment rewritten to describe the actual transform (`\\u003c`). Pure clarity / refactor; no behaviour change. Full suite: 551/551. * Address Copilot round-3 on #343 (comment + buffer input) - src/mashlib/index.js: rewrote the escapeForScriptBlock comment to describe the transform unambiguously: "the JSON string-escape for U+003C — the six characters backslash-u-0-0-3-c". The previous attempt kept losing the literal `\\u003c` text to the rendering pipeline and reading as `<` in source, inverting the security rationale. - src/mashlib/index.js: escapeForScriptBlock now coerces input via `String(jsonLdString)`, so a Buffer (e.g. straight from `storage.read()`) passes through cleanly instead of throwing on `.replace`. Buffer.byteLength already accepted both, so dataIsland's size cap was already buffer-safe. - src/mashlib/index.js: JSDoc for `opts.embedJsonLd` now declares `string|Buffer` on both wrapper functions. - test/data-island.test.js: new regression test passing a Buffer payload directly, asserting the island emits with the expected body content. Full suite: 552/552. * Apply size cap to escaped body, not raw input (#343 round-4) DATA_ISLAND_MAX_BYTES was being checked against the *pre-escape* input. escapeForScriptBlock can expand input up to 6x (each `<` byte becomes the six-char escape `\\u003c`), so a `<`-heavy body just under the cap would balloon the HTML response well past it. Move the size check after the escape and gate on the bytes that will actually appear in the response. Comment updated accordingly. New regression test: a half-cap-size payload of pure `<` bytes — 6x expansion guarantees it would slip past the old check but is correctly rejected now. Full suite: 553/553. * Address PR #343 round-5 review (Copilot) Three small refinements: 1. dataIsland(): add cheap raw-byte pre-check before escaping. Since escapeForScriptBlock can only grow the body (each `<` becomes 6 bytes), a raw payload already over the cap is guaranteed to be over after escaping — skip the work. Post-escape check still guards the `<`-heavy expansion case. 2. mashlib/index.js dataIsland doc comment: clarify the 6x escape expansion using prose form for the escape sequence so it survives round-trips through tooling that might otherwise interpret the literal characters. 3. test/data-island.test.js post-escape cap test comment: same clarification. No behaviour change beyond the pre-check fast path; all 553 tests still pass. * Address PR #343 round-6 review (Copilot) Two genuine optimizations from this round (the other flags were re-runs of points already addressed in earlier rounds — pre-check, Buffer coercion, stats.size guard, module-mode wrapper — all already present in HEAD): 1. Container path: use compact JSON.stringify(jsonLd) for the embed instead of serializeJsonLd(), which pretty-prints with 2-space indent. The HTTP body still uses serializeJsonLd; only the inlined data island goes compact, so we don't waste bytes against DATA_ISLAND_MAX_BYTES on whitespace nothing reads. 2. Resource path: pass the storage.read() Buffer through to embedJsonLd directly. dataIsland() already coerces Buffer → string via String(jsonLdString), so the eager buf.toString('utf8') in the handler was redundant. All 553 tests still pass. * PR #343 round-7 review: test comment clarity Two doc-only fixes from this round; the other 7 inline comments re-flag points already addressed in earlier rounds (raw-byte pre-check, Buffer coercion, stats.size guard, module-mode wrapper, compact embed serialization). 1. Header comment: include `data-uri="..."` in the script tag example so it matches the actual emission shape the tests assert. 2. Inline comment on the escape-presence assertion: refer to the six-character escape sequence in prose form ("backslash-u-0-0-3-c") so the source intent survives tooling that converts the literal characters back to `<`. No code change; all 553 tests still pass.
Summary
Browser reloads of mashlib-rendered RDF sometimes showed raw Turtle/JSON-LD body instead of the mashlib view. Hard refresh always worked; soft refresh could fail.
Root cause
Three code paths emitted different `Vary` headers for variants of the same URL:
Chromium caches get confused when variants disagree on `Vary` and can serve the cached Turtle body on top-level navigation → browser renders it as text. Aggressive caching on data variants (no `Cache-Control`) made it stick.
Fix
Security benefit (bonus, not just UX)
Without revalidation, a cached response from one auth state could be served to another. `must-revalidate` + `no-cache` forces the browser to check `If-None-Match` on every use, so auth changes can't leak stale bodies.
Test plan
Fixes #315