feat: content-hash author-written asset urls so they cannot go stale - #1196
Closed
vivek7405 wants to merge 5 commits into
Closed
feat: content-hash author-written asset urls so they cannot go stale#1196vivek7405 wants to merge 5 commits into
vivek7405 wants to merge 5 commits into
Conversation
An app author's own asset urls never got the framework's content hash, so they sat at stable urls and went stale behind a CDN. That is what shipped two visible regressions on webjs.dev in one day (a pre-redesign tailwind.css after #1179, then the un-fixed logo marks after #1185), and it is the reason the purge workflow exists at all. The framework already fingerprints everything IT emits (module specifiers, importmap targets, modulepreload hints), and versionModuleImports does the same for author-written module specifiers in served source. This closes the last gap: a <link> or <img> written by hand in a layout. The serving half was already built. dev.js serves a `?v=`-carrying /public/ request `immutable` for a year while an un-versioned one gets the 1h fallback, verified live against the origin. So this both makes staleness structurally impossible (new bytes mean a new url) and upgrades these assets from a 1 hour cache to an immutable one. Applied in buildDocumentParts, the one seam every response path funnels through (buffered, streamed, and buildDocument), so no call site can be missed. Both branches are covered, including the user-supplied shell that the website itself uses. The matcher is anchored on a literal tag name, and that is load-bearing rather than incidental: this site RENDERS CODE SAMPLES of markup, and a bare `src="…"` match would rewrite the code a reader is looking at. A highlighted sample escapes `<` and splits the token across spans, so a literal `<img` never appears in one and cannot be reached. Tests pin that, since it is the property the whole approach rests on. Per-url behaviour delegates to withAssetHash, so every existing guard holds unchanged: a no-op in dev (output stays byte-identical), and for cross-origin, protocol-relative, relative, and unresolvable urls. An author-supplied query is left alone. basePath is a parameter rather than module state on purpose: a setter a call site forgets fails silently and only on a sub-path deploy, the hardest place to notice it. Counterfactuals verified both layers: neutering the transform fails the 5 positive unit assertions, and neutering the ssr.js wiring fails the 2 integration assertions. Refs #1194
framework-dev.md said these urls were NOT fingerprinted and that the CDN purge was what covered them, which is now the opposite of the truth. built-ins.md already CLAIMED every public/ asset got a `?v=` hash in production. That was inaccurate before this change (an author-written url never reached the fingerprinter), and it is accurate now, but it was also overbroad in a way worth fixing: an asset reached from a stylesheet `url()`, a srcset candidate list, or a runtime-built string still keeps its plain path and the 1h fallback. Say which references are covered rather than implying all of them are. Both notes keep the purge workflow's justification intact, since it remains the safety net for exactly those uncovered cases. The skill lives canonically once at the repo-root .agents/skills/webjs/ (see create.js L643), so a scaffolded app inherits this with no second copy to sync. Refs #1194
Review found a coverage gap. A streamed boundary's HTML is enqueued straight onto the response stream and never passes through buildDocumentParts, so the SAME `<img src="/public/logo.svg">` picked up `?v=` when it rendered in the shell but not when it resolved inside a `<webjs-suspense>`. Nothing broke (a streamed asset kept today's 1h cache and stayed purge-covered), but behaviour differing by WHERE a component happens to render is a difference no author could predict from the code they wrote. Apply the same transform to the boundary chunk. It is the identical function with the identical guards, and it is naturally idempotent because a url already carrying `?v=` is skipped, so a chunk that somehow passed through twice is unchanged. Also measured the cost while reviewing, since this runs over the whole document on every render: 0.22ms at 50KB, 0.60ms at 500KB, and 1.10ms at 1.1MB (the size the changelog page actually reaches). Not a concern. No new failures against the clean-tree baseline (69 pre-existing environmental failures, name-for-name identical). Refs #1194
Deep review found three confirmed defects in this change, all jury-backed
and all reproduced here before fixing.
SECURITY, the serious one. `versionAssetUrls` runs over the whole
rendered document, and an app's markup can carry attacker-controlled
data: `<img src=${user.avatarUrl}>`, a markdown image, a CMS-configured
path. `resolveUrlToFile` maps ANY same-origin path under the project
root and checks containment against that root only, which was harmless
while it saw framework-emitted urls exclusively. Fed author urls, it
turned one attribute into a file-existence and content-fingerprint
oracle: `<img src="/.env">` came back `/.env?v=<hash>`, publishing both
that the file exists and a stable fingerprint of its exact bytes (so a
rotation, or a guessed value, is detectable) for files the serve path
deliberately 404s. Reproduced against /.env, /db/app.db, and a
.server.ts source. It was also a synchronous read of an
attacker-chosen file on the render path.
Mirror the serve gate from dev.js instead: under public/, plus the
root-served remaps, refusing traversal. The check is a pure string test
run BEFORE any resolution, so a rejected path never touches the disk.
FRAGMENTS. The guard skipped a url containing `?` but not `#`, so
`/x.svg#logo` became `/x.svg#logo?v=H`. The browser then requested
`/x.svg` with no query, silently losing the immutable caching this
exists for, and the fragment became `logo?v=H`, matching no element id.
SVG sprites and media fragments are both real. Split the fragment, hash
the path, re-append.
FRAMES. The `x-webjs-frame` branch slices the raw render output and
returns before buildDocumentParts, so the same `<img>` shipped versioned
on a full load and un-versioned on a frame nav, and the frame swap wrote
the stable url over the fingerprinted one in the live DOM. That
re-opened the exact staleness this change closes, and broke
frame-render.js's byte-equivalence invariant. Same shape as the streamed
Suspense gap already fixed here, and missed for the same reason.
Each fix has a regression test with a verified counterfactual, and the
full suite is failure-for-failure identical to the clean-tree baseline.
Refs #1194
Deep review round 2 found three more confirmed defects, all reproduced
here before fixing.
CUSTOM ELEMENTS. `ASSET_TAG_RE` ended in `\b`, which treats `-` as a
word boundary, so it also matched `<img-zoom>`, `<link-preview>`,
`<source-map>`, `<script-editor>`. This framework is web-components-
first, so those are ordinary component names and their `src` / `href` is
a reactive PROP, meaning author data. Rewriting it changed the value the
component hydrated with: `this.src.endsWith('.svg')` went false, an
equality check against the path a server action returned stopped
matching, and the value churned every deploy. Use `(?![-\w])`.
PRELOAD HINTS. A `<link rel=preload>` does not fetch the asset, it warms
the cache for a request some OTHER consumer makes, and that cache is
keyed on the full url including the query. The website preloads three
self-hosted fonts whose real request comes from `@font-face url()` in
the stylesheet, and CSS `url()` is deliberately out of scope here, so
versioning the hint guaranteed a miss: each font fetched twice, with the
browser logging "preloaded but not used", turning the optimization at
layout.ts into a pessimization on the next deploy. Skip hint rels.
ROOT-SERVED PATHS. The gate admitted `/favicon.ico`, `/sw.js`, and
`/offline.html` to mirror the serve path, but the serve path REMAPS them
into `public/` and `resolveUrlToFile` does not. With a stray
`favicon.ico` at the project root, `/favicon.ico` was stamped with the
hash of the ROOT file while the server served `public/favicon.ico`'s
bytes and marked them immutable for a year, so changing the real icon
left the url identical and pinned the stale copy indefinitely, strictly
worse than the 1h cache it replaced. My earlier comment claiming these
"no-op anyway" was only true when no root file existed, and nothing
enforced that. Drop them: they gain nothing, since a page references its
icon as `/public/favicon.svg`.
Each fix has a regression test with a verified counterfactual (18 now),
and the suite stays failure-for-failure identical to the clean baseline.
Refs #1194
Collaborator
Author
|
Closing unmerged. Two deep-review rounds found six confirmed major defects, five of them the same structural bug: a regex over the assembled document cannot tell framework output from author data. Full rationale and the replacement design are on #1194. Superseded by an explicit opt-in |
8 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An app author's own asset urls never got the framework's content hash, so they sat at stable urls and went stale behind a CDN. That is what shipped two visible regressions on webjs.dev in one day (pre-redesign
tailwind.cssafter #1179, un-fixed logo marks after #1185), and it is why the purge workflow exists.The framework already fingerprints everything it emits, and
versionModuleImportsdoes the same for author-written module specifiers (#369). This closes the last gap: a<link>or<img>written by hand in a layout.The serving half was already built. Verified live against the origin:
Cache-Control/public/tailwind.csspublic, max-age=3600/public/tailwind.css?v=abc123public, max-age=31536000, immutableSo this makes staleness structurally impossible and upgrades these assets from a 1 hour cache to an immutable one.
Why the matcher is anchored on a tag name
This site renders code samples of markup. A bare
src="…"match would rewrite the code a reader is looking at, on/docsand/brand. A highlighted sample escapes<and splits the token across spans, so a literal<imgnever appears in one and cannot be reached. That property is pinned by a test, because the whole approach rests on it.Where it hooks in
buildDocumentPartsinssr.js, the one seam every response path funnels through (buffered, streamed,buildDocument), so no call site can be missed. Both branches are covered, including the user-supplied shell the website itself uses.basePathis a parameter, not module state: a setter a call site forgets fails silently and only on a sub-path deploy, the hardest place to notice.Test plan
withAssetHash, hash changes with bytes, dev no-op, cross-origin / relative / already-queried / unresolvable untouched, non-asset<a href>untouched, single quotesbuildDocumentPartsbranchesdist, unchanged)webjs checkcleanDocs
framework-dev.mdsaid these urls were NOT fingerprinted, now the opposite of the truth.built-ins.mdalready claimed everypublic/asset got?v=, which was inaccurate before and is accurate now, but also overbroad: an asset reached from a stylesheeturl(), asrcsetlist, or a runtime-built string still keeps the 1h fallback. Both corrected. The purge workflow stays as the safety net for exactly those cases.Refs #1194