diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6bc0724cf..0d7f44028 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -110,6 +110,43 @@ jobs: [ -L "$root/previous" ] || { echo "::error::no previous release retained after an update"; exit 1; } bash -n "$root/bin/manage" bash -n "$root/bin/run-server" + webview-smoketest: + name: Webview Boot Smoketest + runs-on: ubuntu-latest + # Its own job, in parallel with Build & Test: this is the only check that + # needs a browser, and pinning that cost to one job keeps `pnpm test` free of + # it. It is also the only check that runs the shipped bundle rather than + # inspecting it — the VS Code webview's CSP is enforced by Chromium, so a + # policy that blocks the app is invisible to every string-level test we have + # (docs/specs/vscode.md -> "CSP policy"). + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version-file: package.json + + - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6.0.10 + + # Frozen for the same supply-chain reason as every other job here. + - name: Install dependencies + run: pnpm install --frozen-lockfile + + # `playwright-core` ships no browser, deliberately — a unit-test run should + # not drag one down. Fetch it here, where it is actually used. + - name: Install Chromium + run: pnpm --filter dormouse exec playwright-core install --with-deps chromium + + # The smoketest loads what the build emits, so the build has to precede + # it. `build:frontend` alone — this job never touches the extension host + # bundle — and its `prebuild:frontend` hook builds the two workspace + # packages the webview bundle imports, which a clean checkout has no dist + # for. + - name: Build the webview frontend + run: pnpm --filter dormouse build:frontend + + - name: Smoketest + run: pnpm --filter dormouse test:smoke standalone-smoketest: name: Standalone Smoketest diff --git a/docs/specs/vscode.md b/docs/specs/vscode.md index f99cca0b9..99f1137d7 100644 --- a/docs/specs/vscode.md +++ b/docs/specs/vscode.md @@ -234,7 +234,9 @@ That allowlist is still a build-time constant, not a runtime value: `vscode-ext/ Get any of it wrong and the failure is remote from its cause: a blank panel, or a render error naming a chunk that is sitting on disk. In both cases the only direct evidence is a CSP violation in the webview console (**Developer: Open Webview Developer Tools**) — nothing reaches an extension-host log, and the extension itself activates normally. -The class arrived with a build-tool upgrade (rolldown began splitting out its shared runtime) and can return the same way, so `vscode-ext/test/webview-html.test.ts` pins it against a fixture of real Vite output: `'strict-dynamic'` is present, `script-src` never gains `'unsafe-inline'`, each named script-loading tag carries the real nonce, no placeholder survives, no tag carries two nonces, and an unmarked document is refused. +CSP is enforced by Chromium, so none of this is observable from string inspection — which is why `vscode-ext/test/webview-boot.smoketest.ts` loads the real bundle under the real policy in a real engine, and why it is the check that would have caught this. Reproducing the pre-fix document makes it fail on all four assertions with the same `script-src-elem` violations the webview console showed. A unit test still guards the transform: `vscode-ext/test/webview-html.test.ts` pins it against a fixture of real Vite output: `'strict-dynamic'` is present, `script-src` never gains `'unsafe-inline'`, each named script-loading tag carries the real nonce, no placeholder survives, no tag carries two nonces, and an unmarked document is refused. That fixture is the limit of what it can prove, though — a fixture cannot notice Vite emitting a shape nobody anticipated, which is the gap the smoketest exists to cover. + +**On the overlap between the two mechanisms.** With the `` in place, Vite's runtime preload helper nonces the `` it injects ahead of a lazy `import()`, which populates the module map and lets the import resolve — so `'strict-dynamic'` could not be shown to be load-bearing by experiment, including with `build.modulePreload` disabled. It stays anyway: it is the mechanism CSP actually specifies for "a script the nonce vouched for may load more", whereas the alternative is an emergent interaction between a bundler's preload helper and the module map. Depending on the latter alone would make the policy correct by accident. ### Webview message authentication @@ -384,7 +386,7 @@ Source of truth: `vscode-ext/src/peer-link.ts` for the sockets and arbitration; `vscode-ext` runs vitest (`pnpm --filter dormouse test`, which typechecks first). The `vscode` module only exists inside a running editor, so `vitest.config.mts` aliases it to a stub. Most modules worth testing import `vscode` as `import type`, which erases; the stub covers what is left, which is the output channel `log.ts` opens and the `Uri.file` that `webview-html.ts` calls at runtime. Nothing else is stubbed on purpose — a test that reaches further should fail loudly rather than pass against a fake that quietly does nothing. -Mostly these are the tests that need real I/O, since the pure halves already live in `lib`. Seven files, all under `vscode-ext/test/`: +Mostly these are the tests that need real I/O, since the pure halves already live in `lib`. All under `vscode-ext/test/`: - **`peer-link.test.ts`** stands up a broker and a client over a real socket: bind-as-lease (first binder wins, idempotent re-announce, taking over a socket whose broker died without unlinking, re-binding when the reclaimed socket is unlinked out from under it, a reclaimed bind answering no role until it is verified, two windows racing for one corpse settling into a broker and a client, handing the Host to a surviving window when the broker dies, an accept-time server error logged rather than thrown, and the permanent stand-down when the shared token can be neither read nor created), the handshake (the three frames over a raw socket with the token never on the wire, a wrong-token proof dropped, a proof replayed from another connection rejected, and a squatter that took the path being served nothing), the socket directory being kept private, cross-window directory and surface ops, provider-local handles for colliding PTY ids, PTY routing and streaming with two viewers, route survival across unsubscribe and re-attach, what a disconnect does to in-flight terminals, forwarded commands, and requests still outstanding against it, and that a client whose socket died reports *unsettled* before its `close` lands, so it agrees with `forwardCommand`. - **`peer-link-protocol.test.ts`** is that link's socket-free half: frame shapes and framing (splits, oversized frames, malformed lines), the PTY routing table, the handshake proof primitives, and the guard that keeps `PEER_REPLY_BUDGET_MS` strictly larger than the `ASK_BUDGET_MS` fan-out it contains. @@ -392,6 +394,7 @@ Mostly these are the tests that need real I/O, since the pure halves already liv - **`message-router.test.ts`** covers the in-window fan-out with the link and the service stubbed out: one answer counted per webview however many it sends, and a late answer for a settled request marking the directory stale instead of being dropped. - **`processed-pty-streams.test.ts`** covers the window's one keyed registry: exactly one listener pair however many attachments exist, none at all with none, per-PTY fan-out, and teardown on exit. - **`webview-html.test.ts`** is the exception to the real-I/O rule above: its subject is a pure string transform, and it is here because the thing it guards cannot be checked anywhere else. It feeds `getWebviewHtml` a fixture of real Vite output from a temp directory and pins the CSP contract — `'strict-dynamic'` present, `script-src` never gaining `'unsafe-inline'`, each named script-loading tag carrying the real nonce, no placeholder surviving, no tag carrying two, and an unmarked document refused. See "CSP policy": every failure in that contract is invisible outside the webview console, so a unit test is the only place it can go red early. +- **`webview-boot.smoketest.ts`** is the only check that *runs* the shipped bundle instead of inspecting it, and it is deliberately not part of `pnpm test` — it needs a built `media/` and a Chromium, so it has its own config (`vitest.smoketest.config.mts`), its own script (`pnpm --filter dormouse test:smoke`), and its own parallel CI job. It 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), stubs `acquireVsCodeApi` so the bundle takes its VS Code path, and asserts four things: no CSP violation fired, the app mounted into `#root`, a lazy chunk was actually requested, and nothing threw. The `acquireVsCodeApi` stub is load-bearing — without it `initPlatform` falls back to `FakePtyAdapter` with `enableRemoteHost` false, and the lazy `RemotePairingModalHost` import that is half of the failure class never happens. Hence the third assertion, which pins that coverage rather than trusting the boot path to keep providing it. `launch-chromium.ts` finds a browser: Playwright's own build first (the version `playwright-core` was released against, installed by CI), then a system Chrome so the smoketest runs locally without an install step. - **`helpers.ts`** holds what the socket suites need — a throwaway `globalStorageUri`, the mirrored socket-path derivation, a poll-with-deadline, `freshModule`, and `fakeWindow`, one window as the link sees it. Separate module instances come from `vi.resetModules()` plus a dynamic import, which is what makes one process able to play two windows. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d65bdafa5..1ff7b44de 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -321,6 +321,9 @@ importers: ovsx: specifier: ^1.0.0 version: 1.1.1(@types/node@24.13.3)(debug@4.4.3(supports-color@7.2.0))(supports-color@7.2.0) + playwright-core: + specifier: ^1.62.1 + version: 1.62.1 tailwindcss: specifier: ^4.3.0 version: 4.3.3 @@ -3973,6 +3976,11 @@ packages: pkg-types@2.3.1: resolution: {integrity: sha512-y+ichcgc2LrADuhLNAx8DFjVfgz91pRxfZdI3UDhxHvcVEZsenLO+7XaU5vOp0u/7V/wZ+plyuQxtrDlZJ+yeg==} + playwright-core@1.62.1: + resolution: {integrity: sha512-wPYSwEBJY9GHraISXqyqtx0na0LpO3XEX7jNDhntbex7tzUS7kLnZsOlFruFJB4Hi/rhDMjXGqHewDZ68nYZVw==} + engines: {node: '>=20'} + hasBin: true + pluralize@2.0.0: resolution: {integrity: sha512-TqNZzQCD4S42De9IfnnBvILN7HAW7riLqsCyp8lgjXeysyPlX5HhqKAcJHHHb9XskE4/a+7VGC9zzx8Ls0jOAw==} @@ -8091,6 +8099,8 @@ snapshots: exsolve: 1.1.0 pathe: 2.0.3 + playwright-core@1.62.1: {} + pluralize@2.0.0: {} pluralize@8.0.0: {} diff --git a/vscode-ext/package.json b/vscode-ext/package.json index 788179bee..417af00e5 100644 --- a/vscode-ext/package.json +++ b/vscode-ext/package.json @@ -100,10 +100,12 @@ }, "scripts": { "postinstall": "chmod +x node_modules/node-pty/prebuilds/*/spawn-helper 2>/dev/null || true", + "prebuild:frontend": "pnpm --filter dor-lib-common build && pnpm --filter server-lib-common build", "build:frontend": "vite build --config vite.config.ts", "pretypecheck": "pnpm --filter dor-lib-common build && pnpm --filter server-lib-common build", "typecheck": "tsc --noEmit -p tsconfig.json", "test": "pnpm typecheck && vitest run", + "test:smoke": "vitest run --config vitest.smoketest.config.mts", "prebuild": "pnpm --filter server-lib-common build", "build": "pnpm stage:dor-cli && node scripts/esbuild.mjs && cp -RL node_modules/node-pty dist/node-pty && rm -rf dist/shell-integration && cp -RL ../standalone/sidecar/shell-integration dist/shell-integration", "stage:dor-cli": "pnpm --filter dor build && node ../scripts/stage-dor-cli.mjs vscode-ext/dor-cli", @@ -126,6 +128,7 @@ "@vscode/vsce": "^3.9.1", "esbuild": "^0.28.0", "ovsx": "^1.0.0", + "playwright-core": "^1.62.1", "tailwindcss": "^4.3.0", "typescript": "^6.0.3", "vite": "^8.0.14", diff --git a/vscode-ext/test/launch-chromium.ts b/vscode-ext/test/launch-chromium.ts new file mode 100644 index 000000000..955d959ec --- /dev/null +++ b/vscode-ext/test/launch-chromium.ts @@ -0,0 +1,40 @@ +import { chromium, type Browser } from 'playwright-core'; + +/** + * Launch a Chromium for the boot smoketest. + * + * The dependency is `playwright-core`, not `playwright`, so `pnpm install` never + * downloads a browser — nobody pays a hundred-megabyte fetch to run the unit + * tests. That leaves finding one at run time. + * + * Playwright's own build is tried first, because it is the one whose version + * this `playwright-core` was released against; CI installs it explicitly (the + * smoketest job in `.github/workflows/ci.yml`). A system Chrome is the fallback, + * so a developer can run the smoketest locally without the install step — note + * that CI images also ship a system Chrome, which is exactly why preferring it + * would quietly ignore the pinned browser. + */ +const SYSTEM_CHROMIUM = [ + '/usr/bin/google-chrome', + '/usr/bin/chromium', + '/usr/bin/chromium-browser', + '/usr/bin/microsoft-edge', + '/Applications/Google Chrome.app/Contents/MacOS/Google Chrome', + 'C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe', +]; + +export async function launchChromium(): Promise { + // An explicit override wins outright, so an unusual install can be named + // rather than guessed at. + const override = process.env.DORMOUSE_SMOKETEST_CHROMIUM; + if (override) return chromium.launch({ executablePath: override }); + + try { + return await chromium.launch(); + } catch (playwrightManagedMissing) { + const { existsSync } = await import('node:fs'); + const system = SYSTEM_CHROMIUM.find((path) => existsSync(path)); + if (!system) throw playwrightManagedMissing; + return chromium.launch({ executablePath: system }); + } +} diff --git a/vscode-ext/test/webview-boot.smoketest.ts b/vscode-ext/test/webview-boot.smoketest.ts new file mode 100644 index 000000000..530d64162 --- /dev/null +++ b/vscode-ext/test/webview-boot.smoketest.ts @@ -0,0 +1,185 @@ +import { createServer, type Server } from 'node:http'; +import { createReadStream, existsSync } from 'node:fs'; +import { extname, join, normalize } from 'node:path'; +import type { AddressInfo } from 'node:net'; +import { fileURLToPath } from 'node:url'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import type { Browser } from 'playwright-core'; + +import { getWebviewHtml } from '../src/webview-html'; +import { launchChromium } from './launch-chromium'; + +/** + * Does the built webview actually boot? + * + * Every other test in this directory checks a transform in isolation, including + * `webview-html.test.ts`, which pins the CSP contract against a *fixture* of + * Vite's output. A fixture cannot notice that Vite started emitting a shape + * nobody anticipated — which is exactly how the webview shipped blank for + * thirteen days (`docs/specs/vscode.md` → "CSP policy"). Only running the real + * bundle under the real policy in a real engine closes that gap, because CSP + * enforcement is the thing under test and no amount of string inspection + * substitutes for it. jsdom is not an option: it does not enforce CSP at all. + * + * Deliberately shallow. It asserts the app mounts and the policy blocked + * nothing — not what the UI looks like, which is Storybook's and Chromatic's + * job. + */ + +const MEDIA_PATH = fileURLToPath(new URL('../media', import.meta.url)); + +const MIME: Record = { + '.js': 'text/javascript', + '.css': 'text/css', + '.html': 'text/html', + '.map': 'application/json', + '.woff2': 'font/woff2', + '.svg': 'image/svg+xml', +}; + +/** + * Serve `media/` the way the VS Code webview does — the document at `/`, assets + * beside it. The origin stands in for `webview.cspSource`, so the CSP under test + * has the same *structure* as the real one: scripts pass on the nonce and + * `'strict-dynamic'` alone, never on a host source. + */ +function serveMedia(document: { html: string }): Promise<{ server: Server; origin: string }> { + const server = createServer((req, res) => { + const path = (req.url ?? '/').split('?')[0]; + if (path === '/' || path === '/index.html') { + res.writeHead(200, { 'content-type': 'text/html' }); + res.end(document.html); + return; + } + // `normalize` collapses any `..` before the prefix check, so a traversal + // cannot escape media/ even though this only ever serves our own build. + const file = normalize(join(MEDIA_PATH, path)); + if (!file.startsWith(MEDIA_PATH) || !existsSync(file)) { + res.writeHead(404).end('not found'); + return; + } + res.writeHead(200, { 'content-type': MIME[extname(file)] ?? 'application/octet-stream' }); + createReadStream(file).pipe(res); + }); + return new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => { + const { port } = server.address() as AddressInfo; + resolve({ server, origin: `http://127.0.0.1:${port}` }); + }); + }); +} + +let browser: Browser; +let server: Server; +let origin: string; + +/** Everything the page reported that should not have happened. */ +const cspViolations: string[] = []; +const pageErrors: string[] = []; +const consoleErrors: string[] = []; +const requested: string[] = []; +const failedRequests: string[] = []; +let rootChildCount = 0; + +beforeAll(async () => { + expect( + existsSync(join(MEDIA_PATH, 'index.html')), + 'vscode-ext/media/index.html is missing — run `pnpm --filter dormouse build:frontend` first', + ).toBe(true); + + // The CSP embeds the origin, and the origin is only known once the server has + // a port — so the server reads its body from a box we fill in after binding. + // Restarting to bake the URLs in would hand out a different port and point the + // document at a closed one. + const document = { html: '' }; + ({ server, origin } = await serveMedia(document)); + + const webview = { + cspSource: origin, + asWebviewUri: () => origin, + } as never; + document.html = getWebviewHtml(webview, MEDIA_PATH).html; + + browser = await launchChromium(); + const page = await browser.newPage(); + + page.on('requestfinished', (req) => requested.push(req.url())); + page.on('requestfailed', (req) => { + requested.push(req.url()); + failedRequests.push(`${req.url()} — ${req.failure()?.errorText ?? 'unknown'}`); + }); + page.on('console', (msg) => { + if (msg.type() === 'error') consoleErrors.push(msg.text()); + }); + page.on('pageerror', (err) => pageErrors.push(String(err))); + // The signal this test exists for. A blocked script fires this and otherwise + // leaves no trace outside the webview console. + await page.addInitScript(() => { + (globalThis as unknown as { __csp: string[] }).__csp = []; + globalThis.addEventListener('securitypolicyviolation', (e) => { + (globalThis as unknown as { __csp: string[] }).__csp.push( + `${e.violatedDirective} blocked ${e.blockedURI}`, + ); + }); + // Without this the bundle takes its non-VS Code path: `FakePtyAdapter`, and + // `enableRemoteHost` false, so the lazy `RemotePairingModalHost` chunk is + // never imported — and a lazy `import()` is half of what broke here. + (globalThis as unknown as { acquireVsCodeApi: unknown }).acquireVsCodeApi = () => ({ + postMessage: () => {}, + getState: () => undefined, + setState: () => {}, + }); + }); + + await page.goto(`${origin}/`, { waitUntil: 'load' }); + // The app mounts behind `resumeOrRestore`, which self-caps at 500ms when no + // host answers. Poll rather than sleep so a fast boot does not pay for it. + await page + .waitForFunction(() => (document.getElementById('root')?.childElementCount ?? 0) > 0, { + timeout: 15_000, + }) + .catch(() => {}); + + rootChildCount = await page.evaluate( + () => document.getElementById('root')?.childElementCount ?? 0, + ); + cspViolations.push( + ...(await page.evaluate(() => (globalThis as unknown as { __csp: string[] }).__csp ?? [])), + ); +}, 120_000); + +afterAll(async () => { + await browser?.close(); + server?.close(); +}); + +describe('the built webview boots under its own CSP', () => { + it('violates no CSP directive', () => { + // Includes the ones that are survivable on their own: a blocked preload + // still means a tag the policy does not cover, and the next bundler change + // could make it fatal. + expect(cspViolations).toEqual([]); + }); + + it('mounts the app into #root', () => { + expect(rootChildCount).toBeGreaterThan(0); + }); + + it('actually exercised the lazy-import path', () => { + // Without this the suite could pass while never fetching a dynamically + // imported chunk — and a lazy `import()` is half of what broke here. Assert + // the coverage rather than trusting the boot path to keep providing it. + expect( + requested.filter((u) => /RemotePairingModalHost-/.test(u)), + `no lazy chunk was requested; requests were:\n${requested.join('\n')}`, + ).not.toEqual([]); + expect(failedRequests).toEqual([]); + }); + + it('throws nothing while booting', () => { + expect(pageErrors).toEqual([]); + // Failing to fetch a chunk surfaces here rather than as a violation, since + // React turns it into a caught render error. + expect(consoleErrors.filter((t) => /import|chunk|Content Security/i.test(t))).toEqual([]); + }); +}); diff --git a/vscode-ext/vitest.config.mts b/vscode-ext/vitest.config.mts index 3ceb6d521..284c8a1ef 100644 --- a/vscode-ext/vitest.config.mts +++ b/vscode-ext/vitest.config.mts @@ -1,3 +1,5 @@ +import { fileURLToPath } from 'node:url'; + import { defineConfig } from 'vitest/config'; /** @@ -12,7 +14,7 @@ import { defineConfig } from 'vitest/config'; export default defineConfig({ resolve: { alias: { - vscode: new URL('test/vscode-stub.ts', import.meta.url).pathname, + vscode: fileURLToPath(new URL('test/vscode-stub.ts', import.meta.url)), }, }, test: { diff --git a/vscode-ext/vitest.smoketest.config.mts b/vscode-ext/vitest.smoketest.config.mts new file mode 100644 index 000000000..d0db84ea0 --- /dev/null +++ b/vscode-ext/vitest.smoketest.config.mts @@ -0,0 +1,26 @@ +import { fileURLToPath } from 'node:url'; + +import { defineConfig } from 'vitest/config'; + +/** + * The webview boot smoketest, kept out of `pnpm test` on purpose: it needs a + * built `media/` and a Chromium, neither of which a unit-test run should + * require. CI gives it its own parallel job (`.github/workflows/ci.yml`). + * + * Same `vscode` stub as the unit config — the smoketest drives the real + * `getWebviewHtml`, so it needs `Uri.file` for exactly the same reason. + */ +export default defineConfig({ + resolve: { + alias: { + vscode: fileURLToPath(new URL('test/vscode-stub.ts', import.meta.url)), + }, + }, + test: { + environment: 'node', + include: ['test/**/*.smoketest.ts'], + // One browser launch plus a real page load; the default 5s is not enough. + testTimeout: 120_000, + hookTimeout: 120_000, + }, +});