Skip to content

test(vscode): boot the built webview under its own CSP - #471

Merged
nedtwigg merged 4 commits into
mainfrom
test/webview-boot-smoketest
Aug 28, 2026
Merged

test(vscode): boot the built webview under its own CSP#471
nedtwigg merged 4 commits into
mainfrom
test/webview-boot-smoketest

Conversation

@nedtwigg

Copy link
Copy Markdown
Member

Follow-up to #470. That PR fixed the webview CSP; this one adds the check that would have caught it.

Why the tests in #470 aren't enough

webview-html.test.ts pins the CSP contract against a fixture of Vite's output. That guards the transform, but a fixture cannot notice Vite emitting a shape nobody anticipated — which is exactly how the panel shipped blank for thirteen days. It's the same blind spot in a new place.

CSP is enforced by Chromium. No string-level assertion can observe it, and jsdom doesn't implement CSP at all, so the only way to close the gap is to run the real bundle under the real policy in a real engine.

What it does

Serves media/ over loopback, builds the document with the real getWebviewHtml (the serving origin standing in for webview.cspSource, so the policy has the same shape), loads it in Chromium, and asserts four things: no CSP violation fired, the app mounted into #root, a lazy chunk was actually requested, and nothing threw.

It fails on the pre-fix document. Reproducing that shape locally (un-nonced <link>/<meta>, bare-nonce script-src) turns all four assertions red with the same violations the webview console showed:

script-src-elem blocked .../assets/rolldown-runtime-CbXtAM7H.js
script-src-elem blocked .../assets/alert-ring-watch-CtaS64b-.js

The acquireVsCodeApi stub is load-bearing. Without it initPlatform falls back to FakePtyAdapter with enableRemoteHost false, and the lazy RemotePairingModalHost import — half the failure class — never happens. The third assertion pins that coverage rather than trusting the boot path to keep providing it.

Cost

  • playwright-core, not playwrightpnpm install downloads no browser. CI installs one in the smoketest job; locally it falls back to a system Chrome.
  • Its own parallel job, and out of pnpm test: it needs a built media/ and a browser, so it has its own config and script. pnpm test is unchanged.

One finding worth flagging

While verifying the test actually catches the bug, I found that 'strict-dynamic' could not be shown to be load-bearing once html.cspNonce is in place — including with build.modulePreload disabled. Vite's runtime preload helper nonces the <link> it injects ahead of a lazy import(), which populates the module map and lets the import resolve.

I left 'strict-dynamic' in. It's the mechanism CSP actually specifies for "a script the nonce vouched for may load more"; the alternative is depending on an emergent interaction between a bundler's preload helper and the module map, which would make the policy correct by accident. Recorded in docs/specs/vscode.md so it doesn't read as unexplained belt-and-braces later.

Also worth knowing: this means #470's 'strict-dynamic' and the html.cspNonce change each independently fix the bug. Neither is redundant in intent — they cover different load paths — but they overlap in effect today.

🤖 Generated with Claude Code

https://claude.ai/code/session_011imCAAwd1M6nSFhJyNNLB4

nedtwigg and others added 2 commits August 28, 2026 15:21
Every existing check inspects the webview as strings — including
`webview-html.test.ts`, which pins the CSP contract against a *fixture* of
Vite's output. A fixture cannot notice Vite emitting a shape nobody
anticipated, which is how the panel shipped blank for thirteen days. CSP is
enforced by Chromium, so only running the bundle under the real policy in a
real engine closes that gap; jsdom does not enforce CSP at all.

Serves `media/` over loopback, builds the document with the real
`getWebviewHtml`, and asserts the app mounts, nothing throws, and no CSP
directive fired. Reproducing the pre-fix document makes it fail on all four
assertions with the same `script-src-elem` violations the webview console
showed.

Stubs `acquireVsCodeApi`, without which `initPlatform` falls back to
`FakePtyAdapter` with `enableRemoteHost` false and never issues the lazy
`import()` that was half the failure — so a third assertion pins that the
lazy chunk really was requested, rather than trusting the boot path to keep
providing that coverage.

Kept out of `pnpm test`: it needs a built `media/` and a browser, so it gets
its own config, script, and parallel CI job. `playwright-core` rather than
`playwright` means `pnpm install` downloads no browser; CI installs one, and
locally it falls back to a system Chrome.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011imCAAwd1M6nSFhJyNNLB4
Both sides added to ci.yml at the same point. Kept both: main's self-host
installer step belongs to `build-and-test`, and the webview smoketest is a
sibling job, so it follows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011imCAAwd1M6nSFhJyNNLB4
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 28, 2026

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 747bc53
Status: ✅  Deploy successful!
Preview URL: https://79760e60.mouseterm.pages.dev
Branch Preview URL: https://test-webview-boot-smoketest.mouseterm.pages.dev

