diff --git a/CLAUDE.md b/CLAUDE.md index 001bd87..1b9aa8c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,8 +130,8 @@ Self-contained Playwright E2E — `npm test` auto-starts everything (no external Tests drive the host origin (`http://localhost:4201`). Suites (`tests/`): - `build-integrity.spec.js` — `dist/` output: both apps enumerated, absolute URL rewriting, - headless markup, stable `dist/style.css` (the marketplace CSS the sub-app pages reference), - and single-page bundle expansion (`tests/fixtures/single-page-bundle/` → two apps). + headless markup, the content-hashed marketplace stylesheet plus its stable `dist/style.css` + alias, and single-page bundle expansion (`tests/fixtures/single-page-bundle/` → two apps). - `web-fragment.spec.js` — shadow-DOM isolation (reframed `wf-html`/`wf-body`; chrome must not leak in), routing + smooth no-reload SPA transitions, cross-app navigation, asset loading (no host-origin 404s), and the documented history limitation (fragment routing is @@ -140,7 +140,9 @@ Tests drive the host origin (`http://localhost:4201`). Suites (`tests/`): Two build-pipeline pieces support this: `apps.json` entries may carry a `prebuilt` path (tarball or dist dir) consumed by `scripts/build-vite.js` (`preparePrebuilt`) for hermetic -offline builds; and the build aliases the bundled marketplace CSS to a stable `dist/style.css`. +offline builds; and the build copies the marketplace stylesheet — identified as the local +stylesheet the landing page loads — to a stable `dist/style.css` alias. Pages themselves +reference the content-hashed bundle Astro injects, so nothing depends on that filename. An entry may also carry `"optional": true`: the build then skips it with a warning when its `prebuilt`/`localPath` artifact is missing, instead of failing. That is how the sibling diff --git a/astro.config.mjs b/astro.config.mjs index a4b78ac..1525eb1 100644 --- a/astro.config.mjs +++ b/astro.config.mjs @@ -25,15 +25,17 @@ export default defineConfig({ }, ], css: { modules: false }, - build: { - rollupOptions: { - output: { - // Stable filename so sub-app pages can reference /{PREFIX}/style.css - assetFileNames: (info) => - info.names?.some(n => n.endsWith('.css')) ? 'style.css' : '_astro/[name]-[hash][extname]', - }, - }, - }, + // No assetFileNames override: CSS is content-hashed like every other asset. + // + // This used to force the name "style.css" onto every CSS asset so that + // /{PREFIX}/style.css was a fixed path. Nothing needs a fixed path — the + // on every page is injected by Astro from Base.astro's CSS import, so + // it always carries whatever name the bundle was given. Forcing a constant + // name only made Rollup disambiguate collisions as style.css / style2.css, + // which the build then had to guess between, and it defeated cache-busting + // for the one stylesheet every page loads (#50). scripts/build-vite.js still + // publishes dist/style.css as an alias of this bundle for anything outside + // this repository that refers to it by that path. }, }); diff --git a/package-lock.json b/package-lock.json index 9f74cab..7f8626c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,7 +8,8 @@ "name": "knowledge-base", "version": "1.0.0", "dependencies": { - "ajv": "^8.17.1" + "ajv": "^8.17.1", + "parse5": "^7.3.0" }, "devDependencies": { "@astrojs/check": "^0.9.10", @@ -3569,7 +3570,6 @@ "version": "6.0.1", "resolved": "https://registry.npmjs.org/entities/-/entities-6.0.1.tgz", "integrity": "sha512-aN97NXWF6AWBTahfVOIrB/NShkzi5H7F9r1s9mD3cDj4Ko5f2qhhVoYMibXF7GlLveb/D2ioWay8lxI97Ven3g==", - "dev": true, "license": "BSD-2-Clause", "engines": { "node": ">=0.12" @@ -5117,7 +5117,6 @@ "version": "7.3.0", "resolved": "https://registry.npmjs.org/parse5/-/parse5-7.3.0.tgz", "integrity": "sha512-IInvU7fabl34qmi9gY8XOVxhYyMyuH2xUNpb2q8/Y+7552KlejkRvqvD19nMoUW/uQGGbqNpA6Tufu5FL5BZgw==", - "dev": true, "license": "MIT", "dependencies": { "entities": "^6.0.0" diff --git a/package.json b/package.json index 8acd925..cd06e7d 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,8 @@ "test:container": "npx playwright test --config=playwright.config.docker.js" }, "dependencies": { - "ajv": "^8.17.1" + "ajv": "^8.17.1", + "parse5": "^7.3.0" }, "devDependencies": { "@astrojs/check": "^0.9.10", diff --git a/scripts/build-vite.js b/scripts/build-vite.js index 021914b..5bc0e89 100644 --- a/scripts/build-vite.js +++ b/scripts/build-vite.js @@ -238,16 +238,19 @@ async function build() { // when those repos re-publish — so it is fixed here rather than assumed. step('1b/4 Hoisting inline scripts → files'); let hoisted = 0; + let droppedBootstraps = 0; for (const app of resolvedApps.filter(a => a.type !== 'iframe')) { const appDir = join(APPS_DIR, app.slug); if (!existsSync(appDir)) continue; - const count = hoistAppInlineScripts(appDir, collectHtmlFiles(appDir)); - if (count > 0) { - hoisted += count; - ok(app.slug + ': ' + count + ' inline script(s) → ' + HOIST_DIR + '/'); - } + const { hoisted: count, dropped } = hoistAppInlineScripts(appDir, collectHtmlFiles(appDir)); + hoisted += count; + droppedBootstraps += dropped; + if (count > 0) ok(app.slug + ': ' + count + ' inline script(s) → ' + HOIST_DIR + '/'); + // Light-only: a hoisted theme bootstrap would run and re-add `dark`, so it + // is deleted here instead — transform.js can only strip it while inline. + if (dropped > 0) ok(app.slug + ': ' + dropped + ' theme bootstrap(s) dropped (light-only)'); } - if (hoisted === 0) log('No inline scripts found — every artifact is already CSP-clean'); + if (hoisted === 0 && droppedBootstraps === 0) log('No inline scripts found — every artifact is already CSP-clean'); // 2. Copy non-HTML sub-app assets → public/{slug}/ so Astro copies them to dist/{slug}/ // HTML files are excluded — they're handled by src/pages/[...path].astro. @@ -261,23 +264,36 @@ async function build() { if (existsSync(slugDir)) rmSync(slugDir, { recursive: true }); } - function copyAssets(src, dest) { + /** + * Copies a sub-app's non-HTML assets, rewriting root-relative CSS url() + * references on the way through. + * + * A sub-app's CSS is authored for the root of its own site, so `url(/fonts/x)` + * means "this app's /fonts/x" — but the app is served from + * /{prefix}/{slug}/. The rewrite targets that absolute path directly rather + * than a relative hop: a relative `../` is only correct for a stylesheet + * exactly one directory deep, and copyAssets recurses to every depth, so + * `{slug}/style.css` used to climb out of the app entirely and + * `{slug}/assets/css/a.css` landed one level short (#49). An absolute target + * needs no depth arithmetic and matches what transform.js does for HTML. + */ + function copyAssets(src, dest, slug) { mkdirSync(dest, { recursive: true }); for (const entry of readdirSync(src).sort()) { const s = join(src, entry); const d = join(dest, entry); - if (statSync(s).isDirectory()) copyAssets(s, d); + if (statSync(s).isDirectory()) copyAssets(s, d, slug); else if (!entry.endsWith('.html')) { copyFileSync(s, d); - // Rewrite root-relative url() references in CSS bundles so they resolve - // correctly when served from a sub-path (e.g. /knowledge-base/{slug}/_astro/). - // Astro/Vite preserves absolute url(/) paths verbatim; from _astro/*.css - // one level up (../) is the slug root, so ../fonts/ becomes - // /knowledge-base/{slug}/fonts/ — matching where static assets are served. if (entry.endsWith('.css')) { - let css = readFileSync(d, 'utf8'); - // url(/path) | url('/path') | url("/path") → url(../path) etc. - const rewritten = css.replace(/url\(\s*(['"]?)\/(?!\/)/g, 'url($1../'); + const css = readFileSync(d, 'utf8'); + // url(/path) | url('/path') | url("/path") → url(/{prefix}/{slug}/path). + // The (?!\/) guard skips protocol-relative //host/…; data: and #ref + // never match, since neither starts with a slash. + const rewritten = css.replace( + /url\(\s*(['"]?)\/(?!\/)/g, + 'url($1/' + PATH_PREFIX + '/' + slug + '/', + ); if (rewritten !== css) writeFileSync(d, rewritten); } } @@ -287,7 +303,7 @@ async function build() { for (const app of packagedResolved) { const srcDir = join(APPS_DIR, app.slug); if (!existsSync(srcDir)) { warn(app.slug + ': apps/ dir missing, skipping asset copy'); continue; } - copyAssets(srcDir, join(PUBLIC_ROOT, app.slug)); + copyAssets(srcDir, join(PUBLIC_ROOT, app.slug), app.slug); ok(app.slug + ' assets → public/' + app.slug + '/'); } @@ -301,21 +317,38 @@ async function build() { execSync('npx astro build', { cwd: ROOT, stdio: 'inherit', env }); ok('Astro build complete'); - // Sub-app pages hardcode the marketplace stylesheet at /__wf/{prefix}/style.css, - // which the gateway/nginx/preview rewrite to /{prefix}/style.css → dist/style.css. - // Astro can emit that bundle with a numeric suffix (e.g. style2.css) when the - // forced "style.css" asset name clashes, which would 404 the sub-app CSS. Alias - // the bundled css to a stable dist/style.css so the reference always resolves. - const distRoot = join(ROOT, 'dist'); - const stableCss = join(distRoot, 'style.css'); - if (existsSync(distRoot) && !existsSync(stableCss)) { - const rootCss = readdirSync(distRoot).filter(f => /^style\d*\.css$/.test(f)); - if (rootCss.length > 0) { - copyFileSync(join(distRoot, rootCss[0]), stableCss); - ok('Marketplace CSS aliased ' + rootCss[0] + ' → style.css'); - } else { - warn('No bundled marketplace CSS at dist root — /' + PATH_PREFIX + '/style.css may 404'); + // Publish dist/style.css as an alias of the marketplace stylesheet. + // + // Pages do not need it: Astro injects the from Base.astro's CSS import, + // with whatever content-hashed name the bundle got. The alias exists because + // /{prefix}/style.css is a URL this deployment has served for a long time and + // something outside this repository may still ask for it. + // + // The bundle is identified by *use*, not by filename: it is the local + // stylesheet the marketplace's own landing page loads. That is the definition + // of "the marketplace stylesheet", and it cannot drift from what the pages + // actually reference the way a filename pattern could (#50). + const distRoot = join(ROOT, 'dist'); + if (existsSync(distRoot)) { + const landing = join(distRoot, 'index.html'); + if (!existsSync(landing)) fail('No dist/index.html — the landing page did not build.'); + + const hrefs = [...readFileSync(landing, 'utf8').matchAll(/]*\brel="stylesheet"[^>]*>/gi)] + .map(tag => tag[0].match(/\bhref="([^"]+)"/)?.[1]) + .filter(href => href?.startsWith('/' + PATH_PREFIX + '/')); + + if (hrefs.length !== 1) { + fail( + 'Expected the landing page to load exactly one local stylesheet — the marketplace bundle — ' + + 'but found ' + hrefs.length + (hrefs.length ? ': ' + hrefs.join(', ') : '') + + '. dist/style.css can only alias an unambiguous one.', + ); } + + const bundle = join(distRoot, hrefs[0].slice(('/' + PATH_PREFIX).length)); + if (!existsSync(bundle)) fail('The landing page references ' + hrefs[0] + ', which is not in dist/.'); + copyFileSync(bundle, join(distRoot, 'style.css')); + ok('Marketplace CSS ' + hrefs[0] + ' aliased → dist/style.css'); } // 4. Summary diff --git a/scripts/hoist-inline-scripts.js b/scripts/hoist-inline-scripts.js index 6b180b9..f0d891f 100644 --- a/scripts/hoist-inline-scripts.js +++ b/scripts/hoist-inline-scripts.js @@ -27,11 +27,20 @@ * Only executable scripts are hoisted. `application/ld+json`, `text/template`, * `importmap` and friends are data, not code — a CSP does not care about them * and moving them would break whatever reads them. + * + * WHAT IS DELETED + * + * A sub-app's dark-mode bootstrap is dropped rather than hoisted. The + * marketplace is light-only and src/utils/transform.js strips that script — but + * only while it is still inline, and this step runs first. Hoisting it would + * turn it into an external file the strip can no longer recognise, and it would + * then run in the browser and re-add `dark` (#48). */ import { createHash } from 'node:crypto'; import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { dirname, join, relative } from 'node:path'; +import { isThemeBootstrap } from '../src/utils/transform.js'; /** Directory (relative to an app root) the hoisted files are written to. */ export const HOIST_DIR = '_kb-inline'; @@ -52,10 +61,11 @@ function isExecutableInline(attrs) { * @param {string} html - document source * @param {string} appDir - app root on disk; hoisted files go under appDir/HOIST_DIR * @param {string} fileRelDir - the document's directory relative to appDir ('' at the root) - * @returns {{html: string, written: number}} + * @returns {{html: string, written: number, dropped: number}} */ export function hoistInlineScripts(html, appDir, fileRelDir) { let written = 0; + let dropped = 0; const out = html.replace( /]*)>([\s\S]*?)<\/script>/gi, @@ -63,6 +73,9 @@ export function hoistInlineScripts(html, appDir, fileRelDir) { if (!isExecutableInline(attrs)) return match; if (body.trim() === '') return match; + // Light-only: the theme bootstrap is deleted, not relocated. + if (isThemeBootstrap(body)) { dropped++; return ''; } + // Content-addressed: two pages sharing a bootstrap share one file, and a // rebuild of unchanged input produces an unchanged name. const hash = createHash('sha256').update(body).digest('hex').slice(0, 16); @@ -81,7 +94,7 @@ export function hoistInlineScripts(html, appDir, fileRelDir) { }, ); - return { html: out, written }; + return { html: out, written, dropped }; } /** @@ -89,18 +102,21 @@ export function hoistInlineScripts(html, appDir, fileRelDir) { * * @param {string} appDir - apps/{slug} * @param {string[]} htmlFiles - absolute paths to that app's HTML files - * @returns {number} how many scripts were hoisted + * @returns {{hoisted: number, dropped: number}} scripts moved to files, and theme + * bootstraps deleted */ export function hoistAppInlineScripts(appDir, htmlFiles) { - let total = 0; + let hoisted = 0; + let dropped = 0; for (const file of htmlFiles) { const fileRelDir = relative(appDir, dirname(file)).replace(/\\/g, '/'); const source = readFileSync(file, 'utf8'); - const { html, written } = hoistInlineScripts(source, appDir, fileRelDir === '.' ? '' : fileRelDir); - if (written > 0) { - writeFileSync(file, html); - total += written; + const result = hoistInlineScripts(source, appDir, fileRelDir === '.' ? '' : fileRelDir); + if (result.written > 0 || result.dropped > 0) { + writeFileSync(file, result.html); + hoisted += result.written; + dropped += result.dropped; } } - return total; + return { hoisted, dropped }; } diff --git a/scripts/setup-test-apps.mjs b/scripts/setup-test-apps.mjs index aec59f8..7ab008f 100644 --- a/scripts/setup-test-apps.mjs +++ b/scripts/setup-test-apps.mjs @@ -110,6 +110,25 @@ const MERMAID_STUB = `/* Test fixture stand-in for the vendored mermaid bundle. window.mermaid = { initialize: function () {}, run: function () {} }; `; +/** + * Regression fixture for #49 — a stylesheet two directories deep that references + * assets from its app root. + * + * The build rewrites root-relative `url()` in copied CSS so it still resolves + * once the app is served from /{prefix}/{slug}/. That rewrite used to hardcode a + * single `../` hop, which is only correct at exactly one level of nesting: from + * `{slug}/assets/` it pointed one directory short, and from `{slug}/` it climbed + * out of the app and into another one's assets. Nothing shipped by the action + * happens to use `url(/…)` today, so without this file the rewrite is untested + * at the depth it got wrong. Asserted by tests/build-integrity.spec.js. + */ +const DEPTH_CHECK_CSS = `/* Fixture: see scripts/setup-test-apps.mjs (#49). Not referenced by the doc. */ +@font-face { font-family: Demo; src: url(/fonts/demo.woff2) format("woff2"); } +.a { background-image: url(data:image/gif;base64,R0lGOD); } +.b { clip-path: url(#clip); } +.c { background-image: url(//cdn.example.com/x.png); } +`; + const bundleDocs = [ { slug: 'platform-overview', @@ -119,6 +138,7 @@ const bundleDocs = [ tags: ['platform', 'reference'], body: PLATFORM_OVERVIEW_BODY, usesMermaid: true, + depthFixture: true, }, { slug: 'release-process', @@ -147,6 +167,7 @@ function writeSinglePageBundle() { hasHeading: true, })); writeFileSync(join(docDir, CSS_PATH), DOC_CSS); + if (doc.depthFixture) writeFileSync(join(docDir, dirname(CSS_PATH), 'depth-check.css'), DEPTH_CHECK_CSS); if (doc.usesMermaid) { writeFileSync(join(docDir, MERMAID_PATH), MERMAID_STUB); // Real init script, not a stub: it is the thing that must stay out of the diff --git a/src/utils/transform.js b/src/utils/transform.js index 95db714..c844a11 100644 --- a/src/utils/transform.js +++ b/src/utils/transform.js @@ -7,6 +7,21 @@ // the layout owns the masthead, fonts, marketplace CSS and for // every page. This module rewrites the sub-app URLs and splits the document into // the parts the layout needs. +// +// WHY A PARSER AND NOT REGEXES +// +// The input is third-party HTML from another repository's release artifact, so +// every "surely no document does that" assumption a regex makes eventually meets +// a document that does. Pattern matching got this wrong four ways (#48): +// a `` inside a script truncated the page, a `<` inside the theme +// bootstrap defeated the strip that keeps the marketplace light-only, several +// URL-bearing attributes were never rewritten, and `href="/x"` inside a code +// sample in the prose was rewritten as if it were a link. parse5 is the same +// tokenizer a browser uses, it is build-time only, and it knows the difference +// between an attribute, a comment and a text node — so all four stop being +// possible rather than being patched one at a time. + +import { parse, serialize } from 'parse5'; // ── URL rewriting ───────────────────────────────────────────────────────────── @@ -32,68 +47,96 @@ function resolveUrl(url, base, prefix, slug) { } } +/** Attributes holding exactly one URL. `data` is handled separately — it is only a URL on . */ +const URL_ATTRS = new Set(['href', 'src', 'action', 'formaction', 'poster']); + +/** Attributes holding a comma-separated candidate list (`url 2x, url 640w`). */ +const SRCSET_ATTRS = new Set(['srcset', 'imagesrcset']); + +/** property/name values whose `content` is a URL. */ +const URL_META = new Set([ + 'og:image', 'og:image:url', 'og:image:secure_url', 'og:url', + 'twitter:image', 'twitter:image:src', +]); + +/** Rewrites every `url(...)` reference in a CSS string (inline `style=` or a ", + ), 'docs'); + expect(bodyHtml).toContain('url(/knowledge-base/demo/bg.png)'); + expect(headHtml).toContain("url('/knowledge-base/demo/docs/img/hero.png')"); + }); + + test('rewrites URL-bearing meta content', () => { + const { headHtml } = run(doc('

x

', '')); + expect(headHtml).toContain('content="/knowledge-base/demo/card.png"'); + expect(headHtml).toContain('content="/not-a-url"'); + }); + + test('leaves external, anchor and data URLs alone', () => { + const { bodyHtml } = run(doc( + 'e' + + 't' + + 'm' + + 'p' + + '', + )); + expect(bodyHtml).toContain('href="https://example.com/x"'); + expect(bodyHtml).toContain('href="#top"'); + expect(bodyHtml).toContain('href="mailto:x@y.z"'); + expect(bodyHtml).toContain('href="//cdn.example.com/x"'); + expect(bodyHtml).toContain('src="data:image/gif;base64,R0lGOD"'); + }); + + test('does not rewrite markup quoted in prose or comments', () => { + const { bodyHtml } = run(doc( + '
<a href="/docs/api">API</a>
' + + '', + )); + expect(bodyHtml, 'a code sample is text, not a link').not.toContain('/knowledge-base/demo/docs/api'); + expect(bodyHtml).toContain('href="/docs/api"'); + }); + + test('removes , which would re-resolve every rewritten URL', () => { + const { headHtml } = run(doc('

x

', '')); + expect(headHtml).not.toContain(' { + const { headHtml } = run(doc('

x

', ''), 'docs'); + expect(headHtml).toContain('href="/knowledge-base/demo/docs/style.css"'); + expect(headHtml).toContain('data-astro-transition-persist="css-knowledge-base-demo-docs-style-css"'); + }); +});