[26/36] Add OC-120 preview media controls

[cjohnsto-nz]

Summary

• Implements image and PDF preview zoom, fit, reset, and rotation controls.
• Loads PDF.js through the request webview’s nonce-bearing module path; PR [22/36] Release 0.8.0 with packaged PDF.js preview #54 owns generated package assets.
• Rejects unsafe image Content-Type values and creates preview images through DOM property assignment.
• Covers stale PDF render cancellation, pixel limits, image load sizing, CSP configuration, SVG routing, and hostile-input regression cases.

Stack

• Base: PR [25/36] Add OC-130 protocol layout stability task #62 (supervisor/add-protocol-layout-stability-task)
• Head: feature/oc-120-preview-media-controls
• Immediate child: PR [27/36] Add OC-130 protocol first paint stability #59 (feature/oc-130-protocol-layout-stability)

Validation

• npm run compile
• npm test -- --run test/previewMediaControls.test.ts — 14 tests
• npm test -- --run — 24 files / 452 tests
• npm run build

[APKiwi]

Verdict: request changes, one injection vector with a one-line fix. The controls themselves (transform helpers, generation-token stale-render cancellation, canvas pixel budget, debounce) are well built.

• Major, security: response Content-Type is injected into privileged webview innerHTML. response.ts:1181 builds data:${mimeType};base64,... where mimeType comes from the raw server-controlled Content-Type header (set unescaped at :843), then injects via container.innerHTML at :1184-1187. A response with Content-Type: image/png" onerror="... reaches the image branch (getPreviewMediaKind only checks startsWith('image/')) and breaks out of the src attribute. The nonce CSP blocks script execution, but style-src carries unsafe-inline so CSS/markup injection and UI spoofing inside the panel are live. It's also a defense-in-depth regression: the old path rendered the same unescaped value inside a sandboxed iframe where injected handlers could never fire. Fix: createElement('img') and assign .src as a property, or validate mimeType against /^image/[a-z0-9.+-]+$/. The SVG branch is already safe via encodeURIComponent. A mimeType-escaping test would have caught this.
• Minor: previewOverlay is now dead, renderPreview always sets it display:none. Remove or repurpose.
• Minor: ${e.message} in the PDF error innerHTML at :1259 is unescaped. Pre-existing, but you're in the file.
• Nit: applyImageTransform runs before load fires and getImageNaturalSize floors to 1, so there's a transient 1x1 frame on large images.

Tests: strong (transform math, CSP directives, Ctrl+wheel including the not-prevented ordinary wheel, reset/hide, PDF viewport calls, debounce with fake timers, pixel cap, stale-render accounting). Gaps: no mimeType escaping test, SVG branch untested, no assertion non-media still routes to the iframe.

Deps: none added in this increment. The pdfjs-dist dep and the vendored media/ blobs enter in the release PR, comments there.

[APKiwi]

Re-review (whole-stack pass, per REVIEW_GUIDE.md)

The Content-Type webview injection is still open, and on re-review it is worse than my first pass concluded, which called it CSS/UI spoofing.

• (high) src/webview/response.ts:1184-1187 builds data:${mimeType};base64,... from the raw response Content-Type (lastContentType, set unescaped at :843) and injects it via container.innerHTML with src="${src}". getPreviewMediaKind (:239) only checks startsWith('image/'), so a Content-Type like image/svg"><iframe src=data:text/html,<payload> (no semicolon, lowercase, survives .split(';')[0].toLowerCase()) plus any base64 body reaches the image branch and breaks out of the src attribute. The base CSP (src/panels/basePanel.ts:417) allows frame-src data: blob:, and a data: document carries no CSP of its own and inherits allow-scripts from the webview host, so inline script inside the injected iframe is not bound by the parent nonce and can run. That is plausibly arbitrary JS in the extension webview (with acquireVsCodeApi().postMessage to the host), one band above the UI-spoofing read. I did not boot a live webview to confirm VS Code adds no extra nested-frame stripping, so treat as high until disproven. The two post-review commits here (691e2d7, 4d30141) only added cspSource to script-src/worker-src for PDF.js, they do not touch the sink. Fix is the same either way: build the <img> with document.createElement('img') and assign .src as a property, or validate mimeType against /^image\/[a-z0-9.+-]+$/ before use.
• (low) The CSP is patched by a regex string-replace (requestPanel.ts:171). If the base template reformats, the replace silently no-ops and PDF.js breaks with no error. Robustness, not security.

Prior minors (dead previewOverlay, unescaped ${e.message} in the PDF error innerHTML at :1259, transient 1x1 frame) all still open.

Coverage gap: test/previewMediaControls.test.ts has no mimeType-escaping test. A malicious-Content-Type breakout test would have caught this and belongs here.

Verdict: changes-needed. Attacker-controlled or MITM'd response body opened in Preview reaches a script-execution surface in the extension webview. Trivial fix plus a breakout test.

[cjohnsto-nz]

The findings are fixed in a2b9b17.

• The Content-Type injection path is closed twice: image media types must match ^image/[a-z0-9][a-z0-9.+-]*$, and the preview <img> is created with DOM APIs with .src assigned as a property. Server-controlled Content-Type text no longer reaches innerHTML.
• A hostile image/svg\"><iframe ... regression case is rejected as non-media, creates no image or nested markup, and routes through the sandboxed non-media iframe path.
• SVG preview now has direct coverage and uses the DOM property sink.
• PDF.js failure messages are rendered with textContent; an HTML-shaped error remains literal text.
• The dead previewOverlay markup and code are removed.
• Image frames remain hidden and unsized until a completed image has non-zero natural dimensions, eliminating the transient 1×1 frame.
• CSP/PDF script injection now uses required replacements. Any base-template drift throws a named error instead of silently disabling PDF preview, and that failure path is tested.

Validation:

• npm run compile passed.
• Focused preview suite passed 14 tests.
• Full suite passed 24 files / 452 tests.
• npm run build passed.
• GitHub build and security checks passed.

The PR #56, PR #54, and PR #64 review fixes each simulate cleanly into this branch. PR #58 → PR #59, PR #58 → PR #71, and PR #58 → PR #72 also simulate cleanly. GitHub reports PR #58 and immediate child PR #59 as MERGEABLE/CLEAN.

[cjohnsto-nz]

Response to the second review: all actionable findings are addressed on the current head (a2b9b17).

• Data-image handling uses strict MIME matching.
• Preview images are created through the DOM and assigned through the src property rather than HTML interpolation.
• A hostile payload regression covers the sink.
• Required CSP replacements throw if their target directive is absent.
• PDF failures are rendered with text-only APIs.
• The obsolete overlay is removed and the preview frame is hidden on failure.

I validated the complete 37-PR composition with npm run build, npm run compile, all 539 tests, and all 47 demo validations passing.