From 6a646754abda7294b4088fcfe6ca9b8e6a40fc5b Mon Sep 17 00:00:00 2001 From: Rick Caves Date: Thu, 17 Sep 2026 13:05:54 -0700 Subject: [PATCH] fix(site-import): resolve cache-busted asset URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A query string or fragment was never stripped before looking a path up in the FileMap, so `assets/site.css?v=8f3a1c` resolved to a key that cannot exist. The reference was reported as `missing-stylesheet` / `missing-script` while the file sat in the archive, and every style rule, colour token and font in that sheet was silently dropped from the import. Static site generators commonly emit a content hash on stylesheet, script and media URLs, so this affects a whole class of input. Measured on one real 6-page export: 401 style rules and 18 colour tokens imported, against 659 and 26 for the byte-identical site without the queries — a 39% and 31% loss, reported only as warnings a user can click past. The handling was inconsistent rather than absent, which is why it survived: `linkRewrite` stripped the query itself before calling `resolveHref`, so internal page links resolved correctly while stylesheets did not, and `assetPlan` stripped one for MIME sniffing but not for resolution. Fixed at both resolution primitives: - `resolveHref` (stylesheets, scripts, CSS @import, and page links via linkRewrite) - `resolveRelativePath` (CSS url(), node src/href/srcset) and removed the now-redundant strip in `linkRewrite`. Its fragment split stays — the fragment is re-attached to the page ref and is not addressing. This cannot affect the rewrite step. Node props are replaced by the resolved FileMap key and `rewriteProps` keys on that key, while CSS passes the original `rawUrl` to `replaceRawUrlInValue` as a separate argument, so stripping changes only which entry is found. Tests: a unit group on `resolveHref` covering query, fragment, both, and empty-after-strip, plus an assertion that a query does not change traversal handling; and an end-to-end group over `buildImportPlan` asserting that a cache-busted site produces a plan identical to the same site without queries. The end-to-end shape matters here — the unit returned a plausible-looking key and every consumer dutifully failed to find it, so the defect was only ever visible downstream. Verified the new tests fail without the source change (11 failures) and pass with it. bun run build, bun test (7067), bun run lint all pass. Co-Authored-By: Claude Opus 5 (1M context) --- docs/features/site-import.md | 11 +- .../siteImport/cacheBustedUrls.test.ts | 110 ++++++++++++++++++ src/__tests__/siteImport/htmlPagePlan.test.ts | 36 ++++++ src/core/siteImport/assetPlan.ts | 15 ++- src/core/siteImport/htmlPagePlan.ts | 16 ++- src/core/siteImport/linkRewrite.ts | 7 +- 6 files changed, 184 insertions(+), 11 deletions(-) create mode 100644 src/__tests__/siteImport/cacheBustedUrls.test.ts diff --git a/docs/features/site-import.md b/docs/features/site-import.md index d3e95af1a..50d5cd5eb 100644 --- a/docs/features/site-import.md +++ b/docs/features/site-import.md @@ -374,13 +374,22 @@ On success the same step switches to its **complete** state — a success mark, | `invalid-rule` | A CSS rule caused `replaceSync` to throw (sheet-level parse error) | | `blocked-property` | A CSS property name is on the security denylist (`behavior`, `-moz-binding`, …) — declaration dropped | | `duplicate-class` | Two `.foo {}` rules in the same file; later declarations win | -| `missing-stylesheet` | A `` href was not found in the FileMap | +| `missing-stylesheet` | A `` href was not found in the FileMap (a cache-busting `?v=…` query is stripped before the lookup, so it is not a cause) | | `asset-upload-failed` | An individual asset upload was rejected by the server; the original FileMap path remains in the import | | `asset-folder-failed` | The asset uploaded and its URL was rewritten, but filing it under the folder mirroring its bundle path failed; it sits at the media root | | `script-order-conflict` | Two pages link the same scripts in contradictory orders; the order from the page that links them first wins | | `external-font` | An `@font-face` with no bundled file (all `src` entries are external URLs) — skipped | | `unresolved-asset` | An HTML/CSS reference to a media file the archive does not contain under that path — one warning per distinct path | +### Cache-busted URLs + +Reference resolution strips any query string or fragment before looking a path up in the FileMap: +`assets/site.css?v=8f3a1c` and `assets/site.css` address the same file. This applies to +``, `' + + '' + + 'Hero' + + 'About' + + '', + ), + mimeType: 'text/html', + }, + 'about.html': { bytes: txt('

About

