From 593b8168eb1e57dea7a6e180348179b7a1225e61 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Fri, 18 Sep 2026 20:00:42 -0700 Subject: [PATCH] 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");