From 07aa8c5c9520c1f68c9ff6d4cdfb990b9a0baa0b Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 31 Jul 2026 00:03:01 +0530 Subject: [PATCH 1/5] feat: content-hash author-written asset urls in SSR output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 or 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 `` never contains a literal `]*>/gi; + +/** + * `href` / `src` inside an already-matched tag, either quote style. `srcset` + * is deliberately excluded: it is a comma-separated candidate list with + * descriptors, so it needs its own parse rather than a whole-value rewrite. + * + * @type {RegExp} + */ +const ASSET_ATTR_RE = /(\s(?:href|src)\s*=\s*)(["'])([^"']*)\2/gi; + +/** + * Append `?v=` to same-origin asset urls an APP AUTHOR wrote by + * hand in a template, so they cannot serve stale bytes at a live url. + * + * The framework already fingerprints every url it emits itself (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 `` or an + * `` written in a layout. + * + * Why it matters beyond cache-busting: `dev.js` already serves a `?v=`-carrying + * `/public/*` request `immutable` for a year, while an un-versioned one gets the + * 1h fallback. So fingerprinting these urls both makes staleness structurally + * impossible (new bytes mean a new url) and upgrades them from a 1 hour cache to + * an immutable one. Without it, a CDN keeps serving the previous copy after a + * deploy until something purges it, which is exactly what shipped two visible + * regressions on webjs.dev in one day (#1179 then #1185). + * + * Per-url behaviour is `withAssetHash`, so every existing guard applies + * unchanged: a no-op when fingerprinting is disabled (dev, so dev output stays + * byte-identical), and for a cross-origin, protocol-relative, relative, or + * unresolvable url. A url that already carries a query is skipped here too, + * since it is author-controlled and may be meaningful. + * + * The base path is a parameter rather than module state on purpose: a setter + * that a call site forgets fails silently and only on a sub-path deploy, which + * is the hardest place to notice it. Same signature shape as `withAssetHash`. + * + * @param {string} html the assembled document (or a fragment of it) + * @param {string} [basePath] the active base path, already applied to the urls + * @returns {string} + */ +export function versionAssetUrls(html, basePath = '') { + if (!_enabled || !_appDir) return html; + if (typeof html !== 'string' || html.indexOf('<') === -1) return html; + return html.replace(ASSET_TAG_RE, (tag) => ( + tag.replace(ASSET_ATTR_RE, (whole, lead, quote, url) => { + // Leave an author-supplied query alone: appending to it risks changing + // a meaning we do not own, and `withAssetHash` would merge with `&`. + if (!url || url.indexOf('?') !== -1) return whole; + const versioned = withAssetHash(url, basePath); + return versioned === url ? whole : `${lead}${quote}${versioned}${quote}`; + }) + )); +} diff --git a/packages/server/src/ssr.js b/packages/server/src/ssr.js index e4ecd651f..161d1aeae 100644 --- a/packages/server/src/ssr.js +++ b/packages/server/src/ssr.js @@ -3,7 +3,7 @@ import { resolve } from 'node:path'; import { renderToString, isNotFound, isRedirect, isForbidden, isUnauthorized, lookupModuleUrl, isLazy, cspNonce } from '@webjsdev/core'; import { importMapTag, vendorIntegrityFor, publishedBuildId, appSourceId, basePath, vendorPreconnectOrigins, vendorPreloadTargets } from './importmap.js'; import { withBasePath } from './base-path.js'; -import { withAssetHash } from './asset-hash.js'; +import { withAssetHash, versionAssetUrls } from './asset-hash.js'; import { jsonForScriptTag } from './script-tag-json.js'; import { transitiveDeps, bareImports } from './module-graph.js'; import { seedingEnabled, collectSeeds, buildSeedScript } from './action-seed.js'; @@ -1176,6 +1176,17 @@ function buildHeadInner(opts) { * @returns {{ prefix: string, streamBody: string, closer: string }} */ function buildDocumentParts(body, wrapOpts) { + // Content-hash asset urls the APP AUTHOR wrote by hand (#1194). Applied here + // rather than at each response site so every caller (buffered, streamed, and + // `buildDocument`) is covered by one seam. A pure no-op in dev and whenever + // fingerprinting is off, so output stays byte-identical there. + // + // The basePath is applied to author urls by the app itself (an author writes + // the url they want served), so it is passed only for FILE RESOLUTION, the + // same way `withAssetHash` uses it. + const bp = basePath(); + const fpAssets = (s) => versionAssetUrls(s, bp); + const shell = extractUserShell(body); if (shell) { const headInner = buildHeadInner(wrapOpts); @@ -1187,12 +1198,12 @@ function buildDocumentParts(body, wrapOpts) { `\n\n\n` + composedHead + `\n\n\n`; - return { prefix, streamBody: hoist.body, closer: `\n\n` }; + return { prefix: fpAssets(prefix), streamBody: fpAssets(hoist.body), closer: `\n\n` }; } // No user shell: framework owns the wrapper. const headHtml = wrapHead(wrapOpts); const { head, body: bodyOut } = hoistHeadTags(headHtml, body); - return { prefix: head, streamBody: bodyOut, closer: `\n\n` }; + return { prefix: fpAssets(head), streamBody: fpAssets(bodyOut), closer: `\n\n` }; } // Re-export for unit testing. diff --git a/packages/server/test/importmap/version-asset-urls.test.js b/packages/server/test/importmap/version-asset-urls.test.js new file mode 100644 index 000000000..c6632baac --- /dev/null +++ b/packages/server/test/importmap/version-asset-urls.test.js @@ -0,0 +1,165 @@ +/** + * Unit tests for `versionAssetUrls` (issue #1194): the SSR pass that appends + * `?v=` to same-origin asset urls an APP AUTHOR wrote by hand in a + * template, so a `` cannot serve + * stale bytes at a live url after a deploy. + * + * Two invariants carry the weight here: + * + * - the hash is byte-identical to what `withAssetHash` computes for the same + * file, so an authored url and a framework-emitted one agree on the cache + * key rather than splitting into two; + * - a RENDERED CODE SAMPLE is never rewritten. The docs and brand pages + * display markup, and silently editing the code a reader is looking at + * would be a lie in the output. The matcher is anchored on a literal tag + * name for exactly this reason, since a highlighted sample escapes `<`. + */ +import { test, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { + setAssetRoots, + clearAssetHashCache, + withAssetHash, + versionAssetUrls, +} from '../../src/asset-hash.js'; + +let root; +let appDir; +let coreDir; + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'webjs-versassets-')); + appDir = join(root, 'app-root'); + coreDir = join(root, 'core-root'); + mkdirSync(join(appDir, 'public', 'brand'), { recursive: true }); + mkdirSync(coreDir, { recursive: true }); + writeFileSync(join(appDir, 'public', 'app.css'), 'body{color:red}'); + writeFileSync(join(appDir, 'public', 'brand', 'logo.svg'), ''); + clearAssetHashCache(); + setAssetRoots({ appDir, coreDir, enabled: true }); +}); + +afterEach(() => { + setAssetRoots({ appDir: '', coreDir: '', enabled: false }); + clearAssetHashCache(); + rmSync(root, { recursive: true, force: true }); +}); + +test('an authored stylesheet link is fingerprinted', () => { + const out = versionAssetUrls(''); + const expected = withAssetHash('/public/app.css'); + assert.notEqual(expected, '/public/app.css', 'precondition: the file hashes'); + assert.equal(out, ``); +}); + +test('img and script sources are fingerprinted too', () => { + const img = versionAssetUrls('logo'); + assert.match(img, /src="\/public\/brand\/logo\.svg\?v=[0-9a-f]+"/); + assert.match(img, /alt="logo"/, 'other attributes survive untouched'); +}); + +test('the hash matches what the framework computes for its own urls', () => { + // If these ever diverge, an authored url and a framework-emitted one become + // two cache keys for one file, which is the bug #369 fixed for modules. + const out = versionAssetUrls(''); + const direct = withAssetHash('/public/app.css'); + assert.ok(out.includes(direct), `authored url ${out} should carry ${direct}`); +}); + +test('the hash changes when the file bytes change', () => { + const before = versionAssetUrls(''); + writeFileSync(join(appDir, 'public', 'app.css'), 'body{color:blue}'); + clearAssetHashCache(); + const after = versionAssetUrls(''); + assert.notEqual(before, after, 'new bytes must produce a new url'); +}); + +test('a rendered code sample is never rewritten', () => { + // A highlighter escapes `<`, so the sample never contains a literal `<img ' + + 'src=' + + '"/public/app.css"'; + assert.equal(versionAssetUrls(tokenized), tokenized); +}); + +test('cross-origin, relative, and unresolvable urls are untouched', () => { + for (const url of [ + 'https://cdn.example.com/app.css', + '//cdn.example.com/app.css', + './app.css', + '/public/does-not-exist.css', + ]) { + const html = ``; + assert.equal(versionAssetUrls(html), html, `${url} must not be rewritten`); + } +}); + +test('a url that already carries a query is left alone', () => { + // Author-controlled and possibly meaningful, so we do not append to it. + const html = ''; + assert.equal(versionAssetUrls(html), html); +}); + +test('a non-asset tag is not rewritten', () => { + // An points at a page, not an asset; versioning it would change a + // user-visible link. + const html = 'download'; + assert.equal(versionAssetUrls(html), html); +}); + +test('single-quoted attributes are handled', () => { + const out = versionAssetUrls(""); + assert.match(out, /href='\/public\/app\.css\?v=[0-9a-f]+'/); +}); + +test('it is a no-op when fingerprinting is disabled', () => { + // Dev must stay byte-identical, which is why dev never calls setAssetRoots. + setAssetRoots({ appDir: '', coreDir: '', enabled: false }); + const html = ''; + assert.equal(versionAssetUrls(html), html); +}); + +/* + * Wiring: the unit tests above prove the transform, these prove it is actually + * REACHED by the SSR document assembly. `buildDocumentParts` is the single seam + * every response path funnels through (buffered, streamed, and `buildDocument`), + * which is why the call lives there rather than at each response site. + */ +const SSR_OPTS = { + moduleUrls: [], preloadUrls: [], metadata: {}, + importMap: null, lazyComponents: null, nonce: '', dev: false, +}; + +test('SSR fingerprints an authored asset in the framework-owned shell', async () => { + const { _buildDocumentParts } = await import('../../src/ssr.js'); + const { streamBody } = _buildDocumentParts( + 'logo', + SSR_OPTS, + ); + assert.match(streamBody, /src="\/public\/brand\/logo\.svg\?v=[0-9a-f]+"/); +}); + +test('SSR fingerprints an authored asset in a user-supplied shell', async () => { + // The root layout may write its own …, which takes a + // different branch of buildDocumentParts. Both must fingerprint, or the site + // that writes its own shell (this one does) silently keeps stale urls. + const { _buildDocumentParts } = await import('../../src/ssr.js'); + const { prefix, streamBody } = _buildDocumentParts( + '' + + '', + SSR_OPTS, + ); + assert.match(prefix, /href="\/public\/app\.css\?v=[0-9a-f]+"/, 'head link fingerprinted'); + assert.match(streamBody, /src="\/public\/brand\/logo\.svg\?v=[0-9a-f]+"/, 'body img fingerprinted'); +}); From 452215fcad38808a3ec3077de31ad21b0ff828a7 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 31 Jul 2026 00:04:31 +0530 Subject: [PATCH 2/5] docs: record that authored asset urls are now fingerprinted 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 --- .agents/skills/webjs/references/built-ins.md | 2 +- framework-dev.md | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.agents/skills/webjs/references/built-ins.md b/.agents/skills/webjs/references/built-ins.md index e554513b8..16733c5dc 100644 --- a/.agents/skills/webjs/references/built-ins.md +++ b/.agents/skills/webjs/references/built-ins.md @@ -82,7 +82,7 @@ export const revalidate = 60; // cache this page's HTML for 60s ### Content-hash asset URLs and conditional GET -Both are automatic, prod-focused, and need no config. In production every served module and `public/` asset gets a per-file `?v=` and `Cache-Control: public, max-age=31536000, immutable`, so a returning client fetches a changed file only when its bytes change. Every cacheable response also carries a weak `ETag`, and a repeat request with a matching `If-None-Match` gets a `304 Not Modified` with no body. Unstorable (`no-store`) and streamed responses are excluded from the ETag path. A `private` response IS validated: `private` forbids SHARED storage, not validation, and the ETag hashes that response's own 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, where a 304 discloses nothing (#1140). That is what keeps the client router's partial responses cheap on a page that opted into caching; a default `no-store` page has nothing to validate either way. Dev is byte-faithful (no hashing). +Both are automatic, prod-focused, and need no config. In production every served module gets a per-file `?v=`, and so does a `public/` asset you reference from markup: an `href` / `src` on a ``, ``; controller.enqueue(encoder.encode(chunk)); } From 5bc4dbae7105bc9cd0f986b7691fec2b783979c6 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 31 Jul 2026 01:21:16 +0530 Subject: [PATCH 4/5] fix: gate the asset fingerprinter to servable public urls 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: ``, 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: `` came back `/.env?v=`, 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 `` 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 --- packages/server/src/asset-hash.js | 67 ++++++++++++++++++- packages/server/src/ssr.js | 11 ++- .../test/importmap/version-asset-urls.test.js | 44 ++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/packages/server/src/asset-hash.js b/packages/server/src/asset-hash.js index d316079d6..c8ab1bbf2 100644 --- a/packages/server/src/asset-hash.js +++ b/packages/server/src/asset-hash.js @@ -509,8 +509,71 @@ export function versionAssetUrls(html, basePath = '') { // Leave an author-supplied query alone: appending to it risks changing // a meaning we do not own, and `withAssetHash` would merge with `&`. if (!url || url.indexOf('?') !== -1) return whole; - const versioned = withAssetHash(url, basePath); - return versioned === url ? whole : `${lead}${quote}${versioned}${quote}`; + if (!isServablePublicUrl(url, basePath)) return whole; + // Split a fragment BEFORE hashing. `withAssetHash` appends `?v=` to + // whatever it is handed, so passing the whole thing would emit + // `/x.svg#logo?v=H`: the browser then requests `/x.svg` with no query + // (losing the immutable caching this exists for) AND the fragment + // becomes `logo?v=H`, which matches no element id. An SVG sprite + // (`#icon`) and a media fragment (`#t=10,20`) are both real, so the + // fragment is preserved and re-appended after the query. + const hashIdx = url.indexOf('#'); + const path = hashIdx === -1 ? url : url.slice(0, hashIdx); + const frag = hashIdx === -1 ? '' : url.slice(hashIdx); + const versioned = withAssetHash(path, basePath); + if (versioned === path) return whole; + return `${lead}${quote}${versioned}${frag}${quote}`; }) )); } + +/** + * Is this url one the STATIC-ASSET serve path would actually serve? + * + * This gate exists for security, not tidiness. `resolveUrlToFile` maps any + * same-origin path to `join(_appDir, …)` and only checks containment against + * `_appDir`, which is the PROJECT ROOT. That was harmless while + * `withAssetHash` saw framework-emitted urls exclusively, but an author's + * markup can carry ATTACKER-CONTROLLED data (``, + * a markdown image, a CMS-configured path). Without this check, such a value + * makes the renderer `readFileSync` + hash ANY file under the project root + * and publish the result: `` comes back as + * `/.env?v=`, leaking 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. It also turns one + * attribute into a synchronous read of an arbitrarily large file on the + * render path. + * + * So mirror the serve gate in `dev.js` exactly: under `public/`, plus the two + * root-served remaps and the root favicon. A path outside that set is left + * untouched and never touches the filesystem, which is also why the check is + * a pure string test performed BEFORE any resolution. + * + * Note the three root-served paths pass this gate but currently no-op anyway: + * the serve path REMAPS them into `public/` (`/sw.js` to `/public/sw.js`) + * while `resolveUrlToFile` does not, so the file is not found and the url is + * returned unchanged. That is the correct fail-safe, and they stay listed + * here so this gate reads as the mirror of the serve gate that it is rather + * than silently diverging from it. + * + * @param {string} url + * @param {string} basePath + * @returns {boolean} + */ +function isServablePublicUrl(url, basePath) { + if (url[0] !== '/' || url[1] === '/') return false; + let p = url; + if (basePath && p.startsWith(basePath + '/')) p = p.slice(basePath.length); + // Strip query / fragment before classifying, matching resolveUrlToFile. + const q = p.indexOf('?'); if (q !== -1) p = p.slice(0, q); + const h = p.indexOf('#'); if (h !== -1) p = p.slice(0, h); + let decoded = p; + try { decoded = decodeURIComponent(p); } catch { /* keep raw */ } + // A traversal segment can escape `public/` after `join` normalises it, and + // the serve path rejects exactly this, so refuse rather than resolve. + if (decoded.includes('..')) return false; + return decoded.startsWith('/public/') + || decoded === '/favicon.ico' + || decoded === '/sw.js' + || decoded === '/offline.html'; +} diff --git a/packages/server/src/ssr.js b/packages/server/src/ssr.js index 90deaa92e..2a9ef482c 100644 --- a/packages/server/src/ssr.js +++ b/packages/server/src/ssr.js @@ -157,7 +157,16 @@ export async function ssrPage(route, params, url, opts) { if (frameId && suspenseCtx.pending.length === 0) { const subtree = extractFrameSubtree(body, frameId); if (subtree !== null) { - const frameRes = htmlResponse(subtree, opts.status || 200, opts.req, url); + // Fingerprint authored asset urls here too (#1194). This branch slices + // the raw render output and returns BEFORE `buildDocumentParts`, so + // without this the same `` inside a `` would ship + // versioned on a full page load and un-versioned on a frame nav. The + // frame swap would then overwrite the fingerprinted attribute in the + // live DOM with the stable url, dropping it back to the short cache + // and re-opening exactly the staleness this change closes. It also + // keeps frame-render.js's byte-equivalence invariant (the subtree + // matches what the client would have extracted from the full page). + const frameRes = htmlResponse(versionAssetUrls(subtree, basePath()), opts.status || 200, opts.req, url); // The subtree is sliced by the x-webjs-frame REQUEST header, so a // shared cache must never serve it to a request that did not send // one (the same #1009 poisoning shape as the reduced-have case). diff --git a/packages/server/test/importmap/version-asset-urls.test.js b/packages/server/test/importmap/version-asset-urls.test.js index c6632baac..817836460 100644 --- a/packages/server/test/importmap/version-asset-urls.test.js +++ b/packages/server/test/importmap/version-asset-urls.test.js @@ -130,6 +130,50 @@ test('it is a no-op when fingerprinting is disabled', () => { assert.equal(versionAssetUrls(html), html); }); +/* + * Security: the transform runs over the whole rendered document, and an app's + * markup can carry ATTACKER-CONTROLLED data (``, a + * markdown image, a CMS path). `resolveUrlToFile` maps any same-origin path + * under the PROJECT ROOT, so without a gate the renderer would readFileSync + + * hash any file there and publish the result, leaking existence and a stable + * fingerprint of the bytes for files the serve path deliberately 404s. + */ +test('a private file is never read or fingerprinted', () => { + writeFileSync(join(appDir, '.env'), 'DATABASE_URL=postgres://u:SECRET@h/db'); + mkdirSync(join(appDir, 'db'), { recursive: true }); + writeFileSync(join(appDir, 'db', 'app.db'), 'SQLITE FORMAT 3'); + mkdirSync(join(appDir, 'lib'), { recursive: true }); + writeFileSync(join(appDir, 'lib', 'session.server.ts'), 'export const KEY = 1'); + + for (const url of ['/.env', '/db/app.db', '/lib/session.server.ts']) { + const html = ``; + assert.equal( + versionAssetUrls(html), html, + `${url} must not be fingerprinted: publishing a hash leaks its existence and its bytes`, + ); + } +}); + +test('a traversal out of public/ is refused', () => { + writeFileSync(join(appDir, '.env'), 'SECRET=1'); + // `join` normalises `..`, so without an explicit refusal these would resolve + // to appDir/.env while still looking like a public path. + for (const url of ['/public/../.env', '/public/%2E%2E/.env']) { + const html = ``; + assert.equal(versionAssetUrls(html), html, `${url} must not escape public/`); + } +}); + +test('a fragment is preserved and the query goes before it', () => { + // An SVG sprite (`#icon`) and a media fragment (`#t=10,20`) are real. Naively + // appending would emit `/x.svg#logo?v=H`: the browser then requests `/x.svg` + // with NO query, losing the immutable caching this exists for, and the + // fragment becomes `logo?v=H`, matching no element id. + const out = versionAssetUrls(''); + assert.match(out, /src="\/public\/brand\/logo\.svg\?v=[0-9a-f]+#logo"/); + assert.ok(!/#logo\?v=/.test(out), 'the query must not land after the fragment'); +}); + /* * Wiring: the unit tests above prove the transform, these prove it is actually * REACHED by the SSR document assembly. `buildDocumentParts` is the single seam From 35b337d71b29ccfd31270d4ecbd5e84f0b8b1527 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 31 Jul 2026 01:39:29 +0530 Subject: [PATCH 5/5] fix: stop the fingerprinter touching components, hints, and root paths 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 ``, ``, ``, ``. 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 `` 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 --- packages/server/src/asset-hash.js | 62 ++++++++++++++----- .../test/importmap/version-asset-urls.test.js | 47 ++++++++++++++ 2 files changed, 92 insertions(+), 17 deletions(-) diff --git a/packages/server/src/asset-hash.js b/packages/server/src/asset-hash.js index c8ab1bbf2..bb4864f00 100644 --- a/packages/server/src/asset-hash.js +++ b/packages/server/src/asset-hash.js @@ -456,9 +456,18 @@ export function versionModuleImports(source, importerAbs) { * never be matched here. That is the whole reason this matches tags rather * than bare `src="…"` occurrences. * + * The trailing guard is `(?![-\w])`, NOT `\b`. A word boundary treats `-` as a + * terminator, so `\b` also matched hyphenated CUSTOM ELEMENTS: ``, + * ``, ``. 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 a component + * hydrates with, so `this.src.endsWith('.svg')` went false and an equality + * check against the path a server action returned stopped matching, and the + * value churned every deploy. + * * @type {RegExp} */ -const ASSET_TAG_RE = /<(?:link|script|img|source)\b[^>]*>/gi; +const ASSET_TAG_RE = /<(?:link|script|img|source)(?![-\w])[^>]*>/gi; /** * `href` / `src` inside an already-matched tag, either quote style. `srcset` @@ -469,6 +478,17 @@ const ASSET_TAG_RE = /<(?:link|script|img|source)\b[^>]*>/gi; */ const ASSET_ATTR_RE = /(\s(?:href|src)\s*=\s*)(["'])([^"']*)\2/gi; +/** + * A `` whose href is a HINT for a later request rather than the request + * itself. See the call site: versioning one desynchronizes it from whatever + * actually fetches the asset. `modulepreload` is listed for completeness, but + * those hrefs are framework-emitted and already carry `?v=`, so the + * already-queried guard skips them anyway. + * + * @type {RegExp} + */ +const PRELOAD_REL_RE = /\srel\s*=\s*["']?(?:preload|prefetch|modulepreload|dns-prefetch|preconnect)\b/i; + /** * Append `?v=` to same-origin asset urls an APP AUTHOR wrote by * hand in a template, so they cannot serve stale bytes at a live url. @@ -505,7 +525,15 @@ export function versionAssetUrls(html, basePath = '') { if (!_enabled || !_appDir) return html; if (typeof html !== 'string' || html.indexOf('<') === -1) return html; return html.replace(ASSET_TAG_RE, (tag) => ( - tag.replace(ASSET_ATTR_RE, (whole, lead, quote, url) => { + // A preload / prefetch hint does not FETCH the asset, it warms the cache + // for a request some OTHER consumer makes later, and the preload cache is + // keyed on the full url including the query. The obvious case is a font: + // the real request comes from `@font-face url()` inside the stylesheet, + // and CSS `url()` is deliberately out of scope here, so versioning the + // hint guarantees it can never match. The asset is then fetched TWICE and + // the browser logs "preloaded but not used", turning an optimization into + // a pessimization. Leave the hint on the url its consumer will request. + PRELOAD_REL_RE.test(tag) ? tag : tag.replace(ASSET_ATTR_RE, (whole, lead, quote, url) => { // Leave an author-supplied query alone: appending to it risks changing // a meaning we do not own, and `withAssetHash` would merge with `&`. if (!url || url.indexOf('?') !== -1) return whole; @@ -544,17 +572,20 @@ export function versionAssetUrls(html, basePath = '') { * attribute into a synchronous read of an arbitrarily large file on the * render path. * - * So mirror the serve gate in `dev.js` exactly: under `public/`, plus the two - * root-served remaps and the root favicon. A path outside that set is left - * untouched and never touches the filesystem, which is also why the check is - * a pure string test performed BEFORE any resolution. - * - * Note the three root-served paths pass this gate but currently no-op anyway: - * the serve path REMAPS them into `public/` (`/sw.js` to `/public/sw.js`) - * while `resolveUrlToFile` does not, so the file is not found and the url is - * returned unchanged. That is the correct fail-safe, and they stay listed - * here so this gate reads as the mirror of the serve gate that it is rather - * than silently diverging from it. + * So the gate is `public/` only. A path outside it is left untouched and never + * touches the filesystem, which is why the check is a pure string test run + * BEFORE any resolution. + * + * The three root-served paths (`/favicon.ico`, `/sw.js`, `/offline.html`) are + * deliberately NOT admitted, even though the serve path answers them. It + * REMAPS them into `public/` while `resolveUrlToFile` does not, so admitting + * them hashed the wrong file: 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 stamped them `immutable` for a + * year. Changing the real icon then left the url byte-identical, pinning the + * stale copy indefinitely, which is worse than the 1h cache this replaces. + * Admitting them would need the remap replicated here, and they gain nothing: + * a page references its icon as `/public/favicon.svg` anyway. * * @param {string} url * @param {string} basePath @@ -572,8 +603,5 @@ function isServablePublicUrl(url, basePath) { // A traversal segment can escape `public/` after `join` normalises it, and // the serve path rejects exactly this, so refuse rather than resolve. if (decoded.includes('..')) return false; - return decoded.startsWith('/public/') - || decoded === '/favicon.ico' - || decoded === '/sw.js' - || decoded === '/offline.html'; + return decoded.startsWith('/public/'); } diff --git a/packages/server/test/importmap/version-asset-urls.test.js b/packages/server/test/importmap/version-asset-urls.test.js index 817836460..f96abba71 100644 --- a/packages/server/test/importmap/version-asset-urls.test.js +++ b/packages/server/test/importmap/version-asset-urls.test.js @@ -164,6 +164,53 @@ test('a traversal out of public/ is refused', () => { } }); +test('a hyphenated custom element is never rewritten', () => { + // This framework is web-components-first, so `img-*` / `link-*` / `source-*` + // are ordinary component names and their `src` / `href` is a reactive PROP, + // i.e. author data. A `\b` guard treats `-` as a word boundary and matched + // them, so a component hydrated with a mutated prop: `this.src` no longer + // ended in `.svg`, an equality check against the path a server action + // returned stopped matching, and the value churned on every deploy. + for (const tag of [ + '', + '', + '', + '', + ]) { + assert.equal(versionAssetUrls(tag), tag, `${tag} is a component, not an asset tag`); + } +}); + +test('a preload hint is left on the url its consumer will request', () => { + // A 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. A font's real request comes from `@font-face url()` in CSS, + // which is out of scope here, so versioning the hint guarantees a miss: the + // font is fetched twice and the browser logs "preloaded but not used". + for (const rel of ['preload', 'prefetch', 'preconnect']) { + const tag = ``; + assert.equal(versionAssetUrls(tag), tag, `rel=${rel} is a hint, not the request`); + } + // A normal stylesheet link IS the request, so it still gets versioned. + assert.match( + versionAssetUrls(''), + /\?v=[0-9a-f]+/, + ); +}); + +test('a root-served path is not fingerprinted against the wrong file', () => { + // The serve path REMAPS /favicon.ico into public/, and resolveUrlToFile does + // not. Admitting it hashed a stray project-root file while the server served + // public/'s bytes, and stamped them immutable for a year, so changing the + // real icon left the url identical and pinned the stale copy indefinitely. + writeFileSync(join(appDir, 'favicon.ico'), 'STRAY-ROOT-FILE'); + writeFileSync(join(appDir, 'public', 'favicon.ico'), 'THE-REAL-ONE'); + for (const url of ['/favicon.ico', '/sw.js', '/offline.html']) { + const html = ``; + assert.equal(versionAssetUrls(html), html, `${url} must not be versioned from the wrong file`); + } +}); + test('a fragment is preserved and the query goes before it', () => { // An SVG sprite (`#icon`) and a media fragment (`#t=10,20`) are real. Naively // appending would emit `/x.svg#logo?v=H`: the browser then requests `/x.svg`