From b87437c4e63f6b20fdd4909b4256daa548f510b6 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:52:02 -0500 Subject: [PATCH 1/8] Markdown images resolve against the file they were written in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A relative image src has always been resolved against the document origin — attn://app, or the Vite dev server — which is not where the markdown file lives, so `![](./diagram.png)` rendered as a broken glyph and only absolute https:/data: URLs ever appeared. The plumbing was already there: the attn:// handler serves any local file from attn://localhost/, which is what ImageViewer and MediaPlayer use. Nothing was pointing markdown images at it. resolveImageSrc() maps an authored src onto that URL, and an `image` NodeView applies it to the DISPLAYED src only. node.attrs.src is never written, which is the whole round-trip guarantee — `image` has no serializer override, so the attr goes to disk verbatim on every save. Encoding is the sharp edge: markdown-it has already percent-encoded the src, and the Rust handler truncates the path at the first '?' or '#' before decoding it. So each segment is decoded once, re-split (an encoded '/' IS a separator to the handler), normalised with '..' clamped at the root, and re-encoded per segment. The view is registered only where a resolver is supplied. The hosted app and the reviewer reading an owner's snapshot pass none — their relative srcs name files on someone else's disk — and keep stock toDOM, byte-identical DOM included. Workspace-relative assets in the hosted app are a separate follow-up. An asset that will not load gets a paper/ink card — an eyebrow, the alt text in serif, the filename in mono, announced as one image — in place of the platform's broken-image glyph. It says "didn't load", not "not found", because a present file with a MIME the webview declines fires the same event. Closes attn-cgev. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn --- scripts/test-e2e.sh | 84 +++ tests/fixtures/chart.svg | 9 + tests/fixtures/diagram.png | Bin 0 -> 176 bytes tests/fixtures/images.md | 39 ++ tests/fixtures/nested/diagram.png | Bin 0 -> 150 bytes web/src/App.svelte | 19 + web/src/lib/Editor.svelte | 40 ++ web/src/lib/image-src-resolution.test.ts | 491 ++++++++++++++++++ web/src/lib/markdown-layer.ts | 149 ++++++ .../lib/prosemirror/image-nodeview.test.ts | 415 +++++++++++++++ web/src/lib/prosemirror/image-nodeview.ts | 206 ++++++++ web/styles/prosemirror.css | 100 ++++ 12 files changed, 1552 insertions(+) create mode 100644 tests/fixtures/chart.svg create mode 100644 tests/fixtures/diagram.png create mode 100644 tests/fixtures/images.md create mode 100644 tests/fixtures/nested/diagram.png create mode 100644 web/src/lib/image-src-resolution.test.ts create mode 100644 web/src/lib/prosemirror/image-nodeview.test.ts create mode 100644 web/src/lib/prosemirror/image-nodeview.ts diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 4a078225..4db1029a 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -266,6 +266,90 @@ result=$("$ATTN" --query '[class*="breadcrumb"], nav[aria-label]' | jq -r '.elem assert_contains "Breadcrumb shows nested path" "$result" "child.md" screenshot "06-nested-file" +# =================================================================== +# TEST SUITE 3: Relative image resolution (attn-cgev) +# =================================================================== + +echo "" +echo "=== Test Suite 3: Relative Images (images.md) ===" + +# Poll a synchronous eval until it stops returning `null`/empty. `--eval` hands +# back whatever the expression evaluates to, JSON-encoded, and does NOT await a +# Promise — so waiting has to happen out here, not in the page. +poll_eval() { + local js="$1" + local tries=0 + local out="" + while [ "$tries" -lt 40 ]; do + out=$("$ATTN" --eval "$js" 2>/dev/null || echo "") + case "$out" in + ''|null|'""'|false|0) ;; + *) echo "$out"; return 0 ;; + esac + sleep 0.1 + tries=$((tries + 1)) + done + echo "$out" +} + +start_daemon "$FIXTURES/images.md" + +# Single-file mode, so directory ordering in tests/fixtures/ is irrelevant here. +"$ATTN" --wait-for '.attn-doc .md-image img' --timeout 5000 >/dev/null 2>&1 + +result=$("$ATTN" --query '.attn-doc .md-image img' | jq -r '.count' 2>/dev/null || echo "0") +assert_truthy "Image nodes rendered" "$result" + +# The whole bug: a relative src used to reach the DOM verbatim and 404 against +# the app origin. It must now address the file through the attn:// handler. +result=$("$ATTN" --query '.attn-doc .md-image img' | jq -r '.elements[0].attributes.src' 2>/dev/null || echo "") +assert_contains "Relative src resolved through attn://" "$result" "attn://localhost/" +assert_contains "Resolved against the markdown file's own directory" "$result" "tests/fixtures/diagram.png" + +# The authored src is what gets serialized back to disk, so it must survive. +# Read through --query rather than --eval: the webview JSON-escapes '/' in a +# returned string, which turns every path assertion into a slash-counting exercise. +result=$("$ATTN" --query '.attn-doc .md-image' | jq -r '.elements[0].attributes["data-src"]' 2>/dev/null || echo "") +assert_eq "Authored src preserved on the node" "$result" "./diagram.png" + +# naturalWidth is not a DOM attribute, and decode is asynchronous. +result=$(poll_eval "Array.from(document.querySelectorAll('.attn-doc .md-image img')).filter((img) => img.complete && img.naturalWidth > 0).length") +assert_truthy "Local image bytes actually decoded (naturalWidth > 0)" "$result" + +# Every local src in the fixture except the deliberate miss should decode: two +# PNGs (sibling, bare, subdirectory) plus the SVG, which carries explicit +# width/height so WebKit reports an intrinsic size for it. +result=$(poll_eval "(() => { const n = Array.from(document.querySelectorAll('.attn-doc .md-image img')).filter((img) => img.complete && img.naturalWidth > 0).length; return n >= 4 ? n : null; })()") +assert_eq "All four local assets decoded" "$result" "4" + +# A missing file gets the document's own placeholder, not the platform glyph. +# Anchored on the deliberate miss by its authored src, NOT on "the first broken +# image": the remote https src in this fixture also fails (there is no network +# in the E2E environment), and which of the two reports `error` first is a race +# between a local 404 and a DNS timeout. +GONE='.attn-doc .md-image[data-src="./gone.png"]' +result=$(poll_eval "document.querySelector('$GONE[data-broken]')?.textContent") +assert_contains "Missing image shows the alt text" "$result" "A diagram that moved" +assert_contains "Missing image names the file" "$result" "gone.png" +assert_contains "Missing image is labelled, not left blank" "$result" "Image didn’t load" + +# The card is announced as one thing, not three loose runs: the eyebrow, alt and +# filename are aria-hidden and the wrapper carries a single composed label. +result=$(poll_eval "document.querySelector('$GONE .md-image-fallback')?.getAttribute('aria-label')") +assert_contains "Missing image is announced as a single image role" "$result" "A diagram that moved" + +# The selection ring and the drag handle both land on the NodeView's own +# element, so the wrapper has to hug the picture rather than span the measure. +result=$(poll_eval "(() => { const w = document.querySelector('.attn-doc .md-image[data-loaded]'); if (!w) return null; const img = w.querySelector('img'); return Math.abs(w.getBoundingClientRect().width - img.getBoundingClientRect().width) < 1 ? 'hugs' : 'spans'; })()") +# `--eval` hands back a JSON-encoded string, hence the contains form. +assert_contains "Image wrapper hugs the image, not the measure" "$result" "hugs" + +# A remote src has no business being rewritten. +result=$("$ATTN" --query '.attn-doc .md-image[data-src^="https:"] img' | jq -r '.elements[0].attributes.src' 2>/dev/null || echo "") +assert_eq "Absolute URL passes through untouched" "$result" "https://example.com/pixel.png" + +screenshot "07-relative-images" + # =================================================================== # Summary # =================================================================== diff --git a/tests/fixtures/chart.svg b/tests/fixtures/chart.svg new file mode 100644 index 00000000..6d59d9d9 --- /dev/null +++ b/tests/fixtures/chart.svg @@ -0,0 +1,9 @@ + + + + + + + + diff --git a/tests/fixtures/diagram.png b/tests/fixtures/diagram.png new file mode 100644 index 0000000000000000000000000000000000000000..0423d96fda05725ee38d092ff1516b282eb18441 GIT binary patch literal 176 zcmeAS@N?(olHy`uVBq!ia0vp^2|(F!Z*MpXGAM8`ICe!I znWARIps#$RfnkI3^o051`s=%x7!@2E7#NvYI3|Ee0U(!!14J@xWw5GV31K41FUWJ4 jHe)ddR9px{H}kKM%DJ8s-2;9BoxtGf>gTe~DWM4fm2oIw literal 0 HcmV?d00001 diff --git a/tests/fixtures/images.md b/tests/fixtures/images.md new file mode 100644 index 00000000..81ad8c58 --- /dev/null +++ b/tests/fixtures/images.md @@ -0,0 +1,39 @@ +# Images + +A fixture for relative image resolution in the native viewer (attn-cgev). +Every src below is authored the way a human or an agent actually writes one; +the viewer resolves each against **this file's own directory** and serves it +through the `attn://` protocol handler. + +## Sibling, dot-slash + +![A diagram](./diagram.png) + +## Sibling, bare + +![The same diagram, addressed without the dot](diagram.png) + +## Subdirectory + +![A diagram in a subdirectory](./nested/diagram.png) + +## Vector + +A referenced `.svg` is an ordinary image node — the browser loads it as an +image document. This is not the embedded-SVG path; that one is for raw `` +blocks written inline in the markdown source. + +![A bar chart](./chart.svg) + +## Absolute URL + +Anything with a scheme passes through untouched, so remote images keep working. + +![A remote pixel](https://example.com/pixel.png) + +## Missing file + +The graceful state: alt text and a filename, not the platform's broken-image +glyph. + +![A diagram that moved](./gone.png) diff --git a/tests/fixtures/nested/diagram.png b/tests/fixtures/nested/diagram.png new file mode 100644 index 0000000000000000000000000000000000000000..28444fdf6f20db5357afe5fab8fa14a1ffd81508 GIT binary patch literal 150 zcmeAS@N?(olHy`uVBq!ia0vp^4j|0I1SD0tpLGJMSWg$nkcwMxZyEA3DDW^G^h$Mc zELCb0l#Eb#d8Oc_^p3xe-(1}VRLH=f!FcS|ZBi)Y1E(?&4Y M)78&qol`;+0OTVf2LJ#7 literal 0 HcmV?d00001 diff --git a/web/src/App.svelte b/web/src/App.svelte index 94f03ebe..17139d19 100644 --- a/web/src/App.svelte +++ b/web/src/App.svelte @@ -125,6 +125,7 @@ extractStructureFromMarkdown, loadMarkdownFromPath, markdownSourceUrl, + resolveImageSrc, } from './lib/markdown-layer'; import { reviewStore } from './lib/review/store.svelte'; import { consumePendingRoomFocus } from './lib/review/pending-room-focus'; @@ -271,6 +272,19 @@ let activeTab = $derived(tabs.find((t) => t.id === activeTabId)); let activePath = $derived(activeTab?.path ?? ''); + + // Rebuilt whenever the active file changes, and deliberately NOT written as + // an inline arrow at the call site: Editor's nodeView reactor tracks this + // prop by identity, and an arrow that merely CLOSES over `activePath` keeps + // one identity forever, so every image NodeView would stay bound to the + // directory of whatever file was open when the editor mounted. `$derived.by` + // with the path read eagerly is what makes the dependency real — a plain + // `$derived((src) => …)` would never re-run either, since the body that + // reads `activePath` is not evaluated while the derived is. + let resolveActiveAssetUrl = $derived.by(() => { + const docPath = activePath; + return (src: string) => resolveImageSrc(docPath, src); + }); let hasActiveTab = $derived(Boolean(activeTab)); let activeFileType = $derived(activeTab?.fileType ?? 'unsupported'); @@ -3679,10 +3693,14 @@ /> {/if} {:else if activeFileType === 'markdown'} + ; + /** + * Maps a markdown image's authored `src` onto a URL this webview can + * actually load, or `null` when it cannot be resolved. Only the DISPLAYED + * src is affected — `node.attrs.src` keeps the authored string, so + * markdown serialization still round-trips it. + * + * Omitted by every caller that has no local file behind the document (the + * hosted app, the reviewer viewing an owner's snapshot): resolving there + * would mint a plausible-looking path to a file that isn't on that + * machine. Unset, the image NodeView is not registered at all and images + * render through the stock spec — byte-identical DOM, including the + * platform's own broken-image glyph. + */ + resolveAssetUrl?: (src: string) => string | null; /** * Invoked once the underlying `EditorView` is mounted (and again after a * full re-mount). Callers use this to dispatch their own meta-only @@ -149,6 +164,7 @@ onDocChange, plugins: extraPlugins, nodeViews: extraNodeViews, + resolveAssetUrl, onReady, collabClientId, collabEpoch = 0, @@ -478,6 +494,14 @@ const builtIn: Record = { task_list_item: taskListItemNodeView, frontmatter: (node) => frontmatterNodeView(node), + // Registered ONLY where there is something to resolve against. With no + // resolver the view would render the authored src verbatim — the same + // bytes the stock `toDOM` emits — but it would still swap the platform's + // broken-image glyph for our placeholder on every relative src the + // hosted app cannot load yet, which is a visible change on a surface + // that is explicitly out of scope until workspace-relative assets land. + // Leaving the key off keeps that DOM byte-identical. + ...(resolveAssetUrl ? { image: (node: PmNode) => imageNodeView(node, resolveAssetUrl) } : {}), code_block(node, editorView, getPos) { const mermaid = mermaidNodeView(node, editorView, getPos); if (mermaid) return mermaid; @@ -1006,6 +1030,22 @@ // Touch the reactive props so Svelte tracks them. void extraPlugins; void extraNodeViews; + // Tracked so switching tabs rebuilds the image NodeViews against the new + // document's directory. buildNodeViews() hands back fresh closures every + // call, so prosemirror-view's identity comparison sees a change and + // redraws; without this line the images keep the previous file's base dir + // and silently resolve against the wrong folder. + // + // Costly on purpose, and the cost is the point: because the closures are + // never identical, `changedNodeViews()` reports a change and prosemirror + // tears down and rebuilds the ENTIRE docView — every mermaid, math, + // code-block and frontmatter view in the document, losing their local + // state (a panned diagram returns to its default view). That is what it + // takes to re-resolve images already on screen, and the one flow where it + // is visible is a path change with unchanged content, i.e. a rename; a tab + // switch replaces the document anyway. Do not add further props to this + // effect casually — each one buys the same full redraw. + void resolveAssetUrl; if (!view) return; const nextState = view.state.reconfigure({ plugins: buildPlugins(lastMarkdown) }); view.updateState(nextState); diff --git a/web/src/lib/image-src-resolution.test.ts b/web/src/lib/image-src-resolution.test.ts new file mode 100644 index 00000000..3eb10bab --- /dev/null +++ b/web/src/lib/image-src-resolution.test.ts @@ -0,0 +1,491 @@ +// Relative image src resolution for the native viewer (attn-cgev). +// +// Run with: +// +// cd web && npx tsx src/lib/image-src-resolution.test.ts +// +// Three invariants, and they are independent: +// +// 1. WHAT THE PARSER HANDS US (cases 1-9). `resolveImageSrc` is specified +// against `node.attrs.src`, not against the bytes an author typed — +// markdown-it normalises hrefs through mdurl on the way in. These cases +// run the REAL parser from schema.ts so the resolver's encoding policy is +// pinned to the parser it actually sits behind, not to an assumption +// about it. +// +// 2. THE ENCODING POLICY (cases 10-24). Exactly one level of percent- +// encoding must survive to the Rust handler, which truncates at the first +// '?' or '#' BEFORE decoding — so those two characters have to be escaped +// even though they arrive literal, and everything mdurl already escaped +// must NOT be escaped twice. +// +// 3. attrs.src IS NEVER REWRITTEN (cases 25-27). `image` has no serializer +// override in schema.ts, so prosemirror-markdown writes `attrs.src` +// verbatim on every save. The resolver is a pure function that returns a +// new string; these cases prove the node it was derived from still +// round-trips. + +import { markdownParser, markdownSerializer } from './schema'; +import type { Node as PmNode } from 'prosemirror-model'; +import { resolveImageSrc } from './markdown-layer'; + +// --------------------------------------------------------------------------- +// Tiny harness (mirrors prosemirror/frontmatter-nodeview.test.ts) +// --------------------------------------------------------------------------- + +interface CaseResult { + name: string; + ok: boolean; + detail?: string; +} + +const cases: Array<() => CaseResult> = []; + +function defineCase(name: string, fn: () => void | string): void { + cases.push(() => { + try { + const note = fn(); + return { name, ok: true, detail: typeof note === 'string' ? note : undefined }; + } catch (err) { + return { name, ok: false, detail: err instanceof Error ? err.message : String(err) }; + } + }); +} + +function assert(cond: boolean, msg: string): asserts cond { + if (!cond) throw new Error(msg); +} + +function assertEq(actual: T, expected: T, msg: string): void { + const a = JSON.stringify(actual); + const b = JSON.stringify(expected); + if (a !== b) throw new Error(`${msg}: expected ${b}, got ${a}`); +} + +/** The `src` the parser stores for the first image in `md`. */ +function parsedSrc(md: string): string | null { + const doc: PmNode | null = markdownParser.parse(md); + assert(doc !== null, 'parser returned null'); + let found: string | null = null; + doc.descendants((node) => { + if (found === null && node.type.name === 'image') found = node.attrs.src as string; + return found === null; + }); + return found; +} + +const DOC = '/Users/me/notes/plan.md'; + +/** Resolve straight from markdown, so the parser sits in the loop. */ +function resolveFromMarkdown(md: string, docPath = DOC): string | null { + const src = parsedSrc(md); + assert(src !== null, `no image node parsed from ${JSON.stringify(md)}`); + return resolveImageSrc(docPath, src); +} + +// --------------------------------------------------------------------------- +// 1. What the parser hands us +// --------------------------------------------------------------------------- + +defineCase('1. dot-slash sibling resolves against the file, not the app origin', () => { + assertEq( + resolveFromMarkdown('![a](./diagram.png)'), + 'attn://localhost/Users/me/notes/diagram.png', + './diagram.png', + ); +}); + +defineCase('2. bare sibling resolves the same as the dot-slash form', () => { + assertEq( + resolveFromMarkdown('![a](diagram.png)'), + 'attn://localhost/Users/me/notes/diagram.png', + 'diagram.png', + ); +}); + +defineCase('3. subdirectory', () => { + assertEq( + resolveFromMarkdown('![a](sub/diagram.png)'), + 'attn://localhost/Users/me/notes/sub/diagram.png', + 'sub/diagram.png', + ); + assertEq( + resolveFromMarkdown('![a](./sub/diagram.png)'), + 'attn://localhost/Users/me/notes/sub/diagram.png', + './sub/diagram.png', + ); +}); + +defineCase('4. parent traversal walks up one directory per `..`', () => { + assertEq( + resolveFromMarkdown('![a](../assets/x.png)'), + 'attn://localhost/Users/me/assets/x.png', + '../assets/x.png', + ); + assertEq( + resolveFromMarkdown('![a](../../assets/x.png)'), + 'attn://localhost/Users/assets/x.png', + '../../assets/x.png', + ); +}); + +defineCase('5. `..` can never climb above the filesystem root', () => { + assertEq( + resolveImageSrc(DOC, '../../../../../../../../etc/passwd'), + 'attn://localhost/etc/passwd', + 'traversal is clamped at /', + ); + assertEq( + resolveImageSrc('/a.md', '../../../x.png'), + 'attn://localhost/x.png', + 'clamped from a root-level document', + ); +}); + +defineCase('6. interior `.` and `..` segments normalise', () => { + assertEq( + resolveImageSrc(DOC, './a/./b/../c/x.png'), + 'attn://localhost/Users/me/notes/a/c/x.png', + 'mixed . and ..', + ); + assertEq( + resolveImageSrc(DOC, 'a//b/x.png'), + 'attn://localhost/Users/me/notes/a/b/x.png', + 'empty segments collapse', + ); +}); + +defineCase('7. a leading slash is FILESYSTEM-absolute, not project-root-relative', () => { + // Deliberate divergence from resolvePath() in App.svelte, which treats a + // leading '/' in a LINK as project-root-relative. Documented at the resolver. + assertEq( + resolveFromMarkdown('![a](/Users/other/x.png)'), + 'attn://localhost/Users/other/x.png', + 'absolute src', + ); + assertEq( + resolveImageSrc('', '/tmp/x.png'), + 'attn://localhost/tmp/x.png', + 'an absolute src needs no document path', + ); +}); + +defineCase('8. absolute URLs and protocol-relative srcs pass through untouched', () => { + for (const src of [ + 'https://example.com/x.png', + 'http://example.com/x.png', + 'data:image/png;base64,iVBORw0KGgo=', + 'blob:attn://localhost/9a2f', + 'attn://localhost/Users/me/x.png', + '//cdn.example.com/x.png', + ]) { + assertEq(resolveImageSrc(DOC, src), src, `passthrough ${src}`); + } + // Through the parser too, since mdurl could in principle rewrite them. + assertEq( + resolveFromMarkdown('![a](https://example.com/x.png)'), + 'https://example.com/x.png', + 'parsed https passthrough', + ); + return '6 schemes'; +}); + +defineCase('9. unresolvable srcs return null rather than inventing a URL', () => { + assertEq(resolveImageSrc(DOC, ''), null, 'empty src'); + assertEq(resolveImageSrc(DOC, '#anchor'), null, 'bare fragment'); + assertEq(resolveImageSrc('', './x.png'), null, 'relative src, no document path'); + assertEq(resolveImageSrc('notes/plan.md', './x.png'), null, 'relative src, relative doc path'); + assertEq(resolveImageSrc(DOC, '.'), null, 'src that normalises to nothing'); + assertEq(resolveImageSrc(DOC, '/'), null, 'src that is only the root'); + // A directory is not an image; the handler would answer these with an empty + // 404 and the URL would look convincing while doing it. + assertEq(resolveImageSrc(DOC, './'), null, 'trailing slash on the current dir'); + assertEq(resolveImageSrc(DOC, '..'), null, 'bare parent'); + assertEq(resolveImageSrc(DOC, './assets/'), null, 'trailing slash on a subdirectory'); +}); + +// --------------------------------------------------------------------------- +// 2. The encoding policy +// --------------------------------------------------------------------------- + +defineCase('10. a space arrives pre-encoded and is NOT encoded again', () => { + // markdown-it only produces an image node for a space when the src is + // angle-wrapped; it hands back './my%20shot.png' either way. + assertEq(parsedSrc('![a](<./my shot.png>)'), './my%20shot.png', 'angle form is normalised'); + assertEq(parsedSrc('![a](./my%20shot.png)'), './my%20shot.png', 'escaped form is preserved'); + assertEq( + resolveFromMarkdown('![a](<./my shot.png>)'), + 'attn://localhost/Users/me/notes/my%20shot.png', + 'single encoding survives', + ); + assertEq( + resolveFromMarkdown('![a](./my%20shot.png)'), + 'attn://localhost/Users/me/notes/my%20shot.png', + 'no double encoding', + ); +}); + +defineCase('11. non-ASCII arrives pre-encoded and is NOT encoded again', () => { + assertEq(parsedSrc('![a](./café.png)'), './caf%C3%A9.png', 'mdurl UTF-8 escapes'); + assertEq( + resolveFromMarkdown('![a](./café.png)'), + 'attn://localhost/Users/me/notes/caf%C3%A9.png', + 'not %25C3%25A9', + ); + // The same file addressed in its already-escaped form must land identically. + assertEq( + resolveFromMarkdown('![a](./caf%C3%A9.png)'), + 'attn://localhost/Users/me/notes/caf%C3%A9.png', + 'idempotent across both spellings', + ); +}); + +defineCase('12. a literal percent survives as exactly one escape', () => { + assertEq(parsedSrc('![a](./100%.png)'), './100%25.png', 'lone % is repaired by mdurl'); + assertEq( + resolveFromMarkdown('![a](./100%.png)'), + 'attn://localhost/Users/me/notes/100%25.png', + 'one level of encoding', + ); + assertEq( + resolveFromMarkdown('![a](./100%25.png)'), + 'attn://localhost/Users/me/notes/100%25.png', + 'the escaped spelling agrees', + ); +}); + +defineCase("13. '#' is escaped even though the parser leaves it literal", () => { + // This is the whole reason encodeURI() is not usable here: the Rust handler + // truncates the path at the first '#' BEFORE percent-decoding, so an + // un-escaped one amputates the filename and 404s. + assertEq(parsedSrc('![a](./weird#hash.png)'), './weird#hash.png', 'parser keeps it raw'); + assertEq( + resolveFromMarkdown('![a](./weird#hash.png)'), + 'attn://localhost/Users/me/notes/weird%23hash.png', + '# → %23', + ); +}); + +defineCase("14. '?' is escaped for the same reason", () => { + assertEq(parsedSrc('![a](./q?uery.png)'), './q?uery.png', 'parser keeps it raw'); + assertEq( + resolveFromMarkdown('![a](./q?uery.png)'), + 'attn://localhost/Users/me/notes/q%3Fuery.png', + '? → %3F', + ); +}); + +defineCase('15. a query-looking suffix is filename bytes, not a query string', () => { + // There is no server behind attn:// to interpret a query, and filenames + // containing '?' are real. The bytes win — stated so the choice is pinned + // rather than discovered. + assertEq( + resolveImageSrc(DOC, './x.png?v=2'), + 'attn://localhost/Users/me/notes/x.png%3Fv%3D2', + 'cache-buster is treated as part of the name', + ); + assertEq( + resolveImageSrc(DOC, './x.png#frag'), + 'attn://localhost/Users/me/notes/x.png%23frag', + 'trailing fragment likewise', + ); +}); + +defineCase("16. an encoded '/' is a separator, because the handler decodes it into one", () => { + // src/main.rs percent-decodes the WHOLE path in a single call before + // fs::read, so a `%2F` that survived to the URL reappears as a real + // separator on the far side. Treating it as one here is what keeps the + // emitted URL a truthful name for the file that actually gets opened. + assertEq(parsedSrc('![a](./a%2Fb.png)'), './a%2Fb.png', 'parser preserves it'); + assertEq( + resolveFromMarkdown('![a](./a%2Fb.png)'), + 'attn://localhost/Users/me/notes/a/b.png', + 'decoded into a directory step', + ); + // The regression this pins: while `%2F` was kept inside a segment, the + // normalisation walk saw no separators to walk, so the traversal below rode + // straight past the clamp and the handler opened a file four directories + // above the one the resolver claimed to have computed. + assertEq( + resolveImageSrc(DOC, 'a%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd'), + 'attn://localhost/etc/passwd', + 'encoded traversal normalises exactly like the literal form', + ); + assertEq( + resolveImageSrc(DOC, 'a/../../../../etc/passwd'), + resolveImageSrc(DOC, 'a%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd'), + 'the two spellings name the same file', + ); + assertEq(resolveImageSrc(DOC, './a%2F'), null, 'a src that decodes to a directory is declined'); +}); + +defineCase("17. '%2E%2E' is normalised as traversal, not smuggled through", () => { + // Decode happens before normalisation precisely so an encoded '..' cannot + // slip past the segment walk as a literal filename. + assertEq( + resolveImageSrc(DOC, '%2E%2E/x.png'), + 'attn://localhost/Users/me/x.png', + 'encoded parent behaves like ..', + ); +}); + +defineCase('18. a malformed escape falls back to raw bytes instead of throwing', () => { + // markdown-it repairs these, but attrs.src also arrives from paste and + // DOM-parse paths that do not go through it. + assertEq( + resolveImageSrc(DOC, './a%zz.png'), + 'attn://localhost/Users/me/notes/a%25zz.png', + 'undecodable segment is encoded as literal bytes', + ); + assertEq( + resolveImageSrc(DOC, './%.png'), + 'attn://localhost/Users/me/notes/%25.png', + 'lone percent', + ); +}); + +defineCase('19. characters mdurl leaves literal are escaped when they must be', () => { + assertEq( + resolveImageSrc(DOC, "./a(b)'c!.png"), + "attn://localhost/Users/me/notes/a(b)'c!.png", + 'encodeURIComponent leaves these alone, and so does the handler', + ); + assertEq( + resolveImageSrc(DOC, './a&b=c,d.png'), + 'attn://localhost/Users/me/notes/a%26b%3Dc%2Cd.png', + 'sub-delims are escaped, which the handler decodes back', + ); +}); + +defineCase('20. a directory component carrying a space is encoded too', () => { + assertEq( + resolveImageSrc(DOC, './my%20dir/x.png'), + 'attn://localhost/Users/me/notes/my%20dir/x.png', + 'per-segment, not whole-path', + ); +}); + +defineCase('21. the DOCUMENT path is raw bytes and is encoded exactly once', () => { + // docPath comes from the daemon, never from mdurl. Decoding it would read a + // literal '%' in a directory name as an escape. + assertEq( + resolveImageSrc('/Users/me/100% notes/plan.md', './x.png'), + 'attn://localhost/Users/me/100%25%20notes/x.png', + 'document directory encoded once', + ); + assertEq( + resolveImageSrc('/Users/me/a%2Fb/plan.md', './x.png'), + 'attn://localhost/Users/me/a%252Fb/x.png', + 'a literal %2F in a directory name is not an escape', + ); +}); + +defineCase('22. the emitted URL always has exactly one slash after the host', () => { + // Two would be parsed as a host, and the handler would never see the path. + for (const src of ['./x.png', '/x.png', '../x.png', 'sub/x.png']) { + const url = resolveImageSrc(DOC, src); + assert(url !== null, `expected a URL for ${src}`); + assert(url.startsWith('attn://localhost/'), `${src}: wrong prefix — ${url}`); + assert(!url.startsWith('attn://localhost//'), `${src}: doubled slash — ${url}`); + } + return '4 shapes'; +}); + +defineCase('23. a Windows drive letter is not mistaken for a URL scheme', () => { + // Windows is not a supported host; the point is only that `C:/…` must not + // fall into the scheme passthrough and must not climb above the drive. + assertEq( + resolveImageSrc(DOC, 'C:/shots/x.png'), + 'attn://localhost/C%3A/shots/x.png', + 'drive-rooted path', + ); + assertEq( + resolveImageSrc(DOC, 'C:/../../x.png'), + 'attn://localhost/C%3A/x.png', + 'pinned at the drive', + ); +}); + +defineCase('24. a backslash is an ordinary filename byte on POSIX', () => { + // Unlike resolvePath(), this does NOT rewrite '\' to '/': the only paths it + // emits are POSIX ones for the attn:// handler, where a backslash is a legal + // name character and rewriting it would address the wrong file. + assertEq( + resolveImageSrc(DOC, './a\\b.png'), + 'attn://localhost/Users/me/notes/a%5Cb.png', + 'escaped, not split', + ); +}); + +// --------------------------------------------------------------------------- +// 3. attrs.src is never rewritten +// --------------------------------------------------------------------------- + +defineCase('25. the resolver does not mutate the node it reads from', () => { + const doc = markdownParser.parse('![a](./diagram.png)'); + assert(doc !== null, 'parser returned null'); + let image: PmNode | null = null; + doc.descendants((node) => { + if (image === null && node.type.name === 'image') image = node; + return image === null; + }); + assert(image !== null, 'no image node'); + const before = (image as PmNode).attrs.src as string; + const resolved = resolveImageSrc(DOC, before); + assert(resolved !== before, 'resolution should have produced a different string'); + assertEq((image as PmNode).attrs.src, './diagram.png', 'attrs.src untouched'); +}); + +defineCase('26. relative srcs round-trip byte-for-byte through the serializer', () => { + // Scoped to the forms that are exact TODAY. The angle-bracket, café, 100% + // and paren spellings are lossy in prosemirror-markdown before this issue + // exists; chasing them here would pin a bug in place. + const exact = [ + '![a](./diagram.png)', + '![a](diagram.png)', + '![a](../up/diagram.png)', + '![a](sub/diagram.png)', + '![a](./my%20shot.png)', + '![a](./weird#hash.png)', + '![a](./q?uery.png)', + '![a](./a%2Fb.png)', + '![a](/Users/x/abs.png)', + '![a](//cdn.example.com/x.png)', + '![a](https://example.com/x.png)', + '![a](data:image/png;base64,iVBORw0KGgo=)', + '![a](attn://localhost/Users/x/abs.png)', + ]; + for (const md of exact) { + const doc = markdownParser.parse(md); + assert(doc !== null, 'parser returned null'); + assertEq(markdownSerializer.serialize(doc), md, `round-trip ${md}`); + } + return `${exact.length} forms`; +}); + +defineCase('27. resolution is a pure function of (docPath, src)', () => { + const a = resolveImageSrc(DOC, './x.png'); + const b = resolveImageSrc(DOC, './x.png'); + assertEq(a, b, 'same inputs, same output'); + assertEq( + resolveImageSrc('/Users/me/other/plan.md', './x.png'), + 'attn://localhost/Users/me/other/x.png', + 'a different document moves the base directory', + ); +}); + +// --------------------------------------------------------------------------- + +function runAllCases(): void { + const results = cases.map((run) => run()); + for (const result of results) { + console.log( + `${result.ok ? 'PASS' : 'FAIL'} ${result.name}${result.detail ? ` — ${result.detail}` : ''}`, + ); + } + const failed = results.filter((r) => !r.ok); + console.log(`\n${results.length - failed.length}/${results.length} image-src cases passed.`); + if (failed.length > 0) process.exit(1); +} + +runAllCases(); diff --git a/web/src/lib/markdown-layer.ts b/web/src/lib/markdown-layer.ts index f7dbd7bc..7409bf5e 100644 --- a/web/src/lib/markdown-layer.ts +++ b/web/src/lib/markdown-layer.ts @@ -108,3 +108,152 @@ export async function loadMarkdownFromPath(path: string): Promise { } return response.text(); } + +// -------------------------------------------------------------------------- +// Relative image resolution (attn-cgev) +// +// A markdown image src is written relative to the FILE, but the native +// document is served from attn://app (or the Vite dev server under `task +// dev`), so `./diagram.png` resolves against the app origin and 404s. Every +// local asset has to go back through the attn:// custom protocol, which serves +// any absolute path: `attn://localhost/` (src/main.rs). +// +// ENCODING. This deliberately does NOT reuse `markdownSourceUrl`'s +// `encodeURI`, because the two functions take different input: +// +// - `markdownSourceUrl` takes a RAW filesystem path straight from the daemon. +// - this takes `node.attrs.src`, which markdown-it has already normalised +// through mdurl: `./café.png` arrives as `./caf%C3%A9.png`, `<./my +// shot.png>` as `./my%20shot.png`, a lone `%` as `%25`. Running encodeURI +// over that double-encodes (`%C3%A9` → `%25C3%25A9`) and the handler then +// looks for a file literally named `caf%C3%A9.png`. +// +// mdurl also leaves `#` and `?` LITERAL, and the handler truncates the path at +// the first of either (src/main.rs, the `raw_path.find(['?', '#'])` line) +// BEFORE percent-decoding — so an un-escaped `#` silently amputates the name. +// encodeURI escapes neither. +// +// The one policy that is coherent for both facts: decode each `/`-delimited +// piece exactly once, then re-encode each with encodeURIComponent — which does +// escape `#`, `?`, `%` and space, and never sees a `/` because the pieces are +// split apart first. Segments taken from `docPath` are NOT decoded: that +// string is raw filesystem bytes, so a document at `/Users/me/100%.md` must +// not have its `%` read as an escape. +// +// An encoded separator IS a separator. `./a%2Fb.png` becomes `a` + `b.png`, +// not one segment named `a/b.png`, because the handler percent-decodes the +// WHOLE path in one call (`percent_decode_str(raw_path)`) before `fs::read` — +// so a `%2F` this side of the wire is a real `/` on the other side no matter +// what we intend by it. Keeping it inside a segment would only make the +// normalisation walk below disagree with the file that actually gets opened: +// `a%2F..%2F..%2Fx.png` would look like an innocent filename here and open +// two directories up there. Decoding before the walk keeps the URL we emit a +// truthful name for the file the handler will read. The cost is that a file +// whose name literally contains the three characters `%2F` is unreachable — +// it is unreachable through this protocol regardless, since the handler's +// single decode leaves no way to spell one. +// +// A consequence worth stating: `![](x.png?v=2)` looks for a file literally +// named `x.png?v=2`. There is no server behind attn:// to interpret a query, +// and filenames containing `#`/`?` are real, so the bytes win. +// +// LEADING SLASH. `![](/img/x.png)` is treated as a FILESYSTEM-absolute path, +// which deliberately diverges from `resolvePath` in App.svelte — that one +// treats a leading `/` in a LINK as project-root-relative. The two disagree on +// purpose: a wrong link target lands the user in a "file not found" shell they +// can navigate out of, whereas a wrong image src renders a placeholder with no +// recourse, and in agent-authored docs an absolute image src is overwhelmingly +// a real path (`/Users/…`, `/tmp/…`) that a project-root join would destroy. +// If that ever needs to change, change it here and in `resolvePath` together. +// -------------------------------------------------------------------------- + +/** `scheme:` — RFC 3986, but requiring 2+ chars so a `C:` drive letter is not + * mistaken for a scheme (the Windows form is handled separately below). */ +const URL_SCHEME = /^[a-zA-Z][a-zA-Z\d+\-.]+:/; + +/** `C:/…` — recognised only so it is not read as a scheme. Windows is not a + * supported host today; the path is passed through as absolute rather than + * silently re-rooted. */ +const WINDOWS_DRIVE = /^[a-zA-Z]:\//; + +/** markdown-it repairs malformed escapes, so its output never throws here — + * but `attrs.src` also arrives from paste and DOM-parse paths that do not go + * through it, and a lone `%` there would throw. Raw bytes are the honest + * fallback. */ +function decodeSegment(segment: string): string { + try { + return decodeURIComponent(segment); + } catch { + return segment; + } +} + +/** + * Map a markdown image `src` onto a URL the webview can actually load. + * + * Returns `null` when the src cannot be resolved — an empty src, a bare + * fragment, or a relative src with no absolute document path to resolve it + * against. Callers render the authored src in that case and let the broken + * state speak for itself; inventing a root-relative attn:// URL would be worse + * than doing nothing (see `is_reserved_localhost_review` in src/main.rs). + * + * Absolute URLs (any scheme) and protocol-relative `//host/…` pass through + * untouched — they already address something the webview can fetch, and + * rewriting them would break remote images. + * + * @param docPath absolute filesystem path of the markdown file, raw/unencoded + * @param src `node.attrs.src` exactly as the parser produced it + */ +export function resolveImageSrc(docPath: string, src: string): string | null { + if (!src) return null; + + // Protocol-relative and fully-qualified URLs are already loadable. + if (src.startsWith('//')) return src; + const windowsDrive = WINDOWS_DRIVE.test(src); + if (!windowsDrive && URL_SCHEME.test(src)) return src; + + // A bare `#anchor` addresses a place in the document, never a file. + if (src.startsWith('#')) return null; + + // Decoded first, then re-split: a `%2F` the parser preserved is a separator + // to the handler, so it has to be one here too or the normalisation walk + // below would be reasoning about a different path than the one opened. + const srcSegments = src.split('/').flatMap((piece) => decodeSegment(piece).split('/')); + + // A src whose last segment names a directory (`.`, `..`, a trailing slash, + // or a bare `/`) can never be an image. Left alone it would normalise to a + // directory URL, which the handler answers with an empty 404 — a plausible + // URL for a thing that is not a file. + const last = srcSegments[srcSegments.length - 1]; + if (last === '' || last === '.' || last === '..') return null; + + // Segments that are pinned against `..` — the filesystem root has none, a + // Windows drive has the drive itself. Escaping above either is nonsense, and + // the handler does no confinement of its own. + let pinned = 0; + let segments: string[]; + if (windowsDrive) { + segments = srcSegments; + pinned = 1; + } else if (src.startsWith('/')) { + segments = srcSegments; + } else { + if (!docPath.startsWith('/')) return null; + const slash = docPath.lastIndexOf('/'); + const dir = slash > 0 ? docPath.slice(0, slash) : ''; + segments = [...dir.split('/'), ...srcSegments]; + } + + const stack: string[] = []; + for (const segment of segments) { + if (!segment || segment === '.') continue; + if (segment === '..') { + if (stack.length > pinned) stack.pop(); + continue; + } + stack.push(segment); + } + if (stack.length === 0) return null; + + return `attn://localhost/${stack.map(encodeURIComponent).join('/')}`; +} diff --git a/web/src/lib/prosemirror/image-nodeview.test.ts b/web/src/lib/prosemirror/image-nodeview.test.ts new file mode 100644 index 00000000..96e59130 --- /dev/null +++ b/web/src/lib/prosemirror/image-nodeview.test.ts @@ -0,0 +1,415 @@ +// Image NodeView: the DOM is resolved, the node is not (attn-cgev). +// +// Run with: +// +// cd web && npx tsx src/lib/prosemirror/image-nodeview.test.ts +// +// The NodeView reaches for the global `document`, as every NodeView in this +// directory does, so `withFakeDocument` swaps in the stub from +// components/ui/accordion/fake-dom.ts and the REAL `imageNodeView` runs +// against it. Two things are being proved: +// +// 1. `` carries the RESOLVED url while `node.attrs.src` keeps the +// authored string. `image` has no serializer override in schema.ts, so +// prosemirror-markdown writes `attrs.src` verbatim on save — a NodeView +// that "helpfully" normalised it would rewrite the user's file. +// +// 2. The failure state is the document's own, not the platform's broken +// image glyph: alt text plus a filename, gated on the 's own error +// event so a slow load is never mistaken for a missing file. +// +// A third group reads the two wiring sites as source text. Both are silent +// failures — the images simply resolve against the wrong directory, with no +// error anywhere — and neither is reachable from a NodeView unit test, so the +// source is the only thing left to hold them to. + +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { markdownParser } from '../schema'; +import type { Node as PmNode } from 'prosemirror-model'; +import type { DecorationSource } from 'prosemirror-view'; +import { FakeElement, withFakeDocument } from '../components/ui/accordion/fake-dom'; +import { resolveImageSrc } from '../markdown-layer'; +import { imageFileName, imageNodeView } from './image-nodeview'; + +// --------------------------------------------------------------------------- +// Tiny harness (mirrors frontmatter-nodeview.test.ts) +// --------------------------------------------------------------------------- + +interface CaseResult { + name: string; + ok: boolean; + detail?: string; +} + +const cases: Array<() => CaseResult> = []; + +function defineCase(name: string, fn: () => void | string): void { + cases.push(() => { + try { + const note = fn(); + return { name, ok: true, detail: typeof note === 'string' ? note : undefined }; + } catch (err) { + return { name, ok: false, detail: err instanceof Error ? err.message : String(err) }; + } + }); +} + +function assert(cond: boolean, msg: string): asserts cond { + if (!cond) throw new Error(msg); +} + +function assertEq(actual: T, expected: T, msg: string): void { + const a = JSON.stringify(actual); + const b = JSON.stringify(expected); + if (a !== b) throw new Error(`${msg}: expected ${b}, got ${a}`); +} + +const DOC = '/Users/me/notes/plan.md'; + +/** `update()` takes decoration arguments this view never reads. Passing an + * empty stand-in keeps the calls honest to the interface without building a + * DecorationSet the assertions would ignore. */ +const NO_INNER_DECORATIONS = [] as unknown as DecorationSource; + +function imageNode(md: string): PmNode { + const doc = markdownParser.parse(md); + assert(doc !== null, 'parser returned null'); + let found: PmNode | null = null; + doc.descendants((node) => { + if (found === null && node.type.name === 'image') found = node; + return found === null; + }); + assert(found !== null, `no image node in ${JSON.stringify(md)}`); + return found as PmNode; +} + +/** Build the view against the fake DOM and hand back the pieces every case + * wants: the wrapper, the , and the placeholder subtree. */ +const NATIVE_RESOLVER = (src: string) => resolveImageSrc(DOC, src); + +/** `'none'` stands for the prop being omitted — passing `undefined` through a + * default parameter would silently reinstate the default resolver. */ +function mount(node: PmNode, resolve: ((src: string) => string | null) | 'none' = NATIVE_RESOLVER) { + const view = imageNodeView(node, resolve === 'none' ? undefined : resolve); + const dom = view.dom as unknown as FakeElement; + const img = dom.findAll('img')[0]; + assert(img !== undefined, 'no in the view'); + const fallback = dom.findByClass('md-image-fallback')[0]; + assert(fallback !== undefined, 'no placeholder in the view'); + return { view, dom, img, fallback }; +} + +// --------------------------------------------------------------------------- +// 1. Resolved DOM, untouched node +// --------------------------------------------------------------------------- + +defineCase('1. the DOM gets the resolved src', () => + withFakeDocument(() => { + const node = imageNode('![A diagram](./diagram.png)'); + const { img } = mount(node); + assertEq( + img.getAttribute('src'), + 'attn://localhost/Users/me/notes/diagram.png', + 'displayed src', + ); + assertEq(img.getAttribute('alt'), 'A diagram', 'alt is carried through'); + }), +); + +defineCase('2. node.attrs.src is never written', () => + withFakeDocument(() => { + const node = imageNode('![A diagram](./diagram.png)'); + const { view } = mount(node); + assertEq(node.attrs.src, './diagram.png', 'attrs.src after construction'); + view.update?.(node, [], NO_INNER_DECORATIONS); + assertEq(node.attrs.src, './diagram.png', 'attrs.src after update'); + // Also exposed on the wrapper, so automation can see what the resolver + // was handed without reaching into ProseMirror state. + const dom = view.dom as unknown as FakeElement; + assertEq(dom.getAttribute('data-src'), './diagram.png', 'data-src is the authored string'); + }), +); + +defineCase('3. with no resolver the authored src is used verbatim', () => + withFakeDocument(() => { + // The app never mounts the view without a resolver — Editor.svelte leaves + // the key off entirely (case 19) — but the parameter is optional, so the + // degenerate case is held to doing nothing rather than inventing a URL. + const { img } = mount(imageNode('![a](./diagram.png)'), 'none'); + assertEq(img.getAttribute('src'), './diagram.png', 'unchanged'); + }), +); + +defineCase('4. a src the resolver declines falls back to the authored string', () => + withFakeDocument(() => { + const { img } = mount(imageNode('![a](./diagram.png)'), () => null); + assertEq(img.getAttribute('src'), './diagram.png', 'null means "leave it alone"'); + }), +); + +defineCase('5. absolute URLs reach the DOM untouched', () => + withFakeDocument(() => { + const { img } = mount(imageNode('![a](https://example.com/x.png)')); + assertEq(img.getAttribute('src'), 'https://example.com/x.png', 'passthrough'); + }), +); + +defineCase('6. update() re-resolves when the src changes and rejects other types', () => + withFakeDocument(() => { + const node = imageNode('![a](./diagram.png)'); + const { view, img } = mount(node); + const moved = imageNode('![a](./nested/diagram.png)'); + assertEq(view.update?.(moved, [], NO_INNER_DECORATIONS), true, 'same type updates in place'); + assertEq( + img.getAttribute('src'), + 'attn://localhost/Users/me/notes/nested/diagram.png', + 're-resolved', + ); + const paragraph = markdownParser.parse('hello')?.firstChild; + assert(paragraph !== null && paragraph !== undefined, 'no paragraph'); + assertEq(view.update?.(paragraph, [], NO_INNER_DECORATIONS), false, 'a different node type is rejected'); + }), +); + +defineCase('7. an unchanged src is not re-written to the DOM', () => + withFakeDocument(() => { + // Re-setting an identical src may not re-fire `load`, so clearing the + // state flags on every update would strand a healthy image with neither + // data-loaded nor data-broken set. + const node = imageNode('![a](./diagram.png)'); + const { view, img, dom } = mount(node); + img.fire('load'); + assertEq(dom.getAttribute('data-loaded'), 'true', 'loaded after the load event'); + view.update?.(node, [], NO_INNER_DECORATIONS); + assertEq(dom.getAttribute('data-loaded'), 'true', 'still loaded after a no-op update'); + }), +); + +// --------------------------------------------------------------------------- +// 2. The failure state +// --------------------------------------------------------------------------- + +defineCase('8. the placeholder stays out of the way until the image fails', () => + withFakeDocument(() => { + const { dom, img } = mount(imageNode('![A diagram that moved](./gone.png)')); + assertEq(dom.getAttribute('data-broken'), null, 'not broken before the error fires'); + assertEq(img.getAttribute('hidden'), null, 'the img is visible while it loads'); + img.fire('error'); + assertEq(dom.getAttribute('data-broken'), 'true', 'broken after the error'); + assertEq(img.getAttribute('hidden'), '', 'the broken glyph is hidden'); + assertEq(dom.getAttribute('data-loaded'), null, 'not loaded'); + }), +); + +defineCase('9. the placeholder carries the alt text and the filename', () => + withFakeDocument(() => { + const { dom, img } = mount(imageNode('![A diagram that moved](./sub/gone%20away.png)')); + img.fire('error'); + const alt = dom.findByClass('md-image-fallback-alt')[0]; + const name = dom.findByClass('md-image-fallback-name')[0]; + const label = dom.findByClass('md-image-fallback-label')[0]; + assert(alt !== undefined && name !== undefined && label !== undefined, 'placeholder parts'); + assertEq(alt.textContent, 'A diagram that moved', 'alt text'); + assertEq(name.textContent, 'gone away.png', 'filename, decoded for reading'); + // What the app observed, not a diagnosis: `error` also fires for a file + // that is present but served with a MIME the webview will not decode. + assertEq(label.textContent, 'Image didn’t load', 'label'); + assertEq(alt.getAttribute('hidden'), null, 'the alt line is shown when there is alt text'); + }), +); + +defineCase('10. an image with no alt text hides the alt line rather than showing an empty one', () => + withFakeDocument(() => { + const { dom, img } = mount(imageNode('![](./gone.png)')); + img.fire('error'); + const alt = dom.findByClass('md-image-fallback-alt')[0]; + assert(alt !== undefined, 'no alt line'); + assertEq(alt.textContent, '', 'empty'); + assertEq(alt.getAttribute('hidden'), '', 'hidden'); + }), +); + +defineCase('11. a recovered src clears the broken state', () => + withFakeDocument(() => { + const node = imageNode('![a](./gone.png)'); + const { view, dom, img } = mount(node); + img.fire('error'); + assertEq(dom.getAttribute('data-broken'), 'true', 'broken'); + view.update?.(imageNode('![a](./diagram.png)'), [], NO_INNER_DECORATIONS); + assertEq(dom.getAttribute('data-broken'), null, 'a new src re-arms the view'); + assertEq(img.getAttribute('hidden'), null, 'the img is shown again'); + }), +); + +defineCase('12. destroy() leaves no listeners behind, and is idempotent', () => + withFakeDocument(() => { + const { view, dom } = mount(imageNode('![a](./diagram.png)')); + const live = dom.listenerCount(); + assert(live > 0, 'expected load/error listeners while alive'); + view.destroy?.(); + assertEq(dom.listenerCount(), 0, 'destroy() left nothing behind'); + view.destroy?.(); + assertEq(dom.listenerCount(), 0, 'destroy() is idempotent'); + return `${live} listeners while alive, 0 after destroy`; + }), +); + +defineCase('13. no stopEvent and no ignoreMutation', () => + withFakeDocument(() => { + // The image node is draggable and an inline leaf: swallowing events would + // kill click-to-NodeSelection and with it keyboard deletion. A NodeView + // with no contentDOM already ignores its own mutations by default. + const { view } = mount(imageNode('![a](./diagram.png)')); + assertEq(view.stopEvent, undefined, 'stopEvent must stay unset'); + assertEq(view.ignoreMutation, undefined, 'ignoreMutation must stay unset'); + assertEq(view.contentDOM, undefined, 'an image is a leaf'); + }), +); + +defineCase('14. imageFileName reads the last segment of any src shape', () => { + assertEq(imageFileName('./sub/a%20b.png'), 'a b.png', 'decoded'); + assertEq(imageFileName('diagram.png'), 'diagram.png', 'bare'); + assertEq(imageFileName('https://example.com/a/b.png'), 'b.png', 'remote'); + assertEq(imageFileName('./a%zz.png'), 'a%zz.png', 'undecodable falls back to raw'); + assertEq(imageFileName(''), '', 'empty'); + assertEq(imageFileName('/'), '/', 'nothing to name'); +}); + +defineCase('15. an image with no alt text leaves the attribute off, as the stock spec does', () => + withFakeDocument(() => { + // prosemirror-markdown declares `alt: { default: null }` and DOMSerializer + // drops null attributes, so `![](x.png)` has never emitted an `alt`. + // `alt=""` would say "decorative", which markdown cannot express and this + // form does not mean. + const { img } = mount(imageNode('![](./diagram.png)')); + assertEq(img.getAttribute('alt'), null, 'no alt attribute at all'); + const withAlt = mount(imageNode('![A diagram](./diagram.png)')); + assertEq(withAlt.img.getAttribute('alt'), 'A diagram', 'still set when there is one'); + }), +); + +defineCase('16. the placeholder is announced as one image, not three loose runs', () => + withFakeDocument(() => { + const { dom, fallback } = mount(imageNode('![A diagram that moved](./gone.png "Figure 3")')); + assertEq(fallback.getAttribute('role'), 'img', 'the card is an image to assistive tech'); + assertEq( + fallback.getAttribute('aria-label'), + 'Image didn\u2019t load. A diagram that moved. Figure 3. gone.png', + 'one composed announcement', + ); + // The markdown title otherwise dies on the hidden , in the one state + // where that author-supplied context is worth most. + assertEq(fallback.getAttribute('title'), 'Figure 3', 'the title survives the broken state'); + for (const cls of ['md-image-fallback-label', 'md-image-fallback-alt', 'md-image-fallback-name']) { + const part = dom.findByClass(cls)[0]; + assert(part !== undefined, `missing ${cls}`); + assertEq(part.getAttribute('aria-hidden'), 'true', `${cls} must not be double-read`); + } + }), +); + +defineCase('17. an image with no alt still announces what happened', () => + withFakeDocument(() => { + // `![](x.png)` is "the author wrote no alt text", not "decorative" — see + // case 15 — so the card is not hidden from assistive tech either. + const { fallback } = mount(imageNode('![](./gone.png)')); + assertEq(fallback.getAttribute('aria-hidden'), null, 'not hidden from the a11y tree'); + assertEq( + fallback.getAttribute('aria-label'), + 'Image didn\u2019t load. gone.png', + 'label and filename, no empty run between them', + ); + }), +); + +defineCase('18. update() re-composes the announcement', () => + withFakeDocument(() => { + const node = imageNode('![before](./gone.png)'); + const { view, fallback } = mount(node); + view.update?.(imageNode('![after](./also-gone.png "T")'), [], NO_INNER_DECORATIONS); + assertEq( + fallback.getAttribute('aria-label'), + 'Image didn\u2019t load. after. T. also-gone.png', + 'recomposed from the new node', + ); + }), +); + +// --------------------------------------------------------------------------- +// 3. The wiring that keeps a NodeView bound to the RIGHT directory +// --------------------------------------------------------------------------- + +const libDir = path.dirname(path.dirname(fileURLToPath(import.meta.url))); +const read = (rel: string): string => fs.readFileSync(path.join(libDir, rel), 'utf8'); + +defineCase("19. Editor's nodeView reactor tracks resolveAssetUrl", () => { + // buildNodeViews() hands back fresh closures on every call, so ProseMirror's + // identity comparison redraws as soon as the effect re-runs — but the effect + // only re-runs for props it actually reads. Without this the images keep the + // previous file's base directory and fail silently. + const editor = read('Editor.svelte'); + const reactor = /React to injected plugins\/nodeViews[\s\S]*?\n \}\);/.exec(editor); + assert(reactor !== null, 'could not find the nodeView reactor effect in Editor.svelte'); + assert( + /void resolveAssetUrl;/.test(reactor[0]), + 'the nodeView reactor must touch `resolveAssetUrl` so switching tabs rebuilds the image views', + ); + assert( + /\.\.\.\(resolveAssetUrl \? \{ image: \(node: PmNode\) => imageNodeView\(node, resolveAssetUrl\) \} : \{\}\),/.test( + editor, + ), + 'buildNodeViews() must register the image NodeView ONLY when a resolver was supplied', + ); +}); + +defineCase('20. App passes a resolver whose identity actually changes', () => { + // The trap this pins: `$derived((src) => resolveImageSrc(activePath, src))` + // reads nothing while the derived evaluates, so it never recomputes and the + // prop keeps one identity for the life of the editor. Reading the path + // eagerly inside `$derived.by` is what makes the dependency real. + const app = read('../App.svelte'); + const derived = /let resolveActiveAssetUrl = \$derived\.by\(\(\) => \{([\s\S]*?)\}\);/.exec(app); + assert(derived !== null, 'App.svelte must build the resolver with $derived.by'); + assert( + /const docPath = activePath;/.test(derived[1]), + 'the active path must be read while the derived evaluates, not only inside the closure', + ); +}); + +defineCase('21. the reviewer snapshot editor is left unresolved', () => { + // That editor renders the OWNER's document; its relative srcs name files on + // the owner's disk. Resolving them would mint a convincing attn:// URL for a + // file that is not on this machine — worse than the authored src. + const app = read('../App.svelte'); + const bound = app.match(/resolveAssetUrl=\{resolveActiveAssetUrl\}/g) ?? []; + assertEq(bound.length, 2, 'exactly the local markdown editor and the editor-only diagnostic'); + // Anchored on `` after the branch opener: the + // non-greedy form stopped at `` 436 characters in, so the + // fragment it checked could never have contained the prop and the case + // passed on a technicality. + const snapshotEditor = /\{:else if isReviewerViewingSnapshot\}[\s\S]*?/.exec(app); + assert(snapshotEditor !== null, 'could not find the reviewer-snapshot editor'); + assert(/ run()); + for (const result of results) { + console.log( + `${result.ok ? 'PASS' : 'FAIL'} ${result.name}${result.detail ? ` — ${result.detail}` : ''}`, + ); + } + const failed = results.filter((r) => !r.ok); + console.log(`\n${results.length - failed.length}/${results.length} image NodeView cases passed.`); + if (failed.length > 0) process.exit(1); +} + +runAllCases(); diff --git a/web/src/lib/prosemirror/image-nodeview.ts b/web/src/lib/prosemirror/image-nodeview.ts new file mode 100644 index 00000000..0f100c54 --- /dev/null +++ b/web/src/lib/prosemirror/image-nodeview.ts @@ -0,0 +1,206 @@ +// Image NodeView — resolves the DISPLAYED src, never the stored one (attn-cgev). +// +// prosemirror-markdown's stock `image` spec is `toDOM: (node) => ['img', +// node.attrs]`, which puts the authored markdown src straight into ``. +// That is correct for `https:`/`data:` and wrong for everything else: the +// native document is served from attn://app, so `./diagram.png` resolves +// against the app origin instead of the markdown FILE's directory. This view +// exists to break that identity — the DOM gets a resolved URL, the node keeps +// the authored string. +// +// The distinction is load-bearing. `image` has no serializer override in +// schema.ts, so prosemirror-markdown writes `node.attrs.src` verbatim on every +// save (`serializeAccepted`). There is no interception point between the attr +// and the file, which means the ONLY way `![](./x.png)` survives a round-trip +// is for nothing to ever write `attrs.src`. Nothing here does. +// +// Note this is the `![](x.svg)` path, not the embedded-SVG path: a REFERENCED +// svg is an ordinary image node loaded by the browser as an image document +// (scripts inert), so `svg-sanitizer.ts` is not — and does not need to be — +// involved. Raw `` blocks written inline in the markdown are a different +// node entirely (`embedded_svg`). +// +// The `` is wrapped in a span so the failure state has somewhere to live: +// an asset that would not load renders alt text and a filename in the +// document's own voice instead of the platform's broken-image glyph. The +// wrapper is `display: block; width: fit-content` in CSS, which keeps the +// healthy case laying out as it did before the wrapper existed AND keeps the +// wrapper hugging the picture — prosemirror puts `ProseMirror-selectednode` +// and `draggable` on the NodeView's own element rather than the inner one, so +// a full-measure wrapper would draw the selection ring across the whole +// reading column and make blank paper to the right of a thumbnail a drag +// handle for it. +// +// This view is registered ONLY when a resolver is supplied (Editor.svelte's +// buildNodeViews). Callers with no local file behind the document — the hosted +// app, the reviewer viewing an owner's snapshot — keep the stock `toDOM`, so +// their DOM is byte-identical to what it was before this file existed. + +import type { Node as PmNode } from 'prosemirror-model'; +import type { NodeView } from 'prosemirror-view'; + +/** Last path segment of an authored src, decoded for display. Used only in the + * placeholder — a reader recognises `diagram.png`, not the whole URL. */ +export function imageFileName(src: string): string { + const trimmed = src.replace(/\/+$/, ''); + const segment = trimmed.slice(trimmed.lastIndexOf('/') + 1); + if (!segment) return src; + try { + return decodeURIComponent(segment); + } catch { + return segment; + } +} + +/** + * NodeView for the `image` node. + * + * @param node the image node + * @param resolveAssetUrl maps an authored src onto something the webview can + * load, or `null` when it cannot be resolved. Optional + * so the view stays testable without one; in the app + * the view is not registered at all when there is no + * resolver. + */ +export function imageNodeView( + node: PmNode, + resolveAssetUrl?: (src: string) => string | null, +): NodeView { + const dom = document.createElement('span'); + dom.className = 'md-image'; + + const img = document.createElement('img'); + + // Built once and only ever re-texted, so `update()` never has to remove + // children — and so the placeholder occupies no space until it is needed. + const fallback = document.createElement('span'); + fallback.className = 'md-image-fallback'; + const fallbackLabel = document.createElement('span'); + fallbackLabel.className = 'md-image-fallback-label'; + // What the app actually observed, not a diagnosis it cannot make. `error` + // fires for a file that is missing, for one served with a MIME the webview + // will not decode as an image (`mime_from_extension` in src/main.rs is + // narrower than `files::detect_file_type`), and for a malformed SVG — the + // file is very much found in the last two. + fallbackLabel.textContent = 'Image didn’t load'; + const fallbackAlt = document.createElement('span'); + fallbackAlt.className = 'md-image-fallback-alt'; + const fallbackName = document.createElement('span'); + fallbackName.className = 'md-image-fallback-name'; + // The card is one thing to a screen reader, not three loose text runs. The + // eyebrow/serif/mono hierarchy carries the three roles visually; read aloud + // it would be an undifferentiated string with nothing marking it as standing + // in for an image, so the pieces are hidden and the wrapper carries a single + // composed label (built in `render`, where alt and title are known). + fallback.setAttribute('role', 'img'); + fallbackLabel.setAttribute('aria-hidden', 'true'); + fallbackAlt.setAttribute('aria-hidden', 'true'); + fallbackName.setAttribute('aria-hidden', 'true'); + fallback.appendChild(fallbackLabel); + fallback.appendChild(fallbackAlt); + fallback.appendChild(fallbackName); + + dom.appendChild(img); + dom.appendChild(fallback); + + const onLoad = (): void => { + dom.removeAttribute('data-broken'); + dom.setAttribute('data-loaded', 'true'); + img.removeAttribute('hidden'); + }; + + const onError = (): void => { + dom.removeAttribute('data-loaded'); + dom.setAttribute('data-broken', 'true'); + // Hidden rather than removed, so a src change can put the same element + // (and its listeners) straight back to work. Note this is NOT a retry + // path: an `update()` carrying the SAME src returns early below without + // touching `img.src`, so a file that reappears on disk stays behind the + // placeholder until the view is rebuilt. That is deliberate — re-arming + // on every update would re-request a known-missing asset on every + // transaction that redraws the node. + img.setAttribute('hidden', ''); + }; + + img.addEventListener('load', onLoad); + img.addEventListener('error', onError); + + // The last src actually written to the DOM. Re-writing an identical src is + // not a no-op worth making: the browser may not re-fire `load` for an + // unchanged, already-decoded image, so clearing the state flags on every + // update would strand a healthy image with neither flag set. + let renderedSrc: string | null = null; + + function render(current: PmNode): void { + const src = typeof current.attrs.src === 'string' ? current.attrs.src : ''; + const alt = typeof current.attrs.alt === 'string' ? current.attrs.alt : ''; + const title = typeof current.attrs.title === 'string' ? current.attrs.title : ''; + + const resolved = resolveAssetUrl ? resolveAssetUrl(src) : null; + // A src the resolver declines stays as authored: it may still be a URL the + // webview understands, and a wrong-but-plausible attn:// URL would be a + // worse answer than the authored one. + const display = resolved ?? src; + + // Set only when there is one, matching the `title` handling below and the + // stock spec: prosemirror-markdown declares `alt: { default: null }` and + // DOMSerializer drops null attributes, so `![](x.png)` has always produced + // an `` with no `alt` at all. Emitting `alt=""` instead would declare + // the image decorative — but markdown has no way to SAY decorative, and + // `![](x.png)` overwhelmingly means "the author wrote no alt text", not + // "skip this". Leaving the attribute off keeps assistive tech announcing + // the filename, which is the more useful of the two readings. + if (alt) img.setAttribute('alt', alt); + else img.removeAttribute('alt'); + if (title) img.setAttribute('title', title); + else img.removeAttribute('title'); + + // The authored src, kept queryable for automation and for anyone reading + // the DOM to check what the resolver was given. + dom.setAttribute('data-src', src); + + fallbackAlt.textContent = alt; + if (alt) fallbackAlt.removeAttribute('hidden'); + else fallbackAlt.setAttribute('hidden', ''); + fallbackName.textContent = imageFileName(src); + // `title` is otherwise lost in the broken state — it lives on the hidden + // , and the broken state is where that author-supplied context is + // most worth having. It joins the announcement and the card's own tooltip. + if (title) fallback.setAttribute('title', title); + else fallback.removeAttribute('title'); + fallback.setAttribute( + 'aria-label', + [fallbackLabel.textContent, alt, title, fallbackName.textContent] + .filter(Boolean) + .join('. '), + ); + + if (display === renderedSrc) return; + renderedSrc = display; + // A fresh src re-arms both handlers; until one fires, the view is neither + // loaded nor broken. + dom.removeAttribute('data-loaded'); + dom.removeAttribute('data-broken'); + img.removeAttribute('hidden'); + img.setAttribute('src', display); + } + + render(node); + + // No `stopEvent` and no `ignoreMutation` on purpose. The image node is + // `draggable` and an inline leaf, so swallowing events would kill + // click-to-NodeSelection and with it keyboard deletion; and a NodeView with + // no `contentDOM` already ignores its own DOM mutations by default. + return { + dom, + update(updatedNode: PmNode): boolean { + if (updatedNode.type !== node.type) return false; + render(updatedNode); + return true; + }, + destroy(): void { + img.removeEventListener('load', onLoad); + img.removeEventListener('error', onError); + }, + }; +} diff --git a/web/styles/prosemirror.css b/web/styles/prosemirror.css index 00683e7c..490a068e 100644 --- a/web/styles/prosemirror.css +++ b/web/styles/prosemirror.css @@ -849,6 +849,106 @@ word-break: break-word; } +/* Images (attn-cgev). The NodeView wraps every image in a span so the missing + state has somewhere to render; `display: block` is what Tailwind preflight + already makes the itself, and the wrapper carries no border, padding + or height, so the image's own `margin: 1rem 0` from base.css collapses + straight through it. The healthy case therefore lays out exactly as it did + before the wrapper existed. + + `width: fit-content` is not cosmetic. prosemirror-view hangs both + `ProseMirror-selectednode` and `draggable` on the NodeView's OWN element, + which is now this wrapper rather than the it used to be; a wrapper + stretched to the measure would draw the selection ring across the entire + reading column around a 96px thumbnail, and make the blank paper beside it + a drag handle. `fit-content` does not establish a BFC, so the margin still + collapses through, and `max-width` keeps an oversized raster inside the + column so `.attn-doc img { max-width: 100% }` still has something to clamp + against. */ +.ProseMirror .md-image { + display: block; + width: fit-content; + max-width: 100%; +} + +/* Match the ring to whichever box is actually showing — the image keeps + base.css's 6px, the placeholder card takes the 8px bordered-box step — so + the outline's corners don't disagree with the thing it encloses. */ +.ProseMirror .md-image.ProseMirror-selectednode { + border-radius: 6px; +} + +.ProseMirror .md-image[data-broken].ProseMirror-selectednode { + border-radius: 8px; +} + +/* The placeholder is off until the reports failure — a src that has not + resolved yet is not an error. */ +.ProseMirror .md-image-fallback { + display: none; +} + +.ProseMirror .md-image[data-broken] .md-image-fallback { + display: flex; + flex-direction: column; + gap: 0.25rem; + align-items: flex-start; + /* Sized to its own contents, not the measure: a missing image is a slot in + the page, and a full-bleed banner would read as louder than the image it + stands in for. */ + width: fit-content; + max-width: 100%; + /* Same slot a working image would have occupied: base.css gives content + images `margin: 1rem 0`. The radius is NOT that image's 6px — this box + draws a full border, which puts it under the "nothing that draws a box is + square" ruling in DESIGN.md, and the ladder's step for a small bordered + card is 8px. */ + margin: 1rem 0; + padding: 0.75rem 0.875rem; + border: 1px dashed var(--border); + border-radius: 8px; + /* The recessed embedded surface shared by pre/code/tables/frontmatter — an + asset that would not load is that same class of object: a slot in the page + where content should be. */ + background: var(--code-block); + font-family: var(--sans); + color: var(--muted-foreground); + /* The card is chrome sitting inside the reading column, so it leaves the + document's leading behind — inherited, `--doc-leading` (1.58–1.9 by + preset) would set an 11px eyebrow in a ~19px line box and leave the card + visibly looser than the frontmatter card it is modelled on. */ + line-height: 1.4; +} + +/* Muted ink, not --destructive: an asset that would not load is not an error + the reader caused, and this matches the .mermaid-error/.math-error register + above. Typography is the Label preset, identical to `.frontmatter-label` — + the other uppercase micro eyebrow in this file and this card's nearest + sibling in the reading column. */ +.ProseMirror .md-image-fallback-label { + font: 600 0.7rem/1.2 var(--sans); + letter-spacing: 0.06em; + text-transform: uppercase; +} + +/* The alt line is DOCUMENT content, so it owes the document-scoped tokens: the + typeset-owned `--doc-font` (Modern re-points it to sans, Terminal to mono) + and `em`, which resolves against `.attn-doc`'s own scale. A hard `--serif` + at a `rem` size would be the only Source Serif text on a Terminal page, set + 11% larger than the prose around it. */ +.ProseMirror .md-image-fallback-alt { + font-family: var(--doc-font, var(--serif)); + font-size: 0.9375em; + color: var(--foreground); +} + +.ProseMirror .md-image-fallback-name { + font-family: var(--mono); + font-size: 0.6875rem; + line-height: 1.2; + word-break: break-all; +} + } /* Frontmatter card focus ring — deliberately unlayered, like the scrollbar From 92e89e27d190ec31c2dfcacacd0cfef04a7a15e0 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:15:24 -0500 Subject: [PATCH 2/8] The unresolvable remote image says it is unresolvable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The absolute-URL fixture points at https://example.com/pixel.png, which 404s. That is deliberate — the E2E assertion is that the exact string survives the resolver untouched, and anchoring it on a live host would fail offline and in CI — but the surrounding prose read as a promise that the image would render, so the placeholder card looked like the bug the fixture exists to disprove. Say what the card means, and where to look instead. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn --- tests/fixtures/images.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/fixtures/images.md b/tests/fixtures/images.md index 81ad8c58..4ef9a7b6 100644 --- a/tests/fixtures/images.md +++ b/tests/fixtures/images.md @@ -29,6 +29,12 @@ blocks written inline in the markdown source. Anything with a scheme passes through untouched, so remote images keep working. +The src below is deliberately unresolvable: the E2E suite asserts the exact +string survives the resolver, and anchoring that on a live host would make the +run fail offline and in CI. **Expect a placeholder card here** — what is being +tested is the `src` attribute, not the pixels. To see a remote image actually +render, swap in any live URL by hand; it will load, because nothing rewrites it. + ![A remote pixel](https://example.com/pixel.png) ## Missing file From dc371bfc8497c917888d441ceccc704516293d24 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:41:36 -0500 Subject: [PATCH 3/8] The hosted app degrades like the native one does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The image NodeView was registered only where a resolver existed, to keep hosted DOM byte-identical. The consequence: hosted fell back to the stock `toDOM` and rendered the platform's broken-image glyph — the exact thing this feature replaces — while native showed the card. Hosted cannot load a relative local asset at all: no filesystem behind `./diagram.png`, and no `https:` in its `img-src`. So the failure there is permanent until workspace-relative assets land, which makes it the failure most worth explaining, not least. Register the view everywhere; surfaces with no resolver render the authored src verbatim, as before, and differ from stock only in what they show when it fails. The reviewer-snapshot editor still gets no resolver — its relative srcs name files on the owner's disk — but now gets the card too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn --- web/src/lib/Editor.svelte | 22 +++++++++++------- .../lib/prosemirror/image-nodeview.test.ts | 23 ++++++++++++------- web/src/lib/prosemirror/image-nodeview.ts | 21 ++++++++++------- 3 files changed, 42 insertions(+), 24 deletions(-) diff --git a/web/src/lib/Editor.svelte b/web/src/lib/Editor.svelte index 7a3e1e40..5eb2cc48 100644 --- a/web/src/lib/Editor.svelte +++ b/web/src/lib/Editor.svelte @@ -494,14 +494,20 @@ const builtIn: Record = { task_list_item: taskListItemNodeView, frontmatter: (node) => frontmatterNodeView(node), - // Registered ONLY where there is something to resolve against. With no - // resolver the view would render the authored src verbatim — the same - // bytes the stock `toDOM` emits — but it would still swap the platform's - // broken-image glyph for our placeholder on every relative src the - // hosted app cannot load yet, which is a visible change on a surface - // that is explicitly out of scope until workspace-relative assets land. - // Leaving the key off keeps that DOM byte-identical. - ...(resolveAssetUrl ? { image: (node: PmNode) => imageNodeView(node, resolveAssetUrl) } : {}), + // Registered on EVERY surface, resolver or not. Where one is supplied + // the view resolves `./x.png` against the document's directory; where + // none is (the hosted app, the reviewer viewing an owner's snapshot) it + // renders the authored src verbatim — the same bytes the stock `toDOM` + // emits — and the src fails exactly as it did before. + // + // What changes for those surfaces is only the FAILURE state, and that is + // the point: hosted has no filesystem behind `./diagram.png` and its CSP + // has no `https:` in `img-src`, so those images cannot load there and + // will not until workspace-relative assets land. Until then the honest + // answer is the same card native shows — alt text and a filename in the + // document's voice — not the platform's broken-image glyph. Shipping the + // graceful state on one of two surfaces is a half-landed feature. + image: (node: PmNode) => imageNodeView(node, resolveAssetUrl), code_block(node, editorView, getPos) { const mermaid = mermaidNodeView(node, editorView, getPos); if (mermaid) return mermaid; diff --git a/web/src/lib/prosemirror/image-nodeview.test.ts b/web/src/lib/prosemirror/image-nodeview.test.ts index 96e59130..9b0aa72d 100644 --- a/web/src/lib/prosemirror/image-nodeview.test.ts +++ b/web/src/lib/prosemirror/image-nodeview.test.ts @@ -134,9 +134,9 @@ defineCase('2. node.attrs.src is never written', () => defineCase('3. with no resolver the authored src is used verbatim', () => withFakeDocument(() => { - // The app never mounts the view without a resolver — Editor.svelte leaves - // the key off entirely (case 19) — but the parameter is optional, so the - // degenerate case is held to doing nothing rather than inventing a URL. + // The hosted app and the reviewer-snapshot editor mount the view with no + // resolver (cases 19 and 21). They get the card on failure but must never + // get a URL nobody asked for, so "no resolver" has to mean "do nothing". const { img } = mount(imageNode('![a](./diagram.png)'), 'none'); assertEq(img.getAttribute('src'), './diagram.png', 'unchanged'); }), @@ -356,11 +356,16 @@ defineCase("19. Editor's nodeView reactor tracks resolveAssetUrl", () => { /void resolveAssetUrl;/.test(reactor[0]), 'the nodeView reactor must touch `resolveAssetUrl` so switching tabs rebuilds the image views', ); + // Unconditional: a surface with no resolver still gets the placeholder card + // rather than the platform's broken-image glyph. A conditional here would + // silently strand the hosted app on the stock `toDOM`. assert( - /\.\.\.\(resolveAssetUrl \? \{ image: \(node: PmNode\) => imageNodeView\(node, resolveAssetUrl\) \} : \{\}\),/.test( - editor, - ), - 'buildNodeViews() must register the image NodeView ONLY when a resolver was supplied', + /^\s*image: \(node: PmNode\) => imageNodeView\(node, resolveAssetUrl\),$/m.test(editor), + 'buildNodeViews() must register the image NodeView on every surface', + ); + assert( + !/resolveAssetUrl \? \{ image:/.test(editor), + 'the image NodeView must not be registered conditionally', ); }); @@ -381,7 +386,9 @@ defineCase('20. App passes a resolver whose identity actually changes', () => { defineCase('21. the reviewer snapshot editor is left unresolved', () => { // That editor renders the OWNER's document; its relative srcs name files on // the owner's disk. Resolving them would mint a convincing attn:// URL for a - // file that is not on this machine — worse than the authored src. + // file that is not on this machine — worse than the authored src. It still + // MOUNTS the view (registration is unconditional), so an unloadable src + // there gets the card; what it must not have is a resolver. const app = read('../App.svelte'); const bound = app.match(/resolveAssetUrl=\{resolveActiveAssetUrl\}/g) ?? []; assertEq(bound.length, 2, 'exactly the local markdown editor and the editor-only diagnostic'); diff --git a/web/src/lib/prosemirror/image-nodeview.ts b/web/src/lib/prosemirror/image-nodeview.ts index 0f100c54..22fb3324 100644 --- a/web/src/lib/prosemirror/image-nodeview.ts +++ b/web/src/lib/prosemirror/image-nodeview.ts @@ -31,10 +31,15 @@ // reading column and make blank paper to the right of a thumbnail a drag // handle for it. // -// This view is registered ONLY when a resolver is supplied (Editor.svelte's -// buildNodeViews). Callers with no local file behind the document — the hosted -// app, the reviewer viewing an owner's snapshot — keep the stock `toDOM`, so -// their DOM is byte-identical to what it was before this file existed. +// This view is registered on every surface (Editor.svelte's buildNodeViews). +// Callers with no local file behind the document — the hosted app, the +// reviewer viewing an owner's snapshot — pass no resolver and so render the +// authored src verbatim, which is what the stock `toDOM` did. They differ from +// stock only when that src fails to load, where they get this file's card +// instead of the platform's broken-image glyph. That is deliberate: those +// surfaces genuinely cannot fetch a relative local asset, so the failure is +// permanent until workspace-relative assets land, and a permanent failure is +// exactly the one worth explaining to the reader. import type { Node as PmNode } from 'prosemirror-model'; import type { NodeView } from 'prosemirror-view'; @@ -57,10 +62,10 @@ export function imageFileName(src: string): string { * * @param node the image node * @param resolveAssetUrl maps an authored src onto something the webview can - * load, or `null` when it cannot be resolved. Optional - * so the view stays testable without one; in the app - * the view is not registered at all when there is no - * resolver. + * load, or `null` when it cannot be resolved. Omitted + * by surfaces with no local document behind them, where + * it means "resolve nothing": the authored src is used + * as-is and, when it fails, the placeholder explains. */ export function imageNodeView( node: PmNode, From 13a4aee376efb43b2a156f7f65c3f21750688761 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:16:23 -0500 Subject: [PATCH 4/8] Collab failures say so instead of looking like success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the local collab harness reported success it had not earned. A stray relay on 8787 exposed all three at once, and between them the user saw only "the reviewer window never opens the shared doc". start_relay polled /health for a 200, which ANY process on the port answers. wrangler survives its own EADDRINUSE, so `kill -0 $RELAY_PID` stayed true while the runtime that failed to bind sat there dead — the harness ran both daemons against a stranger's relay and its unrelated Durable Object state. Refuse to start on a held port, naming the process holding it, and treat "Address already in use" in the log as fatal. `attn review join` printed "join request sent to the running attn daemon" and exited 0 once the socket WRITE succeeded; the daemon parsed the invite afterwards, so every later failure was invisible. Add ReviewJoinWait, which replies only after the join has run, and ReviewManager::join_blocking, which does the same work as the Join command arm and additionally returns the outcome. The CLI now names the room it joined, or exits non-zero saying why it could not. dev-collab offered exactly one invite paste and fell through to "Ctrl+C to stop" on failure, so a bad first attempt meant restarting everything. Loop until a join succeeds or the user submits an empty line. Fixes attn-1kvp, attn-q8gs, attn-0cnt. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q6b8P61pFqkaCSmM83FkWn --- scripts/dev-collab.sh | 106 +++++++++++++++++++++--------- src/cli_review.rs | 51 +++++++++------ src/daemon.rs | 145 ++++++++++++++++++++++++++++++++++++++++++ src/review/manager.rs | 33 ++++++++++ 4 files changed, 286 insertions(+), 49 deletions(-) diff --git a/scripts/dev-collab.sh b/scripts/dev-collab.sh index 67793d16..748eb3a6 100755 --- a/scripts/dev-collab.sh +++ b/scripts/dev-collab.sh @@ -86,6 +86,14 @@ require_fixture() { fi } +# The port `ATTN_RELAY_URL` points at. Everything below needs it as a number, +# and defaulting to 8787 keeps a URL without an explicit port working. +relay_port() { + local port + port=$(printf '%s' "$ATTN_RELAY_URL" | sed -E 's#^[a-z]+://[^/:]+:?([0-9]*).*#\1#') + printf '%s' "${port:-8787}" +} + # Start Miniflare via the relay package. Installs deps on first run. # Waits for /health to return 200 before returning. start_relay() { @@ -94,7 +102,26 @@ start_relay() { (cd "$PROJECT_DIR/relay" && npm ci) >/dev/null fi - log "Starting Miniflare relay (wrangler dev --local --port 8787)" + # Refuse to start on a port someone else already holds (attn-1kvp). The + # health poll below cannot tell our relay from a stranger's — a foreign + # process answers /health identically — and wrangler SURVIVES its own + # EADDRINUSE, so `kill -0 $RELAY_PID` stays true while the runtime that + # failed to bind sits there dead. The harness then runs both daemons + # against a relay holding unrelated Durable Object state, and the user + # sees a reviewer that never opens the shared doc, a 409 on device + # registration, and a missing .secret — three symptoms that point + # nowhere near a port clash. Fail here, where the cause is still legible. + local port + port=$(relay_port) + local holder + holder=$(lsof -ti "tcp:$port" -sTCP:LISTEN 2>/dev/null | head -1 || true) + if [ -n "$holder" ]; then + err "port $port is already in use by PID $holder ($(ps -p "$holder" -o comm= 2>/dev/null || echo 'unknown'))" + err "another relay or dev server is running. Stop it first: kill $holder" + return 1 + fi + + log "Starting Miniflare relay (wrangler dev --local --port $port)" ( cd "$PROJECT_DIR/relay" exec npm run dev @@ -110,6 +137,14 @@ start_relay() { tail -30 "$RELAY_LOG" >&2 || true return 1 fi + # wrangler keeps running after a failed bind, so the liveness check + # above never fires for the one failure that matters. The log is the + # only place it is stated. + if grep -q "Address already in use" "$RELAY_LOG" 2>/dev/null; then + err "relay could not bind port $port — another process took it" + tail -10 "$RELAY_LOG" >&2 || true + return 1 + fi if curl -fsS "$ATTN_RELAY_URL/health" >/dev/null 2>&1; then log "Relay listening on $ATTN_RELAY_URL (health OK)" return 0 @@ -152,37 +187,48 @@ join_reviewer() { log "Click [Share] in the OWNER window — the one showing '$FIXTURE_PATH'." log "(The reviewer window shows '$REVIEWER_FIXTURE_PATH' until it joins.) Copy the invite, then paste it here." log "(Empty line cancels and leaves the daemons running.)" - printf 'Paste invite > ' - IFS= read -r pasted || true - if [ -z "$pasted" ]; then - log "No invite supplied — daemons remain up. Ctrl+C to stop." - return 0 - fi - # The share dialog offers two copyable things: the bare attn:// URL and - # the full `npx attnmd review join 'attn://…'` one-liner. Accept either - # (plus stray quotes/whitespace) by extracting the invite URL from - # whatever was pasted — passing the npx command through verbatim used to - # send garbage to the daemon while this script still claimed success. - invite=$(printf '%s' "$pasted" | grep -oE "attn://review/[^'\"[:space:]]+" | head -1) - if [ -z "$invite" ]; then - err "no attn://review/… invite found in the pasted text — copy either the Direct link or the npx command from the Share dialog" - log "Daemons remain up. Ctrl+C to stop." - return 0 - fi + # Loop until a join succeeds or the user cancels (attn-0cnt). A single + # prompt meant any first-attempt failure — a truncated paste, a relay + # hiccup, an invite the daemon declined — could only be retried by tearing + # the whole harness down and starting over, since by then the script had + # already fallen through to "Ctrl+C to stop". The daemons are still up and + # the owner can share again, so the only thing missing was somewhere to + # put the next invite. + while true; do + printf 'Paste invite > ' + IFS= read -r pasted || true + if [ -z "$pasted" ]; then + log "No invite supplied — daemons remain up. Ctrl+C to stop." + return 0 + fi - log "Reviewer joining (windowed daemon)..." - # Route the join to the already-running reviewer DAEMON via its ATTN_HOME - # socket — deliberately NOT `--as-agent`, which forks a separate *headless* - # agent process (no window, no UI) and leaves the reviewer window idle. - # The daemon-routed join makes the reviewer's own window switch to the - # shared document, which is the experience a human reviewer expects. - if ATTN_HOME="$ATTN_DUAL_REVIEWER" ATTN_RELAY_URL="$ATTN_RELAY_URL" \ - "$ATTN_BIN" review join "$invite"; then - log "Reviewer joined — both windows are now collaborating." - else - err "reviewer join failed — see daemon logs under $ATTN_DUAL_REVIEWER/" - fi + # The share dialog offers two copyable things: the bare attn:// URL and + # the full `npx attnmd review join 'attn://…'` one-liner. Accept either + # (plus stray quotes/whitespace) by extracting the invite URL from + # whatever was pasted — passing the npx command through verbatim used to + # send garbage to the daemon while this script still claimed success. + invite=$(printf '%s' "$pasted" | grep -oE "attn://review/[^'\"[:space:]]+" | head -1) + if [ -z "$invite" ]; then + err "no attn://review/… invite found in the pasted text — copy either the Direct link or the npx command from the Share dialog" + continue + fi + + log "Reviewer joining (windowed daemon)..." + # Route the join to the already-running reviewer DAEMON via its ATTN_HOME + # socket — deliberately NOT `--as-agent`, which forks a separate *headless* + # agent process (no window, no UI) and leaves the reviewer window idle. + # The daemon-routed join makes the reviewer's own window switch to the + # shared document, which is the experience a human reviewer expects. + if ATTN_HOME="$ATTN_DUAL_REVIEWER" ATTN_RELAY_URL="$ATTN_RELAY_URL" \ + "$ATTN_BIN" review join "$invite"; then + log "Reviewer joined — both windows are now collaborating." + return 0 + fi + + err "reviewer join failed — see $ATTN_DUAL_REVIEWER/attn.log" + err "share again from the owner window and paste the new invite, or press Enter to give up." + done } # Guard against double-fire: SIGINT + EXIT would otherwise both invoke diff --git a/src/cli_review.rs b/src/cli_review.rs index e30952f3..dc6b7098 100644 --- a/src/cli_review.rs +++ b/src/cli_review.rs @@ -491,6 +491,12 @@ fn validate_invite_for_join(invite: &str) -> Result<()> { Ok(()) } +/// How long the CLI waits for a join to complete before giving up on the +/// answer. Generous enough for a cold relay handshake, short enough that a +/// wedged network does not pin a terminal open. The daemon keeps trying past +/// this point — only the CLI stops waiting. +const JOIN_WAIT_TIMEOUT: Duration = Duration::from_secs(30); + /// Hand the invite to the running attn daemon so it joins as its OWN device /// identity (the same device the app window presents). This keeps the CLI join /// consistent with the daemon — a no-`--as-agent` join shows up in the app. @@ -501,31 +507,38 @@ fn validate_invite_for_join(invite: &str) -> Result<()> { fn run_join_via_daemon(invite: &str) -> Result<()> { validate_invite_for_join(invite)?; crate::daemon::replace_stale_daemon().context("check running attn daemon")?; - match crate::daemon::send_review_join(invite) { - Ok(()) => { - println!("join request sent to the running attn daemon"); - println!(" invite: {invite}"); - return Ok(()); + // Wait for the join to actually run rather than for the socket write to + // succeed (attn-q8gs). The old fire-and-forget path printed "join request + // sent" and exited 0 for joins the daemon went on to reject, which left + // the reviewer window on its old document with nothing in the terminal + // suggesting anything was wrong — the failure was only ever visible in + // the daemon's own log. + match crate::daemon::send_review_join_wait(invite, Some(JOIN_WAIT_TIMEOUT)) { + Ok(room_id) => { + println!("joined review room {room_id}"); + Ok(()) } Err(_err) if crate::daemon::send_info().is_err() => { + // No daemon at all — that error is "no daemon running", not a + // failed join. Start one on the current directory and retry, which + // keeps the invite one-liner useful for a first-time reviewer. start_app_for_join()?; wait_for_daemon(Duration::from_secs(8))?; - crate::daemon::send_review_join(invite).map_err(|join_err| { - anyhow::anyhow!( - "started attn, but could not send the review invite ({join_err}).\n\ - Invite: {invite}" - ) - })?; - } - Err(err) => { - return Err(anyhow::anyhow!( - "could not send the review invite to the running attn daemon ({err})." - )); + let room_id = crate::daemon::send_review_join_wait(invite, Some(JOIN_WAIT_TIMEOUT)) + .map_err(|join_err| { + anyhow::anyhow!( + "started attn, but the review join failed ({join_err}).\n\ + Invite: {invite}" + ) + })?; + println!("joined review room {room_id}"); + Ok(()) } + Err(err) => Err(anyhow::anyhow!( + "review join failed ({err}).\n\ + Invite: {invite}" + )), } - println!("join request sent to the running attn daemon"); - println!(" invite: {invite}"); - Ok(()) } fn start_app_for_join() -> Result<()> { diff --git a/src/daemon.rs b/src/daemon.rs index dfe6a14f..eb089c37 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -90,6 +90,20 @@ pub enum SocketMessage { #[serde(rename = "review_join")] ReviewJoin { invite: String }, + /// Join a review room and WAIT for the outcome (attn-q8gs). + /// + /// `ReviewJoin` above is fire-and-forget: the daemon Oks the write and + /// joins afterwards, so a CLI caller cannot tell a completed join from + /// one the daemon rejected. This variant blocks the socket reply until + /// the join has actually run, so `attn review join` can exit non-zero + /// when it failed. + #[serde(rename = "review_join_wait", rename_all = "camelCase")] + ReviewJoinWait { + invite: String, + #[serde(default)] + timeout_ms: Option, + }, + /// Share the current path as a review room. CLI agents call this to /// host a review without going through the UI. Real handling lives in /// `ReviewManager` (issue attn-nnj.2.8); for now the daemon logs and @@ -199,6 +213,9 @@ pub enum SocketResponse { }, #[serde(rename = "review_verdicts_wait")] ReviewVerdictsWait { outcome: VerdictWaitOutcome }, + /// Outcome of a `ReviewJoinWait` — the room actually joined. + #[serde(rename_all = "camelCase")] + ReviewJoinWait { room_id: String }, #[serde(rename = "review_suggestions_submitted", rename_all = "camelCase")] ReviewSuggestionsSubmitted { submitted_count: usize }, #[serde(rename = "durable_shares", rename_all = "camelCase")] @@ -481,6 +498,26 @@ pub fn send_review_join(invite: &str) -> Result<()> { } } +/// Send a `ReviewJoinWait` and report the room actually joined (attn-q8gs). +/// +/// The waiting counterpart to `send_review_join`: the daemon replies only once +/// the join has run, so an `Err` here means the join genuinely failed rather +/// than merely that the socket write did. +pub fn send_review_join_wait(invite: &str, timeout: Option) -> Result { + let timeout_ms = + timeout.map(|duration| u64::try_from(duration.as_millis()).unwrap_or(u64::MAX)); + let msg = SocketMessage::ReviewJoinWait { + invite: invite.to_string(), + timeout_ms, + }; + match send_command(&msg)? { + Some(SocketResponse::ReviewJoinWait { room_id }) => Ok(room_id), + Some(SocketResponse::Error { message }) => bail!("{message}"), + Some(other) => bail!("unexpected response: {other:?}"), + None => bail!("no daemon running"), + } +} + /// Hand a path to the running daemon to share for review, as ITS OWN device /// identity (the app window then shows the invite). `path` may be a single file /// or a directory — a directory publishes a snapshot per `*.md` under it @@ -796,6 +833,44 @@ fn query_review_verdicts( } } +/// Run a join to completion and report what happened (attn-q8gs). +/// +/// Unlike `wait_review_verdicts` there is no polling to do: the manager's join +/// is synchronous once driven, so the whole wait is the call itself. The +/// timeout exists so a wedged network cannot pin a CLI caller open forever; +/// it is enforced by running the join on a worker thread and giving up on the +/// RESULT, not on the join — the daemon keeps going and the window still +/// updates if it lands late. +fn wait_review_join( + review_manager: Option<&Arc>, + invite: String, + timeout_ms: Option, +) -> SocketResponse { + let Some(manager) = review_manager else { + return SocketResponse::Error { + message: "ReviewManager unavailable".to_string(), + }; + }; + let manager = Arc::clone(manager); + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(manager.join_blocking(invite)); + }); + let timeout = Duration::from_millis(timeout_ms.unwrap_or(30_000)); + match rx.recv_timeout(timeout) { + Ok(Ok(room_id)) => SocketResponse::ReviewJoinWait { room_id }, + Ok(Err(message)) => SocketResponse::Error { + message: format!("join failed: {message}"), + }, + Err(_) => SocketResponse::Error { + message: format!( + "join did not complete within {}s — the daemon is still trying; see its log", + timeout.as_secs() + ), + }, + } +} + fn wait_review_verdicts( review_manager: Option<&Arc>, participant_id: &crate::review::ids::ParticipantId, @@ -1112,6 +1187,15 @@ fn handle_client( serde_json::to_string(&resp).unwrap_or_default() ); } + Ok(SocketMessage::ReviewJoinWait { invite, timeout_ms }) => { + log_review_join_intent(&invite); + let resp = wait_review_join(review_manager, invite, timeout_ms); + let _ = writeln!( + stream, + "{}", + serde_json::to_string(&resp).unwrap_or_default() + ); + } // Review socket commands routed through `ReviewManager` (issue // attn-nnj.2.8). The manager logs each command, emits a stub // `ReviewUpdate` back into the event loop, and we Ok the caller. @@ -1593,6 +1677,67 @@ mod tests { // Pass = no panic. } + #[test] + fn review_join_wait_round_trips_over_the_socket_protocol() { + // The whole point of attn-q8gs is that the CLI learns the outcome, so + // the request must carry the timeout and the response must carry the + // room — a silently-dropped field here would put us back to a caller + // that cannot tell a join from a write. + let json = serde_json::to_string(&SocketMessage::ReviewJoinWait { + invite: "attn://review/abc#v=3".to_string(), + timeout_ms: Some(30_000), + }) + .expect("serialize join-wait"); + assert_eq!( + json, + r#"{"type":"review_join_wait","invite":"attn://review/abc#v=3","timeoutMs":30000}"# + ); + let decoded: SocketMessage = serde_json::from_str(&json).expect("deserialize join-wait"); + match decoded { + SocketMessage::ReviewJoinWait { invite, timeout_ms } => { + assert_eq!(invite, "attn://review/abc#v=3"); + assert_eq!(timeout_ms, Some(30_000)); + } + other => panic!("unexpected message: {other:?}"), + } + + // Omitted timeout is legal — the daemon supplies the default. + let decoded: SocketMessage = + serde_json::from_str(r#"{"type":"review_join_wait","invite":"attn://review/abc"}"#) + .expect("deserialize without timeout"); + match decoded { + SocketMessage::ReviewJoinWait { timeout_ms, .. } => assert_eq!(timeout_ms, None), + other => panic!("unexpected message: {other:?}"), + } + } + + #[test] + fn review_join_wait_response_carries_the_room() { + let json = serde_json::to_string(&SocketResponse::ReviewJoinWait { + room_id: "room-1".to_string(), + }) + .expect("serialize response"); + assert!( + json.contains("\"roomId\":\"room-1\""), + "response must name the joined room, got {json}" + ); + } + + #[test] + fn wait_review_join_without_a_manager_is_an_error_not_a_success() { + // A daemon that failed to open its review store hands `None` here. + // Reporting Ok would recreate exactly the bug this fixes. + match wait_review_join(None, "attn://review/abc".to_string(), Some(10)) { + SocketResponse::Error { message } => { + assert!( + message.contains("ReviewManager unavailable"), + "unexpected message: {message}" + ); + } + other => panic!("expected an error, got {other:?}"), + } + } + #[test] fn from_diff_daemon_batch_serialization() { let json = serde_json::to_string(&SocketMessage::ReviewSubmitSuggestions { diff --git a/src/review/manager.rs b/src/review/manager.rs index bbf2c0d0..bd1578b3 100644 --- a/src/review/manager.rs +++ b/src/review/manager.rs @@ -2353,6 +2353,39 @@ impl ReviewManager { /// Translate a `JoinOutcome` (or its error) into the corresponding /// `ReviewUpdate` and dispatch it. + /// Join a room and REPORT whether it worked (attn-q8gs). + /// + /// `ReviewCommand::Join` is fire-and-forget: `submit` hands it to the + /// dispatch loop, the caller is told nothing, and a failure surfaces only + /// as a `ReviewUpdate::Error` in the window and a line in the daemon log. + /// That is right for the in-app path — the window is watching — and wrong + /// for `attn review join`, whose caller is a terminal that exits before + /// any update arrives and so reported success for joins that never + /// happened. + /// + /// This runs the SAME work as the `Join` arm of `submit` — same + /// bootstrapper call, same `emit_join_outcome`, so the window still + /// updates exactly as it would have — and additionally returns the + /// outcome to whoever asked. Returns the joined `RoomId` on success, or a + /// human-readable reason on failure. + pub fn join_blocking(&self, invite: String) -> Result { + let (Some(bootstrap), Some(runtime)) = (self.bootstrap.as_ref(), self.runtime.as_ref()) + else { + return Err("review bootstrapper unavailable (daemon started without one)".to_string()); + }; + let cache = self.verifying_keys.clone(); + let result = runtime.block_on(bootstrap.join(&invite, cache)); + // Summarise BEFORE handing ownership to the emitter, which consumes + // the result. The emit must still happen: it starts the room runtime + // and flips the window onto the shared document. + let summary = match &result { + Ok(outcome) => Ok(outcome.room_id.as_str().to_string()), + Err(err) => Err(format!("{err}")), + }; + self.emit_join_outcome(result); + summary + } + fn emit_join_outcome( &self, result: Result, From e8a3e591ac300a8a54df53c1d3b11347e9f56064 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 18:06:09 -0500 Subject: [PATCH 5/8] Decide which referenced images a share may carry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First half of attn-udu8. A reviewer sees the placeholder card for every image because share_lifecycle.rs:632 lets a share carry only md/markdown/html/htm, so the bytes never leave the owner's machine. Before any of them can, something has to decide which are safe to send. This module is that gate, and is written as one rather than as a path helper. The document being scanned is often agent-authored — nobody read every line — so `![](../../../.ssh/id_rsa.png)` is a realistic thing to find in it. Every src is refused unless it is provably a plain, allowlisted image file inside the directory the user chose to share. Deliberately stricter than resolveImageSrc in markdown-layer.ts, which resolves srcs for DISPLAY on the machine that already holds the files: showing someone a file they can already open costs nothing, transmitting it is irreversible. So a filesystem-absolute src is displayed but never sent, and a src escaping the share root is displayed but never sent. Three details carry the weight. Symlinks are refused BEFORE canonicalising, because canonicalize() follows the link and would report the target as an ordinary file inside the root. An encoded %2F is split as a separator, because it becomes one before the file is opened. And srcs come from comrak, not a regex, so a path inside a code fence never causes a file read. Skips are recorded with a reason rather than dropped — an image silently missing from a review is the thing this feature exists to stop. --- src/review/assets.rs | 527 +++++++++++++++++++++++++++++++++++++++++++ src/review/mod.rs | 1 + 2 files changed, 528 insertions(+) create mode 100644 src/review/assets.rs diff --git a/src/review/assets.rs b/src/review/assets.rs new file mode 100644 index 00000000..b57ad5dc --- /dev/null +++ b/src/review/assets.rs @@ -0,0 +1,527 @@ +//! Which images a shared document references, and which of them may be sent. +//! +//! A markdown image src is written relative to the FILE. A reviewer has the +//! document's text and nothing else, so every relative src fails on their +//! machine and renders the placeholder card from `image-nodeview.ts`. To show +//! them the picture the owner has to publish the bytes as `DocType::Asset` +//! snapshots (attn-udu8). +//! +//! That makes this module a gate on outbound file reads, and it is written as +//! one. The document being scanned is frequently agent-authored — nobody read +//! every line of it — and a single `![](../../../.ssh/id_rsa)` would otherwise +//! turn "share my notes" into an exfiltration primitive. Every src is refused +//! unless it is provably a plain, allowlisted image file inside the directory +//! the user chose to share. +//! +//! The rules are deliberately STRICTER than `resolveImageSrc` in +//! markdown-layer.ts, which resolves srcs for DISPLAY on the machine that +//! already has the files. Displaying a file the user can open anyway costs +//! nothing; transmitting it is irreversible. Two rules in particular diverge: +//! a filesystem-absolute src is displayed but never sent, and a src that +//! escapes the share root is displayed but never sent. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +/// Extensions whose bytes may be published. An allowlist rather than a +/// denylist: an unknown extension is a file we have no reason to send. +/// `svg` is included and is the one entry that carries script — it is safe +/// HERE because a referenced svg is loaded as an image document (scripts +/// inert), the same reasoning `image-nodeview.ts` records for the display +/// side. It must never be reused for a src that could become a frame. +const IMAGE_EXTENSIONS: &[(&str, &str)] = &[ + ("png", "image/png"), + ("jpg", "image/jpeg"), + ("jpeg", "image/jpeg"), + ("gif", "image/gif"), + ("webp", "image/webp"), + ("bmp", "image/bmp"), + ("ico", "image/x-icon"), + ("svg", "image/svg+xml"), +]; + +/// Largest single asset that may be published. Generous for a screenshot or a +/// diagram, small enough that one pathological file cannot dominate a share. +pub const MAX_ASSET_BYTES: u64 = 8 * 1024 * 1024; + +/// Ceiling on everything one document contributes. A snapshot is base64url'd +/// into an encrypted envelope, so the wire cost is ~4/3 of this. +pub const MAX_TOTAL_BYTES: u64 = 32 * 1024 * 1024; + +/// Ceiling on how many assets one document contributes, so a generated file +/// with a thousand thumbnails cannot stall a share. +pub const MAX_ASSET_COUNT: usize = 64; + +/// An image the owner will publish alongside the document. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReferencedImage { + /// The src exactly as authored, e.g. `./chart.svg`. This is the key the + /// reviewer looks up: it is what their copy of the document contains. + pub authored_src: String, + /// Absolute path to the file on the owner's disk. + pub path: PathBuf, + pub media_type: String, + pub bytes: u64, +} + +/// Why a referenced src will not be sent. Carried so the Share dialog can say +/// what it is leaving out — an image silently missing from a review is the +/// thing this whole feature exists to stop. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkipReason { + /// `https:`, `data:`, protocol-relative — already loadable, or not a file. + NotLocal, + /// Resolved outside the shared directory. Includes every filesystem- + /// absolute src: `/Users/me/secret.png` is outside any share root that + /// does not contain it, and treating it otherwise would make the + /// confinement depend on how the src happened to be spelled. + EscapesShareRoot, + /// A symlink, or reached through one. Refused rather than resolved: the + /// link's target is chosen by whoever wrote the file, not by the user. + Symlink, + /// Not a regular file — a directory, device, socket, fifo. + NotARegularFile, + NotAnImage, + Missing, + TooLarge, + /// The per-document count or byte budget was already spent. + BudgetExhausted, +} + +impl SkipReason { + /// One short phrase, for the Share dialog and the daemon log. + pub fn describe(self) -> &'static str { + match self { + SkipReason::NotLocal => "not a local file", + SkipReason::EscapesShareRoot => "outside the shared folder", + SkipReason::Symlink => "symbolic link", + SkipReason::NotARegularFile => "not a regular file", + SkipReason::NotAnImage => "not a supported image type", + SkipReason::Missing => "file not found", + SkipReason::TooLarge => "larger than the 8 MB limit", + SkipReason::BudgetExhausted => "share image budget reached", + } + } +} + +/// A src that will not be sent, and why. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkippedImage { + pub authored_src: String, + pub reason: SkipReason, +} + +/// What one document contributes to a share. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct ImageScan { + pub included: Vec, + pub skipped: Vec, +} + +impl ImageScan { + pub fn total_bytes(&self) -> u64 { + self.included.iter().map(|image| image.bytes).sum() + } + + pub fn is_empty(&self) -> bool { + self.included.is_empty() + } +} + +/// Collect the image srcs a markdown document references, in source order, +/// deduplicated by authored spelling. +/// +/// Uses comrak rather than a regex so the set matches what actually renders: +/// a src inside a fenced code block or an inline-code span is not an image and +/// must not cause a file read. +fn image_srcs(markdown: &str) -> Vec { + use comrak::nodes::NodeValue; + use comrak::{Arena, Options, parse_document}; + + let arena = Arena::new(); + let mut options = Options::default(); + options.extension.strikethrough = true; + options.extension.table = true; + options.extension.tasklist = true; + options.extension.autolink = true; + options.extension.footnotes = true; + let root = parse_document(&arena, markdown, &options); + + let mut seen: HashSet = HashSet::new(); + let mut srcs = Vec::new(); + for node in root.descendants() { + if let NodeValue::Image(link) = &node.data.borrow().value { + let src = link.url.clone(); + if !src.is_empty() && seen.insert(src.clone()) { + srcs.push(src); + } + } + } + srcs +} + +/// True when the src addresses something other than a path on this disk. +fn is_non_local(src: &str) -> bool { + if src.starts_with("//") || src.starts_with('#') { + return true; + } + // `scheme:` — the prefix before the first colon must be a well-formed + // scheme of 2+ characters, so a bare Windows drive letter (`C:/…`) is not + // read as one. Windows is not a supported host; such a src falls through + // to the confinement check like any other path. + let Some(colon) = src.find(':') else { + return false; + }; + let scheme = &src[..colon]; + scheme.len() >= 2 + && scheme.starts_with(|c: char| c.is_ascii_alphabetic()) + && scheme + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '+' || c == '-' || c == '.') +} + +/// Percent-decode one path segment, falling back to the raw bytes on a +/// malformed escape. Mirrors `decodeSegment` in markdown-layer.ts: the src has +/// already been through a markdown parser's URL normalisation, so `café.png` +/// arrives as `caf%C3%A9.png` and must be decoded to name the real file. +fn decode_segment(segment: &str) -> String { + percent_encoding::percent_decode_str(segment) + .decode_utf8() + .map(|decoded| decoded.into_owned()) + .unwrap_or_else(|_| segment.to_string()) +} + +/// Resolve an authored src to a lexical path, without touching the disk. +/// +/// Returns `None` for a src that cannot name a file at all. Splitting on `/` +/// AFTER decoding matters for the same reason it does in markdown-layer.ts: an +/// encoded `%2F` becomes a real separator, so it has to be treated as one here +/// or the traversal check below would reason about a different path than the +/// one that gets opened. +fn resolve_lexically(doc_dir: &Path, src: &str) -> Option { + let segments: Vec = src + .split('/') + .flat_map(|piece| { + decode_segment(piece) + .split('/') + .map(str::to_string) + .collect::>() + }) + .collect(); + let last = segments.last()?; + if last.is_empty() || last == "." || last == ".." { + return None; + } + + let mut path = if src.starts_with('/') { + PathBuf::from("/") + } else { + doc_dir.to_path_buf() + }; + for segment in &segments { + if segment.is_empty() || segment == "." { + continue; + } + if segment == ".." { + // Pop lexically. The result is checked against the share root + // afterwards, so a src that climbs out is caught there. + path.pop(); + continue; + } + path.push(segment); + } + Some(path) +} + +fn media_type_for(path: &Path) -> Option<&'static str> { + let extension = path.extension()?.to_str()?.to_ascii_lowercase(); + IMAGE_EXTENSIONS + .iter() + .find(|(ext, _)| *ext == extension) + .map(|(_, media)| *media) +} + +/// Decide which images referenced by `doc_path` may be published. +/// +/// `share_root` is the directory the user chose to share — a single shared +/// file's own parent, or the shared folder. Nothing outside it is ever sent, +/// whatever the document asks for. +pub fn scan_document_images(markdown: &str, doc_path: &Path, share_root: &Path) -> ImageScan { + let mut scan = ImageScan::default(); + let Some(doc_dir) = doc_path.parent() else { + return scan; + }; + // Canonicalise the root once so the comparison below is between two real + // paths — a share root reached through a symlinked parent would otherwise + // never prefix-match its own contents. + let Ok(root) = std::fs::canonicalize(share_root) else { + return scan; + }; + + let mut spent_bytes: u64 = 0; + for src in image_srcs(markdown) { + let skip = |reason: SkipReason, scan: &mut ImageScan| { + scan.skipped.push(SkippedImage { + authored_src: src.clone(), + reason, + }); + }; + + if is_non_local(&src) { + skip(SkipReason::NotLocal, &mut scan); + continue; + } + let Some(lexical) = resolve_lexically(doc_dir, &src) else { + skip(SkipReason::Missing, &mut scan); + continue; + }; + if media_type_for(&lexical).is_none() { + skip(SkipReason::NotAnImage, &mut scan); + continue; + } + // Refuse symlinks BEFORE canonicalising: canonicalize() would follow + // the link and report the target as an ordinary file inside the root, + // which is exactly the check being evaded. + match std::fs::symlink_metadata(&lexical) { + Ok(meta) if meta.file_type().is_symlink() => { + skip(SkipReason::Symlink, &mut scan); + continue; + } + Ok(meta) if !meta.is_file() => { + skip(SkipReason::NotARegularFile, &mut scan); + continue; + } + Ok(_) => {} + Err(_) => { + skip(SkipReason::Missing, &mut scan); + continue; + } + } + let Ok(canonical) = std::fs::canonicalize(&lexical) else { + skip(SkipReason::Missing, &mut scan); + continue; + }; + if !canonical.starts_with(&root) { + skip(SkipReason::EscapesShareRoot, &mut scan); + continue; + } + let Ok(meta) = std::fs::metadata(&canonical) else { + skip(SkipReason::Missing, &mut scan); + continue; + }; + if meta.len() > MAX_ASSET_BYTES { + skip(SkipReason::TooLarge, &mut scan); + continue; + } + if scan.included.len() >= MAX_ASSET_COUNT + || spent_bytes.saturating_add(meta.len()) > MAX_TOTAL_BYTES + { + skip(SkipReason::BudgetExhausted, &mut scan); + continue; + } + let media_type = media_type_for(&canonical).unwrap_or("application/octet-stream"); + spent_bytes = spent_bytes.saturating_add(meta.len()); + scan.included.push(ReferencedImage { + authored_src: src.clone(), + path: canonical, + media_type: media_type.to_string(), + bytes: meta.len(), + }); + } + scan +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use tempfile::TempDir; + + fn write(dir: &Path, rel: &str, bytes: &[u8]) -> PathBuf { + let path = dir.join(rel); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).expect("mkdir"); + } + fs::write(&path, bytes).expect("write"); + path + } + + #[test] + fn collects_relative_srcs_in_source_order_without_duplicates() { + let srcs = image_srcs("![a](./one.png)\n\n![b](two.png)\n\n![c](./one.png)\n"); + assert_eq!(srcs, vec!["./one.png", "two.png"]); + } + + #[test] + fn a_src_inside_code_is_not_an_image() { + // The whole reason for parsing rather than regexing: this must not + // cause a file read, let alone a publish. + let srcs = image_srcs("```\n![a](./secret.png)\n```\n\nand `![b](./also.png)`\n"); + assert!(srcs.is_empty(), "got {srcs:?}"); + } + + #[test] + fn remote_and_data_srcs_are_skipped_as_not_local() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + let markdown = "![a](https://example.com/x.png)\n\n![b](data:image/png;base64,AA)\n\n![c](//cdn/x.png)\n"; + let scan = scan_document_images(markdown, &doc, tmp.path()); + assert!(scan.included.is_empty()); + assert_eq!(scan.skipped.len(), 3); + assert!( + scan.skipped + .iter() + .all(|s| s.reason == SkipReason::NotLocal) + ); + } + + #[test] + fn a_sibling_image_is_included_with_its_media_type() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + write(tmp.path(), "chart.svg", b""); + let scan = scan_document_images("![a](./chart.svg)", &doc, tmp.path()); + assert_eq!(scan.included.len(), 1, "skipped: {:?}", scan.skipped); + assert_eq!(scan.included[0].authored_src, "./chart.svg"); + assert_eq!(scan.included[0].media_type, "image/svg+xml"); + assert_eq!(scan.included[0].bytes, 6); + } + + #[test] + fn a_subdirectory_image_is_included() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + write(tmp.path(), "nested/diagram.png", b"\x89PNG"); + let scan = scan_document_images("![a](./nested/diagram.png)", &doc, tmp.path()); + assert_eq!(scan.included.len(), 1, "skipped: {:?}", scan.skipped); + assert_eq!(scan.included[0].media_type, "image/png"); + } + + #[test] + fn traversal_out_of_the_share_root_is_refused() { + // The case this module exists for. + let tmp = TempDir::new().expect("tmp"); + let root = tmp.path().join("shared"); + fs::create_dir_all(&root).expect("mkdir"); + let doc = write(&root, "doc.md", b""); + write(tmp.path(), "outside/secret.png", b"\x89PNG"); + let scan = scan_document_images("![a](../outside/secret.png)", &doc, &root); + assert!(scan.included.is_empty(), "leaked: {:?}", scan.included); + assert_eq!(scan.skipped[0].reason, SkipReason::EscapesShareRoot); + } + + #[test] + fn an_absolute_src_is_refused_even_when_the_file_exists() { + let tmp = TempDir::new().expect("tmp"); + let root = tmp.path().join("shared"); + fs::create_dir_all(&root).expect("mkdir"); + let doc = write(&root, "doc.md", b""); + let outside = write(tmp.path(), "outside/secret.png", b"\x89PNG"); + let markdown = format!("![a]({})", outside.display()); + let scan = scan_document_images(&markdown, &doc, &root); + assert!(scan.included.is_empty(), "leaked: {:?}", scan.included); + assert_eq!(scan.skipped[0].reason, SkipReason::EscapesShareRoot); + } + + #[test] + fn an_encoded_separator_cannot_smuggle_a_traversal() { + // `%2F` decodes to a separator before the path is opened, so it has to + // be one while the traversal is being judged. + let tmp = TempDir::new().expect("tmp"); + let root = tmp.path().join("shared"); + fs::create_dir_all(&root).expect("mkdir"); + let doc = write(&root, "doc.md", b""); + write(tmp.path(), "outside/secret.png", b"\x89PNG"); + let scan = scan_document_images("![a](..%2Foutside%2Fsecret.png)", &doc, &root); + assert!(scan.included.is_empty(), "leaked: {:?}", scan.included); + assert_eq!(scan.skipped[0].reason, SkipReason::EscapesShareRoot); + } + + #[cfg(unix)] + #[test] + fn a_symlink_inside_the_root_is_refused() { + // Canonicalising first would report the TARGET as an ordinary file + // inside the root and publish someone's private key as a png. + let tmp = TempDir::new().expect("tmp"); + let root = tmp.path().join("shared"); + fs::create_dir_all(&root).expect("mkdir"); + let doc = write(&root, "doc.md", b""); + let secret = write(tmp.path(), "outside/secret.png", b"\x89PNG"); + std::os::unix::fs::symlink(&secret, root.join("innocent.png")).expect("symlink"); + let scan = scan_document_images("![a](./innocent.png)", &doc, &root); + assert!(scan.included.is_empty(), "leaked: {:?}", scan.included); + assert_eq!(scan.skipped[0].reason, SkipReason::Symlink); + } + + #[test] + fn a_non_image_extension_is_refused() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + write(tmp.path(), "id_rsa", b"PRIVATE KEY"); + let scan = scan_document_images("![a](./id_rsa)", &doc, tmp.path()); + assert!(scan.included.is_empty()); + assert_eq!(scan.skipped[0].reason, SkipReason::NotAnImage); + } + + #[test] + fn an_oversize_image_is_refused() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + write( + tmp.path(), + "huge.png", + &vec![0u8; (MAX_ASSET_BYTES + 1) as usize], + ); + let scan = scan_document_images("![a](./huge.png)", &doc, tmp.path()); + assert!(scan.included.is_empty()); + assert_eq!(scan.skipped[0].reason, SkipReason::TooLarge); + } + + #[test] + fn a_missing_file_is_reported_not_published() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + let scan = scan_document_images("![a](./gone.png)", &doc, tmp.path()); + assert!(scan.included.is_empty()); + assert_eq!(scan.skipped[0].reason, SkipReason::Missing); + } + + #[test] + fn a_directory_named_like_an_image_is_refused() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + fs::create_dir_all(tmp.path().join("trap.png")).expect("mkdir"); + let scan = scan_document_images("![a](./trap.png)", &doc, tmp.path()); + assert!(scan.included.is_empty()); + assert_eq!(scan.skipped[0].reason, SkipReason::NotARegularFile); + } + + #[test] + fn the_count_budget_bounds_one_document() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + let mut markdown = String::new(); + for i in 0..(MAX_ASSET_COUNT + 5) { + write(tmp.path(), &format!("img{i}.png"), b"\x89PNG"); + markdown.push_str(&format!("![a](./img{i}.png)\n\n")); + } + let scan = scan_document_images(&markdown, &doc, tmp.path()); + assert_eq!(scan.included.len(), MAX_ASSET_COUNT); + assert_eq!(scan.skipped.len(), 5); + assert!( + scan.skipped + .iter() + .all(|s| s.reason == SkipReason::BudgetExhausted) + ); + } + + #[test] + fn a_percent_encoded_name_resolves_to_the_real_file() { + let tmp = TempDir::new().expect("tmp"); + let doc = write(tmp.path(), "doc.md", b""); + write(tmp.path(), "my shot.png", b"\x89PNG"); + let scan = scan_document_images("![a](./my%20shot.png)", &doc, tmp.path()); + assert_eq!(scan.included.len(), 1, "skipped: {:?}", scan.skipped); + assert_eq!(scan.included[0].authored_src, "./my%20shot.png"); + } +} diff --git a/src/review/mod.rs b/src/review/mod.rs index c6028f66..43733ab4 100644 --- a/src/review/mod.rs +++ b/src/review/mod.rs @@ -10,6 +10,7 @@ pub mod agent; pub mod agent_identity; pub mod anchors; +pub mod assets; pub mod apply; pub mod bootstrap; pub mod compression; From 44609fad3ac1132fa755cb9311d6a71d366deea4 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 18:09:36 -0500 Subject: [PATCH 6/8] A share carries the images its documents reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second half of attn-udu8's owner side. Every markdown target is scanned before the selection is recorded — a wire path is only minted for a path in selected_paths, so an asset outside it publishes fine and then has no name the reviewer can look it up by — and each surviving image is published as a DocType::Asset snapshot down the same envelope path as a document. The relay still sees only ciphertext. The share root is what the user actually chose: a folder share confines to the folder, a single-file share to that file's own directory. An image that fails to publish is logged and skipped rather than added to publish_errors. A share fails when a DOCUMENT cannot be sent; losing a picture must not cost the reviewer the review. Skipped srcs are logged with the reason from review::assets, so "why is this image missing" is answerable from the daemon log rather than by guessing. --- src/review/bootstrap.rs | 171 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 170 insertions(+), 1 deletion(-) diff --git a/src/review/bootstrap.rs b/src/review/bootstrap.rs index a99d2b6d..d1dc3d12 100644 --- a/src/review/bootstrap.rs +++ b/src/review/bootstrap.rs @@ -1947,10 +1947,41 @@ impl Bootstrapper { grant_signature: None, }, )?; + // Images the selected documents reference travel WITH them (attn-udu8). + // Scanned before the selection is recorded, because a wire path is only + // minted for a path in `selected_paths` — an asset outside it publishes + // fine and then has no name the reviewer can look it up by. + // + // The share root is what the user chose: a folder share confines to the + // folder, a single-file share to that file's own directory. Nothing + // outside it is ever read — see `review::assets`. + let share_root: PathBuf = if is_dir { + path.clone() + } else { + path.parent() + .map(std::path::Path::to_path_buf) + .unwrap_or_else(|| path.clone()) + }; + let image_scan = scan_share_images(&doc_targets, &share_root); + let asset_targets: Vec = image_scan + .included + .iter() + .map(|img| img.path.clone()) + .collect(); + for skipped in &image_scan.skipped { + tracing::info!( + "share: not sending image {} ({})", + skipped.authored_src, + skipped.reason.describe() + ); + } + if !has_explicit_selection { record_local_share(self.store.root(), &room_id, &path, is_dir)?; } else { - record_local_share_selection(self.store.root(), &room_id, &path, &doc_targets)?; + let mut selection = doc_targets.clone(); + selection.extend(asset_targets.iter().cloned()); + record_local_share_selection(self.store.root(), &room_id, &path, &selection)?; } // 6b. Publish the initial snapshot(s) so reviewers get the doc bytes the @@ -1994,6 +2025,39 @@ impl Bootstrapper { } } } + // Assets after the documents: a failure to publish one costs the + // reviewer a picture, never the review. `publish_errors` is + // deliberately NOT extended — an unreadable image must not fail a + // share the way an unreadable document does. + for image in &image_scan.included { + match self + .publish_asset_snapshot(&room_id, &image.path, &image.media_type, now_ms) + .await + { + Ok((file_id, snapshot_id)) => { + if has_explicit_selection { + match manifest_entry_for_snapshot( + &self.store, + &room_id, + &image.path, + &file_id, + &snapshot_id, + ) { + Ok(entry) => manifest_entries.push(entry), + Err(err) => tracing::warn!( + "share: image {} published but has no manifest entry: {err}", + image.path.display() + ), + } + } + } + Err(err) => tracing::warn!( + "share: could not publish image {}: {err}", + image.path.display() + ), + } + } + if published == 0 || (has_explicit_selection && !publish_errors.is_empty()) { return Err(BootstrapError::InvalidShare(format!( "the selected files could not all be published for {}{}", @@ -3170,6 +3234,70 @@ impl Bootstrapper { /// at the blob via `encryptedBlobRef`. Ciphertexts above the relay's /// 1 MiB inline threshold spill to R2 (presign + PUT); at or below, the /// blob envelope rides the normal outbox. + /// Publish one image as a `DocType::Asset` snapshot (attn-udu8). + /// + /// Same envelope path as a document — the relay stays content-blind, + /// seeing only ciphertext — with the bytes base64url'd because a snapshot + /// payload is JSON. The wire path is the asset's path relative to the + /// share root, which is what lets a reviewer find it: they resolve + /// `./chart.svg` against their copy of the document's own wire path and + /// look up the result. + pub async fn publish_asset_snapshot( + &self, + room_id: &RoomId, + path: &std::path::Path, + media_type: &str, + now_ms: u64, + ) -> Result<(FileId, SnapshotId), BootstrapError> { + use crate::review::crypto::ids::{content_hash, derive_file_id, derive_snapshot_id}; + use crate::review::model::{DocType, SnapshotAssetEncoding}; + use base64::Engine as _; + use base64::engine::general_purpose::URL_SAFE_NO_PAD; + + let bytes = std::fs::read(path) + .map_err(|e| BootstrapError::Store(format!("read asset {}: {e}", path.display())))?; + let base_hash = content_hash(&bytes); + let room_secret = load_room_secret(self.store.root(), room_id)?; + let display_path = + selected_share_wire_path(self.store.root(), room_id, path)?.ok_or_else(|| { + BootstrapError::Store(format!( + "asset {} has no portable share path", + path.display() + )) + })?; + let file_id = derive_file_id(&room_secret, &display_path, &base_hash); + let snapshot_id = derive_snapshot_id(room_id, &file_id, &base_hash, now_ms as i64); + let plaintext = SnapshotPlaintext { + doc_type: DocType::Asset, + content: Some(URL_SAFE_NO_PAD.encode(&bytes)), + anchor_index: None, + media_type: Some(media_type.to_string()), + encoding: Some(SnapshotAssetEncoding::Base64url), + manifest: None, + annotation: None, + }; + let published = self + .publish_snapshot_plaintext( + room_id, + file_id.clone(), + snapshot_id, + Some(display_path), + base_hash, + plaintext, + now_ms, + ) + .await?; + record_share_file_id(self.store.root(), room_id, path, &file_id)?; + tracing::info!( + "published asset file={} snapshot={} bytes={} room={}", + published.0.as_str(), + published.1.as_str(), + bytes.len(), + room_id.as_str(), + ); + Ok(published) + } + pub async fn publish_snapshot( &self, room_id: &RoomId, @@ -4457,6 +4585,47 @@ fn record_share_file_id( /// Wire paths for owner-curated shares are normalized relative to the project /// root. Legacy file/folder shares retain their historical absolute display /// path until those links migrate to manifests. +/// Scan every markdown document in a share for the images it references +/// (attn-udu8). +/// +/// Results are unioned across documents and deduplicated by resolved path, so +/// a logo referenced by six files is sent once. HTML targets are skipped: an +/// HTML share already carries its own assets through the workspace entries, +/// and its srcs are not markdown srcs. +fn scan_share_images( + doc_targets: &[PathBuf], + share_root: &std::path::Path, +) -> crate::review::assets::ImageScan { + use crate::review::assets::{ImageScan, scan_document_images}; + use std::collections::HashSet; + + let mut combined = ImageScan::default(); + let mut seen_paths: HashSet = HashSet::new(); + let mut seen_skips: HashSet<(String, usize)> = HashSet::new(); + for doc_path in doc_targets { + if is_html_path(doc_path) { + continue; + } + let Ok(source) = std::fs::read_to_string(doc_path) else { + continue; + }; + let scan = scan_document_images(&source, doc_path, share_root); + for image in scan.included { + if seen_paths.insert(image.path.clone()) { + combined.included.push(image); + } + } + for skipped in scan.skipped { + // Same src skipped for the same reason in two documents is one + // fact, not two. + if seen_skips.insert((skipped.authored_src.clone(), skipped.reason as usize)) { + combined.skipped.push(skipped); + } + } + } + combined +} + fn selected_share_wire_path( store_root: &std::path::Path, room_id: &RoomId, From d2e92e7e8dd0b780f190e5287780b269f9e9d7a6 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 27 Aug 2026 21:00:13 -0500 Subject: [PATCH 7/8] A reviewer can prove the owner authored the manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workspace manifest hydration failed for every v3 reviewer, so no reviewer ever saw a multi-file workspace — or, since attn-udu8, any image. The check that gated it loaded the ROOM SECRET, on the stated belief that "every joined/owned room persists the invite secret before starting its transport". That was never true of a v3 join: the key tree is one-way (room_secret -> root_key -> read_capability_key -> leaves) and a reviewer only ever holds a leaf, so the check could not pass for the one party it was checking. It was also asking the wrong question. The manifest FileId it compared travels inside every manifest snapshot a reviewer receives, so knowing it is not evidence of having authored anything — a secret-derived identifier is a name, not a signature. What needs proving is authorship, so prove it: accept the manifest when the OWNER signed the event carrying it. Deliberately NOT re-deriving that FileId from read_capability_key. It would unbreak hydration in about one line and make forgery strictly easier, since every reviewer holds that key and could then mint a manifest that passes. The owner's public key rides in the invite fragment, which is the one channel that already carries room-opening secrets and, being a fragment, never reaches the relay — the party a pinned key exists to defend against. The joiner verifies the existing grant against it locally rather than trusting the relay's verdict, and pins it in RoomAccessV3. Verification is a full verify_event, not a signingKeyId compare, because replay re-verifies nothing of its own and a check that only held live would evaporate on restart. Both fragment parsers move together. Each re-renders through its own composer and compares, so a disagreement about field order would make each side reject the other's invites; PARITY_VIEW_FRAGMENT / PARITY_COMMENT_FRAGMENT are asserted verbatim on both sides, which nothing previously pinned. Note the skew is asymmetric: old invites still parse, but an OLD client meets `owner=` as an unknown field and refuses, so owner and reviewer builds move together. Rooms with no pinned key — owners, v2 joiners — keep the original check, and fail closed when they have neither. Also lets an asset carry a manifest entry (attn-udu8): assets were refused there only because nothing but a document had ever been a share target, and bytes with no binding are indistinguishable from bytes never sent. Fixes attn-lb7p, attn-lb7p.1, attn-lb7p.2, attn-lb7p.3, attn-yhv8. --- src/review/bootstrap.rs | 195 +++++++++++++++++- src/review/manager.rs | 236 +++++++++++++++++++++- src/review/mod.rs | 2 +- src/review/share_lifecycle.rs | 36 +++- web/src/lib/review/browser-invite.test.ts | 72 +++++++ web/src/lib/review/browser-invite.ts | 37 +++- 6 files changed, 551 insertions(+), 27 deletions(-) diff --git a/src/review/bootstrap.rs b/src/review/bootstrap.rs index d1dc3d12..3d29e2e0 100644 --- a/src/review/bootstrap.rs +++ b/src/review/bootstrap.rs @@ -535,6 +535,20 @@ pub struct ParsedInviteFragmentV3 { pub read_capability_key: [u8; 32], pub write_admission_key: Option<[u8; 32]>, pub grant_signature: Option<[u8; 64]>, + /// The owner's PUBLIC signing key, pinned by the joiner (attn-lb7p). + /// + /// Carried here rather than read from the relay's device directory + /// because the directory is relay-controlled: a relay that could + /// substitute the owner's key could authorise its own manifests. The + /// fragment is the one channel that already carries room-opening secrets + /// and, being a URL fragment, is never transmitted to the relay at all. + /// + /// `Option` so a fragment minted before this field existed still parses — + /// the joiner then has no key to pin and manifest hydration falls back + /// (see `validate_workspace_manifest_binding`). The reverse skew does not + /// degrade: an OLD parser meets `owner=` as an unknown field and refuses + /// the invite outright, so owner and reviewer builds move together. + pub owner_public_signing_key: Option<[u8; 32]>, } /// Build a strict canonical v3 capability fragment, including the leading `#`. @@ -543,15 +557,23 @@ pub fn build_invite_fragment_v3( read_capability_key: &[u8; 32], write_admission_key: Option<&[u8; 32]>, grant_signature: Option<&[u8; 64]>, + owner_public_signing_key: Option<&[u8; 32]>, ) -> Result { let read = URL_SAFE_NO_PAD.encode(read_capability_key); + // Appended LAST in every form. The position is load-bearing: several tests + // and the browser parity assertion pin the LEADING substring through + // `read=`, and the parser below re-renders through this function and + // compares byte-for-byte, so moving a field is a wire break. + let owner = owner_public_signing_key + .map(|key| format!("&owner={}", URL_SAFE_NO_PAD.encode(key))) + .unwrap_or_default(); match (tier, write_admission_key, grant_signature) { - (InviteTierV3::View, None, None) => Ok(format!("#v=3&tier=view&read={read}")), + (InviteTierV3::View, None, None) => Ok(format!("#v=3&tier=view&read={read}{owner}")), (InviteTierV3::View, _, _) => Err(BootstrapError::InviteParse( "view tier must not include write capability or grant".into(), )), (InviteTierV3::Comment | InviteTierV3::Suggest, Some(write), Some(grant)) => Ok(format!( - "#v=3&tier={}&read={read}&write={}&grant={}", + "#v=3&tier={}&read={read}&write={}&grant={}{owner}", tier.as_str(), URL_SAFE_NO_PAD.encode(write), URL_SAFE_NO_PAD.encode(grant), @@ -577,7 +599,7 @@ pub fn parse_invite_fragment_v3(fragment: &str) -> Result Result Result Result, pub grant_tier: Option, pub grant_signature: Option, + /// The owner's public signing key as pinned from the invite (attn-lb7p), + /// base64url-no-pad like `grant_signature`. + /// + /// This is what `validate_workspace_manifest_binding` authenticates a + /// workspace manifest against. It is read off disk rather than taken from + /// the live import, so the check is a full local re-verification that is + /// equally valid on replay — replay re-verifies no signatures of its own. + /// + /// `default` + `skip_serializing_if` are deliberate rather than implicit: + /// `load_room_access_v3` hard-errors on any decode failure, and a decode + /// error there would silently downgrade a reviewer who had already joined. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub owner_public_signing_key: Option, } -fn save_room_access_v3( +pub(crate) fn save_room_access_v3( root: &std::path::Path, room_id: &RoomId, access: &RoomAccessV3, @@ -4757,10 +4825,18 @@ fn manifest_entry_for_snapshot( let plaintext = snapshot .plaintext .ok_or_else(|| BootstrapError::Store("published snapshot plaintext is missing".into()))?; + // An asset gets a manifest entry like any other shared file (attn-udu8). + // It was refused here only because, before referenced images travelled + // with a share, nothing but a document was ever a share target — and + // without an entry the reviewer receives the bytes with no binding, which + // is indistinguishable from not receiving them. + // + // A manifest still cannot be an entry of itself. let kind = match plaintext.doc_type { DocType::Markdown => WorkspaceManifestEntryKind::Markdown, DocType::Html => WorkspaceManifestEntryKind::Html, - DocType::Asset | DocType::WorkspaceManifest => { + DocType::Asset => WorkspaceManifestEntryKind::Asset, + DocType::WorkspaceManifest => { return Err(BootstrapError::InvalidShare(format!( "{} is not a shareable document snapshot", path.display() @@ -4775,7 +4851,17 @@ fn manifest_entry_for_snapshot( snapshot_id: snapshot_id.clone(), path: wire_path, kind, - media_type: None, + // Required for an asset entry and rejected on any other — see + // `validate_manifest_entry`. It comes from the snapshot the owner + // already published, so the manifest cannot disagree with the payload. + media_type: match kind { + WorkspaceManifestEntryKind::Asset => { + Some(plaintext.media_type.clone().ok_or_else(|| { + BootstrapError::Store("asset snapshot has no media type".into()) + })?) + } + _ => None, + }, byte_length: raw.len() as u64, content_hash: snapshot.base_hash, }) @@ -5409,16 +5495,22 @@ mod tests { use crate::review::crypto::kdf::{derive_read_keys_v3, derive_room_key_tree_v3}; let tree = derive_room_key_tree_v3(&[0x7a; 32]); + let owner_public = [0x5c; 32]; let fragment = build_invite_fragment_v3( InviteTierV3::View, tree.read_keys.read_capability_key.as_bytes(), None, None, + Some(&owner_public), ) .expect("build view fragment"); let parsed = parse_invite_fragment_v3(&fragment).expect("parse view fragment"); assert_eq!(parsed.tier, InviteTierV3::View); assert!(parsed.write_admission_key.is_none()); + // A view-tier reviewer needs the workspace as much as a writer, and + // has no grant to identify the owner by, so the key must ride along + // on this tier too (attn-lb7p). + assert_eq!(parsed.owner_public_signing_key, Some(owner_public)); let read_only = derive_read_keys_v3(&parsed.read_capability_key); assert_eq!( read_only.event_key.as_bytes(), @@ -5485,6 +5577,7 @@ mod tests { write_admission_key: Some([2; 32]), grant_tier: Some(crate::review::transport::inbound::GrantTier::Comment), grant_signature: Some(URL_SAFE_NO_PAD.encode([3; 64])), + owner_public_signing_key: Some(URL_SAFE_NO_PAD.encode([4; 32])), }; save_room_access_v3(root.path(), &room_id, &access).expect("save"); assert_eq!( @@ -5541,6 +5634,7 @@ mod tests { write_admission_key: Some(*tree.write_admission_key.as_bytes()), grant_tier: None, grant_signature: None, + owner_public_signing_key: None, }, ) .unwrap(); @@ -5567,6 +5661,7 @@ mod tests { fn v3_fragment_parser_rejects_duplicates_unknown_mismatch_and_bad_lengths() { let read = URL_SAFE_NO_PAD.encode([1u8; 32]); let write = URL_SAFE_NO_PAD.encode([2u8; 32]); + let owner = URL_SAFE_NO_PAD.encode([4u8; 32]); for invalid in [ format!("#v=3&tier=view&read={read}&read={read}"), format!("#v=3&tier=view&read={read}&future=x"), @@ -5574,6 +5669,13 @@ mod tests { format!("#v=3&tier=comment&read={read}"), "#v=3&tier=view&read=AQ".to_string(), format!("#tier=view&v=3&read={read}"), + // attn-lb7p: the owner key is length-disciplined like the others, + // must not repeat, and must come LAST — the parser re-renders + // through the builder and compares, so a fragment that carries the + // right fields in the wrong order is still refused. + format!("#v=3&tier=view&read={read}&owner=AQ"), + format!("#v=3&tier=view&read={read}&owner={owner}&owner={owner}"), + format!("#v=3&tier=view&owner={owner}&read={read}"), ] { assert!( parse_invite_fragment_v3(&invalid).is_err(), @@ -5582,6 +5684,87 @@ mod tests { } } + /// The canonical fragments that BOTH implementations must produce, byte + /// for byte, from the same inputs (attn-lb7p). + /// + /// Rust and TypeScript each re-render through their own composer and + /// compare the result to the input, so a disagreement about field ORDER or + /// base64 spelling is not cosmetic drift — each side would reject the + /// other's invites, and hosted reviewers would simply stop being able to + /// open native invites. Nothing else in the tree pins the two together, so + /// this literal is duplicated verbatim in + /// `web/src/lib/review/browser-invite.test.ts` and must be changed in both + /// places or not at all. + const PARITY_VIEW_FRAGMENT: &str = "#v=3&tier=view&read=AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQE&owner=BAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ"; + const PARITY_COMMENT_FRAGMENT: &str = "#v=3&tier=comment&read=AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQE&write=AgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgI&grant=AwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw&owner=BAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ"; + + #[test] + fn v3_fragment_shape_matches_the_browser_implementation_byte_for_byte() { + let read = [1u8; 32]; + let write = [2u8; 32]; + let grant = [3u8; 64]; + let owner = [4u8; 32]; + assert_eq!( + build_invite_fragment_v3(InviteTierV3::View, &read, None, None, Some(&owner)) + .expect("view fragment"), + PARITY_VIEW_FRAGMENT + ); + assert_eq!( + build_invite_fragment_v3( + InviteTierV3::Comment, + &read, + Some(&write), + Some(&grant), + Some(&owner) + ) + .expect("comment fragment"), + PARITY_COMMENT_FRAGMENT + ); + // And both round-trip back through our own parser. + for fragment in [PARITY_VIEW_FRAGMENT, PARITY_COMMENT_FRAGMENT] { + let parsed = parse_invite_fragment_v3(fragment).expect("parity fragment parses"); + assert_eq!(parsed.owner_public_signing_key, Some(owner)); + } + } + + #[test] + fn v3_fragment_without_an_owner_key_still_parses() { + // Backward skew: an invite minted before attn-lb7p has no owner key. + // It must still open — the joiner simply pins nothing and falls back. + // (The forward skew does NOT degrade: an old parser meets `owner=` as + // an unknown field and refuses, which is why owner and reviewer builds + // have to move together.) + let read = URL_SAFE_NO_PAD.encode([1u8; 32]); + let parsed = parse_invite_fragment_v3(&format!("#v=3&tier=view&read={read}")) + .expect("pre-attn-lb7p fragment must still parse"); + assert!(parsed.owner_public_signing_key.is_none()); + } + + #[test] + fn v3_invite_grant_verifies_against_the_key_the_fragment_pins() { + // The joiner checks the grant against the invite's own key rather than + // one the relay supplied — that substitution is the whole reason to + // pin. A grant minted by a different key must not verify. + use crate::review::crypto::signing::DeviceSigningKey; + let secret = [0x3b; 32]; + let owner = DeviceSigningKey::from_bytes(&[0x11; 32]).expect("owner key"); + let impostor = DeviceSigningKey::from_bytes(&[0x22; 32]).expect("impostor key"); + let room_id = derive_room_id_v3(&secret); + let url = build_invite_url_v3(&room_id, &secret, InviteTierV3::Comment, &owner) + .expect("build comment invite"); + let ParsedInviteAny::V3(parsed) = parse_invite_any(&url).expect("parse invite") else { + panic!("expected a v3 invite"); + }; + let pinned = parsed + .fragment + .owner_public_signing_key + .expect("invite must pin the owner key"); + assert_eq!(pinned, owner.verifying_key().to_bytes()); + verify_invite_grant_v3(&parsed, &pinned).expect("owner grant verifies against pinned key"); + verify_invite_grant_v3(&parsed, &impostor.verifying_key().to_bytes()) + .expect_err("a grant must not verify against an unrelated key"); + } + #[test] fn browser_invite_uses_configured_path_and_fragment_only_secret() { let secret = [0x5Au8; 32]; diff --git a/src/review/manager.rs b/src/review/manager.rs index bd1578b3..61c91d92 100644 --- a/src/review/manager.rs +++ b/src/review/manager.rs @@ -4338,17 +4338,68 @@ fn validate_workspace_manifest_binding( .as_ref() .ok_or_else(|| "workspace manifest payload is missing its manifest".to_string())?; - // Every joined/owned room persists the invite secret before starting its - // transport. It is therefore available at this production hydration - // boundary and lets native receivers reject an ordinary file masquerading - // as the room's one synthetic manifest document. - let room_secret = crate::review::bootstrap::load_room_secret(store.root(), room_id) - .map_err(|err| format!("load room secret for manifest FileId binding: {err}"))?; - let expected_manifest_file_id = derive_workspace_manifest_file_id(&room_secret); - if *manifest_file_id != expected_manifest_file_id { - return Err( - "manifest event fileId is not the room's synthetic manifest FileId".to_string(), - ); + // Who published this manifest? (attn-lb7p) + // + // This check used to ask a different question — does the manifest's FileId + // equal one derived from the room secret — on the stated belief that + // "every joined/owned room persists the invite secret before starting its + // transport". That was never true of a v3 JOIN: the v3 key tree is + // one-way (room_secret -> root_key -> read_capability_key -> leaves) and a + // reviewer only ever holds a leaf, so no v3 reviewer could satisfy it and + // every one of them lost the workspace entirely. + // + // It was also the wrong question. That FileId travels inside every + // manifest snapshot a reviewer receives, so knowing it is not evidence of + // having authored anything — a secret-derived identifier is a name, not a + // signature. What actually needs proving is authorship, so prove it: the + // manifest is accepted when the OWNER signed the event carrying it, + // verified against the key the joiner pinned from the invite. + // + // `verify_event` is a full re-verification, not a comparison against + // whatever the import decided. That matters because replay + // (`replay_room_to_webview`) re-verifies nothing of its own — an identity + // check that only held on the live path would evaporate on restart. + let pinned_owner_key = crate::review::bootstrap::load_room_access_v3(store.root(), room_id) + .map_err(|err| format!("load room access for manifest authorship: {err}"))? + .and_then(|access| access.owner_public_signing_key); + match pinned_owner_key { + Some(encoded) => { + use base64::Engine as _; + let bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(encoded.as_bytes()) + .map_err(|err| format!("pinned owner key base64url decode: {err}"))?; + let bytes: [u8; 32] = bytes + .as_slice() + .try_into() + .map_err(|_| "pinned owner key must decode to 32 bytes".to_string())?; + let owner = crate::review::crypto::signing::DeviceVerifyingKey::from_bytes(&bytes) + .map_err(|err| format!("pinned owner key: {err}"))?; + crate::review::crypto::signing::verify_event( + &owner, + &manifest_event.meta, + &manifest_event.body, + &manifest_event.auth, + ) + .map_err(|err| format!("workspace manifest was not signed by the room owner: {err}"))?; + // The synthetic FileId is deliberately NOT compared here. It is + // the manifest's name — reviewers match snapshots to entries by + // it — and a name a reviewer cannot compute, and could replay if + // it could, is not a credential. + } + None => { + // No pinned key: a v2 room, or a v3 room joined before the invite + // carried one. Fall back to the original check, which still works + // for the parties that can satisfy it (owners, v2 joiners) and + // still fails closed for the v3 reviewers it never worked for. + let room_secret = crate::review::bootstrap::load_room_secret(store.root(), room_id) + .map_err(|err| format!("load room secret for manifest FileId binding: {err}"))?; + let expected_manifest_file_id = derive_workspace_manifest_file_id(&room_secret); + if *manifest_file_id != expected_manifest_file_id { + return Err( + "manifest event fileId is not the room's synthetic manifest FileId".to_string(), + ); + } + } } let mut earlier_snapshots = Vec::new(); @@ -5311,6 +5362,169 @@ mod tests { (tmp, store, room_id, manifest_event) } + // ----------------------------------------------------------------- + // Manifest AUTHORSHIP (attn-lb7p). + // + // Everything above this point exercises the FALLBACK branch: the harness + // writes shares/.secret and pins no owner key, which is exactly the + // shape an owner or a v2 joiner has. That is why the whole suite kept + // passing when the check was replaced — and why these tests exist. They + // are the only coverage of the path a v3 reviewer actually takes. + // ----------------------------------------------------------------- + + /// Re-sign an already-persisted manifest event in place. + /// + /// Safe to do after `append_event`: `validate_workspace_manifest_binding` + /// verifies the signature on the event it is HANDED, and uses the store + /// only to confirm that event_id is present and to find earlier snapshots. + /// meta and body are untouched, so the id still matches the stored copy. + fn sign_manifest_as( + event: &mut crate::review::model::ReviewEvent, + key: &crate::review::crypto::signing::DeviceSigningKey, + ) { + event.auth = crate::review::crypto::signing::sign_event(key, &event.meta, &event.body) + .expect("sign manifest event"); + } + + fn pin_owner_key( + store: &ReviewStore, + room_id: &RoomId, + key: &crate::review::crypto::signing::DeviceSigningKey, + ) { + use base64::Engine as _; + crate::review::bootstrap::save_room_access_v3( + store.root(), + room_id, + &crate::review::bootstrap::RoomAccessV3 { + read_capability_key: [9; 32], + write_admission_key: None, + grant_tier: None, + grant_signature: None, + owner_public_signing_key: Some( + base64::engine::general_purpose::URL_SAFE_NO_PAD + .encode(key.verifying_key().to_bytes()), + ), + }, + ) + .expect("pin owner key"); + } + + fn signing_key(seed: u8) -> crate::review::crypto::signing::DeviceSigningKey { + crate::review::crypto::signing::DeviceSigningKey::from_bytes(&[seed; 32]) + .expect("signing key from seed") + } + + fn hydrated( + store: &ReviewStore, + room_id: &RoomId, + event: &mut crate::review::model::ReviewEvent, + ) -> bool { + use crate::review::model::ReviewEventBody; + rehydrate_snapshot_event(store, room_id, event); + matches!( + &event.body, + ReviewEventBody::SnapshotCreated { + inline_snapshot: Some(_), + .. + } + ) + } + + #[test] + fn rehydrate_manifest_accepts_owner_signed_manifest_without_any_room_secret() { + // The bug this epic exists for. A v3 reviewer never receives the room + // secret, so there is no shares/.secret on their disk — and + // until now that alone rejected every manifest they were sent. + let (_tmp, store, room_id, mut event) = + bound_manifest_case(ManifestBindingTamper::MissingRoomSecret); + let owner = signing_key(0x11); + pin_owner_key(&store, &room_id, &owner); + sign_manifest_as(&mut event, &owner); + assert!( + hydrated(&store, &room_id, &mut event), + "an owner-signed manifest must hydrate with no room secret on disk" + ); + } + + #[test] + fn rehydrate_manifest_rejects_manifest_signed_by_a_reviewer_identity() { + // The forgery the check is FOR. Note the manifest carries the room's + // real synthetic FileId — the old check would have waved this through + // on any machine that could compute it. Authorship is what decides. + let (_tmp, store, room_id, mut event) = bound_manifest_case(ManifestBindingTamper::None); + let owner = signing_key(0x11); + let reviewer = signing_key(0x22); + pin_owner_key(&store, &room_id, &owner); + sign_manifest_as(&mut event, &reviewer); + assert!( + !hydrated(&store, &room_id, &mut event), + "a manifest signed by a non-owner must be rejected even with the correct FileId" + ); + } + + #[test] + fn rehydrate_manifest_rejects_a_tampered_body_under_a_valid_owner_key() { + // Distinguishes a real signature check from a signingKeyId string + // compare: the key id still matches the owner, only the bytes moved. + use crate::review::model::ReviewEventBody; + let (_tmp, store, room_id, mut event) = bound_manifest_case(ManifestBindingTamper::None); + let owner = signing_key(0x11); + pin_owner_key(&store, &room_id, &owner); + sign_manifest_as(&mut event, &owner); + if let ReviewEventBody::SnapshotCreated { + owner_display_path, .. + } = &mut event.body + { + *owner_display_path = Some("workspace.manifest.tampered".to_string()); + } + assert!( + !hydrated(&store, &room_id, &mut event), + "a body edited after signing must not verify" + ); + } + + #[test] + fn rehydrate_manifest_survives_replay() { + // replay_room_to_webview re-verifies nothing of its own, so a check + // satisfiable only from live import state would evaporate on restart. + // Hydrating twice from disk proves this one reads only persisted data. + let (_tmp, store, room_id, event) = + bound_manifest_case(ManifestBindingTamper::MissingRoomSecret); + let owner = signing_key(0x11); + pin_owner_key(&store, &room_id, &owner); + let mut signed = event.clone(); + sign_manifest_as(&mut signed, &owner); + let mut first = signed.clone(); + let mut second = signed; + assert!(hydrated(&store, &room_id, &mut first), "first hydration"); + assert!( + hydrated(&store, &room_id, &mut second), + "replay must hydrate identically" + ); + } + + #[test] + fn rehydrate_manifest_falls_back_to_room_secret_when_no_owner_key_is_pinned() { + // Owners and v2 joiners have no pinned key and DO have the secret. + // Their behavior must be exactly what it was before this change. + let (_tmp, store, room_id, mut event) = bound_manifest_case(ManifestBindingTamper::None); + assert!( + hydrated(&store, &room_id, &mut event), + "the pre-existing room-secret path must still accept" + ); + } + + #[test] + fn rehydrate_manifest_fails_closed_with_neither_a_pinned_key_nor_a_secret() { + // No way to establish authorship at all: refuse, do not fall open. + let (_tmp, store, room_id, mut event) = + bound_manifest_case(ManifestBindingTamper::MissingRoomSecret); + assert!( + !hydrated(&store, &room_id, &mut event), + "with no owner key and no secret the manifest must be refused" + ); + } + #[test] fn rehydrate_manifest_binds_valid_nested_markdown_and_binary_entries() { use crate::review::model::{DocType, ReviewEventBody}; diff --git a/src/review/mod.rs b/src/review/mod.rs index 43733ab4..3990280c 100644 --- a/src/review/mod.rs +++ b/src/review/mod.rs @@ -10,8 +10,8 @@ pub mod agent; pub mod agent_identity; pub mod anchors; -pub mod assets; pub mod apply; +pub mod assets; pub mod bootstrap; pub mod compression; pub mod crypto; diff --git a/src/review/share_lifecycle.rs b/src/review/share_lifecycle.rs index 512557e0..3b7fb122 100644 --- a/src/review/share_lifecycle.rs +++ b/src/review/share_lifecycle.rs @@ -1112,8 +1112,24 @@ pub async fn resolve_public_share_to_room_invite( ShareLinkTier::Comment => InviteTierV3::Comment, ShareLinkTier::Suggest => InviteTierV3::Suggest, }; - let fragment = build_invite_fragment_v3(invite_tier, &read, write.as_ref(), grant.as_ref()) - .map_err(|error| ShareLifecycleError::Invalid(error.to_string()))?; + // The bundle has carried the owner's public key all along (share.rs + // `ShareCapabilityBundle::owner_signing_key`, validated as canonical 32 + // bytes when the bundle is opened); this resolver simply discarded it. + // It has to reach the fragment here too, or a hosted share mints an + // invite whose joiner has no key to pin while a native share's does. + let owner_public: [u8; 32] = URL_SAFE_NO_PAD + .decode(opened.owner_signing_key.as_bytes()) + .map_err(|_| ShareLifecycleError::Invalid("bundle owner key is invalid".into()))? + .try_into() + .map_err(|_| ShareLifecycleError::Invalid("bundle owner key length is invalid".into()))?; + let fragment = build_invite_fragment_v3( + invite_tier, + &read, + write.as_ref(), + grant.as_ref(), + Some(&owner_public), + ) + .map_err(|error| ShareLifecycleError::Invalid(error.to_string()))?; Ok(format!("attn://review/{current_room}{fragment}")) } @@ -3919,9 +3935,19 @@ mod tests { ) .await .expect("resolve comment link"); - let fragment = - build_invite_fragment_v3(InviteTierV3::Comment, &read, Some(&write), Some(&grant)) - .expect("comment fragment"); + // The resolver must carry the bundle's owner key into the fragment + // (attn-lb7p); rebuilding without it here would assert the old shape + // and hide exactly the omission this pins. + // Same key the mock bundle at the top of this test carries. + let owner_public = [0x55_u8; 32]; + let fragment = build_invite_fragment_v3( + InviteTierV3::Comment, + &read, + Some(&write), + Some(&grant), + Some(&owner_public), + ) + .expect("comment fragment"); assert_eq!(invite, format!("attn://review/{room_id}{fragment}")); } diff --git a/web/src/lib/review/browser-invite.test.ts b/web/src/lib/review/browser-invite.test.ts index f9e3fd6d..984ea05a 100644 --- a/web/src/lib/review/browser-invite.test.ts +++ b/web/src/lib/review/browser-invite.test.ts @@ -575,6 +575,78 @@ defineCase('v3 fragments reject duplicate, unknown, mismatch, length, and noncan // Runner // --------------------------------------------------------------------------- +// --------------------------------------------------------------------------- +// attn-lb7p — the owner key the joiner pins, and Rust<->TS fragment parity. +// --------------------------------------------------------------------------- + +/** + * The canonical fragments BOTH implementations must produce, byte for byte, + * from the same inputs. + * + * Each side re-renders through its own composer and compares to the input, so a + * disagreement about field ORDER or base64 spelling is not cosmetic drift — + * each would reject the other's invites and hosted reviewers would silently + * stop being able to open native ones. Nothing else pins the two together. + * These literals are duplicated verbatim in `src/review/bootstrap.rs` + * (`PARITY_VIEW_FRAGMENT` / `PARITY_COMMENT_FRAGMENT`) and must change in both + * places or not at all. + */ +const PARITY_VIEW_FRAGMENT = + '#v=3&tier=view&read=AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQE&owner=BAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ'; +const PARITY_COMMENT_FRAGMENT = + '#v=3&tier=comment&read=AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQE&write=AgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgI&grant=AwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw&owner=BAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ'; + +const filled = (byte: number, length: number): Uint8Array => new Uint8Array(length).fill(byte); + +defineCase('v3 fragment shape matches the Rust implementation byte for byte', () => { + assertEq( + composeInviteFragmentV3('view', filled(1, 32), undefined, undefined, filled(4, 32)), + PARITY_VIEW_FRAGMENT, + 'view fragment', + ); + assertEq( + composeInviteFragmentV3('comment', filled(1, 32), filled(2, 32), filled(3, 64), filled(4, 32)), + PARITY_COMMENT_FRAGMENT, + 'comment fragment', + ); +}); + +defineCase('v3 owner key round-trips and is exposed for grant verification', () => { + for (const fragment of [PARITY_VIEW_FRAGMENT, PARITY_COMMENT_FRAGMENT]) { + const parsed = parseInviteFragmentV3(fragment); + assertEq( + parsed.ownerPublicSigningKey, + 'BAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ', + 'owner key surfaces as base64url for verifyDeviceGrantV3', + ); + } +}); + +defineCase('a v3 fragment minted before the owner key still parses', () => { + // Backward skew: pre-attn-lb7p invites must still open, pinning nothing. + const fragment = composeInviteFragmentV3('view', filled(1, 32)); + const parsed = parseInviteFragmentV3(fragment); + assert(parsed.ownerPublicSigningKey === undefined, 'no key pinned'); +}); + +defineCase('v3 fragments reject a malformed or misplaced owner key', () => { + const read = 'AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQE'; + const owner = 'BAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ'; + for (const invalid of [ + `#v=3&tier=view&read=${read}&owner=AQ`, + `#v=3&tier=view&read=${read}&owner=${owner}&owner=${owner}`, + `#v=3&tier=view&owner=${owner}&read=${read}`, + ]) { + let threw = false; + try { + parseInviteFragmentV3(invalid); + } catch { + threw = true; + } + assert(threw, `accepted ${invalid}`); + } +}); + let passed = 0; let failed = 0; for (const run of cases) { diff --git a/web/src/lib/review/browser-invite.ts b/web/src/lib/review/browser-invite.ts index 8c892f28..457af449 100644 --- a/web/src/lib/review/browser-invite.ts +++ b/web/src/lib/review/browser-invite.ts @@ -51,6 +51,15 @@ export interface ParsedInviteFragmentV3 { writeAdmissionKey?: Uint8Array; /** Owner Ed25519 proof; absent for view and required for writable tiers. */ grantSignature?: string; + /** + * The owner's public signing key, pinned from the invite (attn-lb7p). + * + * Kept as the base64url string rather than bytes: every hosted consumer of + * an owner key takes a 43-char string (`verifyDeviceGrantV3`), and + * re-encoding bytes risks a spelling that differs from what was signed. + * Optional so a fragment minted before the field existed still parses. + */ + ownerPublicSigningKey?: string; } export interface ParsedInviteV3 extends ParsedInviteFragmentV3 { @@ -239,16 +248,27 @@ export function composeInviteFragmentV3( readCapabilityKey: Uint8Array, writeAdmissionKey?: Uint8Array, grantSignature?: Uint8Array, + ownerPublicSigningKey?: Uint8Array, ): string { requireCapabilityKey(readCapabilityKey, 'read'); if (tier !== 'view' && tier !== 'comment' && tier !== 'suggest') { throw new InviteParseError(`unknown v3 invite tier: ${String(tier)}`); } + // Appended LAST in every form, matching build_invite_fragment_v3 in + // src/review/bootstrap.rs byte for byte. Both parsers re-render through + // their composer and compare, so a difference in field ORDER between the two + // implementations is not a cosmetic drift — it makes each side reject the + // other's invites. + if (ownerPublicSigningKey !== undefined) { + requireCapabilityKey(ownerPublicSigningKey, 'owner'); + } + const ownerSuffix = + ownerPublicSigningKey === undefined ? '' : `&owner=${base64UrlEncode(ownerPublicSigningKey)}`; if (tier === 'view') { if (writeAdmissionKey !== undefined || grantSignature !== undefined) { throw new InviteParseError('view tier must not include write capability or grant'); } - return `#v=3&tier=view&read=${base64UrlEncode(readCapabilityKey)}`; + return `#v=3&tier=view&read=${base64UrlEncode(readCapabilityKey)}${ownerSuffix}`; } if (writeAdmissionKey === undefined) { throw new InviteParseError(`${tier} tier requires write capability`); @@ -257,7 +277,7 @@ export function composeInviteFragmentV3( if (!(grantSignature instanceof Uint8Array) || grantSignature.length !== 64) { throw new InviteParseError(`${tier} tier requires a 64-byte owner grant signature`); } - return `#v=3&tier=${tier}&read=${base64UrlEncode(readCapabilityKey)}&write=${base64UrlEncode(writeAdmissionKey)}&grant=${base64UrlEncode(grantSignature)}`; + return `#v=3&tier=${tier}&read=${base64UrlEncode(readCapabilityKey)}&write=${base64UrlEncode(writeAdmissionKey)}&grant=${base64UrlEncode(grantSignature)}${ownerSuffix}`; } /** Compose a complete native or hosted v3 invite URL. */ @@ -287,7 +307,7 @@ export function parseInviteFragmentV3(fragment: string): ParsedInviteFragmentV3 throw new InviteParseError('malformed v3 fragment field'); } const [key, value] = pair as [string, string]; - if (!['v', 'tier', 'read', 'write', 'grant'].includes(key)) { + if (!['v', 'tier', 'read', 'write', 'grant', 'owner'].includes(key)) { throw new InviteParseError(`unknown v3 fragment field: ${key}`); } if (fields.has(key)) throw new InviteParseError(`duplicate v3 fragment field: ${key}`); @@ -303,7 +323,15 @@ export function parseInviteFragmentV3(fragment: string): ParsedInviteFragmentV3 const writeAdmissionKey = write === undefined ? undefined : decodeCapability(write, 'write'); const grant = fields.get('grant'); const grantBytes = grant === undefined ? undefined : decodeGrant(grant); - const canonical = composeInviteFragmentV3(tier, readCapabilityKey, writeAdmissionKey, grantBytes); + const owner = fields.get('owner'); + const ownerBytes = owner === undefined ? undefined : decodeCapability(owner, 'owner'); + const canonical = composeInviteFragmentV3( + tier, + readCapabilityKey, + writeAdmissionKey, + grantBytes, + ownerBytes, + ); if (canonical !== fragment) { throw new InviteParseError('v3 fragment is not in canonical field order or encoding'); } @@ -313,6 +341,7 @@ export function parseInviteFragmentV3(fragment: string): ParsedInviteFragmentV3 readCapabilityKey, ...(writeAdmissionKey === undefined ? {} : { writeAdmissionKey }), ...(grant === undefined ? {} : { grantSignature: grant }), + ...(owner === undefined ? {} : { ownerPublicSigningKey: owner }), }; } From 654f1d4635ee7b430e4c089fc7aea3aece448a46 Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Fri, 28 Aug 2026 10:27:24 -0500 Subject: [PATCH 8/8] A reviewer sees the images the document references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last mile of attn-udu8, and the original report: "we don't render the images when they are referenced". Owners were fixed first, then the manifest binding that carried assets (attn-lb7p); this is the reviewer finally seeing pixels instead of a placeholder. The bytes were already arriving. They stopped in the frontend store, which refuses inline assets on the applyEvent path — deliberately, because that method is shared with the hosted session where the payload is still sender-supplied and a manifest may be parked awaiting its entries. Widening that gate would have admitted assets on the untrusted lane too. So use the front door instead. ReviewUpdate::SnapshotCreated carried a pair of ids and had no production emitter at all — it was constructed only under cfg(test) — even though types.ts documents window.__attn__.reviewSnapshot as exactly this route. It now carries the snapshot, and the daemon emits one for each asset after rehydrate_snapshot_event has discarded the sender's value, re-read the blob locally, and checked its length and content hash against the signed BlobRef. Replay emits them too, or images would vanish on restart while a fresh join showed them. Reviewer-side resolution is its own module rather than a reuse of resolveImageSrc, which maps srcs onto `attn://localhost/` — on a reviewer's machine that is nothing, or an unrelated file at the same path. Document and asset wire paths both come from selected_share_wire_path, so a src resolved against the document's own path lands on the asset's key. One rule deliberately differs from the local resolver: `..` that would climb above the share root returns null rather than clamping, since clamping would let `../../chart.svg` resolve onto a root-level asset nobody referenced. data: rather than blob: — no decode (the bytes are already base64url), no object-URL lifetime to get wrong against Editor's full docView rebuild, and no dependency on what a custom-scheme origin permits. A src with no matching asset stays null so the card shows. A reviewer cannot tell "skipped by policy" from "not yet arrived", and inventing a URL for either would be worse than saying so. Verified live: reviewer joins a share of tests/fixtures/images.md and decodes 96x64, 96x64, 64x64 and 120x64; the remote and missing srcs keep their cards; data-src still holds the authored string on every node. Native only. The hosted session zeroes asset bytes after hashing them by design; carrying them into its IndexedDB replay is a separate change. --- src/main.rs | 19 +- src/review/manager.rs | 199 ++++++++++++++- web/src/App.svelte | 30 ++- .../lib/prosemirror/image-nodeview.test.ts | 47 +++- web/src/lib/prosemirror/image-nodeview.ts | 11 +- web/src/lib/review/asset-resolution.test.ts | 236 ++++++++++++++++++ web/src/lib/review/asset-resolution.ts | 158 ++++++++++++ web/src/lib/review/store.svelte.ts | 6 + web/src/lib/types.ts | 17 +- 9 files changed, 698 insertions(+), 25 deletions(-) create mode 100644 web/src/lib/review/asset-resolution.test.ts create mode 100644 web/src/lib/review/asset-resolution.ts diff --git a/src/main.rs b/src/main.rs index b983f59f..129427db 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1465,6 +1465,9 @@ fn build_review_dispatch_js( // here keeps room_id on the Rust side for manager-side observers. let json = match update { ReviewUpdate::EventImported { event, .. } => serde_json::to_string(event)?, + // Same reason as EventImported: `reviewSnapshot(snapshot: ReviewSnapshot)` + // wants a bare snapshot, not a `{kind:"snapshot_created", ...}` wrapper. + ReviewUpdate::SnapshotCreated { snapshot, .. } => serde_json::to_string(snapshot)?, _ => serde_json::to_string(update)?, }; Ok(format!( @@ -2484,7 +2487,7 @@ mod tests { // a unit test, so we instead pin the JS string the handler would call // `evaluate_script(...)` with — that string is what wry hands wkwebview. - use crate::review::manager::ReviewUpdate; + use crate::review::manager::{ReviewSnapshotPayload, ReviewUpdate}; use crate::review::model::{ExactReason, PositionAnchor, ResolvedAnchor}; #[test] @@ -2579,8 +2582,18 @@ mod tests { ( ReviewUpdate::SnapshotCreated { room_id: room.clone(), - snapshot_id: "snap-1".to_string(), - file_id: "file-1".to_string(), + snapshot: ReviewSnapshotPayload { + room_id: room.clone(), + file_id: "file-1".to_string(), + snapshot_id: "snap-1".to_string(), + owner_display_path: Some("chart.svg".to_string()), + created_at: 0, + base_hash: "hash".to_string(), + byte_length: 4, + doc_type: crate::review::model::DocType::Asset, + media_type: Some("image/svg+xml".to_string()), + asset_content: Some("PHN2Zy8+".to_string()), + }, }, "reviewSnapshot", ), diff --git a/src/review/manager.rs b/src/review/manager.rs index 61c91d92..fe93bc26 100644 --- a/src/review/manager.rs +++ b/src/review/manager.rs @@ -212,6 +212,31 @@ pub enum ReviewCommand { // `EventImported` carries a full `ReviewEvent` (~816B). Boxing it would churn // every match/construct site across the IPC + transport layers for a payload // that is built once and immediately consumed — not worth the indirection. +/// One imported snapshot, shaped to the frontend `ReviewSnapshot` +/// (`web/src/lib/types.ts`) so it deserializes straight into the store. +/// +/// `asset_content` is the only field the document lanes never set: it is the +/// base64url payload of a `DocType::Asset` snapshot, kept encoded because the +/// frontend turns it into a `data:` URL and would only have to re-encode it. +/// Documents keep using `content`, which is UTF-8 source. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ReviewSnapshotPayload { + pub room_id: RoomId, + pub file_id: String, + pub snapshot_id: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub owner_display_path: Option, + pub created_at: u64, + pub base_hash: String, + pub byte_length: u64, + pub doc_type: crate::review::model::DocType, + #[serde(skip_serializing_if = "Option::is_none")] + pub media_type: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub asset_content: Option, +} + #[allow(clippy::large_enum_variant)] #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde( @@ -255,10 +280,24 @@ pub enum ReviewUpdate { event: crate::review::model::ReviewEvent, }, /// A new `SnapshotNode` was created (owner-side) or imported (reviewer-side). + /// + /// Carries the whole snapshot rather than a pair of ids (attn-udu8). The + /// id-only form had no production emitter at all — it was constructed + /// only under `#[cfg(test)]` — so `window.__attn__.reviewSnapshot`, which + /// `types.ts` documents as the route by which Rust hands an imported + /// snapshot to the frontend, never actually fired. + /// + /// This is the sanctioned lane for asset bytes. The frontend store + /// deliberately refuses inline assets arriving on `applyEvent`, because + /// that method is shared with the hosted session where the payload is + /// still sender-supplied. What travels here has already been through + /// `rehydrate_snapshot_event`: the sender's value was discarded outright, + /// the blob re-read locally, and its length and content hash checked + /// against the signed `BlobRef`. So this admits assets by the front door + /// instead of widening the gate. SnapshotCreated { room_id: RoomId, - snapshot_id: String, - file_id: String, + snapshot: ReviewSnapshotPayload, }, /// An anchor was (re)resolved against the current replica. The payload /// matches the frontend `ReviewAnchorResolutionUpdate` shape exactly so @@ -3253,10 +3292,16 @@ impl ReviewManager { } }; rehydrate_snapshot_event(&self.store, room_id, &mut event); + // Replay must emit these too, or images vanish on every restart + // while a live join shows them. + let asset = asset_snapshot_update(room_id, &event); (self.update_tx)(ReviewUpdate::EventImported { room_id: room_id.clone(), event, }); + if let Some(asset) = asset { + (self.update_tx)(asset); + } replayed += 1; } if replayed > 0 { @@ -4173,6 +4218,55 @@ impl crate::review::transport::DeviceKeyRefresher for BootstrapKeyRefresher { /// sees the room; the snapshot fills in on a later replay) and we log the /// gap. The blob-before-event outbox ordering makes this rare: by the time /// the event arrives, the blob envelope has already been processed. +/// Build a `SnapshotCreated` update for an ASSET snapshot (attn-udu8). +/// +/// Reads only `inline_snapshot`, which `rehydrate_snapshot_event` has already +/// filled from the locally persisted blob after discarding whatever the sender +/// claimed and checking the blob's length and content hash against the signed +/// `BlobRef`. So this introduces no new trust decision — it forwards a payload +/// the daemon has already authenticated. +/// +/// `None` for documents: they reach the frontend inside `EventImported` +/// already, and duplicating them here would put two rows in the store for one +/// snapshot. Assets are the only kind the frontend drops on that path. +fn asset_snapshot_update( + room_id: &RoomId, + event: &crate::review::model::ReviewEvent, +) -> Option { + use crate::review::model::{DocType, ReviewEventBody}; + + let ReviewEventBody::SnapshotCreated { + file_id, + snapshot_id, + owner_display_path, + base_hash, + inline_snapshot: Some(plaintext), + .. + } = &event.body + else { + return None; + }; + if plaintext.doc_type != DocType::Asset { + return None; + } + let content = plaintext.content.clone()?; + Some(ReviewUpdate::SnapshotCreated { + room_id: room_id.clone(), + snapshot: ReviewSnapshotPayload { + room_id: room_id.clone(), + file_id: file_id.as_str().to_string(), + snapshot_id: snapshot_id.as_str().to_string(), + owner_display_path: owner_display_path.clone(), + created_at: event.meta.created_at, + base_hash: base_hash.as_str().to_string(), + byte_length: content.len() as u64, + doc_type: DocType::Asset, + media_type: plaintext.media_type.clone(), + asset_content: Some(content), + }, + }) +} + fn rehydrate_snapshot_event( store: &crate::review::store::ReviewStore, room_id: &RoomId, @@ -4524,10 +4618,17 @@ fn forward_transport_event( } } rehydrate_snapshot_event(store, &rid, &mut event); + // Assets ride a second update: the frontend store refuses them on + // the EventImported path (that method is shared with the hosted + // session, where the payload is still sender-supplied). + let asset = asset_snapshot_update(&rid, &event); (update_tx)(ReviewUpdate::EventImported { room_id: rid, event, }); + if let Some(asset) = asset { + (update_tx)(asset); + } if is_verdict { observers.verdict_revision_tx.send_modify(|revision| { *revision = revision.wrapping_add(1); @@ -5430,6 +5531,86 @@ mod tests { ) } + #[test] + fn asset_snapshot_update_forwards_only_authenticated_assets() { + use crate::review::model::{DocType, ReviewEventBody, SnapshotAssetEncoding}; + + let room_id: RoomId = dummy_id("room-asset-update"); + + fn snapshot_event_with_inline( + room_id: &RoomId, + path: &str, + plaintext: crate::review::model::SnapshotPlaintext, + ) -> crate::review::model::ReviewEvent { + stub_review_event( + room_id, + crate::review::model::ReviewEventBody::SnapshotCreated { + file_id: dummy_id("file-1"), + snapshot_id: dummy_id("snap-1"), + owner_display_path: Some(path.to_string()), + parent_snapshot_id: None, + base_hash: dummy_id("hash-1"), + encrypted_blob_ref: None, + inline_snapshot: Some(plaintext), + }, + ) + } + + // A document snapshot must NOT produce one: documents already reach + // the frontend inside EventImported, and a second row would duplicate + // them. + let markdown = crate::review::model::SnapshotPlaintext { + doc_type: DocType::Markdown, + content: Some("# hi".to_string()), + anchor_index: None, + media_type: None, + encoding: None, + manifest: None, + annotation: None, + }; + let event = snapshot_event_with_inline(&room_id, "images.md", markdown); + assert!( + asset_snapshot_update(&room_id, &event).is_none(), + "a markdown snapshot must not emit an asset update" + ); + + // An asset does, carrying its bytes and media type. + let asset = crate::review::model::SnapshotPlaintext { + doc_type: DocType::Asset, + content: Some("PHN2Zy8-".to_string()), + anchor_index: None, + media_type: Some("image/svg+xml".to_string()), + encoding: Some(SnapshotAssetEncoding::Base64url), + manifest: None, + annotation: None, + }; + let event = snapshot_event_with_inline(&room_id, "chart.svg", asset); + let Some(ReviewUpdate::SnapshotCreated { snapshot, .. }) = + asset_snapshot_update(&room_id, &event) + else { + panic!("an asset snapshot must emit an update"); + }; + assert_eq!(snapshot.doc_type, DocType::Asset); + assert_eq!(snapshot.owner_display_path.as_deref(), Some("chart.svg")); + assert_eq!(snapshot.media_type.as_deref(), Some("image/svg+xml")); + assert_eq!(snapshot.asset_content.as_deref(), Some("PHN2Zy8-")); + + // And an event whose inline payload was never hydrated emits nothing — + // the only bytes this may forward are ones rehydrate_snapshot_event + // already re-read and hash-checked locally. + let mut bare = event.clone(); + if let ReviewEventBody::SnapshotCreated { + inline_snapshot, .. + } = &mut bare.body + { + *inline_snapshot = None; + } + assert!( + asset_snapshot_update(&room_id, &bare).is_none(), + "an unhydrated event must not emit an asset update" + ); + } + #[test] fn rehydrate_manifest_accepts_owner_signed_manifest_without_any_room_secret() { // The bug this epic exists for. A v3 reviewer never receives the room @@ -6888,8 +7069,18 @@ mod tests { assert_eq!( ReviewUpdate::SnapshotCreated { room_id: room_id.clone(), - snapshot_id: "s".to_string(), - file_id: "f".to_string() + snapshot: ReviewSnapshotPayload { + room_id: room_id.clone(), + file_id: "file-1".to_string(), + snapshot_id: "snap-1".to_string(), + owner_display_path: Some("chart.svg".to_string()), + created_at: 0, + base_hash: "hash".to_string(), + byte_length: 4, + doc_type: crate::review::model::DocType::Asset, + media_type: Some("image/svg+xml".to_string()), + asset_content: Some("PHN2Zy8+".to_string()), + } } .callback_name(), "reviewSnapshot" diff --git a/web/src/App.svelte b/web/src/App.svelte index 17139d19..ddf63ac8 100644 --- a/web/src/App.svelte +++ b/web/src/App.svelte @@ -127,6 +127,7 @@ markdownSourceUrl, resolveImageSrc, } from './lib/markdown-layer'; + import { buildSharedAssetResolver } from './lib/review/asset-resolution'; import { reviewStore } from './lib/review/store.svelte'; import { consumePendingRoomFocus } from './lib/review/pending-room-focus'; import ReviewMargin from './lib/ReviewMargin.svelte'; @@ -285,6 +286,26 @@ const docPath = activePath; return (src: string) => resolveImageSrc(docPath, src); }); + + // The reviewer's counterpart (attn-udu8). A reviewer has no copy of the + // owner's disk, so `resolveActiveAssetUrl` is precisely wrong for them — it + // mints `attn://localhost/`, which on their machine is nothing, or + // an unrelated file that happens to share the path. What they do have is the + // asset snapshots that travelled with the document, keyed by the same wire + // paths the document itself is published under. + // + // Same eager-read discipline as above, and for the same reason: everything + // the closure depends on is read WHILE the derived evaluates, so the prop + // identity actually changes when the viewed document or the asset set does. + // A separate identifier from `resolveActiveAssetUrl` on purpose — the two + // resolve against different worlds, and the wiring test counts the bindings + // of each. + let resolveReviewAssetUrl = $derived.by(() => { + const snapshots = reviewStore.snapshots; + const roomId = reviewStore.currentRoomId; + const docWirePath = reviewSnapshot?.ownerDisplayPath; + return buildSharedAssetResolver(snapshots, roomId, docWirePath); + }); let hasActiveTab = $derived(Boolean(activeTab)); let activeFileType = $derived(activeTab?.fileType ?? 'unsupported'); @@ -3662,6 +3683,7 @@ bind:this={editorRef} markdown={collabActive ? (collabSeedMarkdown || effectiveMarkdown) : effectiveMarkdown} editable={false} + resolveAssetUrl={resolveReviewAssetUrl} onLinkNavigate={handleEditorLinkNavigate} onSuggestionClick={handleSuggestionClick} onSave={saveEdits} @@ -3693,9 +3715,11 @@ /> {/if} {:else if activeFileType === 'markdown'} - + { ); }); -defineCase('21. the reviewer snapshot editor is left unresolved', () => { - // That editor renders the OWNER's document; its relative srcs name files on - // the owner's disk. Resolving them would mint a convincing attn:// URL for a - // file that is not on this machine — worse than the authored src. It still - // MOUNTS the view (registration is unconditional), so an unloadable src - // there gets the card; what it must not have is a resolver. +defineCase('21. the reviewer snapshot editor resolves against the share, not the disk', () => { + // This case used to assert the reviewer editor had NO resolver, because the + // only one available mapped srcs onto the OWNER's disk. attn-udu8 gave it a + // resolver of its own, over the assets that travelled with the share — so + // the requirement inverts: it must resolve, and must not use the local one. + // Binding `resolveActiveAssetUrl` here would mint a convincing attn:// URL + // for a file that is not on this machine. const app = read('../App.svelte'); - const bound = app.match(/resolveAssetUrl=\{resolveActiveAssetUrl\}/g) ?? []; - assertEq(bound.length, 2, 'exactly the local markdown editor and the editor-only diagnostic'); + const local = app.match(/resolveAssetUrl=\{resolveActiveAssetUrl\}/g) ?? []; + assertEq(local.length, 2, 'local resolver: the markdown editor and the editor-only diagnostic'); + const shared = app.match(/resolveAssetUrl=\{resolveReviewAssetUrl\}/g) ?? []; + assertEq(shared.length, 1, 'shared resolver: exactly the reviewer-snapshot editor'); // Anchored on `` after the branch opener: the // non-greedy form stopped at `` 436 characters in, so the // fragment it checked could never have contained the prop and the case @@ -400,9 +403,33 @@ defineCase('21. the reviewer snapshot editor is left unresolved', () => { assert(snapshotEditor !== null, 'could not find the reviewer-snapshot editor'); assert(/ { + // Same trap as case 20, one derived over. Editor tracks the prop by + // identity, so a closure that merely CLOSES over the snapshot list would + // keep one identity for the editor's life and the images would stay bound + // to whichever document was open when it mounted. + const app = read('../App.svelte'); + const derived = /let resolveReviewAssetUrl = \$derived\.by\(\(\) => \{([\s\S]*?)\}\);/.exec(app); + assert(derived !== null, 'App.svelte must build the reviewer resolver with $derived.by'); + for (const dependency of [ + 'const snapshots = reviewStore.snapshots;', + 'const roomId = reviewStore.currentRoomId;', + 'const docWirePath = reviewSnapshot?.ownerDisplayPath;', + ]) { + assert( + derived[1].includes(dependency), + `the reviewer resolver must read \`${dependency}\` while the derived evaluates`, + ); + } }); // --------------------------------------------------------------------------- diff --git a/web/src/lib/prosemirror/image-nodeview.ts b/web/src/lib/prosemirror/image-nodeview.ts index 22fb3324..1314c0bf 100644 --- a/web/src/lib/prosemirror/image-nodeview.ts +++ b/web/src/lib/prosemirror/image-nodeview.ts @@ -62,10 +62,13 @@ export function imageFileName(src: string): string { * * @param node the image node * @param resolveAssetUrl maps an authored src onto something the webview can - * load, or `null` when it cannot be resolved. Omitted - * by surfaces with no local document behind them, where - * it means "resolve nothing": the authored src is used - * as-is and, when it fails, the placeholder explains. + * load, or `null` when it cannot be resolved. Two + * resolvers exist: the local one maps a src onto the + * file beside the document on THIS disk, and the + * reviewer's maps it onto an asset that travelled with + * a share. Omitting it means "resolve nothing" — the + * authored src is used as-is and, when it fails, the + * placeholder explains. */ export function imageNodeView( node: PmNode, diff --git a/web/src/lib/review/asset-resolution.test.ts b/web/src/lib/review/asset-resolution.test.ts new file mode 100644 index 00000000..e2924ec6 --- /dev/null +++ b/web/src/lib/review/asset-resolution.test.ts @@ -0,0 +1,236 @@ +// Reviewer-side asset resolution (attn-udu8). +// +// Standalone tsx script, like every other web test here — run by +// web/scripts/run-tests.mjs, not vitest. + +import { sharedAssetPathFor, assetDataUrl, buildSharedAssetResolver } from './asset-resolution'; +import type { ReviewSnapshot } from '../types'; + +interface CaseResult { + name: string; + ok: boolean; + detail?: string; +} +const cases: Array<() => CaseResult> = []; + +function defineCase(name: string, fn: () => void | string): void { + cases.push(() => { + try { + const detail = fn(); + return { name, ok: true, detail: typeof detail === 'string' ? detail : undefined }; + } catch (error) { + return { name, ok: false, detail: error instanceof Error ? error.message : String(error) }; + } + }); +} + +function assert(condition: boolean, message: string): void { + if (!condition) throw new Error(message); +} + +function assertEq(actual: T, expected: T, message: string): void { + if (actual !== expected) { + throw new Error(`${message}: expected ${String(expected)}, got ${String(actual)}`); + } +} + +// --------------------------------------------------------------------------- +// sharedAssetPathFor — must agree with resolve_lexically in src/review/assets.rs +// --------------------------------------------------------------------------- + +defineCase('a sibling src resolves to the document\u2019s own directory', () => { + assertEq(sharedAssetPathFor('images.md', './diagram.png'), 'diagram.png', 'dot-slash'); + assertEq(sharedAssetPathFor('images.md', 'diagram.png'), 'diagram.png', 'bare'); + assertEq(sharedAssetPathFor('docs/images.md', './diagram.png'), 'docs/diagram.png', 'nested doc'); +}); + +defineCase('a subdirectory src keeps its subdirectory', () => { + assertEq(sharedAssetPathFor('images.md', './nested/diagram.png'), 'nested/diagram.png', 'nested'); + assertEq( + sharedAssetPathFor('docs/images.md', 'sub/chart.svg'), + 'docs/sub/chart.svg', + 'nested doc + nested src', + ); +}); + +defineCase('.. walks up, and is REFUSED rather than clamped at the root', () => { + // The deliberate divergence from resolveImageSrc in markdown-layer.ts, which + // clamps because it walks an absolute filesystem path. Here a src that + // climbs past the share root names nothing; clamping would let it land on an + // unrelated root-level asset. + assertEq(sharedAssetPathFor('docs/images.md', '../chart.svg'), 'chart.svg', 'up one'); + assertEq( + sharedAssetPathFor('a/b/images.md', '../../chart.svg'), + 'chart.svg', + 'up two to the root', + ); + assertEq(sharedAssetPathFor('images.md', '../chart.svg'), null, 'above the root is refused'); + assertEq( + sharedAssetPathFor('docs/images.md', '../../../chart.svg'), + null, + 'climbing well past the root is refused, not clamped', + ); +}); + +defineCase('an encoded separator is a separator', () => { + // %2F becomes a real / everywhere downstream, so the walk has to treat it as + // one — otherwise `..%2F..%2Fx.png` would look like an innocent filename. + assertEq(sharedAssetPathFor('images.md', 'nested%2Fdiagram.png'), 'nested/diagram.png', 'encoded'); + assertEq(sharedAssetPathFor('images.md', '..%2Fchart.svg'), null, 'encoded climb refused'); +}); + +defineCase('a percent-encoded name decodes to the real file', () => { + assertEq(sharedAssetPathFor('images.md', './my%20shot.png'), 'my shot.png', 'space'); +}); + +defineCase('non-local srcs are declined', () => { + for (const src of [ + 'https://example.com/x.png', + 'data:image/png;base64,AA', + '//cdn.example.com/x.png', + '#anchor', + 'attn://localhost/Users/x/y.png', + ]) { + assertEq(sharedAssetPathFor('images.md', src), null, `declined ${src}`); + } +}); + +defineCase('a Windows drive letter is not mistaken for a scheme', () => { + // Two-char minimum on the scheme. It still resolves to nothing useful, but + // it must not be short-circuited as "remote". + const resolved = sharedAssetPathFor('images.md', 'C:/x.png'); + assert(resolved === null || typeof resolved === 'string', 'no throw'); +}); + +defineCase('directory-ish and empty srcs are declined', () => { + for (const src of ['', './', '.', '..', 'nested/']) { + assertEq(sharedAssetPathFor('images.md', src), null, `declined ${JSON.stringify(src)}`); + } +}); + +defineCase('an absolute src cannot name a share asset', () => { + // Wire paths are root-relative; an absolute src is an owner-disk path. + assertEq(sharedAssetPathFor('images.md', '/Users/x/chart.svg'), 'Users/x/chart.svg', 'no base'); +}); + +defineCase('the result is NFC, because wire paths are', () => { + // e + combining acute normalizes to the precomposed form. + const decomposed = 'cafe\u0301.png'; + assertEq(sharedAssetPathFor('images.md', decomposed), 'caf\u00e9.png', 'NFC'); +}); + +// --------------------------------------------------------------------------- +// assetDataUrl +// --------------------------------------------------------------------------- + +defineCase('base64url is translated and padded into a data: URL', () => { + // `-` and `_` are the base64url-only characters; padding restores a multiple + // of four. + assertEq( + assetDataUrl('image/png', 'ab-d_g'), + 'data:image/png;base64,ab+d/g==', + 'translated and padded', + ); + assertEq(assetDataUrl('image/png', 'AAAA'), 'data:image/png;base64,AAAA', 'already aligned'); +}); + +defineCase('a missing or malformed media type yields null, not data:undefined', () => { + assertEq(assetDataUrl(undefined, 'AAAA'), null, 'no media type'); + assertEq(assetDataUrl('image/png', undefined), null, 'no content'); + assertEq(assetDataUrl('not-a-media-type', 'AAAA'), null, 'malformed media type'); +}); + +// --------------------------------------------------------------------------- +// buildSharedAssetResolver +// --------------------------------------------------------------------------- + +function assetSnapshot(path: string, overrides: Partial = {}): ReviewSnapshot { + return { + roomId: 'room-1', + fileId: `file-${path}`, + snapshotId: `snap-${path}`, + ownerDisplayPath: path, + createdAt: 1, + createdBy: 'owner' as ReviewSnapshot['createdBy'], + baseHash: 'hash' as ReviewSnapshot['baseHash'], + byteLength: 4, + docType: 'asset', + mediaType: 'image/png', + assetContent: 'AAAA', + ...overrides, + } as ReviewSnapshot; +} + +defineCase('an asset that travelled with the document resolves to its bytes', () => { + const resolve = buildSharedAssetResolver([assetSnapshot('diagram.png')], 'room-1', 'images.md'); + assertEq(resolve('./diagram.png'), 'data:image/png;base64,AAAA', 'resolved'); +}); + +defineCase('a src with no matching asset resolves to null, not a guess', () => { + // Policy-skipped images (remote, symlink, oversized, budget-exhausted) and + // not-yet-arrived ones are indistinguishable here, and the honest answer for + // both is the placeholder card. + const resolve = buildSharedAssetResolver([assetSnapshot('diagram.png')], 'room-1', 'images.md'); + assertEq(resolve('./gone.png'), null, 'missing asset'); +}); + +defineCase('assets from another room are never used', () => { + const foreign = assetSnapshot('diagram.png', { roomId: 'room-2' }); + const resolve = buildSharedAssetResolver([foreign], 'room-1', 'images.md'); + assertEq(resolve('./diagram.png'), null, 'cross-room isolation'); +}); + +defineCase('document snapshots are not treated as assets', () => { + const doc = assetSnapshot('images.md', { + docType: 'markdown', + mediaType: undefined, + assetContent: undefined, + }); + const resolve = buildSharedAssetResolver([doc], 'room-1', 'images.md'); + assertEq(resolve('./images.md'), null, 'markdown is not an asset'); +}); + +defineCase('the newest snapshot for a path wins', () => { + const older = assetSnapshot('diagram.png', { snapshotId: 'old', assetContent: 'AAAA' }); + const newer = assetSnapshot('diagram.png', { + snapshotId: 'new', + createdAt: 2, + assetContent: 'BBBB', + }); + const resolve = buildSharedAssetResolver([older, newer], 'room-1', 'images.md'); + assertEq(resolve('./diagram.png'), 'data:image/png;base64,BBBB', 'newest wins'); +}); + +defineCase('no room or no document path yields a resolver that declines everything', () => { + assertEq(buildSharedAssetResolver([assetSnapshot('a.png')], null, 'images.md')('./a.png'), null, 'no room'); + assertEq(buildSharedAssetResolver([assetSnapshot('a.png')], 'room-1', null)('./a.png'), null, 'no doc'); +}); + +defineCase('repeated resolution of one asset is memoised', () => { + const resolve = buildSharedAssetResolver([assetSnapshot('diagram.png')], 'room-1', 'images.md'); + const first = resolve('./diagram.png'); + const second = resolve('diagram.png'); + assertEq(first, second, 'same URL for the same asset via two spellings'); +}); + +let passed = 0; +let failed = 0; +for (const run of cases) { + const r = run(); + if (r.ok) { + passed += 1; + console.log(` ok ${r.name}${r.detail ? ` — ${r.detail}` : ''}`); + } else { + failed += 1; + console.error(` FAIL ${r.name}\n ${r.detail ?? '(no detail)'}`); + } +} +console.log(`\n${passed} passed, ${failed} failed`); + +interface NodeProcessShape { + exit?: (code: number) => void; +} +const nodeProcess: NodeProcessShape | undefined = ( + globalThis as unknown as { process?: NodeProcessShape } +).process; +if (failed > 0) nodeProcess?.exit?.(1); diff --git a/web/src/lib/review/asset-resolution.ts b/web/src/lib/review/asset-resolution.ts new file mode 100644 index 00000000..9c0969b4 --- /dev/null +++ b/web/src/lib/review/asset-resolution.ts @@ -0,0 +1,158 @@ +// Resolving a shared document's image srcs against the assets that travelled +// with it (attn-udu8, reviewer half). +// +// A reviewer has no copy of the owner's disk, so the resolver in +// markdown-layer.ts is exactly wrong here: it maps a src onto an +// `attn://localhost/` URL, which on a reviewer's machine is either +// nothing or — worse — some unrelated file that happens to sit at the same +// path. What a reviewer has instead is the set of asset snapshots the owner +// published alongside the document, each carrying the bytes and a wire path. +// +// PATH SPACE. Document `ownerDisplayPath` and asset `ownerDisplayPath` are +// both minted by `selected_share_wire_path` in src/review/bootstrap.rs, which +// strips the share root, admits only `Component::Normal` segments, joins with +// `/`, and NFC-normalises. So they live in one root-relative space and a src +// resolved against the document's own path lands on the asset's key. The +// manifest is NOT consulted: it is a binding artifact, and the asset snapshot +// already carries the only key needed. + +import type { ReviewSnapshot } from '../types'; + +/** `scheme:` — 2+ chars so a `C:` drive letter is not mistaken for one. + * Mirrors `is_non_local` in src/review/assets.rs. */ +function isNonLocal(src: string): boolean { + if (src.startsWith('//') || src.startsWith('#')) return true; + const colon = src.indexOf(':'); + if (colon < 0) return false; + const scheme = src.slice(0, colon); + return ( + scheme.length >= 2 && + /^[a-zA-Z]/.test(scheme) && + /^[a-zA-Z0-9+\-.]+$/.test(scheme) + ); +} + +/** Decode one segment, falling back to raw bytes on a malformed escape — + * same contract as `decodeSegment` in markdown-layer.ts and `decode_segment` + * in src/review/assets.rs. */ +function decodeSegment(segment: string): string { + try { + return decodeURIComponent(segment); + } catch { + return segment; + } +} + +/** + * Map an authored src onto the wire path of the asset it names, or `null`. + * + * @param docWirePath the document's own `ownerDisplayPath` + * @param src `node.attrs.src` exactly as authored + * + * Returns `null` for anything that cannot name an asset in this share: a + * remote or `data:` src, a bare fragment, a directory-ish path, or one that + * climbs above the share root. + */ +export function sharedAssetPathFor(docWirePath: string, src: string): string | null { + if (!src || isNonLocal(src)) return null; + + // Decoded first, then re-split: an encoded `%2F` is a real separator to + // everything downstream, so it has to be one while the walk below reasons + // about where the path lands. + const segments = src.split('/').flatMap((piece) => decodeSegment(piece).split('/')); + const last = segments[segments.length - 1]; + if (last === '' || last === '.' || last === '..') return null; + + // An absolute src cannot name a share asset: wire paths are root-relative. + // (The owner's own window sees absolute paths on both sides, so the dirname + // join below still matches there.) + const base = src.startsWith('/') + ? [] + : docWirePath.split('/').slice(0, -1).filter((segment) => segment !== ''); + + const stack = [...base]; + for (const segment of segments) { + if (!segment || segment === '.') continue; + if (segment === '..') { + // Refuse rather than clamp. markdown-layer.ts clamps because it walks an + // ABSOLUTE path and something has to stop at the filesystem root; here a + // src that climbs past the share root names nothing, and clamping would + // let `../../chart.svg` silently resolve onto a root-level asset that + // the author never referenced. + if (stack.length === 0) return null; + stack.pop(); + continue; + } + stack.push(segment); + } + if (stack.length === 0) return null; + // Wire paths are NFC; an authored src need not be. + return stack.join('/').normalize('NFC'); +} + +/** + * Build a `data:` URL from an asset snapshot's payload. + * + * `data:` rather than `blob:` on purpose. The bytes arrive already base64url + * encoded, so this is string work with no `Uint8Array` round trip; there is no + * object-URL lifetime to manage against Editor.svelte's full docView rebuild + * (which would revoke URLs still referenced by live `` elements); and it + * needs nothing of the origin, which for the native app is a custom scheme. + * + * Returns `null` without a media type rather than minting `data:undefined;…`. + */ +export function assetDataUrl( + mediaType: string | undefined, + base64url: string | undefined, +): string | null { + if (!mediaType || !base64url) return null; + if (!/^[\w.+-]+\/[\w.+-]+$/.test(mediaType)) return null; + const standard = base64url.replace(/-/g, '+').replace(/_/g, '/'); + const padding = standard.length % 4 === 0 ? '' : '='.repeat(4 - (standard.length % 4)); + return `data:${mediaType};base64,${standard}${padding}`; +} + +/** + * A resolver for the reviewer's snapshot editor. + * + * Returns `null` for any src with no matching asset, which is the honest + * answer: an image the owner's policy declined to send (remote, symlinked, + * oversized, budget-exhausted — see src/review/assets.rs) is indistinguishable + * from one that has not arrived yet, and inventing a URL for either would be + * worse than the placeholder card. + */ +export function buildSharedAssetResolver( + snapshots: readonly ReviewSnapshot[], + roomId: string | null, + docWirePath: string | null | undefined, +): (src: string) => string | null { + if (!roomId || !docWirePath) return () => null; + + // Newest wins, matching how the document snapshot itself is chosen. + const assets = new Map(); + for (const snapshot of snapshots) { + if (snapshot.roomId !== roomId) continue; + if (snapshot.docType !== 'asset') continue; + const key = snapshot.ownerDisplayPath; + if (!key) continue; + const existing = assets.get(key); + if (!existing || snapshot.createdAt > existing.createdAt) assets.set(key, snapshot); + } + if (assets.size === 0) return () => null; + + // Memoised per snapshot: the NodeView calls the resolver on construction and + // again on every update, and a resolver identity change redraws every image + // in the document. + const urls = new Map(); + return (src: string): string | null => { + const path = sharedAssetPathFor(docWirePath, src); + if (path === null) return null; + const snapshot = assets.get(path); + if (!snapshot) return null; + const cached = urls.get(snapshot.snapshotId); + if (cached !== undefined) return cached; + const url = assetDataUrl(snapshot.mediaType, snapshot.assetContent); + urls.set(snapshot.snapshotId, url); + return url; + }; +} diff --git a/web/src/lib/review/store.svelte.ts b/web/src/lib/review/store.svelte.ts index 45f625e4..d6ab061d 100644 --- a/web/src/lib/review/store.svelte.ts +++ b/web/src/lib/review/store.svelte.ts @@ -837,6 +837,12 @@ export class ReviewStore { // manifest may be waiting for referenced R2 entries, and exposing it // now would bypass that binding gate. Content-less pointer events still // create the placeholder that a later authenticated blob replaces. + // + // This gate no longer costs the native reviewer their images (attn-udu8): + // the daemon emits a separate `reviewSnapshot` update for asset + // snapshots, which reaches `applySnapshot` below carrying bytes it has + // already re-read locally and checked against the signed BlobRef. Assets + // arrive by that door rather than by widening this one. if (inline != null && document === undefined) return; const snapshot: ReviewSnapshot = { roomId: event.meta.roomId, diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts index 37354aef..a7a95880 100644 --- a/web/src/lib/types.ts +++ b/web/src/lib/types.ts @@ -1268,8 +1268,23 @@ export interface ReviewSnapshot { docType?: DocType; /** Raw UTF-8 source: markdown source for `markdown`, HTML for `html`. */ content?: string; - /** Validated, inert metadata for binary assets. Asset bytes are never rendered here. */ + /** Validated, inert metadata for binary assets. */ mediaType?: string; + /** + * Asset bytes, unpadded base64url, exactly as the snapshot carried them + * (attn-udu8). + * + * NATIVE LANE ONLY. The daemon sets this on a `SnapshotCreated` update after + * `rehydrate_snapshot_event` has discarded the sender's value and + * re-verified the blob's length and content hash against the signed + * `BlobRef`. The hosted session deliberately zeroes asset bytes after + * hashing them and must never populate this. + * + * Kept encoded because the only consumer turns it into a `data:` URL. It is + * separate from `content` — which is UTF-8 source — so that filters keyed on + * `typeof content === 'string'` keep meaning "a document". + */ + assetContent?: string; /** Validated, inert workspace topology. It contains no entry bodies. */ workspaceManifest?: WorkspaceSnapshotManifest; anchorIndex?: AnchorIndex;