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/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/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/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/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/bootstrap.rs b/src/review/bootstrap.rs index a99d2b6d..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 = 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 +2061,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 {}{}", @@ -2282,6 +2382,11 @@ impl Bootstrapper { write_admission_key: Some(*room_keys.write_admission_key.as_bytes()), grant_tier: None, grant_signature: None, + // The owner pins its OWN key. Without this an owner falls to + // the room-secret fallback, which still works for them and so + // would leave the new path untested on the one machine that + // mints manifests. + owner_public_signing_key: Some(identity.public_signing_key.clone()), }, )?; record_local_share(self.store.root(), &room_id, &path, path.is_dir())?; @@ -2715,6 +2820,16 @@ impl Bootstrapper { let grant = parsed.fragment.grant_signature.ok_or_else(|| { BootstrapError::InviteParse("writable v3 invite missing owner grant".into()) })?; + // Verify the owner's grant LOCALLY against the key the invite pins, + // before registering anything (attn-lb7p). The relay checks this too, + // but the relay is the party a pinned key exists to defend against: + // trusting its verdict would make the whole pin decorative. An invite + // minted before the field existed has no key to check against and is + // let through unverified, exactly as it was before this change — it + // simply never gets manifest hydration. + if let Some(owner_public) = parsed.fragment.owner_public_signing_key { + verify_invite_grant_v3(&parsed, &owner_public)?; + } let read_keys = crate::review::crypto::kdf::derive_read_keys_v3(&parsed.fragment.read_capability_key); let wire_kind = match kind { @@ -2836,6 +2951,10 @@ impl Bootstrapper { write_admission_key: Some(write_key), grant_tier: Some(grant_tier), grant_signature: Some(URL_SAFE_NO_PAD.encode(grant)), + owner_public_signing_key: parsed + .fragment + .owner_public_signing_key + .map(|key| URL_SAFE_NO_PAD.encode(key)), }, )?; Ok(JoinOutcome { @@ -3170,6 +3289,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, @@ -4316,9 +4499,22 @@ pub struct RoomAccessV3 { pub write_admission_key: Option<[u8; 32]>, 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, @@ -4457,6 +4653,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, @@ -4588,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() @@ -4606,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, }) @@ -5240,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(), @@ -5316,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!( @@ -5372,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(); @@ -5398,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"), @@ -5405,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(), @@ -5413,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 bbf2c0d0..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 @@ -2353,6 +2392,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, @@ -3220,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 { @@ -4140,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, @@ -4305,17 +4432,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(); @@ -4440,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); @@ -5278,6 +5463,249 @@ 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 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 + // 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}; @@ -6641,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/src/review/mod.rs b/src/review/mod.rs index c6028f66..3990280c 100644 --- a/src/review/mod.rs +++ b/src/review/mod.rs @@ -11,6 +11,7 @@ pub mod agent; pub mod agent_identity; pub mod anchors; 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/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 00000000..0423d96f Binary files /dev/null and b/tests/fixtures/diagram.png differ diff --git a/tests/fixtures/images.md b/tests/fixtures/images.md new file mode 100644 index 00000000..4ef9a7b6 --- /dev/null +++ b/tests/fixtures/images.md @@ -0,0 +1,45 @@ +# 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. + +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 + +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 00000000..28444fdf Binary files /dev/null and b/tests/fixtures/nested/diagram.png differ diff --git a/web/src/App.svelte b/web/src/App.svelte index 94f03ebe..ddf63ac8 100644 --- a/web/src/App.svelte +++ b/web/src/App.svelte @@ -125,7 +125,9 @@ extractStructureFromMarkdown, loadMarkdownFromPath, 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'; @@ -271,6 +273,39 @@ 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); + }); + + // 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'); @@ -3648,6 +3683,7 @@ bind:this={editorRef} markdown={collabActive ? (collabSeedMarkdown || effectiveMarkdown) : effectiveMarkdown} editable={false} + resolveAssetUrl={resolveReviewAssetUrl} onLinkNavigate={handleEditorLinkNavigate} onSuggestionClick={handleSuggestionClick} onSave={saveEdits} @@ -3679,10 +3715,16 @@ /> {/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,20 @@ const builtIn: Record = { task_list_item: taskListItemNodeView, frontmatter: (node) => frontmatterNodeView(node), + // 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; @@ -1006,6 +1036,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..e5ae6a49 --- /dev/null +++ b/web/src/lib/prosemirror/image-nodeview.test.ts @@ -0,0 +1,449 @@ +// 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 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'); + }), +); + +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', + ); + // 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( + /^\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', + ); +}); + +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 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 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 + // passed on a technicality. + const snapshotEditor = /\{:else if isReviewerViewingSnapshot\}[\s\S]*?/.exec(app); + 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`, + ); + } +}); + +// --------------------------------------------------------------------------- + +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 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..1314c0bf --- /dev/null +++ b/web/src/lib/prosemirror/image-nodeview.ts @@ -0,0 +1,214 @@ +// 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 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'; + +/** 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. 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, + 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/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/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 }), }; } 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; 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