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