'), mimeType: 'text/html' }, + 'assets/site.css': { bytes: txt(CSS), mimeType: 'text/css' }, + 'assets/app.js': { bytes: txt('console.log("app")'), mimeType: 'application/javascript' }, + 'assets/img/hero.png': { bytes: new Uint8Array([0x89, 0x50, 0x4e, 0x47]), mimeType: 'image/png' }, + }, + } +} + +/** The same site with every query removed — the control. */ +function plainSite(): FileMap { + const src = cacheBustedSite() + const files: FileMap['files'] = {} + for (const [k, v] of Object.entries(src.files)) { + files[k] = k.endsWith('.html') || k.endsWith('.css') + ? { ...v, bytes: txt(new TextDecoder().decode(v.bytes).replace(/\?(?:v=\w+|utm_source=nav)/g, '')) } + : v + } + return { files } +} + +describe('cache-busted asset URLs', () => { + it('a stylesheet with ?v= is found, not reported missing', () => { + const plan = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + const missing = plan.warnings.filter((w) => w.kind === 'missing-stylesheet') + expect(missing).toHaveLength(0) + // Select by source — page order is not part of this contract. + const index = plan.pages.find((p) => p.source === 'index.html')! + expect(index.linkedCssPaths).toContain('assets/site.css') + }) + + it('a script with ?v= is found, not reported missing', () => { + const plan = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + expect(plan.warnings.filter((w) => w.kind === 'missing-script')).toHaveLength(0) + }) + + it('its style rules and root colour tokens are actually parsed', () => { + // The user-visible cost of the bug: the sheet was skipped, so every rule and token in it was lost. + const plan = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + expect(plan.styleRules.length).toBeGreaterThan(0) + expect(plan.colors.some((c) => c.value.toLowerCase() === '#0055ff')).toBe(true) + }) + + it('a cache-busted image is queued as an asset', () => { + const plan = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + expect(plan.assets.some((a) => a.sourcePath === "assets/img/hero.png")).toBe(true) + }) + + it('a cache-busted url() inside CSS resolves too', () => { + const plan = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + expect(plan.warnings.filter((w) => w.kind === 'unresolved-asset')).toHaveLength(0) + }) + + it('produces the same plan as the identical site without query strings', () => { + // The strongest statement available: cache-busting is addressing, not content, so it must make no + // difference to the outcome at all. + const busted = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + const plain = buildImportPlan({ fileMap: plainSite(), currentSite: makeEmptySiteDocument() }) + expect(busted.pages).toHaveLength(plain.pages.length) + expect(busted.styleRules).toHaveLength(plain.styleRules.length) + expect(busted.colors).toHaveLength(plain.colors.length) + expect(busted.assets).toHaveLength(plain.assets.length) + expect(busted.warnings).toHaveLength(plain.warnings.length) + }) + + it('an internal page link keeps working when it carries a query', () => { + // linkRewrite used to strip the query itself before calling resolveHref. That strip now lives in + // resolveHref, so this pins that the behaviour survived the move. + const plan = buildImportPlan({ fileMap: cacheBustedSite(), currentSite: makeEmptySiteDocument() }) + expect(plan.pages.map((p) => p.slug).sort()).toEqual(['about', 'index']) + }) +}) diff --git a/src/__tests__/siteImport/htmlPagePlan.test.ts b/src/__tests__/siteImport/htmlPagePlan.test.ts index ea3308314..110d87990 100644 --- a/src/__tests__/siteImport/htmlPagePlan.test.ts +++ b/src/__tests__/siteImport/htmlPagePlan.test.ts @@ -95,6 +95,42 @@ describe('resolveHref', () => { expect(resolveHref('style.css', 'index.html')).toBe('style.css') }) + // Static site generators routinely cache-bust stylesheet and script URLs. The FileMap is keyed by path, so + // a query has to be stripped before the lookup or the reference resolves to a key that cannot exist and is + // reported missing while the file sits in the archive. + it('strips a cache-busting query string', () => { + expect(resolveHref('assets/site.css?v=8f3a1c', 'index.html')).toBe('assets/site.css') + }) + + it('strips a query string on a root-relative href', () => { + expect(resolveHref('/assets/site.css?v=8f3a1c', 'pages/about.html')).toBe('assets/site.css') + }) + + it('strips a query string before resolving a parent-relative href', () => { + expect(resolveHref('../css/main.css?ver=2', 'pages/about.html')).toBe('css/main.css') + }) + + it('strips a trailing fragment', () => { + expect(resolveHref('assets/icons.svg#arrow', 'index.html')).toBe('assets/icons.svg') + }) + + it('strips both a query and a fragment', () => { + expect(resolveHref('assets/icons.svg?v=2#arrow', 'index.html')).toBe('assets/icons.svg') + }) + + it('returns null when nothing remains after stripping', () => { + expect(resolveHref('?v=8f3a1c', 'index.html')).toBeNull() + }) + + // The point is not what traversal resolves TO — `joinPaths` clamps an escaping `..` to the root by design, + // which is pre-existing behaviour. The point is that stripping a query must not CHANGE that handling. + it('a query string does not alter traversal handling', () => { + for (const href of ['../../secrets.css', '../secrets.css', 'a/../../secrets.css', '/../secrets.css']) { + expect(resolveHref(`${href}?v=1`, 'index.html')).toBe(resolveHref(href, 'index.html')) + expect(resolveHref(`${href}#frag`, 'pages/about.html')).toBe(resolveHref(href, 'pages/about.html')) + } + }) + it('resolves nested relative href', () => { expect(resolveHref('./main.css', 'styles/base.css')).toBe('styles/main.css') }) diff --git a/src/core/siteImport/assetPlan.ts b/src/core/siteImport/assetPlan.ts index b130389f1..f3668315a 100644 --- a/src/core/siteImport/assetPlan.ts +++ b/src/core/siteImport/assetPlan.ts @@ -652,13 +652,22 @@ function normalizeAssetPath(path: string): string { /** * Resolve a raw URL against a base file path to produce a FileMap key. * Returns null for traversal-escaping paths or empty strings. + * + * As in `resolveHref`, a query string or fragment is stripped before the + * lookup — it addresses a file, it does not identify one. Callers keep the + * original `rawUrl` for the in-place CSS string replacement + * (`replaceRawUrlInValue`), so stripping here affects only which FileMap entry + * is found. */ function resolveRelativePath(rawUrl: string, basePath: string): string | null { const baseDir = dirname(basePath) - const resolved = rawUrl.startsWith('/') - ? rawUrl.slice(1) // root-relative: strip leading / - : joinPaths(baseDir, rawUrl) + const pathOnly = rawUrl.split(/[?#]/)[0] + if (!pathOnly) return null + + const resolved = pathOnly.startsWith('/') + ? pathOnly.slice(1) // root-relative: strip leading / + : joinPaths(baseDir, pathOnly) // Reject escaped or empty results if (!resolved || resolved.startsWith('../')) return null diff --git a/src/core/siteImport/htmlPagePlan.ts b/src/core/siteImport/htmlPagePlan.ts index dfa9868ab..459a63e15 100644 --- a/src/core/siteImport/htmlPagePlan.ts +++ b/src/core/siteImport/htmlPagePlan.ts @@ -292,14 +292,24 @@ function attrValue(attrs: string, name: string): string | null { * * Returns a normalized FileMap key, or null when the href is external, * a fragment, a data URL, or cannot be resolved to a safe relative path. + * + * A query string or fragment is stripped first: `site.css?v=8f3a1c` addresses + * the same file as `site.css`, and the FileMap is keyed by path. Static site + * generators routinely emit cache-busting queries on stylesheet and script + * URLs, and without this every one of them resolves to a key that cannot + * exist — reported as `missing-stylesheet` / `missing-script` while the file + * sits in the archive. */ export function resolveHref(href: string, htmlFilePath: string): string | null { // Skip external, protocol-relative, data, fragment, mailto, tel if (/^https?:\/\/|^\/\/|^data:|^mailto:|^tel:|^#/.test(href)) return null - const normalized = href.startsWith('/') - ? href.slice(1) // root-relative: strip leading / - : joinPaths(dirname(htmlFilePath), href) + const pathOnly = href.split(/[?#]/)[0] + if (!pathOnly) return null + + const normalized = pathOnly.startsWith('/') + ? pathOnly.slice(1) // root-relative: strip leading / + : joinPaths(dirname(htmlFilePath), pathOnly) // Must not escape to a parent-of-root path if (!normalized || normalized.startsWith('../') || normalized.includes('/../')) return null diff --git a/src/core/siteImport/linkRewrite.ts b/src/core/siteImport/linkRewrite.ts index 5554d49c3..94930f212 100644 --- a/src/core/siteImport/linkRewrite.ts +++ b/src/core/siteImport/linkRewrite.ts @@ -67,12 +67,11 @@ function hrefToPageRef( ): string | null { if (!href) return null - // Split off the fragment (kept) and any query (dropped — CMS routes are slugs). + // Split off the fragment, which is KEPT and re-attached to the page ref below. + // Any query string is dropped by `resolveHref` — CMS routes are slugs. const hashIdx = href.indexOf('#') const fragment = hashIdx >= 0 ? href.slice(hashIdx) : '' - let pathPart = hashIdx >= 0 ? href.slice(0, hashIdx) : href - const queryIdx = pathPart.indexOf('?') - if (queryIdx >= 0) pathPart = pathPart.slice(0, queryIdx) + const pathPart = hashIdx >= 0 ? href.slice(0, hashIdx) : href // Pure same-page anchor (`#features`) or empty → not an internal page link. if (!pathPart) return null