View logs

CI caught what a warm working tree hid: `build:frontend` resolves
`server-lib-common` and `dor-lib-common` through node_modules, so it needs
their dist — but only `build` and `typecheck` had a hook to produce one.
Locally they were already built from an earlier full build, so the smoketest
job was the first thing to run it against a clean checkout.

Give it the same `pre` hook the sibling scripts have, rather than fixing it
in the workflow: anyone running `build:frontend` from a fresh clone hits
this, not just CI.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011imCAAwd1M6nSFhJyNNLB4

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0d8a24df fixed the server-lib-common resolution failure I was going to flag, and the smoketest now actually runs in CI — test/webview-boot.smoketest.ts (4 tests) 2316ms, all passing, in job 99002688954. prebuild:frontend is the better shape than patching the workflow step, since a clean local checkout gets it too.

Two small things left, both inline. The MEDIA_PATH one is the only one with teeth: URL.pathname is not a filesystem path, and launch-chromium.ts lists a Windows Chrome location, so running this locally on Windows is contemplated and is exactly what it breaks.

The rest holds up. I checked the load-bearing claim about the acquireVsCodeApi stub — main.tsx gates enableRemoteHost on typeof acquireVsCodeApi === "function", Wall.tsx mounts the lazy RemotePairingModalHost only behind that prop, and the build emits it as its own assets/RemotePairingModalHost-*.js chunk — so the third assertion is pinning real coverage rather than a tautology. Keeping the file off the unit config's test/**/*.test.ts glob and out of pnpm test is right, and scripts/loopback-lint.mjs skips it correctly under its [\\/]tests?[\\/] rule, so the new loopback bind doesn't need an ALLOWED entry.

Comment thread vscode-ext/test/webview-boot.smoketest.ts Outdated
Comment thread vscode-ext/test/webview-boot.smoketest.ts
Comment thread vscode-ext/package.json Outdated
…churn

`URL.pathname` is not a filesystem path: it stays percent-encoded, so a
checkout under a directory with a space misses every `join`/`existsSync`,
and on Windows it yields a leading-slash drive path. `launch-chromium.ts`
lists a Windows Chrome location, so local Windows runs are contemplated and
are exactly what this broke.

Fixed in both vitest configs too, not just the new code — the smoketest
config copied the pattern from the unit config, and leaving one of them
converting the same path a different way is worse than the original.

The `—` escapes were incidental: the script that added `playwright-core`
rewrote package.json with json.dumps, whose `ensure_ascii` defaults to true.
JSON-equivalent, but unrelated to this diff.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011imCAAwd1M6nSFhJyNNLB4
@nedtwigg

Copy link
Copy Markdown
Member Author

Both fixed in 747bc53b.

MEDIA_PATH — you're right, and it had teeth. Switched to fileURLToPath. I also applied the same conversion in both vitest configs, not just the new code: vitest.smoketest.config.mts copied the .pathname pattern from the pre-existing vitest.config.mts, and fixing only mine would leave the two converting the same path different ways — a worse state than the original. That makes one line of this diff a pre-existing fix; happy to split it out if you'd rather keep the PR strictly scoped.

package.json — correct, and thanks for catching it: the script I used to add playwright-core rewrote the file with Python's json.dumps, whose ensure_ascii defaults to true. Restored. The net diff against main is now only the two scripts and the dependency:

+    "prebuild:frontend": "pnpm --filter dor-lib-common build && pnpm --filter server-lib-common build",
+    "test:smoke": "vitest run --config vitest.smoketest.config.mts",
+    "playwright-core": "^1.62.1",

On the acquireVsCodeApi verification — appreciated, since that assertion is the one most at risk of quietly becoming a tautology. That's precisely why it asserts the chunk was requested rather than trusting the boot path: if a future change stops mounting RemotePairingModalHost at boot, the test goes red and says so, instead of passing while covering one load path less.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fixes verified — MEDIA_PATH now goes through fileURLToPath, and package.json's net diff against main is exactly the two scripts plus the dependency.

On splitting the pre-existing vitest.config.mts line out: keep it here. Leaving the two configs converting the same path two different ways is the worse state, and a one-line PR that only exists to make them agree costs a full review round for less signal than the sentence you already wrote in the commit message.

@nedtwigg
nedtwigg merged commit 01d1328 into main Aug 28, 2026
11 checks passed
@nedtwigg
nedtwigg deleted the test/webview-boot-smoketest branch August 28, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants