From f311ca1990324bf77a0aea826279eb3d4e953eb7 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Fri, 18 Sep 2026 19:23:18 -0700 Subject: [PATCH 1/5] feat: make links in Claude's output clickable Claude Code prints links as OSC 8 hyperlinks whose visible text is hard-wrapped into terminal-width pieces -- urlDetector's header records a 346-character sign-in URL arriving as five emissions, each carrying the whole URL in its parameter and about 80 characters on screen. WebLinksAddon regex-matches the painted characters row by row, so against Claude it matches a fragment or nothing, which is why the URL toast exists. xterm 5.5 hands over the exact parameter through `linkHandler`, so the slicing stops mattering. WebLinksAddon stays for plain-text URLs in ordinary shell output; the two cover different cases and neither replaces the other. Both now share one failure reporter and one validator. No new key handling was needed. xterm's mousedown handler is `if (areMouseEventsActive && !shouldForceSelection(e)) return cancel(e)`, so holding the force-selection modifier lets the event reach the link layer while Claude still holds the mouse -- Shift+click, or Option+click on macOS, which this terminal already enables for text selection. The hover card is the security half rather than decoration. OSC 8 decouples the label from the target completely: a container can print `https://claude.ai` and link it anywhere, which is strictly worse than the userinfo spoofing already guarded against and which invalidated the justification for opening a click without confirmation ("a deliberate act on visible text"). Hovering now shows the real origin, in full and never truncated, because truncating it is the spoof. A target that fails validation says so and deliberately echoes nothing of itself. The hint names the modifier for the platform, from xterm's own `isMac` list, so it cannot tell a Mac user to press a key that does nothing. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/terminal/TerminalView.test.tsx | 143 ++++++++++- app/src/components/terminal/TerminalView.tsx | 241 ++++++++++++++++-- 2 files changed, 365 insertions(+), 19 deletions(-) diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index b59da3b..3d4c1d2 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -1,6 +1,10 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, fireEvent, cleanup, act } from "@testing-library/react"; -import TerminalView, { supersedes } from "./TerminalView"; +import TerminalView, { + OSC8_HOVER_CLASS, + createOsc8LinkHandler, + supersedes, +} from "./TerminalView"; import { useAppState } from "../../store/appState"; import { uploadHostFileToTerminal, @@ -1074,3 +1078,140 @@ describe("TerminalView — releasing a captured mouse", () => { expect(terminalInput).not.toHaveBeenCalled(); }); }); + +describe("the hover hint names the key that actually works", () => { + const platform = (value: string) => + Object.defineProperty(navigator, "platform", { value, configurable: true }); + const original = navigator.platform; + afterEach(() => platform(original)); + + // xterm gates this on its own `isMac`; if the hint and the gate disagree the + // user is told to press a key that does nothing. + it("says Option on a Mac, because that is xterm's force-selection modifier there", () => { + platform("MacIntel"); + const host = document.createElement("div"); + createOsc8LinkHandler(() => host).hover?.( + new MouseEvent("mousemove"), + "https://example.com/x", + { start: { x: 1, y: 1 }, end: { x: 1, y: 1 } }, + ); + expect(host.textContent).toContain("Option+click"); + expect(host.textContent).not.toContain("Shift+click"); + }); + + it("says Shift everywhere else", () => { + platform("Linux x86_64"); + const host = document.createElement("div"); + createOsc8LinkHandler(() => host).hover?.( + new MouseEvent("mousemove"), + "https://example.com/x", + { start: { x: 1, y: 1 }, end: { x: 1, y: 1 } }, + ); + expect(host.textContent).toContain("Shift+click"); + }); +}); + +describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => { + /** + * The handler is exercised directly rather than through a rendered terminal. + * + * xterm decides *when* to call it from cell geometry, and jsdom gives every + * element a zero-sized box — so a test driving the mouse over the pane would + * be asserting that jsdom's layout engine exists, not that this app validates + * what it opens. What xterm hands over is the OSC 8 parameter verbatim, which + * is exactly what these arguments are. + */ + const range = { + start: { x: 1, y: 1 }, + end: { x: 80, y: 1 }, + } as unknown as Parameters< + NonNullable["hover"]> + >[2]; + + let host: HTMLDivElement; + let handler: ReturnType; + + beforeEach(() => { + host = document.createElement("div"); + document.body.appendChild(host); + handler = createOsc8LinkHandler(() => host); + }); + + function hoverCard(): HTMLElement | null { + return host.querySelector(`.${OSC8_HOVER_CLASS}`); + } + + it("refuses a target that fails validation, without reaching the opener", () => { + // The visible text can be anything; the parameter is what gets opened, and + // a container is free to put a scheme in it that the host must never hand + // to an OS-level opener. + handler.activate(new MouseEvent("click"), "javascript:alert(1)", range); + handler.activate(new MouseEvent("click"), "file:///etc/passwd", range); + handler.activate( + new MouseEvent("click"), + "https://claude.ai@evil.tld/authorize", + range, + ); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("opens a valid target through the one sink", async () => { + const url = + "https://claude.ai/oauth/authorize?code=true&client_id=abc123&scope=user%3Ainference"; + await act(async () => { + handler.activate(new MouseEvent("click"), url, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(url); + }); + + it("shows the real origin on hover, not the text on screen", () => { + // The point of the affordance. OSC 8 decouples label from target: the row + // can read `https://claude.ai` while the parameter points anywhere. + handler.hover?.( + new MouseEvent("mousemove"), + "https://evil.example.com/claude.ai/oauth/authorize?code=true", + range, + ); + + const card = hoverCard(); + expect(card).not.toBeNull(); + const origin = card!.querySelector('[data-testid="osc8-hover-origin"]'); + expect(origin?.textContent).toBe("https://evil.example.com"); + // Whole origin or nothing — a truncated one is the spoof this prevents. + expect(origin?.textContent).not.toContain("…"); + expect(card!.textContent).not.toContain("https://claude.ai"); + + handler.leave?.(new MouseEvent("mouseout"), "https://evil.example.com/", range); + expect(hoverCard()).toBeNull(); + }); + + it("says so on hover when the target would be refused", () => { + handler.hover?.(new MouseEvent("mousemove"), "javascript:alert(1)", range); + + const card = hoverCard(); + expect(card).not.toBeNull(); + expect(card!.querySelector('[data-testid="osc8-hover-origin"]')).toBeNull(); + // Never echo the rejected target: it is untrusted text on its way to a DOM + // node, and the only thing worth saying is that clicking does nothing. + expect(card!.textContent).not.toContain("javascript:"); + }); + + it("pushes the shared toast when the host opener fails", async () => { + vi.mocked(openUrlExternal).mockRejectedValueOnce(new Error("no opener")); + + await act(async () => { + handler.activate(new MouseEvent("click"), "https://example.com/x", range); + await Promise.resolve(); + }); + + const toasts = useAppState.getState().toasts; + expect(toasts).toHaveLength(1); + expect(toasts[0].kind).toBe("error"); + expect(toasts[0].detail).toContain("no opener"); + // Same card as every other dead-opener report in this view. + expect(toasts[0].dedupeKey).toBe("host-open-failed"); + }); +}); diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 888e1f6..3b989e7 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -1,5 +1,5 @@ import { useCallback, useEffect, useRef, useState } from "react"; -import { Terminal } from "@xterm/xterm"; +import { Terminal, type ILinkHandler } from "@xterm/xterm"; import { FitAddon } from "@xterm/addon-fit"; import { WebglAddon } from "@xterm/addon-webgl"; import { WebLinksAddon } from "@xterm/addon-web-links"; @@ -21,6 +21,7 @@ import { extendsUrl, parseUrlRelayOsc, sanitizeRelayUrl, + urlOrigin, } from "../../lib/urlRelay"; import { classifyDrop, DROP_BLOCKED_TOAST } from "../../lib/dropTarget"; import { useSignInOpenTarget } from "../../hooks/useSignInOpenTarget"; @@ -106,6 +107,205 @@ export function supersedes( return extendsUrl(next.url, current.url); } +/** + * Class xterm requires on an element that floats over the terminal. + * + * Not decoration: `ILinkHandler.hover`'s contract is that the hover element + * lives inside `Terminal.element` and carries this class, otherwise xterm's + * own hit-testing does not know to stop at it and mouse events fall through to + * whatever link is underneath. + */ +export const OSC8_HOVER_CLASS = "xterm-hover"; + +/** + * Report a failed handoff to the host's browser. + * + * One sink, one card. See the long note on `handleOpenUrl` for what this catch + * does *not* catch on Linux; a click that appears to do nothing is the + * complaint either way, so every route that opens a URL says the same thing in + * the same place. + */ +function reportOpenFailure(e: unknown) { + useAppState.getState().pushToast({ + kind: "error", + message: "Could not open that link in your browser", + detail: String(e), + // A dead opener fails for every link in the buffer. One card. + dedupeKey: "host-open-failed", + }); +} + +/** + * Makes OSC 8 hyperlinks clickable, and shows where they actually go. + * + * ## Why xterm's own link matching is not enough + * + * `WebLinksAddon` matches *rendered text*, row by row. Claude Code prints its + * links as OSC 8 hyperlinks whose visible text is hard-wrapped into + * terminal-width pieces — measured against 2.1.226, a 346-character sign-in + * URL arrives as five emissions, each carrying the whole URL in its OSC 8 + * parameter and about 80 characters of it on screen (see `lib/urlDetector.ts`, + * which had to grow the same second branch). So the addon matches a fragment + * or nothing at all, which is the entire reason the URL toast exists. xterm + * hands `linkHandler` the complete parameter instead, however the label was + * sliced, so this covers exactly the case the addon cannot — and the addon + * stays, because it covers the plain-text URLs in ordinary shell output that + * carry no OSC 8 at all. + * + * ## The gesture is Shift+click (Option+click on macOS) + * + * Claude holds mouse tracking (`?1000`/`?1002`/`?1003`), and xterm's mousedown + * handler cancels the event before the link layer whenever tracking is on — + * *unless* `shouldForceSelection(e)` is true, which is `e.shiftKey`, or + * `e.altKey` on macOS with `macOptionClickForcesSelection` set (this view sets + * it). So the modifier that already exists for selecting text is the one that + * reaches a link, and no new key handling is involved. A plain click keeps + * going to the program, which is what a TUI needs. + * + * ## The hover card is the security half, not a nicety + * + * OSC 8 fully decouples the visible text from the target: the container can + * print `https://claude.ai` and link it anywhere. That is strictly worse than + * the userinfo spoofing `sanitizeRelayUrl` already rejects, because here + * nothing in the painted row is even *derived* from the destination. So the + * origin of the real target is shown before the user commits, the same way the + * URL toast shows it and for the same reason ({@link urlOrigin}'s note): the + * origin decides where the user's credentials end up, so it is rendered in + * full and the *remainder* is the only part an ellipsis may eat. + * + * The card sits at the bottom of the pane rather than beside the pointer — + * where a browser puts it, and never underneath the cursor, so it cannot + * flicker the link out from under the hover that summoned it. + * + * @param getHost returns `Terminal.element`, which does not exist until + * `term.open()` has run — hence a getter rather than the element. + */ +/** + * The modifier that opens an OSC 8 link, named the way the user's platform + * names it. + * + * This is not our choice and it must not drift: xterm only lets a click reach + * the link layer while a program holds the mouse when its own + * `shouldForceSelection` says so, and that is + * `isMac ? e.altKey && macOptionClickForcesSelection : e.shiftKey`. The list + * below is xterm's `isMac` verbatim (`common/Platform.ts`) so the hint cannot + * disagree with the behaviour it describes — a hint that names the wrong key + * is worse than no hint, because the user concludes the link is broken. + * + * `macOptionClickForcesSelection` is set on the terminal, so the Mac branch is + * live rather than theoretical. + */ +function openModifierLabel(): string { + const platform = typeof navigator === "undefined" ? "" : navigator.platform; + const isMac = ["Macintosh", "MacIntel", "MacPPC", "Mac68K"].includes(platform); + return isMac ? "Option+click to open" : "Shift+click to open"; +} + +export function createOsc8LinkHandler( + getHost: () => HTMLElement | null, +): ILinkHandler { + let card: HTMLDivElement | null = null; + + const clear = () => { + card?.remove(); + card = null; + }; + + const span = (text: string, style: Partial) => { + const el = document.createElement("span"); + el.textContent = text; + Object.assign(el.style, style); + return el; + }; + + return { + activate(_event, text) { + // Same sink and same rule as the WebLinksAddon branch: this came off the + // container's output, so it is validated before it reaches the OS + // opener. One implementation — `sanitizeRelayUrl` — on purpose. + const safe = sanitizeRelayUrl(text); + if (!safe) { + console.warn("Refusing to open a link that failed validation"); + return; + } + openUrlExternal(safe).catch(reportOpenFailure); + }, + + hover(_event, text) { + clear(); + const host = getHost(); + if (!host) return; + + card = document.createElement("div"); + card.className = OSC8_HOVER_CLASS; + card.dataset.testid = "osc8-hover"; + Object.assign(card.style, { + position: "absolute", + left: "8px", + bottom: "8px", + maxWidth: "calc(100% - 16px)", + zIndex: "30", + display: "flex", + alignItems: "baseline", + gap: "6px", + padding: "3px 8px", + fontSize: "12px", + fontFamily: "monospace", + background: "var(--bg-secondary)", + border: "1px solid var(--border-color)", + borderRadius: "6px", + boxShadow: "var(--shadow-overlay)", + color: "var(--text-primary)", + } as Partial); + + const safe = sanitizeRelayUrl(text); + const origin = safe && urlOrigin(safe); + if (!safe || !origin) { + // Nothing of the rejected target is echoed into the DOM — it is + // untrusted text, and the only useful thing to say is that the click + // will not do anything. + card.appendChild( + span("This link will not be opened — it is not a web address", { + color: "var(--text-secondary)", + }), + ); + } else { + const rest = safe.startsWith(origin) ? safe.slice(origin.length) : safe; + const originEl = span(origin, { + fontWeight: "700", + // The part that decides where the credentials go. Never truncated: + // truncating it *is* the spoof. + flexShrink: "0", + overflowWrap: "anywhere", + }); + originEl.dataset.testid = "osc8-hover-origin"; + card.appendChild(originEl); + + const restEl = span(rest, { + color: "var(--text-secondary)", + overflow: "hidden", + textOverflow: "ellipsis", + whiteSpace: "nowrap", + minWidth: "0", + }); + restEl.dataset.testid = "osc8-hover-rest"; + card.appendChild(restEl); + + const hint = span(openModifierLabel(), { + color: "var(--text-secondary)", + flexShrink: "0", + marginLeft: "4px", + }); + card.appendChild(hint); + } + + host.appendChild(card); + }, + + leave: clear, + }; +} + export default function TerminalView({ sessionId, active }: Props) { const containerRef = useRef(null); const terminalContainerRef = useRef(null); @@ -410,7 +610,10 @@ export default function TerminalView({ sessionId, active }: Props) { useEffect(() => { if (!containerRef.current) return; - const term = new Terminal({ + // Annotated because `linkHandler` below refers to `term` (for the element + // it must anchor its hover card to, which does not exist until + // `term.open()`), and TypeScript cannot infer a type it is already using. + const term: Terminal = new Terminal({ cursorBlink: true, fontSize: 14, // Let the user select text even while a program holds the mouse. @@ -420,6 +623,11 @@ export default function TerminalView({ sessionId, active }: Props) { // the only way to copy from a mouse-driven TUI is to take the mouse back // first. `SelectionService.shouldForceSelection`. macOptionClickForcesSelection: true, + // OSC 8 hyperlinks — the form Claude Code prints its links in, and the + // one `WebLinksAddon` structurally cannot match. See + // `createOsc8LinkHandler`, including why the gesture is the same + // Shift/Option the line above is about. + linkHandler: createOsc8LinkHandler(() => term.element ?? null), fontFamily: "'JetBrains Mono', 'Fira Code', 'Cascadia Code', Menlo, Monaco, monospace", theme: { background: "#0d1117", @@ -454,27 +662,24 @@ export default function TerminalView({ sessionId, active }: Props) { // eslint-disable-next-line no-control-regex const urlRegex = /https?:\/\/[^\s'"`<>\x00-\x20\x7f]+/; const webLinksAddon = new WebLinksAddon((_event, uri) => { - // Same sink, same rule: what xterm matched came off the container's - // output, so it is validated before it reaches the OS opener. A click - // here is a deliberate act on visible text, but "visible" is exactly - // what a userinfo-spoofed URL subverts. + // Same sink, same rule as `createOsc8LinkHandler`: what xterm matched + // came off the container's output, so it is validated before it reaches + // the OS opener. + // + // This branch is the one where the click really is an act on visible + // text — the match *is* the painted characters — so the spoof it has to + // survive is a userinfo-spoofed URL, which `sanitizeRelayUrl` rejects. + // An OSC 8 link is not like that at all: its label and its target are + // unrelated strings, which is why that handler shows the target's origin + // on hover before a click can happen. Neither branch replaces the other: + // this one covers plain-text URLs in ordinary shell output, which carry + // no hyperlink parameter for xterm to hand over. const safe = sanitizeRelayUrl(uri); if (!safe) { console.warn("Refusing to open a link that failed validation"); return; } - // Same failure reporting as the toast's Open button — see the long note - // on `handleOpenUrl`, including what this catch does *not* catch on - // Linux. A click that appears to do nothing is the complaint either way. - openUrlExternal(safe).catch((e) => - useAppState.getState().pushToast({ - kind: "error", - message: "Could not open that link in your browser", - detail: String(e), - // A dead opener fails for every link in the buffer. One card. - dedupeKey: "host-open-failed", - }), - ); + openUrlExternal(safe).catch(reportOpenFailure); }, { urlRegex }); term.loadAddon(webLinksAddon); From ac50c388911b2d3c06c9f2d16fac253e5ada44d2 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Fri, 18 Sep 2026 19:45:11 -0700 Subject: [PATCH 2/5] fix: gate OSC 8 link activation instead of merely hinting at it Review of this branch found its central premise was false. The claim was that xterm cancels a mousedown before the link layer while a program holds the mouse, so only a Shift+click could reach a link. None of that holds: `cancel()` is `if (this.options.cancelEvents || force)` and `cancelEvents` defaults to false and is never set here, so it does nothing; the mouse reporting listeners bind to `.xterm` while the Linkifier is constructed on `screenElement`, a descendant, so the link layer sees the event first regardless; and `_handleMouseUp` checks neither the modifier nor the button before calling `activate`. So a plain click opened the link, and so did a right-click. That is not a missing convenience. OSC 8 lets the container wrap any clickable TUI widget -- a menu row, a "1. Yes", a file chip -- in a link to anywhere, and because the mouse report still reaches the program afterwards the widget responds too and nothing looks wrong. The hover card was the only mitigation, and it assumes a user deliberately reaching for a link. `opensOnClick` is now a real gate: primary button only, and while a program tracks the mouse the force-selection modifier is required -- the gesture the user already has for "this click is for the terminal, not the program". With nothing tracking, a bare click opens, which is what WebLinksAddon already does for plain-text URLs in the same buffer. The mode is read per click through a getter rather than captured, and `syncMouseCapture` and the gate share one expression, because a gate that disagreed with the badge would be the hole again. The gate and the hint also share one modifier predicate, and the hint is conditional on tracking, so it can never name a key that does nothing. Three more from the same review. The origin span had `flexShrink: 0`, which beats `overflowWrap` under flexbox, so an attacker-controlled 600-character origin ran off the pane and hid the registrable domain -- the same spoof as an ellipsis, without one; it now wraps and the remainder is what gives way. The card had no `pointerEvents: none`, and `xterm-hover` is inert at this placement, so a card under the pointer took `mouseleave` from screenElement and made bottom-row links flicker and refuse to activate at all. And the design doc comment had come adrift from its function. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/terminal/TerminalView.test.tsx | 297 ++++++++++++++++-- app/src/components/terminal/TerminalView.tsx | 216 ++++++++++--- 2 files changed, 438 insertions(+), 75 deletions(-) diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 3d4c1d2..49d7acc 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -45,6 +45,33 @@ const ptyOutput = vi.hoisted(() => ({ listeners: new Map void>(), })); +/** + * What `TerminalView` actually handed the `Terminal` constructor, and the + * instances it built. + * + * The real xterm is kept — these tests depend on its parser, its modes and its + * DOM — and only the constructor is wrapped, because the wiring of + * `linkHandler` is otherwise unobservable from outside: xterm decides when to + * call it from cell geometry that jsdom has no layout for, so deleting the + * `linkHandler:` line changed nothing any assertion could see. + */ +const xterm = vi.hoisted(() => ({ + options: null as Record | null, + instances: [] as unknown[], +})); + +vi.mock("@xterm/xterm", async (importOriginal) => { + const actual = await importOriginal(); + class SpyTerminal extends actual.Terminal { + constructor(options?: ConstructorParameters[0]) { + super(options); + xterm.options = (options ?? null) as Record | null; + xterm.instances.push(this); + } + } + return { ...actual, Terminal: SpyTerminal }; +}); + /** * Shift+Enter has to reach the container as ESC+CR. * @@ -161,6 +188,8 @@ beforeEach(() => { useAppState.setState({ toasts: [] }); document.body.innerHTML = ""; useAppState.setState({ sessions: [] }); + xterm.options = null; + xterm.instances.length = 0; }); afterEach(() => { @@ -1085,29 +1114,42 @@ describe("the hover hint names the key that actually works", () => { const original = navigator.platform; afterEach(() => platform(original)); - // xterm gates this on its own `isMac`; if the hint and the gate disagree the - // user is told to press a key that does nothing. + const hoverHint = (tracking: boolean): string => { + const host = document.createElement("div"); + createOsc8LinkHandler( + () => host, + () => tracking, + ).hover?.(new MouseEvent("mousemove"), "https://example.com/x", { + start: { x: 1, y: 1 }, + end: { x: 1, y: 1 }, + }); + return host.textContent ?? ""; + }; + + // The hint and the gate read one predicate; these pin that they cannot + // drift, because a hint naming a key the gate does not accept is the bug + // that was already fixed once on this branch. it("says Option on a Mac, because that is xterm's force-selection modifier there", () => { platform("MacIntel"); - const host = document.createElement("div"); - createOsc8LinkHandler(() => host).hover?.( - new MouseEvent("mousemove"), - "https://example.com/x", - { start: { x: 1, y: 1 }, end: { x: 1, y: 1 } }, - ); - expect(host.textContent).toContain("Option+click"); - expect(host.textContent).not.toContain("Shift+click"); + expect(hoverHint(true)).toContain("Option+click"); + expect(hoverHint(true)).not.toContain("Shift+click"); }); it("says Shift everywhere else", () => { platform("Linux x86_64"); - const host = document.createElement("div"); - createOsc8LinkHandler(() => host).hover?.( - new MouseEvent("mousemove"), - "https://example.com/x", - { start: { x: 1, y: 1 }, end: { x: 1, y: 1 } }, - ); - expect(host.textContent).toContain("Shift+click"); + expect(hoverHint(true)).toContain("Shift+click"); + }); + + // No program holds the mouse, so no modifier is needed — and naming one + // would tell the user to press a key the gate ignores. + it("names no modifier at all while nothing is tracking the mouse", () => { + platform("Linux x86_64"); + const hint = hoverHint(false); + expect(hint).toContain("Click to open"); + expect(hint).not.toContain("Shift+click"); + + platform("MacIntel"); + expect(hoverHint(false)).not.toContain("Option+click"); }); }); @@ -1130,28 +1172,35 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => let host: HTMLDivElement; let handler: ReturnType; + /** What the terminal's live mouse-tracking mode says, per test. */ + let tracking: boolean; beforeEach(() => { host = document.createElement("div"); document.body.appendChild(host); - handler = createOsc8LinkHandler(() => host); + tracking = false; + handler = createOsc8LinkHandler( + () => host, + () => tracking, + ); }); + afterEach(() => host.remove()); + function hoverCard(): HTMLElement | null { return host.querySelector(`.${OSC8_HOVER_CLASS}`); } + const click = (init: MouseEventInit = {}) => + new MouseEvent("click", { button: 0, ...init }); + it("refuses a target that fails validation, without reaching the opener", () => { // The visible text can be anything; the parameter is what gets opened, and // a container is free to put a scheme in it that the host must never hand // to an OS-level opener. - handler.activate(new MouseEvent("click"), "javascript:alert(1)", range); - handler.activate(new MouseEvent("click"), "file:///etc/passwd", range); - handler.activate( - new MouseEvent("click"), - "https://claude.ai@evil.tld/authorize", - range, - ); + handler.activate(click(), "javascript:alert(1)", range); + handler.activate(click(), "file:///etc/passwd", range); + handler.activate(click(), "https://claude.ai@evil.tld/authorize", range); expect(openUrlExternal).not.toHaveBeenCalled(); }); @@ -1160,13 +1209,75 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => const url = "https://claude.ai/oauth/authorize?code=true&client_id=abc123&scope=user%3Ainference"; await act(async () => { - handler.activate(new MouseEvent("click"), url, range); + handler.activate(click(), url, range); await Promise.resolve(); }); expect(openUrlExternal).toHaveBeenCalledWith(url); }); + describe("the gate on activation", () => { + const URL = "https://example.com/x"; + + /** + * The attack this gate exists for. + * + * xterm's mouse-reporting mousedown does *not* cancel anything — + * `cancelEvents` defaults to false — and the Linkifier is a descendant of + * the element those listeners are bound to, so the link layer sees every + * click first and `_handleMouseUp` activates with no modifier, button or + * mode check of its own. A TUI widget the user is meant to click can + * therefore be wrapped in an OSC 8 pointing anywhere, and a plain click + * opens the host browser on it while the mouse report still reaches the + * program, so nothing looks wrong. The modifier is the only thing that + * separates "I clicked the menu item" from "I asked to leave the app". + */ + it("refuses a plain click while a program is tracking the mouse", () => { + tracking = true; + + handler.activate(click(), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("opens on the force-selection modifier while tracking", async () => { + tracking = true; + + await act(async () => { + handler.activate(click({ shiftKey: true }), URL, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + + it("opens on a plain click when nothing holds the mouse", async () => { + // A normal shell. This is what `WebLinksAddon` does for the plain-text + // URLs in the same buffer, and asking for a modifier here would read as + // a broken link. + tracking = false; + + await act(async () => { + handler.activate(click(), URL, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + + it("ignores every button but the primary one", () => { + // Right-click is the context menu this pane already binds; middle-click + // is paste. Neither is a request to leave the app. + tracking = false; + + handler.activate(click({ button: 2 }), URL, range); + handler.activate(click({ button: 1 }), URL, range); + handler.activate(click({ button: 2, shiftKey: true }), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + }); + it("shows the real origin on hover, not the text on screen", () => { // The point of the affordance. OSC 8 decouples label from target: the row // can read `https://claude.ai` while the parameter points anywhere. @@ -1180,14 +1291,49 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => expect(card).not.toBeNull(); const origin = card!.querySelector('[data-testid="osc8-hover-origin"]'); expect(origin?.textContent).toBe("https://evil.example.com"); - // Whole origin or nothing — a truncated one is the spoof this prevents. - expect(origin?.textContent).not.toContain("…"); expect(card!.textContent).not.toContain("https://claude.ai"); handler.leave?.(new MouseEvent("mouseout"), "https://evil.example.com/", range); expect(hoverCard()).toBeNull(); }); + it("keeps a very long origin whole, and gives way in the remainder instead", () => { + // The attacker picks the origin's length. `https://claude.ai.<300 a's> + // .evil.tld/` parses, passes every `sanitizeRelayUrl` rule, and under a + // non-shrinking flex item runs off the right edge of the pane — which + // hides the registrable domain just as effectively as an ellipsis would. + const origin = `https://claude.ai.${"a".repeat(300)}.${"b".repeat(200)}.evil.tld`; + handler.hover?.(new MouseEvent("mousemove"), `${origin}/oauth?code=1`, range); + + const originEl = hoverCard()!.querySelector( + '[data-testid="osc8-hover-origin"]', + )!; + // Whole origin or nothing: every character is in the DOM... + expect(originEl.textContent).toBe(origin); + // ...and it is allowed to wrap rather than be clipped or pushed off-pane. + expect(originEl.style.flexShrink).not.toBe("0"); + expect(originEl.style.whiteSpace).not.toBe("nowrap"); + expect(originEl.style.overflowWrap).toBe("anywhere"); + + // The truncatable half is the remainder, and only the remainder. + const restEl = hoverCard()!.querySelector( + '[data-testid="osc8-hover-rest"]', + )!; + expect(restEl.style.textOverflow).toBe("ellipsis"); + expect(restEl.style.whiteSpace).toBe("nowrap"); + }); + + it("cannot take the pointer away from the link that summoned it", () => { + // The card is appended to `Terminal.element`, a *sibling* of the + // `screenElement` the Linkifier listens on, so `xterm-hover` buys nothing + // here: a card under the pointer means `mouseleave` on screenElement, the + // card is torn down, and the mouseup that would activate the link lands on + // the card instead of the terminal. + handler.hover?.(new MouseEvent("mousemove"), "https://example.com/x", range); + + expect(hoverCard()!.style.pointerEvents).toBe("none"); + }); + it("says so on hover when the target would be refused", () => { handler.hover?.(new MouseEvent("mousemove"), "javascript:alert(1)", range); @@ -1199,11 +1345,37 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => expect(card!.textContent).not.toContain("javascript:"); }); + it("does not call a refused web address something other than a web address", () => { + // `https://claude.ai@evil.tld/` is a perfectly good URL; it is refused + // because the userinfo makes the visible host a lie. Telling the user it + // "is not a web address" is false, and a false explanation teaches them to + // distrust the card. + handler.hover?.( + new MouseEvent("mousemove"), + "https://claude.ai@evil.tld/authorize", + range, + ); + + expect(hoverCard()!.textContent).not.toContain("not a web address"); + }); + + it("drops a stale card when the pane is no longer on screen", () => { + // `leave` only ever arrives from the Linkifier's `_clearCurrentLink`, and + // switching tabs from the keyboard moves no pointer: without this, the + // card is still sitting there when the user comes back. + handler.hover?.(new MouseEvent("mousemove"), "https://example.com/x", range); + expect(hoverCard()).not.toBeNull(); + + handler.dismiss(); + + expect(hoverCard()).toBeNull(); + }); + it("pushes the shared toast when the host opener fails", async () => { vi.mocked(openUrlExternal).mockRejectedValueOnce(new Error("no opener")); await act(async () => { - handler.activate(new MouseEvent("click"), "https://example.com/x", range); + handler.activate(click(), "https://example.com/x", range); await Promise.resolve(); }); @@ -1215,3 +1387,70 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => expect(toasts[0].dedupeKey).toBe("host-open-failed"); }); }); + +describe("the link handler is wired into the terminal, and reads its live mode", () => { + const range = { + start: { x: 1, y: 1 }, + end: { x: 80, y: 1 }, + } as unknown as Parameters< + NonNullable["hover"]> + >[2]; + + /** What the mounted view passed as `linkHandler`. */ + function wiredHandler() { + const handler = xterm.options?.linkHandler as + | ReturnType + | undefined; + if (!handler) throw new Error("no linkHandler was passed to Terminal"); + return handler; + } + + /** Feed the terminal a DECSET the way the container would. */ + async function write(data: string) { + const term = xterm.instances.at(-1) as { write(d: string, cb: () => void): void }; + await act( + () => new Promise((resolve) => term.write(data, resolve)), + ); + } + + it("passes one at all — without it OSC 8 links are inert", () => { + mountSession("claude"); + + const handler = wiredHandler(); + expect(typeof handler.activate).toBe("function"); + expect(typeof handler.hover).toBe("function"); + }); + + // The gate has to ask the terminal, not a boolean captured at construction: + // the mode changes whenever the container prints a DECSET, which is several + // times a second in Claude Code. + it("refuses a plain click once the container turns mouse tracking on", async () => { + mountSession("claude"); + await write("\x1b[?1002h"); + + wiredHandler().activate( + new MouseEvent("click", { button: 0 }), + "https://example.com/x", + range, + ); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("opens again once the container gives the mouse back", async () => { + mountSession("claude"); + await write("\x1b[?1002h"); + await write("\x1b[?1002l"); + + await act(async () => { + wiredHandler().activate( + new MouseEvent("click", { button: 0 }), + "https://example.com/x", + range, + ); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith("https://example.com/x"); + }); +}); diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 3b989e7..7a45a74 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -108,12 +108,14 @@ export function supersedes( } /** - * Class xterm requires on an element that floats over the terminal. + * Marks the hover card, for xterm's stylesheet and for the tests. * - * Not decoration: `ILinkHandler.hover`'s contract is that the hover element - * lives inside `Terminal.element` and carries this class, otherwise xterm's - * own hit-testing does not know to stop at it and mouse events fall through to - * whatever link is underneath. + * It does *not* make xterm route pointer events around the card. xterm only + * consults this class inside `Linkifier._handleMouseMove`, which is registered + * on `screenElement`; the card is appended to `Terminal.element`, a *sibling* + * of that node, so the check never sees it. What keeps the card out of the way + * is `pointerEvents: "none"` on the card itself — see `hover` below for what + * goes wrong without it. */ export const OSC8_HOVER_CLASS = "xterm-hover"; @@ -135,6 +137,91 @@ function reportOpenFailure(e: unknown) { }); } +/** + * `ILinkHandler`, plus the one thing xterm never asks for. + * + * `leave` is only ever reached through `Linkifier._clearCurrentLink`, i.e. a + * pointer that moved. Switching tabs from the keyboard moves no pointer and + * the Linkifier's dispose path does not clear either, so the card outlives the + * pane and is still there when the user comes back. {@link dismiss} is how the + * view says "this pane is gone" without pretending to be a mouse event. + */ +export type Osc8LinkHandler = ILinkHandler & { dismiss(): void }; + +/** + * Is a program holding the mouse? + * + * One expression, two readers that must never disagree: the status-bar badge + * (`syncMouseCapture`) and the gate on opening a link ({@link opensOnClick}). + * A gate that thought tracking was off while the badge said it was on would be + * the whole security hole back again. + */ +function terminalTracksMouse(term: Terminal): boolean { + return term.modes.mouseTrackingMode !== "none"; +} + +/** xterm's `isMac` verbatim (`common/Platform.ts`), so we split where it does. */ +function isMacPlatform(): boolean { + const platform = typeof navigator === "undefined" ? "" : navigator.platform; + return ["Macintosh", "MacIntel", "MacPPC", "Mac68K"].includes(platform); +} + +/** + * xterm's `SelectionService.shouldForceSelection`, mirrored. + * + * The modifier is not our choice and it must not drift: while a program holds + * the mouse, this is the one gesture the user already has for "this click is + * for the terminal, not for the program", so it is the gesture that may open a + * link. xterm's rule is + * `isMac ? e.altKey && macOptionClickForcesSelection : e.shiftKey`, and this + * view sets `macOptionClickForcesSelection`, so the Mac branch is live rather + * than theoretical. + * + * The gate and the hint below both call this. A hint that names a key the gate + * does not accept is worse than no hint — the user concludes the link is + * broken — and that is a bug this branch has already shipped once, so the two + * are not allowed separate answers. + */ +function forcesSelection(event: { altKey: boolean; shiftKey: boolean }): boolean { + return isMacPlatform() ? event.altKey : event.shiftKey; +} + +/** + * What the card tells the user to do, for the state the terminal is in *now*. + * + * Conditional because the gesture is: with no program tracking the mouse a + * plain click opens the link, and naming a modifier then would send the user + * hunting for a key that changes nothing. + */ +function openHintLabel(mouseTracking: boolean): string { + if (!mouseTracking) return "Click to open"; + return isMacPlatform() ? "Option+click to open" : "Shift+click to open"; +} + +/** + * Whether this click is a request to leave the app for the host browser. + * + * Two refusals, for two different mistakes: + * + * - Anything but the primary button. xterm's `Linkifier._handleMouseUp` + * checks neither the button nor the mouse mode, so without this a + * *right*-click activates the link as well as opening this pane's context + * menu, and a middle-click paste opens it too. + * - A plain click while a program is tracking the mouse. That is the + * dangerous one: OSC 8 lets the container wrap any clickable TUI widget — + * a menu row, a "1. Yes", a file chip — in a link to anywhere, and because + * the mouse report still reaches the program the widget also responds, so + * nothing looks wrong. Requiring the force-selection modifier there makes + * the two intents distinguishable. + * + * With nothing tracking the mouse a bare click is correct and expected: it is + * what `WebLinksAddon` does for the plain-text URLs in the same buffer. + */ +function opensOnClick(event: MouseEvent, mouseTracking: boolean): boolean { + if (event.button !== 0) return false; + return !mouseTracking || forcesSelection(event); +} + /** * Makes OSC 8 hyperlinks clickable, and shows where they actually go. * @@ -152,15 +239,21 @@ function reportOpenFailure(e: unknown) { * stays, because it covers the plain-text URLs in ordinary shell output that * carry no OSC 8 at all. * - * ## The gesture is Shift+click (Option+click on macOS) + * ## xterm applies no gate of its own, so this one does * - * Claude holds mouse tracking (`?1000`/`?1002`/`?1003`), and xterm's mousedown - * handler cancels the event before the link layer whenever tracking is on — - * *unless* `shouldForceSelection(e)` is true, which is `e.shiftKey`, or - * `e.altKey` on macOS with `macOptionClickForcesSelection` set (this view sets - * it). So the modifier that already exists for selecting text is the one that - * reaches a link, and no new key handling is involved. A plain click keeps - * going to the program, which is what a TUI needs. + * There is a tempting story in which xterm's mouse-reporting mousedown cancels + * the event before the link layer sees it, leaving only the force-selection + * modifier a way through. It is false in both halves. That branch calls + * `cancel(e)`, which is a no-op unless `cancelEvents` is set and it defaults to + * false; and the mouse-reporting listeners are bound on `Terminal.element` + * while the Linkifier is bound on `screenElement`, a descendant, so bubbling + * reaches the link first no matter what. `Linkifier._handleMouseUp` then + * activates the link with no check on the button, the modifier or the mouse + * mode. + * + * So the gate is {@link opensOnClick}, applied in `activate`, and the mouse + * mode it asks about is read from the terminal on every click rather than + * captured — the container changes it whenever it likes. * * ## The hover card is the security half, not a nicety * @@ -179,31 +272,14 @@ function reportOpenFailure(e: unknown) { * * @param getHost returns `Terminal.element`, which does not exist until * `term.open()` has run — hence a getter rather than the element. + * @param isMouseTracking answers "is a program holding the mouse right now?" + * — a getter for the same reason, and the *only* reason: the answer + * changes under us. */ -/** - * The modifier that opens an OSC 8 link, named the way the user's platform - * names it. - * - * This is not our choice and it must not drift: xterm only lets a click reach - * the link layer while a program holds the mouse when its own - * `shouldForceSelection` says so, and that is - * `isMac ? e.altKey && macOptionClickForcesSelection : e.shiftKey`. The list - * below is xterm's `isMac` verbatim (`common/Platform.ts`) so the hint cannot - * disagree with the behaviour it describes — a hint that names the wrong key - * is worse than no hint, because the user concludes the link is broken. - * - * `macOptionClickForcesSelection` is set on the terminal, so the Mac branch is - * live rather than theoretical. - */ -function openModifierLabel(): string { - const platform = typeof navigator === "undefined" ? "" : navigator.platform; - const isMac = ["Macintosh", "MacIntel", "MacPPC", "Mac68K"].includes(platform); - return isMac ? "Option+click to open" : "Shift+click to open"; -} - export function createOsc8LinkHandler( getHost: () => HTMLElement | null, -): ILinkHandler { + isMouseTracking: () => boolean, +): Osc8LinkHandler { let card: HTMLDivElement | null = null; const clear = () => { @@ -219,7 +295,10 @@ export function createOsc8LinkHandler( }; return { - activate(_event, text) { + activate(event, text) { + // Opening the host browser is the one thing in this pane the container + // must not be able to provoke on its own. See `opensOnClick`. + if (!opensOnClick(event, isMouseTracking())) return; // Same sink and same rule as the WebLinksAddon branch: this came off the // container's output, so it is validated before it reaches the OS // opener. One implementation — `sanitizeRelayUrl` — on purpose. @@ -244,7 +323,16 @@ export function createOsc8LinkHandler( left: "8px", bottom: "8px", maxWidth: "calc(100% - 16px)", + boxSizing: "border-box", zIndex: "30", + // The card lands under the pointer for a link in the bottom rows, and + // it is not a sibling the Linkifier hit-tests around (see + // `OSC8_HOVER_CLASS`). Without this, `screenElement` gets `mouseleave` + // the moment the card appears — card removed, pointer back on the + // link, card back: a flicker loop — and worse, the `mouseup` that + // activates the link lands on the card, so the link cannot be opened + // at all. Nothing here is interactive, so nothing is lost. + pointerEvents: "none", display: "flex", alignItems: "baseline", gap: "6px", @@ -253,6 +341,9 @@ export function createOsc8LinkHandler( fontFamily: "monospace", background: "var(--bg-secondary)", border: "1px solid var(--border-color)", + // The origin wraps, so the card grows downward rather than sideways; + // this is the backstop for anything that still cannot fit. + overflow: "hidden", borderRadius: "6px", boxShadow: "var(--shadow-overlay)", color: "var(--text-primary)", @@ -263,9 +354,12 @@ export function createOsc8LinkHandler( if (!safe || !origin) { // Nothing of the rejected target is echoed into the DOM — it is // untrusted text, and the only useful thing to say is that the click - // will not do anything. + // will not do anything. Deliberately not "it is not a web address": + // `https://claude.ai@evil.tld/` and an over-length URL both are one, + // and a card that explains a refusal wrongly teaches the user to + // distrust the card. card.appendChild( - span("This link will not be opened — it is not a web address", { + span("This link will not be opened — it failed the URL safety check", { color: "var(--text-secondary)", }), ); @@ -273,10 +367,19 @@ export function createOsc8LinkHandler( const rest = safe.startsWith(origin) ? safe.slice(origin.length) : safe; const originEl = span(origin, { fontWeight: "700", - // The part that decides where the credentials go. Never truncated: - // truncating it *is* the spoof. - flexShrink: "0", + // The part that decides where the credentials go, so all of it is + // shown: truncating it *is* the spoof, and so is pushing its tail + // off the right edge of the pane. The attacker picks the length — + // `https://claude.ai.<300 chars>.evil.tld` parses and passes every + // `sanitizeRelayUrl` rule — so "do not shrink" is not enough: + // `flex-shrink: 0` pins a flex item at its max-content width and the + // text never wraps, it just overflows. It wraps instead, onto as + // many lines as it needs, and the truncatable remainder below is the + // thing that gives way. + flexShrink: "1", + minWidth: "0", overflowWrap: "anywhere", + whiteSpace: "normal", }); originEl.dataset.testid = "osc8-hover-origin"; card.appendChild(originEl); @@ -291,7 +394,7 @@ export function createOsc8LinkHandler( restEl.dataset.testid = "osc8-hover-rest"; card.appendChild(restEl); - const hint = span(openModifierLabel(), { + const hint = span(openHintLabel(isMouseTracking()), { color: "var(--text-secondary)", flexShrink: "0", marginLeft: "4px", @@ -303,6 +406,8 @@ export function createOsc8LinkHandler( }, leave: clear, + + dismiss: clear, }; } @@ -312,6 +417,9 @@ export default function TerminalView({ sessionId, active }: Props) { const termRef = useRef(null); const fitRef = useRef(null); const webglRef = useRef(null); + // Held only so the hover card can be taken down when this pane leaves the + // screen — see `Osc8LinkHandler.dismiss`. + const osc8LinkHandlerRef = useRef(null); const detectorRef = useRef(null); const { sendInput, pasteImage, resize, onOutput, onExit } = useTerminal(); const gpuRenderingSetting = useAppState(s => s.appSettings?.terminal_gpu_rendering ?? null); @@ -578,7 +686,7 @@ export default function TerminalView({ sessionId, active }: Props) { const syncMouseCapture = useCallback(() => { const term = termRef.current; if (!term) return; - const captured = term.modes.mouseTrackingMode !== "none"; + const captured = terminalTracksMouse(term); if (captured === mouseCapturedRef.current) return; mouseCapturedRef.current = captured; setMouseCaptured(captured); @@ -625,9 +733,15 @@ export default function TerminalView({ sessionId, active }: Props) { macOptionClickForcesSelection: true, // OSC 8 hyperlinks — the form Claude Code prints its links in, and the // one `WebLinksAddon` structurally cannot match. See - // `createOsc8LinkHandler`, including why the gesture is the same - // Shift/Option the line above is about. - linkHandler: createOsc8LinkHandler(() => term.element ?? null), + // `createOsc8LinkHandler`, including why opening one while a program + // holds the mouse needs the same Shift/Option the line above is about. + // Both arguments are getters because neither answer exists yet: the + // element arrives with `term.open()`, and the mouse mode changes + // whenever the container prints a DECSET. + linkHandler: (osc8LinkHandlerRef.current = createOsc8LinkHandler( + () => term.element ?? null, + () => terminalTracksMouse(term), + )), fontFamily: "'JetBrains Mono', 'Fira Code', 'Cascadia Code', Menlo, Monaco, monospace", theme: { background: "#0d1117", @@ -968,9 +1082,19 @@ export default function TerminalView({ sessionId, active }: Props) { webglRef.current = null; term.dispose(); termRef.current = null; + osc8LinkHandlerRef.current = null; }; }, [sessionId]); // eslint-disable-line react-hooks/exhaustive-deps + // A hover card only ever clears on a *pointer* leaving the link, so switching + // tabs from the keyboard leaves one hanging over a pane nobody is looking at, + // to be found still there on the way back. Hiding the wrapper does not fire + // `mouseleave`, so nothing else would. + useEffect(() => { + if (active) return; + osc8LinkHandlerRef.current?.dismiss(); + }, [active]); + // Manage WebGL lifecycle and re-fit when tab becomes active. // Only the active terminal holds a WebGL context to avoid exhausting // the browser's limited pool (~8-16 contexts). From 593b8168eb1e57dea7a6e180348179b7a1225e61 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Fri, 18 Sep 2026 20:00:42 -0700 Subject: [PATCH 3/5] fix: a selection is not a request to leave the app Re-review found the gate did not cover the gesture users actually make. xterm's `Linkifier._handleMouseUp` has no click-count check, no distance threshold and no timestamp, so it activates on the mouseup that *ends a selection* as readily as on a click. Double-clicking a word or dragging across a few characters inside an OSC 8 link therefore opened the browser. Worse with a program holding the mouse: the only way to select text there is Shift/Option+drag, which is byte-identical to the gesture the gate accepted as a deliberate request to open. A container wrapping each output row in a link would have harvested every legitimate copy. `term.hasSelection()` is the load-bearing check: a drag is one press and one release, so its click count is 1 and `detail` cannot see it. `detail > 1` is belt-and-braces for the case where the selection came out empty, and for not depending on the selection model being written before the Linkifier's listener runs -- it is, but the check costs nothing. Drag distance was rejected rather than forgotten: xterm hands `activate` only the mouseup, so measuring it means binding our own listener and keeping a second source of truth about one gesture. The hover card's promise is now sticky. The hint was computed once at hover while the gate re-read the mode at mouseup, so a card reading "Shift+click to open" could be on screen while a bare click opened the link. The gate now requires the modifier if either the card asked for it or the live mode does. The same gate is applied to the WebLinksAddon branch, which had none. That also closes a real bypass: `OscLinkProvider` drops non-http(s) OSC 8 targets before `linkHandler` sees them, so a `javascript:` target with an `https://evil.tld` label fell through to WebLinks and opened ungated. What is not closed, and is now recorded rather than papered over: the mouse mode is a permission the container grants itself. It can drop tracking before the pointer arrives and hold it off through the click. The selection and click-count checks hold either way, so the mass-harvest variant is gone, but the real fix needs a signal the container cannot write and this pane does not have one. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/terminal/TerminalView.test.tsx | 362 +++++++++++++++++- app/src/components/terminal/TerminalView.tsx | 219 +++++++++-- 2 files changed, 524 insertions(+), 57 deletions(-) diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 49d7acc..7fce0eb 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -60,6 +60,18 @@ const xterm = vi.hoisted(() => ({ instances: [] as unknown[], })); +/** + * The click handler `TerminalView` hands `WebLinksAddon`. + * + * Captured for the same reason the `Terminal` constructor is: xterm decides + * when to call it from cell geometry jsdom has no layout for, so the only way + * to ask "does the plain-text-URL path apply the same gate as the OSC 8 one?" + * is to hold the function and call it. + */ +const webLinks = vi.hoisted(() => ({ + handler: null as null | ((event: MouseEvent, uri: string) => void), +})); + vi.mock("@xterm/xterm", async (importOriginal) => { const actual = await importOriginal(); class SpyTerminal extends actual.Terminal { @@ -72,6 +84,18 @@ vi.mock("@xterm/xterm", async (importOriginal) => { return { ...actual, Terminal: SpyTerminal }; }); +vi.mock("@xterm/addon-web-links", async (importOriginal) => { + const actual = await importOriginal(); + type Args = ConstructorParameters; + class SpyWebLinksAddon extends actual.WebLinksAddon { + constructor(...args: Args) { + super(...args); + webLinks.handler = (args[0] ?? null) as typeof webLinks.handler; + } + } + return { ...actual, WebLinksAddon: SpyWebLinksAddon }; +}); + /** * Shift+Enter has to reach the container as ESC+CR. * @@ -190,6 +214,7 @@ beforeEach(() => { useAppState.setState({ sessions: [] }); xterm.options = null; xterm.instances.length = 0; + webLinks.handler = null; }); afterEach(() => { @@ -1114,12 +1139,16 @@ describe("the hover hint names the key that actually works", () => { const original = navigator.platform; afterEach(() => platform(original)); - const hoverHint = (tracking: boolean): string => { + const hoverHint = ( + tracking: boolean, + macOptionClickForcesSelection = true, + ): string => { const host = document.createElement("div"); - createOsc8LinkHandler( - () => host, - () => tracking, - ).hover?.(new MouseEvent("mousemove"), "https://example.com/x", { + createOsc8LinkHandler(() => host, () => ({ + mouseTracking: tracking, + hasSelection: false, + macOptionClickForcesSelection, + })).hover?.(new MouseEvent("mousemove"), "https://example.com/x", { start: { x: 1, y: 1 }, end: { x: 1, y: 1 }, }); @@ -1151,6 +1180,18 @@ describe("the hover hint names the key that actually works", () => { platform("MacIntel"); expect(hoverHint(false)).not.toContain("Option+click"); }); + + // `macOptionClickForcesSelection` defaults to false in xterm and this view + // sets it true, so the Mac branch is only live because of that line. If it + // ever goes, Option stops being the force-selection modifier and the gate + // can never pass while a program holds the mouse — so the card must not go + // on naming a key that does nothing. + it("does not promise Option+click when the option behind it is off", () => { + platform("MacIntel"); + const hint = hoverHint(true, false); + expect(hint).not.toContain("Option+click"); + expect(hint).not.toContain("Shift+click"); + }); }); describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => { @@ -1172,17 +1213,30 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => let host: HTMLDivElement; let handler: ReturnType; - /** What the terminal's live mouse-tracking mode says, per test. */ - let tracking: boolean; + /** + * What the terminal answers about itself when the gate asks, per test. + * + * Mutable rather than fixed at construction because both of the first two + * change *under* the handler: the container sets the mouse mode with a + * DECSET, and the selection is whatever the gesture that ended in this + * mouseup left behind. + */ + let state: { + mouseTracking: boolean; + hasSelection: boolean; + macOptionClickForcesSelection: boolean; + }; beforeEach(() => { host = document.createElement("div"); document.body.appendChild(host); - tracking = false; - handler = createOsc8LinkHandler( - () => host, - () => tracking, - ); + state = { + mouseTracking: false, + hasSelection: false, + // What `TerminalView` sets on the real terminal. + macOptionClickForcesSelection: true, + }; + handler = createOsc8LinkHandler(() => host, () => state); }); afterEach(() => host.remove()); @@ -1191,8 +1245,9 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => return host.querySelector(`.${OSC8_HOVER_CLASS}`); } + /** A real single click: one press, one release, `detail` 1. */ const click = (init: MouseEventInit = {}) => - new MouseEvent("click", { button: 0, ...init }); + new MouseEvent("click", { button: 0, detail: 1, ...init }); it("refuses a target that fails validation, without reaching the opener", () => { // The visible text can be anything; the parameter is what gets opened, and @@ -1233,7 +1288,7 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => * separates "I clicked the menu item" from "I asked to leave the app". */ it("refuses a plain click while a program is tracking the mouse", () => { - tracking = true; + state.mouseTracking = true; handler.activate(click(), URL, range); @@ -1241,7 +1296,7 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => }); it("opens on the force-selection modifier while tracking", async () => { - tracking = true; + state.mouseTracking = true; await act(async () => { handler.activate(click({ shiftKey: true }), URL, range); @@ -1255,7 +1310,7 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => // A normal shell. This is what `WebLinksAddon` does for the plain-text // URLs in the same buffer, and asking for a modifier here would read as // a broken link. - tracking = false; + state.mouseTracking = false; await act(async () => { handler.activate(click(), URL, range); @@ -1268,7 +1323,7 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => it("ignores every button but the primary one", () => { // Right-click is the context menu this pane already binds; middle-click // is paste. Neither is a request to leave the app. - tracking = false; + state.mouseTracking = false; handler.activate(click({ button: 2 }), URL, range); handler.activate(click({ button: 1 }), URL, range); @@ -1276,6 +1331,186 @@ describe("createOsc8LinkHandler — clicking a link Claude Code printed", () => expect(openUrlExternal).not.toHaveBeenCalled(); }); + + /** + * Selecting text is not asking to leave the app. + * + * `Linkifier._handleMouseUp` has no `detail` check, no drag threshold and + * no timestamp — it activates whenever the mouseup lands on the same link + * the mousedown did. `SelectionService` is bound on the *document* and the + * Linkifier on `screenElement`, so the selection gesture and the link + * activation both run, the link layer first. Every gesture below is one a + * user makes to *copy* a string, and none of them may open a browser. + */ + describe("a selection gesture is not a click", () => { + it("refuses a double-click, which selects the word under it", () => { + // xterm selects the word on the *mousedown* of the second click, so + // by this mouseup the selection is already there. + state.hasSelection = true; + + handler.activate(click({ detail: 2 }), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("refuses a triple-click, which selects the whole row", () => { + state.hasSelection = true; + + handler.activate(click({ detail: 3 }), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + // The one the click count cannot see: a drag is a single press and a + // single release, so `detail` is 1 throughout. Only the selection it + // left behind distinguishes it from a click. + it("refuses a drag that selected characters, at click count 1", () => { + state.hasSelection = true; + + handler.activate(click(), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + /** + * The worst version, and the reason the modifier alone is not a gate. + * + * While a program holds the mouse, Shift/Option+drag is the *only* way + * to select text at all — so "the deliberate request to leave the app" + * and "I am copying this line" are byte-identical gestures. A container + * that wraps each of its output rows in an OSC 8 turns every legitimate + * copy into a browser open. + */ + it("refuses a force-selection drag while a program holds the mouse", () => { + state.mouseTracking = true; + state.hasSelection = true; + + handler.activate(click({ shiftKey: true }), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + // Belt to the selection check's braces: independent of whether xterm + // managed to select anything (a double-click on trailing whitespace + // selects nothing), a second click is not a first one. + it("refuses a repeat click even when nothing ended up selected", () => { + state.hasSelection = false; + + handler.activate(click({ detail: 2 }), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + }); + + /** + * The card and the gate must not disagree about what the user has to do. + * + * The hint is computed once, when the pointer arrives; the mode it was + * computed from is the container's to change, and `?1002l` takes effect + * synchronously with the write. So a card reading "Shift+click to open" + * can be on screen while the live mode says a bare click is enough — + * which is also the shape of the flicker attack in FINDING 2. The gate + * therefore honours the *stricter* of what was promised and what is true + * now: a modifier the card asked for is still required when the click + * lands. + */ + describe("what the card promised still binds when the click lands", () => { + it("keeps demanding the modifier after the container drops tracking", () => { + state.mouseTracking = true; + handler.hover?.(new MouseEvent("mousemove"), URL, range); + expect(host.textContent).toContain("+click to open"); + + // `?1002l`, mid-hover. + state.mouseTracking = false; + handler.activate(click(), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("still opens on the modifier the card named", async () => { + state.mouseTracking = true; + handler.hover?.(new MouseEvent("mousemove"), URL, range); + state.mouseTracking = false; + + await act(async () => { + handler.activate(click({ shiftKey: true }), URL, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + + it("does not hold a stale demand against the next link", async () => { + state.mouseTracking = true; + handler.hover?.(new MouseEvent("mousemove"), URL, range); + handler.leave?.(new MouseEvent("mouseout"), URL, range); + + // A plain shell now, and a fresh card that says so. + state.mouseTracking = false; + handler.hover?.(new MouseEvent("mousemove"), URL, range); + expect(host.textContent).toContain("Click to open"); + + await act(async () => { + handler.activate(click(), URL, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + }); + + /** + * FINDING 6: the modifier is xterm's, including the option it hangs on. + * + * xterm's rule is `isMac ? altKey && macOptionClickForcesSelection : + * shiftKey`. Hardcoding `altKey` agrees with the app only for as long as + * the app keeps setting that option, and nothing tells you when it stops. + */ + describe("the Mac modifier follows the terminal's own option", () => { + const platform = (value: string) => + Object.defineProperty(navigator, "platform", { + value, + configurable: true, + }); + const original = navigator.platform; + afterEach(() => platform(original)); + + it("opens on Option+click while the option is on", async () => { + platform("MacIntel"); + state.mouseTracking = true; + state.macOptionClickForcesSelection = true; + + await act(async () => { + handler.activate(click({ altKey: true }), URL, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + + it("refuses Option+click when the terminal does not treat it as force-select", () => { + platform("MacIntel"); + state.mouseTracking = true; + state.macOptionClickForcesSelection = false; + + handler.activate(click({ altKey: true }), URL, range); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("ignores the option off a Mac, where Shift is the modifier", async () => { + platform("Linux x86_64"); + state.mouseTracking = true; + state.macOptionClickForcesSelection = false; + + await act(async () => { + handler.activate(click({ shiftKey: true }), URL, range); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + }); }); it("shows the real origin on hover, not the text on screen", () => { @@ -1454,3 +1689,96 @@ describe("the link handler is wired into the terminal, and reads its live mode", expect(openUrlExternal).toHaveBeenCalledWith("https://example.com/x"); }); }); + +/** + * FINDING 4: the sibling path opens the same browser. + * + * `WebLinksAddon` matches rendered text and activates through the same + * `Linkifier._handleMouseUp`, with the same absence of any check. It also + * picks up links the OSC 8 handler never sees: `OscLinkProvider` drops a + * non-http(s) hyperlink target before `linkHandler` is reached, which leaves + * the addon free to match the *label* — so an OSC 8 with a `javascript:` + * target and an `https://evil.tld/x` label arrives here and nowhere else. + * Both routes end at `openUrlExternal`, so both ask the same question first. + */ +describe("the plain-text URL path is gated the same way", () => { + function webLinksHandler() { + if (!webLinks.handler) throw new Error("no handler was passed to WebLinksAddon"); + return webLinks.handler; + } + + function term() { + return xterm.instances.at(-1) as unknown as { + write(d: string, cb: () => void): void; + select(column: number, row: number, length: number): void; + }; + } + + async function write(data: string) { + await act(() => new Promise((resolve) => term().write(data, resolve))); + } + + const click = (init: MouseEventInit = {}) => + new MouseEvent("click", { button: 0, detail: 1, ...init }); + const URL = "https://example.com/x"; + + it("opens on a plain click in an ordinary shell", async () => { + mountSession("bash"); + + await act(async () => { + webLinksHandler()(click(), URL); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + + it("refuses a plain click while a program holds the mouse", async () => { + mountSession("claude"); + await write("\x1b[?1002h"); + + webLinksHandler()(click(), URL); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("opens on the force-selection modifier while tracking", async () => { + mountSession("claude"); + await write("\x1b[?1002h"); + + await act(async () => { + webLinksHandler()(click({ shiftKey: true }), URL); + await Promise.resolve(); + }); + + expect(openUrlExternal).toHaveBeenCalledWith(URL); + }); + + it("refuses the mouseup that ended a selection", async () => { + mountSession("bash"); + await write("https://example.com/x"); + await act(async () => { + term().select(0, 0, 5); + }); + + webLinksHandler()(click(), URL); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("refuses a repeat click", async () => { + mountSession("bash"); + + webLinksHandler()(click({ detail: 2 }), URL); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); + + it("still refuses a target that fails validation", async () => { + mountSession("bash"); + + webLinksHandler()(click(), "https://claude.ai@evil.tld/authorize"); + + expect(openUrlExternal).not.toHaveBeenCalled(); + }); +}); diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 7a45a74..4c6b5ac 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -160,6 +160,31 @@ function terminalTracksMouse(term: Terminal): boolean { return term.modes.mouseTrackingMode !== "none"; } +/** + * Everything the gate asks the terminal, sampled at the moment of the click. + * + * A struct rather than three getters because the three are read together and + * must describe one instant: `hasSelection` is only meaningful against the + * `mouseTracking` that decided which gestures could have produced it. + */ +export interface ClickContext { + /** {@link terminalTracksMouse} — the container's to change, at any time. */ + mouseTracking: boolean; + /** Does the terminal hold a selection *right now*? See {@link opensOnClick}. */ + hasSelection: boolean; + /** xterm's `macOptionClickForcesSelection`, read rather than assumed. */ + macOptionClickForcesSelection: boolean; +} + +function readClickContext(term: Terminal): ClickContext { + return { + mouseTracking: terminalTracksMouse(term), + hasSelection: term.hasSelection(), + macOptionClickForcesSelection: + term.options.macOptionClickForcesSelection ?? false, + }; +} + /** xterm's `isMac` verbatim (`common/Platform.ts`), so we split where it does. */ function isMacPlatform(): boolean { const platform = typeof navigator === "undefined" ? "" : navigator.platform; @@ -173,17 +198,24 @@ function isMacPlatform(): boolean { * the mouse, this is the one gesture the user already has for "this click is * for the terminal, not for the program", so it is the gesture that may open a * link. xterm's rule is - * `isMac ? e.altKey && macOptionClickForcesSelection : e.shiftKey`, and this - * view sets `macOptionClickForcesSelection`, so the Mac branch is live rather - * than theoretical. + * `isMac ? e.altKey && rawOptions.macOptionClickForcesSelection : e.shiftKey`, + * and the option is read from the terminal rather than assumed: this view sets + * it true today, so the two agreed, but xterm's default is false and nothing + * would have reported the day that line went. A hardcoded `altKey` would then + * accept a modifier xterm no longer treats as force-select. * * The gate and the hint below both call this. A hint that names a key the gate * does not accept is worse than no hint — the user concludes the link is * broken — and that is a bug this branch has already shipped once, so the two * are not allowed separate answers. */ -function forcesSelection(event: { altKey: boolean; shiftKey: boolean }): boolean { - return isMacPlatform() ? event.altKey : event.shiftKey; +function forcesSelection( + event: { altKey: boolean; shiftKey: boolean }, + macOptionClickForcesSelection: boolean, +): boolean { + return isMacPlatform() + ? event.altKey && macOptionClickForcesSelection + : event.shiftKey; } /** @@ -191,35 +223,85 @@ function forcesSelection(event: { altKey: boolean; shiftKey: boolean }): boolean * * Conditional because the gesture is: with no program tracking the mouse a * plain click opens the link, and naming a modifier then would send the user - * hunting for a key that changes nothing. + * hunting for a key that changes nothing. The last branch is the same rule + * once more: on a Mac with `macOptionClickForcesSelection` off there *is* no + * force-selection modifier, so {@link opensOnClick} can never pass while a + * program holds the mouse, and naming Option would be naming a dead key. */ -function openHintLabel(mouseTracking: boolean): string { - if (!mouseTracking) return "Click to open"; - return isMacPlatform() ? "Option+click to open" : "Shift+click to open"; +function openHintLabel(ctx: ClickContext): string { + if (!ctx.mouseTracking) return "Click to open"; + if (!isMacPlatform()) return "Shift+click to open"; + if (!ctx.macOptionClickForcesSelection) { + return "Not clickable while a program holds the mouse"; + } + return "Option+click to open"; } /** - * Whether this click is a request to leave the app for the host browser. + * Whether this mouseup is a request to leave the app for the host browser. * - * Two refusals, for two different mistakes: + * xterm asks none of this. `Linkifier._handleMouseUp` activates whenever the + * mouseup lands on the same link the mousedown did — no button check, no mode + * check, no `detail`, no drag threshold, no timestamp (`SelectionService` has + * a `_mouseDownTimeStamp`; the Linkifier has nothing). Four refusals, for four + * different mistakes: * - * - Anything but the primary button. xterm's `Linkifier._handleMouseUp` - * checks neither the button nor the mouse mode, so without this a - * *right*-click activates the link as well as opening this pane's context - * menu, and a middle-click paste opens it too. - * - A plain click while a program is tracking the mouse. That is the - * dangerous one: OSC 8 lets the container wrap any clickable TUI widget — - * a menu row, a "1. Yes", a file chip — in a link to anywhere, and because - * the mouse report still reaches the program the widget also responds, so - * nothing looks wrong. Requiring the force-selection modifier there makes - * the two intents distinguishable. + * - **Anything but the primary button.** Without this a *right*-click + * activates the link as well as opening this pane's context menu, and a + * middle-click paste opens it too. + * - **A mouseup that ended a selection.** This is the load-bearing one, and + * the reason is that a drag is a single press and a single release, so its + * click count is 1 and nothing else distinguishes it from a click. Both + * gestures a user makes to *copy* a string end here: drag across a few + * characters, or double-click a word (xterm selects it on the second + * mousedown, so the selection is already in the model by the time this + * runs). Worse, while a program holds the mouse Shift/Option+drag is the + * *only* way to select at all — byte-identical to the modifier below — so + * without this check a container that wraps each output row in an OSC 8 + * turns every legitimate copy into a browser open. The selection check is + * also cheap to be wrong about in the safe direction: xterm's + * `_handleSingleClick` clears the model on the mousedown of a plain click, + * so an old selection elsewhere in the buffer is already gone by the time a + * real click on a link arrives here. + * - **A repeat click**, `detail > 1`. Belt to the above's braces: it holds + * even when the selection came out empty (a double-click on trailing + * whitespace selects nothing) and it does not depend on xterm having + * updated the selection model before the Linkifier's listener runs. `> 1` + * rather than `!== 1` because a synthesised event carries `detail` 0. + * Comparing mousedown and mouseup *coordinates* would be a third signal, + * but xterm hands this handler only the mouseup — the mousedown is not + * ours to see without binding our own listener to the host element, which + * is a second source of truth about the same gesture. + * - **A plain click while a modifier is required.** OSC 8 lets the container + * wrap any clickable TUI widget — a menu row, a "1. Yes", a file chip — in + * a link to anywhere, and because the mouse report still reaches the + * program the widget also responds, so nothing looks wrong. Requiring the + * force-selection modifier there makes the two intents distinguishable. * - * With nothing tracking the mouse a bare click is correct and expected: it is - * what `WebLinksAddon` does for the plain-text URLs in the same buffer. + * `modifierPromised` is that last requirement made sticky, and it is about the + * card rather than the click: the hint is rendered once, at hover, from a mode + * the container may change before the user's finger comes down. The gate + * honours the stricter of what the card promised and what is true now, so a + * card reading "Shift+click to open" cannot be on screen while a bare click + * opens the link. + * + * **What this does not close.** `mouseTracking` is a permission the attacker + * grants itself — see `activate`. + * + * With nothing tracking the mouse and nothing promised, a bare click is + * correct and expected: it is what `WebLinksAddon` does for the plain-text + * URLs in the same buffer, which is why that handler applies this same gate. */ -function opensOnClick(event: MouseEvent, mouseTracking: boolean): boolean { +function opensOnClick( + event: MouseEvent, + ctx: ClickContext, + modifierPromised = false, +): boolean { if (event.button !== 0) return false; - return !mouseTracking || forcesSelection(event); + if (event.detail > 1) return false; + if (ctx.hasSelection) return false; + if (!ctx.mouseTracking && !modifierPromised) return true; + return forcesSelection(event, ctx.macOptionClickForcesSelection); } /** @@ -251,9 +333,35 @@ function opensOnClick(event: MouseEvent, mouseTracking: boolean): boolean { * activates the link with no check on the button, the modifier or the mouse * mode. * - * So the gate is {@link opensOnClick}, applied in `activate`, and the mouse - * mode it asks about is read from the terminal on every click rather than - * captured — the container changes it whenever it likes. + * So the gate is {@link opensOnClick}, applied in `activate`, and everything it + * asks about is read from the terminal at the moment of the click rather than + * captured — the container changes the mouse mode whenever it likes, and the + * selection is whatever the gesture that ended in this mouseup left behind. + * + * ## What the mouse mode is worth, honestly + * + * Reading it fresh makes it *current*; it does not make it *trustworthy*. The + * mode is set by the container, with a DECSET, and `?1002l` takes effect + * synchronously with the write — so a hostile container can drop tracking for + * a few hundred milliseconds at a time and a plain click that lands in one of + * those windows passes the mode half of the gate. It cannot time the user's + * click, but it does not need to: a fraction of clicks is enough, and the only + * tell is the status-bar badge flickering. This is a **known residual**, not + * something this gate closes, and the freshness of the read must not be read + * as an answer to it. + * + * Two things narrow it, neither of which depends on the mode. The selection + * and click-count checks hold in either tracking state, so the gestures a user + * makes to copy text are refused whatever the container has the mode set to — + * which removes the "wrap every row in an OSC 8 and harvest the shift-drags" + * version entirely. And the hover card's promise is sticky (see + * `modifierPromised`): the flicker now has to cover the *hover* as well as the + * click, because a card drawn while tracking was on goes on demanding the + * modifier after the container drops it. What remains is a container that + * drops tracking before the pointer arrives and holds it off until the click — + * at which point the card also says "Click to open", so the user is at least + * not being told one thing and given another. The real fix is a signal the + * container cannot write, and there is none in this pane today. * * ## The hover card is the security half, not a nicety * @@ -272,19 +380,29 @@ function opensOnClick(event: MouseEvent, mouseTracking: boolean): boolean { * * @param getHost returns `Terminal.element`, which does not exist until * `term.open()` has run — hence a getter rather than the element. - * @param isMouseTracking answers "is a program holding the mouse right now?" - * — a getter for the same reason, and the *only* reason: the answer - * changes under us. + * @param readState samples {@link ClickContext} — a getter for the same + * reason, and the *only* reason: every one of those answers changes + * under us, between the hover and the click that follows it. */ export function createOsc8LinkHandler( getHost: () => HTMLElement | null, - isMouseTracking: () => boolean, + readState: () => ClickContext, ): Osc8LinkHandler { let card: HTMLDivElement | null = null; + /** + * Did the card the user is looking at name a modifier? + * + * Written on every hover, cleared with the card. xterm only activates a link + * it is currently hovering (`Linkifier._currentLink`), so there is always a + * fresh hover behind a click — which is what makes this the promise the user + * actually read, rather than a stale one. See `opensOnClick`. + */ + let modifierPromised = false; const clear = () => { card?.remove(); card = null; + modifierPromised = false; }; const span = (text: string, style: Partial) => { @@ -297,8 +415,10 @@ export function createOsc8LinkHandler( return { activate(event, text) { // Opening the host browser is the one thing in this pane the container - // must not be able to provoke on its own. See `opensOnClick`. - if (!opensOnClick(event, isMouseTracking())) return; + // may not provoke on its own *and* the one thing no selection gesture + // may provoke by accident. See `opensOnClick` — including the residual + // it does not close. + if (!opensOnClick(event, readState(), modifierPromised)) return; // Same sink and same rule as the WebLinksAddon branch: this came off the // container's output, so it is validated before it reaches the OS // opener. One implementation — `sanitizeRelayUrl` — on purpose. @@ -314,6 +434,11 @@ export function createOsc8LinkHandler( clear(); const host = getHost(); if (!host) return; + const ctx = readState(); + // Sampled here and held, because this is what the card is about to tell + // the user — and the gate has to honour it even if the container has + // moved on by the time they click. + modifierPromised = ctx.mouseTracking; card = document.createElement("div"); card.className = OSC8_HOVER_CLASS; @@ -394,7 +519,7 @@ export function createOsc8LinkHandler( restEl.dataset.testid = "osc8-hover-rest"; card.appendChild(restEl); - const hint = span(openHintLabel(isMouseTracking()), { + const hint = span(openHintLabel(ctx), { color: "var(--text-secondary)", flexShrink: "0", marginLeft: "4px", @@ -740,7 +865,7 @@ export default function TerminalView({ sessionId, active }: Props) { // whenever the container prints a DECSET. linkHandler: (osc8LinkHandlerRef.current = createOsc8LinkHandler( () => term.element ?? null, - () => terminalTracksMouse(term), + () => readClickContext(term), )), fontFamily: "'JetBrains Mono', 'Fira Code', 'Cascadia Code', Menlo, Monaco, monospace", theme: { @@ -775,10 +900,23 @@ export default function TerminalView({ sessionId, active }: Props) { // misses OAuth URLs that end mid-line). // eslint-disable-next-line no-control-regex const urlRegex = /https?:\/\/[^\s'"`<>\x00-\x20\x7f]+/; - const webLinksAddon = new WebLinksAddon((_event, uri) => { - // Same sink, same rule as `createOsc8LinkHandler`: what xterm matched - // came off the container's output, so it is validated before it reaches - // the OS opener. + const webLinksAddon = new WebLinksAddon((event, uri) => { + // Same gate and same sink as `createOsc8LinkHandler`, because this + // reaches the same `openUrlExternal` through the same + // `Linkifier._handleMouseUp`, which checks nothing here either. Without + // it a container that prints a plausible-looking `https://` row in a TUI + // got a browser open on a plain click while it held the mouse, and a + // double-click that merely selected a URL opened it. + // + // It is also the only gate on a real bypass of the OSC 8 one: + // `OscLinkProvider` drops a hyperlink whose target is not http(s) + // *before* `linkHandler` sees it (`allowNonHttpProtocols` is unset), so + // an OSC 8 carrying a `javascript:` target and an `https://evil.tld/x` + // label leaves the addon free to match the label. That click arrives + // here and nowhere else. + // + // No `modifierPromised`: this path paints an underline rather than a + // card, so it promises the user nothing to be held to. // // This branch is the one where the click really is an act on visible // text — the match *is* the painted characters — so the spoof it has to @@ -788,6 +926,7 @@ export default function TerminalView({ sessionId, active }: Props) { // on hover before a click can happen. Neither branch replaces the other: // this one covers plain-text URLs in ordinary shell output, which carry // no hyperlink parameter for xterm to hand over. + if (!opensOnClick(event, readClickContext(term))) return; const safe = sanitizeRelayUrl(uri); if (!safe) { console.warn("Refusing to open a link that failed validation"); From c6f9c1d43ffbcc7344abc460e896ac97cd5abd64 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Fri, 18 Sep 2026 20:07:59 -0700 Subject: [PATCH 4/5] fix: tighten the click-count check and stop three comments overstating Third-round review polish; no behaviour change beyond the first item. `detail > 1` was justified in a comment by noting a synthesised event carries `detail` 0 -- which is an argument for letting untrusted synthetic events through the click-count half of the gate. A mouseup derived from a real click always carries `detail >= 1`, so the check is now `!== 1`. Nothing in the container can dispatch a DOM event, so this is hardening rather than a hole; the comment now says that instead of the reverse. Three comments claimed more than they hold. The selection check's paragraph read as though it caught every copy gesture: it sees a drag only once the drag has spanned a cell, so a press and release inside one character cell -- or a drag walked back to its start -- still opens the link. That is the gap the rejected mousedown/mouseup distance check would have closed, and it is now recorded beside the reason for rejecting it. `?1002l` was described as taking effect synchronously with the write; it takes effect when xterm parses it, on its queued write task. And `modifierPromised` was described as written on every hover, when `hover()` clears and returns early with no host element -- which leaves it false, the stricter direction. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/components/terminal/TerminalView.tsx | 28 +++++++++++++++----- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 4c6b5ac..d2ab87b 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -263,11 +263,24 @@ function openHintLabel(ctx: ClickContext): string { * `_handleSingleClick` clears the model on the mousedown of a plain click, * so an old selection elsewhere in the buffer is already gone by the time a * real click on a link arrives here. + * + * The limit of this check, stated because the paragraph above reads + * absolute: it sees a drag only once the drag has spanned a *cell*. A press + * and release inside one character cell, or a drag walked back to where it + * started, leaves `finalSelectionEnd === finalSelectionStart`, so + * `hasSelection()` is false and the link opens. Nothing reached the + * clipboard in that case and the card showed the real origin first, so the + * cost is small — but it is the gap a mousedown/mouseup distance check + * would have closed, and it is the price of not keeping that second source + * of truth. * - **A repeat click**, `detail > 1`. Belt to the above's braces: it holds * even when the selection came out empty (a double-click on trailing * whitespace selects nothing) and it does not depend on xterm having - * updated the selection model before the Linkifier's listener runs. `> 1` - * rather than `!== 1` because a synthesised event carries `detail` 0. + * updated the selection model before the Linkifier's listener runs. It is + * `!== 1`, not `> 1`: a mouseup derived from a real click always carries + * `detail >= 1`, so `> 1` would have waved through anything synthesised + * with `detail` 0. Nothing in the container can dispatch a DOM event, so + * that is hardening rather than a hole being closed. * Comparing mousedown and mouseup *coordinates* would be a third signal, * but xterm hands this handler only the mouseup — the mousedown is not * ours to see without binding our own listener to the host element, which @@ -298,7 +311,7 @@ function opensOnClick( modifierPromised = false, ): boolean { if (event.button !== 0) return false; - if (event.detail > 1) return false; + if (event.detail !== 1) return false; if (ctx.hasSelection) return false; if (!ctx.mouseTracking && !modifierPromised) return true; return forcesSelection(event, ctx.macOptionClickForcesSelection); @@ -341,8 +354,9 @@ function opensOnClick( * ## What the mouse mode is worth, honestly * * Reading it fresh makes it *current*; it does not make it *trustworthy*. The - * mode is set by the container, with a DECSET, and `?1002l` takes effect - * synchronously with the write — so a hostile container can drop tracking for + * mode is set by the container, with a DECSET, and `?1002l` takes effect as + * soon as xterm *parses* it (on its queued write task, not synchronously with + * the container's output) — so a hostile container can drop tracking for * a few hundred milliseconds at a time and a plain click that lands in one of * those windows passes the mode half of the gate. It cannot time the user's * click, but it does not need to: a fraction of clicks is enough, and the only @@ -392,7 +406,9 @@ export function createOsc8LinkHandler( /** * Did the card the user is looking at name a modifier? * - * Written on every hover, cleared with the card. xterm only activates a link + * Written whenever a card is drawn, and cleared with it — `hover()` clears + * and returns early when there is no host element, which leaves this false, + * the stricter of the two directions. xterm only activates a link * it is currently hovering (`Linkifier._currentLink`), so there is always a * fresh hover behind a click — which is what makes this the promise the user * actually read, rather than a stale one. See `opensOnClick`. From 83c9c249519a7545fd5a410f39ca3cffcd3eba5f Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Fri, 18 Sep 2026 20:08:36 -0700 Subject: [PATCH 5/5] test: give two synthesised clicks the detail a real click carries The previous commit tightened the gate's click-count check from `> 1` to `!== 1`, which two tests in the wiring block did not survive: they built `new MouseEvent("click", { button: 0 })` directly rather than through the `click()` helper, so `detail` defaulted to 0 and the gate refused them. The gate is right and the tests were wrong -- a mouseup derived from a real click always carries `detail >= 1`, and 0 is exactly the synthetic-event shape the tightening was for. Both now pass `detail: 1`. I pushed the previous commit without noticing this, having read a truncated test summary that hid the failure. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/components/terminal/TerminalView.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 7fce0eb..723f0e8 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -1664,7 +1664,7 @@ describe("the link handler is wired into the terminal, and reads its live mode", await write("\x1b[?1002h"); wiredHandler().activate( - new MouseEvent("click", { button: 0 }), + new MouseEvent("click", { button: 0, detail: 1 }), "https://example.com/x", range, ); @@ -1679,7 +1679,7 @@ describe("the link handler is wired into the terminal, and reads its live mode", await act(async () => { wiredHandler().activate( - new MouseEvent("click", { button: 0 }), + new MouseEvent("click", { button: 0, detail: 1 }), "https://example.com/x", range, );