From bb9e666872f4912d89480876623b13739e5c12de Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 18:16:10 +0530 Subject: [PATCH 01/10] feat: make linked workspace names clickable in the sidebar, CLI link picker, and post-scan dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `workspace-sidebar.tsx`: the workspace name + manage URL in the TUI sidebar tile now render underlined/accent-styled and open the browser on click (mouse events land on the enclosing ``, not the inline ``, matching the pattern already used for the footer's docs/ community links since raw `` crashes in this JSX layer). - `workspace.tsx`: extracted the guarded `open()` + toast-on-failure logic from `WorkspaceLinkedDialog` into a shared `openManageUrl()` helper, and added an "Open in browser" option to `AlreadyLinkedDialog` (shown on re-entering an already-linked project) using the same helper. - `cli/cmd/link.ts`: `altimate-code link`'s "Currently linked to X" prompt and the matching picker row now wrap the workspace name in a real OSC 8 terminal hyperlink (+ underline for visual affordance), so clicking it in a supporting terminal opens the workspace directly — no click-handler needed since the terminal itself renders the link. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 37 +++++- .../plugin/tui/altimate/workspace-sidebar.tsx | 29 ++++- .../src/plugin/tui/altimate/workspace.tsx | 114 ++++++++++++------ 3 files changed, 137 insertions(+), 43 deletions(-) diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index 587f4bf9d..2a76a52a6 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -42,6 +42,27 @@ import { recordApprovedBinding } from "@/altimate/workspace/state" const CREATE_NEW_SENTINEL = "__create_new__" const SET_UP_IN_BROWSER_SENTINEL = "__browser_handoff__" +/** Wrap ``text`` in an OSC 8 terminal hyperlink pointing at ``url``, or return + * ``text`` unchanged when ``url`` is null. Unlike the TUI's `` (which + * crashes in the current @opentui/solid JSX layer — see workspace-sidebar.tsx), + * plain stdout can emit OSC 8 directly: supporting terminals (iTerm2, Ghostty, + * kitty, Windows Terminal, ...) render it as a real clickable link, and + * terminals that don't recognize the sequence just skip the invisible control + * bytes — the visible text is unaffected either way, so no capability check + * is needed before emitting it. */ +function hyperlink(text: string, url: string | null): string { + if (!url) return text + const OSC8 = "\x1b]8;;" + const ST = "\x1b\\" + // Underline as a visual affordance that this text is clickable — OSC 8 + // alone carries no default styling. ``\x1b[24m`` (underline-off only, not + // a full ``\x1b[0m`` reset) so it doesn't clobber a color clack already + // applied around the whole line (e.g. the dim wrapper on a submitted value). + const UNDERLINE = "\x1b[4m" + const UNDERLINE_OFF = "\x1b[24m" + return `${OSC8}${url}${ST}${UNDERLINE}${text}${UNDERLINE_OFF}${OSC8}${ST}` +} + export const LinkCommand = cmd({ command: "link", describe: "Link this project to an Altimate workspace", @@ -128,8 +149,15 @@ export const LinkCommand = cmd({ // (freemium only today). Enterprise / localhost / custom-domain callers // silently fall back to the CLI-side quick create. const creds = await AltimateApi.getCredentials() - const browserAvailable = - resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) !== null + const workspaceWebBase = resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) + const browserAvailable = workspaceWebBase !== null + // Deterministic from tenant + id, same derivation as the TUI's + // buildManageUrl (workspace.tsx) — null on BYOK/unresolvable, in which + // case the name below prints as plain (non-clickable) text. + const currentManageUrl = + currentId !== undefined && workspaceWebBase + ? `${workspaceWebBase.toString().replace(/\/$/, "")}/w/${currentId}` + : null const options: Array<{ value: string; label: string; hint?: string }> = [ // Only offer browser handoff for UNLINKED projects (CodeRabbit cycle 5). @@ -162,14 +190,15 @@ export const LinkCommand = cmd({ }, ...list.map((dm) => ({ value: String(dm.id), - label: dm.id === currentId ? `● ${dm.name}` : ` ${dm.name}`, + label: + dm.id === currentId ? `● ${hyperlink(dm.name, currentManageUrl)}` : ` ${dm.name}`, hint: dm.id === currentId ? "currently linked here" : undefined, })), ] const pick = await prompts.select({ message: existing - ? `Currently linked to "${currentName}". Pick a workspace (or create a new one):` + ? `Currently linked to "${hyperlink(currentName!, currentManageUrl)}". Pick a workspace (or create a new one):` : "Pick a workspace to link (or create a new one):", options, initialValue: currentId !== undefined ? String(currentId) : CREATE_NEW_SENTINEL, diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index d9e88edf4..36d8ef26f 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -13,6 +13,7 @@ import { readLocalBinding, type CachedBinding } from "@/altimate/workspace/state import { resolveWorkspaceWebUrl } from "@/altimate/workspace/browser-handoff" import { getResolvedWorkspaceId } from "@/altimate/workspace/session-context" import { AltimateApi } from "@/altimate/api/client" +import { openManageUrl } from "./workspace" const id = "altimate:sidebar-workspace" @@ -97,8 +98,26 @@ function View(props: { api: TuiPluginApi }) { > {(b) => ( <> - - {b().datamateName} + {/* Clicking the name (or the URL line below) opens the workspace + * in the browser — the manage URL is deterministic from tenant + * + id (see resolveManageBase above), so there's no extra + * round-trip before it's clickable. The whole line is the + * click target (mouse events only land on block-level + * ``/``, not inline ``/`` nodes), while only + * the name itself is styled to look like a link — matching the + * footer's community/docs links (sidebar/footer.tsx), which use + * the same span-style + onMouseUp pair because raw `` + * hyperlink nodes crash in this JSX layer. */} + { + const url = manageUrl() + if (url) openManageUrl(props.api, url) + }} + > + + {(_u) => {b().datamateName}} + {/* ``pinned via --workspace`` means "this SESSION was launched * with --workspace and it resolved to this id". It does NOT * mean "the current binding was set by --workspace" — if the @@ -117,7 +136,11 @@ function View(props: { api: TuiPluginApi }) { - {(u) => {u()}} + {(u) => ( + openManageUrl(props.api, u())}> + {u()} + + )} )} diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index f59f0275b..2fe7a9a03 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -297,26 +297,11 @@ function WorkspaceLinkedDialog(props: LinkedProps) { current={props.manageUrl ? "open" : "done"} onSelect={(option) => { if (option.value === "open" && props.manageUrl) { - const url = props.manageUrl // Guard before delegating to open() — a rogue manage_url with a // non-http protocol would otherwise dispatch to an unrelated OS // scheme handler. buildManageUrl only ever emits http(s) URLs from // resolveWorkspaceWebUrl, but the guard survives future changes. - if (!isSafeHttpUrl(url)) { - props.api.ui.toast({ - variant: "warning", - message: `Refused to open a non-http URL: ${url}`, - duration: 15_000, - }) - } else { - open(url).catch(() => { - props.api.ui.toast({ - variant: "warning", - message: `Could not open browser. Copy this URL: ${url}`, - duration: 15_000, - }) - }) - } + openManageUrl(props.api, props.manageUrl) } props.api.ui.dialog.clear() }} @@ -579,9 +564,10 @@ async function createAndBindInline( /** True when the URL parses and its protocol is exactly ``http:`` or ``https:``. * Used before handing a server-supplied URL to ``open()`` (which would otherwise * dispatch to whatever OS scheme handler matches the protocol). Kept exported - * as a top-level helper because both ``showLinkedConfirmation`` (below) and - * the on-demand link paths need the same guard. */ -function isSafeHttpUrl(url: string): boolean { + * as a top-level helper — ``openManageUrl`` below is the sole in-module caller, + * but ``cli/cmd/link.ts`` deliberately keeps its own copy (CLI/TUI split, see + * that file's comment) rather than importing this one. */ +export function isSafeHttpUrl(url: string): boolean { try { const u = new URL(url) return u.protocol === "http:" || u.protocol === "https:" @@ -590,6 +576,29 @@ function isSafeHttpUrl(url: string): boolean { } } +/** Guarded ``open(url)`` for a workspace manage-URL, with the same + * refuse-and-toast / catch-and-toast behavior as ``WorkspaceLinkedDialog``'s + * "open" action below. Shared with ``workspace-sidebar.tsx`` (both live under + * this TUI plugin path — unlike ``isSafeHttpUrl``'s CLI/TUI split, there's no + * reason for these two call sites to diverge). */ +export function openManageUrl(api: TuiPluginApi, url: string) { + if (!isSafeHttpUrl(url)) { + api.ui.toast({ + variant: "warning", + message: `Refused to open a non-http URL: ${url}`, + duration: 15_000, + }) + return + } + open(url).catch(() => { + api.ui.toast({ + variant: "warning", + message: `Could not open browser. Copy this URL: ${url}`, + duration: 15_000, + }) + }) +} + /** Pick the rebind endpoint that matches which identifier the pre-check * resolved the binding on. Shared with cli/cmd/link.ts through duplicated * code (M3) — the modules deliberately don't cross-import so the CLI @@ -636,9 +645,24 @@ interface AlreadyLinkedProps { } function AlreadyLinkedDialog(props: AlreadyLinkedProps) { + // Best-effort — same deterministic tenant+id derivation as buildManageUrl's + // other callers. null on BYOK/unresolvable, in which case the "Open in + // browser" option below is simply omitted. + const [manageUrl, setManageUrl] = createSignal(null) + onMount(async () => { + setManageUrl(await buildManageUrl(props.workspaceId)) + }) + // Title carries the primary context (workspace name + drift/unverified hint) // since DialogSelect doesn't take a top-level description block. Verbose but // it puts the critical info in the user's field of view before they pick. + // + // The plugin-facing ``TuiDialogSelectProps`` (packages/plugin/src/tui.ts) + // only takes a plain ``title: string`` — no ``titleView``/JSX escape hatch + // like the native ``packages/tui`` DialogSelect has (see + // dialog-move-session.tsx) — so the workspace name inside the title can't + // be made clickable the way the sidebar tile is. "Open in browser" as a + // selectable option (below) is the equivalent affordance within that API. const title = () => { const parts: string[] = [`Project is linked to workspace "${props.workspaceName}"`] const now = props.identifier.repoRemote ?? props.identifier.projectPath @@ -646,32 +670,50 @@ function AlreadyLinkedDialog(props: AlreadyLinkedProps) { if (props.unverified) parts.push("(⚠ unverified — server unreachable, showing cached value)") return parts.join(" ") } + const options = () => { + const opts = [ + { + title: "Attach and continue", + value: "attach", + description: "Use this workspace for the session.", + }, + { + title: "Re-link to a different workspace", + value: "relink", + description: "Swap this project's workspace.", + }, + ] + if (manageUrl()) { + opts.push({ + title: "Open in browser", + value: "open", + description: "View this workspace on the web.", + }) + } + opts.push({ + title: "Skip for now", + value: "skip", + description: "Close this prompt without changing the link.", + }) + return opts + } return ( { if (option.value === "attach" || option.value === "skip") { props.api.ui.dialog.clear() return } + if (option.value === "open") { + const url = manageUrl() + if (url) openManageUrl(props.api, url) + // Stay open — opening the browser isn't a decision about the link + // itself, so the user can still Attach/Re-link/Skip afterward. + return + } // relink → picker with the current workspace id as expected_current so // a concurrent re-link by another client 412s cleanly. matchedBy // determines which rebind endpoint the picker will call (M3). From 6fb1c43e32be86469bf7de566bc9ffc57ffed90c Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 19:38:22 +0530 Subject: [PATCH 02/10] =?UTF-8?q?fix:=20address=20bot=20review=20findings?= =?UTF-8?q?=20on=20#1274=20=E2=80=94=20control-char=20injection,=20URL=20j?= =?UTF-8?q?oin,=20dead=20export?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `cli/cmd/link.ts`: sanitize workspace names (strip C0/C1 control bytes, including ESC) before they reach the OSC 8 hyperlink wrapper — an attacker/account-controlled workspace name containing its own `\x1b]8;;` could otherwise prematurely close our hyperlink and open a spoofed one with our trusted URL as inert visible prefix text. (CodeRabbit + cubic) - `cli/cmd/link.ts`: only emit the underline visual affordance when the terminal is one we're reasonably confident renders OSC 8 (conservative TERM_PROGRAM/VTE/Konsole/Windows Terminal allowlist) — SGR underline is far more universally rendered than OSC 8 itself, so emitting it unconditionally made names look clickable in terminals where they weren't. (cubic) - `cli/cmd/link.ts` + `workspace.tsx` + `workspace-sidebar.tsx`: build the manage URL via real `URL` pathname/search/hash manipulation instead of string-concatenating `toString()` — the dev-only `ALTIMATE_WORKSPACE_WEB_URL` override can carry its own path/query, and naive concatenation landed `/w/` inside the query string instead of the path. Fixed in all three occurrences of the same pattern, not just the one CodeRabbit/cubic flagged in link.ts. (CodeRabbit + cubic) - `workspace.tsx`: drop the `export` on `isSafeHttpUrl` — no external importers; `link.ts` deliberately keeps its own private copy. (Kilo) Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 75 +++++++++++++++---- .../plugin/tui/altimate/workspace-sidebar.tsx | 11 ++- .../src/plugin/tui/altimate/workspace.tsx | 28 +++++-- 3 files changed, 89 insertions(+), 25 deletions(-) diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index 2a76a52a6..af60ce306 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -42,25 +42,76 @@ import { recordApprovedBinding } from "@/altimate/workspace/state" const CREATE_NEW_SENTINEL = "__create_new__" const SET_UP_IN_BROWSER_SENTINEL = "__browser_handoff__" +/** Strip C0/C1 control bytes (including ESC) from server-controlled text + * before it reaches a raw-stdout escape-sequence wrapper. Workspace names + * come from ``WorkspaceApi.listDatamates()`` with no charset validation — an + * attacker-controlled name containing its own ``\x1b]8;;`` could otherwise + * prematurely close our hyperlink and open a spoofed one pointing wherever + * they choose, with our trusted URL as the visible (but inert) prefix. + * (CodeRabbit + cubic, PR #1274.) */ +function stripControlChars(text: string): string { + // eslint-disable-next-line no-control-regex + return text.replace(/[\x00-\x1f\x7f]/g, "") +} + +/** Conservative allowlist of terminals known to render OSC 8 hyperlinks. + * There's no capability query as reliable as opentui's device-attribute + * detection (used by the TUI side) available to a plain CLI process, so this + * errs toward false negatives — worst case a supporting terminal renders + * plain text instead of a link, which is a strict improvement over the + * inverse (underlining text that turns out not to be clickable). Mirrors the + * checks the `supports-hyperlinks` package uses, inlined to avoid a new + * dependency for one CLI affordance. */ +function terminalSupportsHyperlinks(): boolean { + if (!process.stdout.isTTY) return false + if (process.env.TERM === "dumb" || process.env.TERM === "linux") return false + const termProgram = process.env.TERM_PROGRAM + if ( + termProgram && + ["iTerm.app", "WezTerm", "Hyper", "vscode", "ghostty", "Tabby", "rio", "Apple_Terminal"].includes(termProgram) + ) + return true + if (process.env.WT_SESSION) return true // Windows Terminal + if (process.env.KONSOLE_VERSION) return true + const vte = Number(process.env.VTE_VERSION) + if (!Number.isNaN(vte) && vte >= 5000) return true // VTE >= 0.50.0 (GNOME Terminal and other VTE-based terms) + return false +} + +/** Append ``/w/`` to ``base``'s pathname using real URL semantics, rather + * than string-concatenating ``toString()``. The dev-only + * ``ALTIMATE_WORKSPACE_WEB_URL`` override (resolveWorkspaceWebUrl) can carry + * its own path/query/fragment (e.g. a local dev server), and naive + * concatenation would land ``/w/`` inside the query string instead of the + * path — clears search/hash for the same reason. (CodeRabbit + cubic, PR #1274.) */ +function buildManageUrl(base: URL, workspaceId: number): string { + const u = new URL(base) + u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` + u.search = "" + u.hash = "" + return u.toString() +} + /** Wrap ``text`` in an OSC 8 terminal hyperlink pointing at ``url``, or return * ``text`` unchanged when ``url`` is null. Unlike the TUI's `` (which * crashes in the current @opentui/solid JSX layer — see workspace-sidebar.tsx), - * plain stdout can emit OSC 8 directly: supporting terminals (iTerm2, Ghostty, - * kitty, Windows Terminal, ...) render it as a real clickable link, and - * terminals that don't recognize the sequence just skip the invisible control - * bytes — the visible text is unaffected either way, so no capability check - * is needed before emitting it. */ + * plain stdout can emit OSC 8 directly: supporting terminals render it as a + * real clickable link, and terminals that don't recognize the sequence just + * skip the invisible control bytes — the visible text is unaffected either + * way, so the OSC 8 wrapping itself needs no capability check. The + * *underline*, however, is a much older and more universally-rendered SGR + * code — emitting it unconditionally would make the name look clickable in + * terminals where it isn't, so it's gated on ``terminalSupportsHyperlinks`` + * (cubic, PR #1274). */ function hyperlink(text: string, url: string | null): string { if (!url) return text + const safeText = stripControlChars(text) const OSC8 = "\x1b]8;;" const ST = "\x1b\\" - // Underline as a visual affordance that this text is clickable — OSC 8 - // alone carries no default styling. ``\x1b[24m`` (underline-off only, not - // a full ``\x1b[0m`` reset) so it doesn't clobber a color clack already - // applied around the whole line (e.g. the dim wrapper on a submitted value). + if (!terminalSupportsHyperlinks()) return `${OSC8}${url}${ST}${safeText}${OSC8}${ST}` const UNDERLINE = "\x1b[4m" const UNDERLINE_OFF = "\x1b[24m" - return `${OSC8}${url}${ST}${UNDERLINE}${text}${UNDERLINE_OFF}${OSC8}${ST}` + return `${OSC8}${url}${ST}${UNDERLINE}${safeText}${UNDERLINE_OFF}${OSC8}${ST}` } export const LinkCommand = cmd({ @@ -155,9 +206,7 @@ export const LinkCommand = cmd({ // buildManageUrl (workspace.tsx) — null on BYOK/unresolvable, in which // case the name below prints as plain (non-clickable) text. const currentManageUrl = - currentId !== undefined && workspaceWebBase - ? `${workspaceWebBase.toString().replace(/\/$/, "")}/w/${currentId}` - : null + currentId !== undefined && workspaceWebBase ? buildManageUrl(workspaceWebBase, currentId) : null const options: Array<{ value: string; label: string; hint?: string }> = [ // Only offer browser handoff for UNLINKED projects (CodeRabbit cycle 5). diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index 36d8ef26f..6d454847e 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -13,7 +13,7 @@ import { readLocalBinding, type CachedBinding } from "@/altimate/workspace/state import { resolveWorkspaceWebUrl } from "@/altimate/workspace/browser-handoff" import { getResolvedWorkspaceId } from "@/altimate/workspace/session-context" import { AltimateApi } from "@/altimate/api/client" -import { openManageUrl } from "./workspace" +import { openManageUrl, joinManageUrlPath } from "./workspace" const id = "altimate:sidebar-workspace" @@ -31,8 +31,8 @@ const POLL_MS = 30_000 * base per (apiUrl, tenant) pair for the life of the process; if the file * changes mid-session, the binding cache invalidation (in state.ts) still * catches it via its own (tenant, apiUrl) top-level scoping. */ -let cachedManageBase: { apiUrl: string; tenant: string; base: string | null } | null = null -async function resolveManageBase(): Promise { +let cachedManageBase: { apiUrl: string; tenant: string; base: URL | null } | null = null +async function resolveManageBase(): Promise { try { const creds = await AltimateApi.getCredentials() if ( @@ -42,8 +42,7 @@ async function resolveManageBase(): Promise { ) { return cachedManageBase.base } - const url = resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) - const base = url ? url.toString().replace(/\/$/, "") : null + const base = resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) cachedManageBase = { apiUrl: creds.altimateUrl, tenant: creds.altimateInstanceName, base } return base } catch { @@ -69,7 +68,7 @@ function View(props: { api: TuiPluginApi }) { return } const base = await resolveManageBase() - setManageUrl(base ? `${base}/w/${b.datamateId}` : null) + setManageUrl(base ? joinManageUrlPath(base, b.datamateId) : null) } finally { refreshInFlight = false } diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 2fe7a9a03..5babd5790 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -241,6 +241,21 @@ function OfferDialog(props: OfferProps) { ) } +/** Append ``/w/`` to ``base``'s pathname using real URL semantics, rather + * than string-concatenating ``toString()``. The dev-only + * ``ALTIMATE_WORKSPACE_WEB_URL`` override (resolveWorkspaceWebUrl) can carry + * its own path/query/fragment (e.g. a local dev server), and naive + * concatenation would land ``/w/`` inside the query string instead of the + * path — clears search/hash for the same reason. (CodeRabbit + cubic on + * PR #1274's identical bug in cli/cmd/link.ts.) */ +export function joinManageUrlPath(base: URL, workspaceId: number): string { + const u = new URL(base) + u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` + u.search = "" + u.hash = "" + return u.toString() +} + /** Build the SaaS manage-workspace URL for a bound workspace. Deterministic * from tenant + id, so any caller can construct it without an extra round-trip. * Returns null when the current deployment isn't the freemium web (BYOK or @@ -250,7 +265,7 @@ async function buildManageUrl(workspaceId: number): Promise { const creds = await AltimateApi.getCredentials() const base = resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) if (!base) return null - return `${base.toString().replace(/\/$/, "")}/w/${workspaceId}` + return joinManageUrlPath(base, workspaceId) } catch { return null } @@ -563,11 +578,12 @@ async function createAndBindInline( /** True when the URL parses and its protocol is exactly ``http:`` or ``https:``. * Used before handing a server-supplied URL to ``open()`` (which would otherwise - * dispatch to whatever OS scheme handler matches the protocol). Kept exported - * as a top-level helper — ``openManageUrl`` below is the sole in-module caller, - * but ``cli/cmd/link.ts`` deliberately keeps its own copy (CLI/TUI split, see - * that file's comment) rather than importing this one. */ -export function isSafeHttpUrl(url: string): boolean { + * dispatch to whatever OS scheme handler matches the protocol). Not exported — + * ``openManageUrl`` below is the sole caller; ``cli/cmd/link.ts`` deliberately + * keeps its own private copy (CLI/TUI split, see that file's comment) rather + * than importing this one. (Kilo, PR #1274 — the prior `export` had no + * external importers.) */ +function isSafeHttpUrl(url: string): boolean { try { const u = new URL(url) return u.protocol === "http:" || u.protocol === "https:" From c4e9901a0fe1f22cd7611557d7add9ad1a6c96ad Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 19:53:06 +0530 Subject: [PATCH 03/10] =?UTF-8?q?fix:=20address=20round-2=20bot=20review?= =?UTF-8?q?=20findings=20on=20#1274=20=E2=80=94=20Apple=5FTerminal=20false?= =?UTF-8?q?=20positive,=20C1=20control=20bytes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `terminalSupportsHyperlinks`: drop `Apple_Terminal` from the allowlist. macOS Terminal.app only gained OSC 8 support in Sequoia (Sept 2024); older versions only auto-linkify plain URLs. `TERM_PROGRAM` alone can't tell a Sequoia+ install apart from an older one, and this function's own documented bias is toward false negatives — so it's excluded rather than assumed current. Verified via search before reverting; my earlier inclusion was an unverified assumption. (Kilo) - `stripControlChars`: extend the stripped range to also cover C1 control bytes (`\x80`-`\x9f`), not just C0 + DEL. ESC (the actual OSC 8 breakout vector) was always covered, but the doc comment overstated what the regex did. (Kilo) Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index af60ce306..c447c47c1 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -50,8 +50,12 @@ const SET_UP_IN_BROWSER_SENTINEL = "__browser_handoff__" * they choose, with our trusted URL as the visible (but inert) prefix. * (CodeRabbit + cubic, PR #1274.) */ function stripControlChars(text: string): string { + // C0 (\x00-\x1f) + DEL (\x7f) + C1 (\x80-\x9f) — the previous range only + // covered C0/DEL, leaving C1 controls unstripped. ESC (the OSC 8 breakout + // vector) was always covered, but the doc comment claimed C1 coverage it + // didn't have. (Kilo, PR #1274.) // eslint-disable-next-line no-control-regex - return text.replace(/[\x00-\x1f\x7f]/g, "") + return text.replace(/[\x00-\x1f\x7f-\x9f]/g, "") } /** Conservative allowlist of terminals known to render OSC 8 hyperlinks. @@ -61,15 +65,21 @@ function stripControlChars(text: string): string { * plain text instead of a link, which is a strict improvement over the * inverse (underlining text that turns out not to be clickable). Mirrors the * checks the `supports-hyperlinks` package uses, inlined to avoid a new - * dependency for one CLI affordance. */ + * dependency for one CLI affordance. + * + * Deliberately excludes ``Apple_Terminal`` (macOS Terminal.app): OSC 8 + * support only landed there in macOS Sequoia (Sept 2024) — older versions + * (Ventura/Sonoma and earlier) only auto-linkify plain-text URLs, not OSC 8. + * ``TERM_PROGRAM`` carries no OS/Terminal-version signal to tell those apart, + * and this function's own stated bias is toward false negatives, so it's + * left off the list rather than guessing the user is on a current-enough + * macOS. (Kilo, PR #1274 — corrects an earlier version of this list that + * included it.) */ function terminalSupportsHyperlinks(): boolean { if (!process.stdout.isTTY) return false if (process.env.TERM === "dumb" || process.env.TERM === "linux") return false const termProgram = process.env.TERM_PROGRAM - if ( - termProgram && - ["iTerm.app", "WezTerm", "Hyper", "vscode", "ghostty", "Tabby", "rio", "Apple_Terminal"].includes(termProgram) - ) + if (termProgram && ["iTerm.app", "WezTerm", "Hyper", "vscode", "ghostty", "Tabby", "rio"].includes(termProgram)) return true if (process.env.WT_SESSION) return true // Windows Terminal if (process.env.KONSOLE_VERSION) return true From 789ff74eb59362d1f39a6455b4f8cc683247db3a Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 20:03:26 +0530 Subject: [PATCH 04/10] =?UTF-8?q?fix:=20address=20multi-model=20review=20f?= =?UTF-8?q?indings=20on=20#1274=20=E2=80=94=20dialog=20selection=20race,?= =?UTF-8?q?=20missing=20CLI=20URL=20fallback,=20unit=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `workspace.tsx`: fix a real selection-desync race in `AlreadyLinkedDialog`. `manageUrl` was fetched async inside the dialog via `onMount`, so the options array could grow from 3 to 4 items ("Open in browser" inserted at index 2, pushing "Skip for now" to 3) after the dialog had already painted. dialog-select.tsx's `store.selected` is a raw numeric index, and the only effect that resyncs it fires on `props.current`/`store.filter` changes — not on the options array changing shape. A user who pressed Down twice to reach "Skip for now" before the promise resolved could press Enter and launch a browser instead. Fixed by resolving `manageUrl` in the caller (`runFlow`) before `dialog.replace()`, matching the synchronous-prop pattern `WorkspaceLinkedDialog`/`showLinkedConfirmation` already use — `AlreadyLinkedDialog` no longer has any async state. - `cli/cmd/link.ts`: print the plain manage URL as a `prompts.log.info` fallback when `terminalSupportsHyperlinks()` is false — previously a non-allowlisted terminal had no way to discover the URL at all (the OSC 8 bytes are invisible there), unlike the TUI side which falls back to a copyable toast on open() failure. - `cli/cmd/link.ts`: guard `hyperlink()` against an empty `text` (would otherwise emit an invisible zero-width clickable region). - `workspace-sidebar.tsx`: scope the sidebar tile's click target to just the name line — "(pinned via --workspace)" now renders on its own non-interactive line instead of being appended inside the clickable `` block, and `onMouseUp` is omitted entirely (not attached as a no-op) until a manage URL actually resolves. - Add `test/cli/cmd/link.test.ts` — unit coverage for the four pure helpers introduced in this PR (`stripControlChars`, `terminalSupportsHyperlinks`, `buildManageUrl`, `hyperlink`), including the control-character-injection and URL-join regressions directly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 20 ++- .../plugin/tui/altimate/workspace-sidebar.tsx | 61 +++---- .../src/plugin/tui/altimate/workspace.tsx | 35 ++-- packages/opencode/test/cli/cmd/link.test.ts | 165 ++++++++++++++++++ 4 files changed, 236 insertions(+), 45 deletions(-) create mode 100644 packages/opencode/test/cli/cmd/link.test.ts diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index c447c47c1..37d0d843d 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -49,7 +49,7 @@ const SET_UP_IN_BROWSER_SENTINEL = "__browser_handoff__" * prematurely close our hyperlink and open a spoofed one pointing wherever * they choose, with our trusted URL as the visible (but inert) prefix. * (CodeRabbit + cubic, PR #1274.) */ -function stripControlChars(text: string): string { +export function stripControlChars(text: string): string { // C0 (\x00-\x1f) + DEL (\x7f) + C1 (\x80-\x9f) — the previous range only // covered C0/DEL, leaving C1 controls unstripped. ESC (the OSC 8 breakout // vector) was always covered, but the doc comment claimed C1 coverage it @@ -75,7 +75,7 @@ function stripControlChars(text: string): string { * left off the list rather than guessing the user is on a current-enough * macOS. (Kilo, PR #1274 — corrects an earlier version of this list that * included it.) */ -function terminalSupportsHyperlinks(): boolean { +export function terminalSupportsHyperlinks(): boolean { if (!process.stdout.isTTY) return false if (process.env.TERM === "dumb" || process.env.TERM === "linux") return false const termProgram = process.env.TERM_PROGRAM @@ -94,7 +94,7 @@ function terminalSupportsHyperlinks(): boolean { * its own path/query/fragment (e.g. a local dev server), and naive * concatenation would land ``/w/`` inside the query string instead of the * path — clears search/hash for the same reason. (CodeRabbit + cubic, PR #1274.) */ -function buildManageUrl(base: URL, workspaceId: number): string { +export function buildManageUrl(base: URL, workspaceId: number): string { const u = new URL(base) u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` u.search = "" @@ -113,8 +113,8 @@ function buildManageUrl(base: URL, workspaceId: number): string { * code — emitting it unconditionally would make the name look clickable in * terminals where it isn't, so it's gated on ``terminalSupportsHyperlinks`` * (cubic, PR #1274). */ -function hyperlink(text: string, url: string | null): string { - if (!url) return text +export function hyperlink(text: string, url: string | null): string { + if (!url || !text) return text const safeText = stripControlChars(text) const OSC8 = "\x1b]8;;" const ST = "\x1b\\" @@ -217,6 +217,16 @@ export const LinkCommand = cmd({ // case the name below prints as plain (non-clickable) text. const currentManageUrl = currentId !== undefined && workspaceWebBase ? buildManageUrl(workspaceWebBase, currentId) : null + // On a terminal `terminalSupportsHyperlinks()` doesn't recognize, the + // OSC 8 wrapping below is invisible bytes and the name renders as plain + // text with no indication a URL exists at all — unlike the TUI, which + // falls back to a toast ("Could not open browser. Copy this URL: ..."). + // Print the plain URL once as a fallback the terminal can't hide, rather + // than leaving it unreachable outside the allowlist. (multi-model + // review, PR #1274.) + if (currentManageUrl && !terminalSupportsHyperlinks()) { + prompts.log.info(`Manage it at: ${currentManageUrl}`) + } const options: Array<{ value: string; label: string; hint?: string }> = [ // Only offer browser handoff for UNLINKED projects (CodeRabbit cycle 5). diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index 6d454847e..6866edf1c 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -100,40 +100,41 @@ function View(props: { api: TuiPluginApi }) { {/* Clicking the name (or the URL line below) opens the workspace * in the browser — the manage URL is deterministic from tenant * + id (see resolveManageBase above), so there's no extra - * round-trip before it's clickable. The whole line is the - * click target (mouse events only land on block-level - * ``/``, not inline ``/`` nodes), while only - * the name itself is styled to look like a link — matching the - * footer's community/docs links (sidebar/footer.tsx), which use - * the same span-style + onMouseUp pair because raw `` - * hyperlink nodes crash in this JSX layer. */} - { - const url = manageUrl() - if (url) openManageUrl(props.api, url) - }} - > + * round-trip before it's clickable. The whole line is the click + * target (mouse events only land on block-level ``/``, + * not inline ``/`` nodes), while only the name itself + * is styled to look like a link — matching the footer's docs/ + * community links (sidebar/footer.tsx), which use the same + * span-style + onMouseUp pair because raw `` hyperlink + * nodes crash in this JSX layer. ``onMouseUp`` is omitted + * entirely (not just a no-op) when there's no URL yet, so the + * name never advertises a click target that does nothing. The + * "pinned via --workspace" hint lives on its own line below + * (rather than appended inline here) so the click region + * doesn't extend over text that isn't part of the link — same + * reasoning as the URL line already being separate. (multi-model + * review, PR #1274.) */} + openManageUrl(props.api, manageUrl()!) : undefined}> {(_u) => {b().datamateName}} - {/* ``pinned via --workspace`` means "this SESSION was launched - * with --workspace and it resolved to this id". It does NOT - * mean "the current binding was set by --workspace" — if the - * user relinks mid-session to a different workspace, the pin - * disappears (id mismatch); if they relink to the same id, - * the pin correctly stays because the launch fact is - * unchanged. Known imprecision: relink-to-same-id looks - * indistinguishable from "never relinked". Accepted per - * altimate-harness-bot round 8 (option b of the review). - * ``getResolvedWorkspaceId`` returns null when the launch - * had no --workspace flag or the flag failed to resolve, - * so the pin never falsely appears for a session that - * wasn't launched with the flag. */} - - {" (pinned via --workspace)"} - + {/* ``pinned via --workspace`` means "this SESSION was launched + * with --workspace and it resolved to this id". It does NOT + * mean "the current binding was set by --workspace" — if the + * user relinks mid-session to a different workspace, the pin + * disappears (id mismatch); if they relink to the same id, + * the pin correctly stays because the launch fact is + * unchanged. Known imprecision: relink-to-same-id looks + * indistinguishable from "never relinked". Accepted per + * altimate-harness-bot round 8 (option b of the review). + * ``getResolvedWorkspaceId`` returns null when the launch + * had no --workspace flag or the flag failed to resolve, + * so the pin never falsely appears for a session that + * wasn't launched with the flag. */} + + (pinned via --workspace) + {(u) => ( openManageUrl(props.api, u())}> diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 5babd5790..218052672 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -652,6 +652,9 @@ interface AlreadyLinkedProps { hasDrift: boolean driftedWas?: string | null unverified?: boolean + /** Pre-resolved by the caller — see ``AlreadyLinkedDialog``'s comment for + * why this must not be fetched async inside the dialog itself. */ + manageUrl: string | null /** Which identifier arm resolved the binding — remote-matched projects * rebind via ``/by-remote``, path-matched via ``/by-path``. Not the same * as ``identifier.repoRemote`` / ``identifier.projectPath``, which reflect @@ -661,13 +664,20 @@ interface AlreadyLinkedProps { } function AlreadyLinkedDialog(props: AlreadyLinkedProps) { - // Best-effort — same deterministic tenant+id derivation as buildManageUrl's - // other callers. null on BYOK/unresolvable, in which case the "Open in - // browser" option below is simply omitted. - const [manageUrl, setManageUrl] = createSignal(null) - onMount(async () => { - setManageUrl(await buildManageUrl(props.workspaceId)) - }) + // ``manageUrl`` is a plain prop, resolved by the caller (``runFlow``) + // BEFORE this dialog is shown — not fetched async in an onMount here. + // dialog-select.tsx's ``store.selected`` is a raw numeric index, and + // nothing re-syncs it when ``props.options`` changes shape (the only + // effect that resyncs selection fires on `props.current`/`store.filter` + // changes, not on the options array). Options here start at 3 items and + // conditionally grow to 4 when "Open in browser" becomes available — if + // that insertion landed asynchronously after the dialog painted, a user + // who already pressed Down to reach "Skip for now" (index 2) would find + // Enter now submits "Open in browser" instead, since the array grew out + // from under a stale index. Keeping this component fully synchronous + // (matching ``WorkspaceLinkedDialog``'s ``manageUrl`` prop, resolved via + // ``showLinkedConfirmation`` before render) removes the moving target + // instead of trying to resync around it. (multi-model review, PR #1274.) // Title carries the primary context (workspace name + drift/unverified hint) // since DialogSelect doesn't take a top-level description block. Verbose but @@ -699,7 +709,7 @@ function AlreadyLinkedDialog(props: AlreadyLinkedProps) { description: "Swap this project's workspace.", }, ] - if (manageUrl()) { + if (props.manageUrl) { opts.push({ title: "Open in browser", value: "open", @@ -724,8 +734,7 @@ function AlreadyLinkedDialog(props: AlreadyLinkedProps) { return } if (option.value === "open") { - const url = manageUrl() - if (url) openManageUrl(props.api, url) + if (props.manageUrl) openManageUrl(props.api, props.manageUrl) // Stay open — opening the browser isn't a decision about the link // itself, so the user can still Attach/Re-link/Skip afterward. return @@ -1145,6 +1154,9 @@ async function runFlow(api: TuiPluginApi, directory: string): Promise { const currentIdent = serverBinding.matchedBy === "remote" ? identifier.repoRemote : identifier.projectPath const hasDrift = boundIdent != null && currentIdent != null && boundIdent !== currentIdent + // Resolved before the dialog renders — see AlreadyLinkedDialog's comment + // on why this can't be fetched async inside the dialog itself. + const manageUrl = await buildManageUrl(serverBinding.datamate.id) api.ui.dialog.replace(() => ( { matchedBy={serverBinding!.matchedBy} hasDrift={hasDrift} driftedWas={hasDrift ? boundIdent : undefined} + manageUrl={manageUrl} /> )) return @@ -1183,6 +1196,7 @@ async function runFlow(api: TuiPluginApi, directory: string): Promise { const currentIdent = cachedMatchedBy === "remote" ? identifier.repoRemote : identifier.projectPath const hasDrift = cachedIdent !== "" && currentIdent != null && cachedIdent !== currentIdent + const manageUrl = await buildManageUrl(local.datamateId) api.ui.dialog.replace(() => ( { matchedBy={cachedMatchedBy} hasDrift={hasDrift} driftedWas={hasDrift ? cachedIdent : undefined} + manageUrl={manageUrl} unverified /> )) diff --git a/packages/opencode/test/cli/cmd/link.test.ts b/packages/opencode/test/cli/cmd/link.test.ts new file mode 100644 index 000000000..6cd32d6e0 --- /dev/null +++ b/packages/opencode/test/cli/cmd/link.test.ts @@ -0,0 +1,165 @@ +// altimate_change - new file +// Unit coverage for the pure-logic helpers in +// packages/opencode/src/cli/cmd/link.ts that back the `altimate-code link` +// picker's clickable-workspace-name affordance: control-char sanitization, +// terminal capability detection, URL joining, and the OSC 8 wrapper itself. +// The interactive `@clack/prompts` flow (LinkCommand.handler) needs a TTY +// and is covered by manual verification (PR #1274), not here. +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import { buildManageUrl, hyperlink, stripControlChars, terminalSupportsHyperlinks } from "../../../src/cli/cmd/link" + +describe("stripControlChars", () => { + test("removes C0 control bytes including ESC", () => { + expect(stripControlChars("a\x1bb\x00c")).toBe("abc") + }) + + test("removes DEL and C1 control bytes", () => { + expect(stripControlChars("a\x7fb\x9fc\x80d")).toBe("abcd") + }) + + test("neutralizes an embedded OSC 8 sequence into inert text", () => { + const malicious = "name\x1b]8;;http://evil.example\x1b\\CLICK ME\x1b]8;;\x1b\\" + const sanitized = stripControlChars(malicious) + expect(sanitized).not.toContain("\x1b") + // The literal (non-ESC) bytes survive as inert text — only the escape + // bytes that would make it a live control sequence are stripped. + expect(sanitized).toBe("name]8;;http://evil.example\\CLICK ME]8;;\\") + }) + + test("leaves ordinary printable text untouched", () => { + expect(stripControlChars("Rakuten Analytics Pipeline")).toBe("Rakuten Analytics Pipeline") + }) +}) + +describe("terminalSupportsHyperlinks", () => { + const ORIGINAL_ENV = { ...process.env } + const ORIGINAL_TTY = process.stdout.isTTY + + beforeEach(() => { + for (const key of ["TERM", "TERM_PROGRAM", "WT_SESSION", "KONSOLE_VERSION", "VTE_VERSION"]) { + delete process.env[key] + } + }) + + afterEach(() => { + process.env = { ...ORIGINAL_ENV } + Object.defineProperty(process.stdout, "isTTY", { value: ORIGINAL_TTY, configurable: true }) + }) + + function setTTY(value: boolean) { + Object.defineProperty(process.stdout, "isTTY", { value, configurable: true }) + } + + test("false when stdout is not a TTY, regardless of TERM_PROGRAM", () => { + setTTY(false) + process.env.TERM_PROGRAM = "iTerm.app" + expect(terminalSupportsHyperlinks()).toBe(false) + }) + + test("false for TERM=dumb or TERM=linux even on a TTY", () => { + setTTY(true) + process.env.TERM = "dumb" + expect(terminalSupportsHyperlinks()).toBe(false) + process.env.TERM = "linux" + expect(terminalSupportsHyperlinks()).toBe(false) + }) + + test("true for known-supporting TERM_PROGRAM values", () => { + setTTY(true) + for (const program of ["iTerm.app", "WezTerm", "Hyper", "vscode", "ghostty", "Tabby", "rio"]) { + process.env.TERM_PROGRAM = program + expect(terminalSupportsHyperlinks()).toBe(true) + } + }) + + test("false for Apple_Terminal — OSC 8 support can't be inferred from TERM_PROGRAM alone", () => { + setTTY(true) + process.env.TERM_PROGRAM = "Apple_Terminal" + expect(terminalSupportsHyperlinks()).toBe(false) + }) + + test("true when WT_SESSION is set (Windows Terminal)", () => { + setTTY(true) + process.env.WT_SESSION = "some-guid" + expect(terminalSupportsHyperlinks()).toBe(true) + }) + + test("true when KONSOLE_VERSION is set", () => { + setTTY(true) + process.env.KONSOLE_VERSION = "220400" + expect(terminalSupportsHyperlinks()).toBe(true) + }) + + test("VTE_VERSION >= 5000 (>= 0.50.0) is supported, below it is not", () => { + setTTY(true) + process.env.VTE_VERSION = "5000" + expect(terminalSupportsHyperlinks()).toBe(true) + process.env.VTE_VERSION = "4800" + expect(terminalSupportsHyperlinks()).toBe(false) + }) + + test("false with no recognized signal at all", () => { + setTTY(true) + expect(terminalSupportsHyperlinks()).toBe(false) + }) +}) + +describe("buildManageUrl", () => { + test("appends /w/ to a bare origin", () => { + expect(buildManageUrl(new URL("https://tenant.ws.myaltimate.com"), 4242)).toBe( + "https://tenant.ws.myaltimate.com/w/4242", + ) + }) + + test("joins via pathname, not string concatenation, when the base carries a query/fragment", () => { + // The dev-only ALTIMATE_WORKSPACE_WEB_URL override can be an arbitrary + // URL (e.g. a local dev server) — naive `toString() + "/w/id"` + // concatenation would land the path inside the query string instead. + const url = buildManageUrl(new URL("http://localhost:3003/base?x=1#frag"), 42) + expect(url).toBe("http://localhost:3003/base/w/42") + }) + + test("normalizes a trailing slash on the base path", () => { + expect(buildManageUrl(new URL("https://host/base/"), 7)).toBe("https://host/base/w/7") + }) +}) + +describe("hyperlink", () => { + const ORIGINAL_ENV = { ...process.env } + const ORIGINAL_TTY = process.stdout.isTTY + + afterEach(() => { + process.env = { ...ORIGINAL_ENV } + Object.defineProperty(process.stdout, "isTTY", { value: ORIGINAL_TTY, configurable: true }) + }) + + test("returns text unchanged when url is null", () => { + expect(hyperlink("anas-skill-test", null)).toBe("anas-skill-test") + }) + + test("wraps text in OSC 8 with no underline when the terminal isn't recognized", () => { + Object.defineProperty(process.stdout, "isTTY", { value: false, configurable: true }) + const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") + expect(out).toBe("\x1b]8;;https://tenant.ws.myaltimate.com/w/4242\x1b\\anas-skill-test\x1b]8;;\x1b\\") + expect(out).not.toContain("\x1b[4m") + }) + + test("wraps text in OSC 8 plus underline when the terminal is recognized as supporting", () => { + Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }) + process.env.TERM_PROGRAM = "iTerm.app" + const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") + expect(out).toBe( + "\x1b]8;;https://tenant.ws.myaltimate.com/w/4242\x1b\\\x1b[4manas-skill-test\x1b[24m\x1b]8;;\x1b\\", + ) + }) + + test("sanitizes an adversarial name so it cannot open a second, spoofed link", () => { + Object.defineProperty(process.stdout, "isTTY", { value: false, configurable: true }) + const malicious = "name\x1b]8;;http://evil.example\x1b\\CLICK ME\x1b]8;;\x1b\\" + const out = hyperlink(malicious, "https://tenant.ws.myaltimate.com/w/4242") + // Exactly one real OSC 8 open + one real OSC 8 close — the malicious + // payload's own OSC 8 bytes were stripped, leaving only inert text. + expect(out.split("\x1b]8;;").length - 1).toBe(2) + expect(out).toContain("name]8;;http://evil.example\\CLICK ME]8;;\\") + }) +}) From 776d8d75d5f945c82cea4057b4d48ae70d87433f Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 20:10:23 +0530 Subject: [PATCH 05/10] =?UTF-8?q?fix:=20address=20round-3=20cubic=20findin?= =?UTF-8?q?gs=20on=20#1274=20=E2=80=94=20non-TTY=20OSC=208=20leak,=20secon?= =?UTF-8?q?d=20URL-join=20duplicate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `hyperlink()`: skip OSC 8 wrapping entirely (return bare sanitized text) when stdout isn't a TTY. Stdin can be a TTY (satisfying the handler's interactive-input check) while stdout is redirected to a file or piped — in that case no terminal is reading the bytes, so the previous "harmless when unrecognized" reasoning didn't hold: raw OSC 8 would land as literal junk in the captured output. `terminalSupportsHyperlinks()` already checked `isTTY` for the underline decision; this applies the same check before emitting any escape bytes at all. - `manageUrlFor`: delegate to `buildManageUrl` instead of its own copy of the pre-fix string-concatenation bug. Two near-identical URL builders in the same file had drifted — the first round's fix only touched `currentManageUrl`'s construction and missed this second one, used by the browser-handoff and create-flow "Manage it at:" lines. - Declined (with reasoning in `buildManageUrl`'s comment): moving the URL-join helper to a module shared with the TUI side. Contradicts this codebase's existing, documented CLI/TUI self-containment convention (same rationale as `isSafeHttpUrl`'s deliberate duplication) — the fix is consolidating within this file, not spanning the boundary. - Updated `link.test.ts` for `hyperlink()`'s new non-TTY early return, and added a test isolating "TTY but unrecognized terminal" (OSC 8, no underline) from "not a TTY at all" (no escape bytes whatsoever) — the previous tests conflated the two via a shared `isTTY: false` setup. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 43 +++++++++++++++------ packages/opencode/test/cli/cmd/link.test.ts | 18 ++++++++- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index 37d0d843d..b0db3d4ac 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -93,7 +93,18 @@ export function terminalSupportsHyperlinks(): boolean { * ``ALTIMATE_WORKSPACE_WEB_URL`` override (resolveWorkspaceWebUrl) can carry * its own path/query/fragment (e.g. a local dev server), and naive * concatenation would land ``/w/`` inside the query string instead of the - * path — clears search/hash for the same reason. (CodeRabbit + cubic, PR #1274.) */ + * path — clears search/hash for the same reason. (CodeRabbit + cubic, PR #1274.) + * + * Both in-file callers (``currentManageUrl`` in the handler, and + * ``manageUrlFor`` below) now go through this one implementation. The + * identical ``workspace.tsx``/``workspace-sidebar.tsx`` copy + * (``joinManageUrlPath``) is deliberately NOT unified with this one across a + * shared module, though: same + * CLI/TUI self-containment reasoning as ``isSafeHttpUrl``'s split (see + * ``cli/cmd/link.ts``'s other comment on that). cubic suggested a shared + * module (PR #1274 round 3); declined for that reason — but *within* this + * file, keep it to one implementation (see ``manageUrlFor``'s comment on + * why two near-identical copies in the same file already drifted once). */ export function buildManageUrl(base: URL, workspaceId: number): string { const u = new URL(base) u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` @@ -105,17 +116,23 @@ export function buildManageUrl(base: URL, workspaceId: number): string { /** Wrap ``text`` in an OSC 8 terminal hyperlink pointing at ``url``, or return * ``text`` unchanged when ``url`` is null. Unlike the TUI's `` (which * crashes in the current @opentui/solid JSX layer — see workspace-sidebar.tsx), - * plain stdout can emit OSC 8 directly: supporting terminals render it as a - * real clickable link, and terminals that don't recognize the sequence just - * skip the invisible control bytes — the visible text is unaffected either - * way, so the OSC 8 wrapping itself needs no capability check. The - * *underline*, however, is a much older and more universally-rendered SGR - * code — emitting it unconditionally would make the name look clickable in - * terminals where it isn't, so it's gated on ``terminalSupportsHyperlinks`` - * (cubic, PR #1274). */ + * plain stdout can emit OSC 8 directly: a terminal directly interpreting the + * bytes either renders a real clickable link or silently skips the sequence + * it doesn't recognize — the visible text is unaffected either way. That + * "harmless when unrecognized" argument only holds when a terminal emulator + * is actually the one reading the bytes, though: with stdout redirected to a + * file or piped into another program (stdin can still be a TTY — the + * interactive-stdin check in the handler doesn't imply stdout is a terminal + * too), there's no interpreter to skip them, so the raw escape sequence + * would land as literal junk in the captured output. Skip the OSC 8 wrapping + * entirely in that case. The *underline* is additionally gated on + * ``terminalSupportsHyperlinks`` — a much older and more universally-rendered + * SGR code than OSC 8, so emitting it unconditionally would make the name + * look clickable in terminals where it isn't. (cubic, PR #1274, rounds 2 + 3.) */ export function hyperlink(text: string, url: string | null): string { if (!url || !text) return text const safeText = stripControlChars(text) + if (!process.stdout.isTTY) return safeText const OSC8 = "\x1b]8;;" const ST = "\x1b\\" if (!terminalSupportsHyperlinks()) return `${OSC8}${url}${ST}${safeText}${OSC8}${ST}` @@ -382,13 +399,17 @@ async function runBrowserHandoff( } /** Best-effort manage-workspace URL for the current credentials. Returns null - * on BYOK / unresolvable deployments — callers omit the "Manage it at" line. */ + * on BYOK / unresolvable deployments — callers omit the "Manage it at" line. + * Delegates the actual join to ``buildManageUrl`` rather than re-deriving it — + * this function had its own copy of the pre-fix string-concatenation bug + * (cubic, PR #1274 round 3): two near-identical builders in the same file + * drifted, and only one got fixed the first time around. */ async function manageUrlFor(workspaceId: number): Promise { try { const creds = await AltimateApi.getCredentials() const base = resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) if (!base) return null - return `${base.toString().replace(/\/$/, "")}/w/${workspaceId}` + return buildManageUrl(base, workspaceId) } catch { return null } diff --git a/packages/opencode/test/cli/cmd/link.test.ts b/packages/opencode/test/cli/cmd/link.test.ts index 6cd32d6e0..19f064de6 100644 --- a/packages/opencode/test/cli/cmd/link.test.ts +++ b/packages/opencode/test/cli/cmd/link.test.ts @@ -137,8 +137,21 @@ describe("hyperlink", () => { expect(hyperlink("anas-skill-test", null)).toBe("anas-skill-test") }) - test("wraps text in OSC 8 with no underline when the terminal isn't recognized", () => { + test("returns bare sanitized text with no escape bytes at all when stdout isn't a TTY", () => { + // Even with a recognized TERM_PROGRAM — stdin can be a TTY (satisfying + // the handler's interactive-input check) while stdout is redirected to + // a file or piped, in which case no terminal is reading these bytes and + // raw OSC 8 would land as literal junk in the captured output. Object.defineProperty(process.stdout, "isTTY", { value: false, configurable: true }) + process.env.TERM_PROGRAM = "iTerm.app" + const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") + expect(out).toBe("anas-skill-test") + expect(out).not.toContain("\x1b") + }) + + test("wraps text in OSC 8 with no underline on a TTY whose terminal isn't recognized", () => { + Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }) + delete process.env.TERM_PROGRAM const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") expect(out).toBe("\x1b]8;;https://tenant.ws.myaltimate.com/w/4242\x1b\\anas-skill-test\x1b]8;;\x1b\\") expect(out).not.toContain("\x1b[4m") @@ -154,7 +167,8 @@ describe("hyperlink", () => { }) test("sanitizes an adversarial name so it cannot open a second, spoofed link", () => { - Object.defineProperty(process.stdout, "isTTY", { value: false, configurable: true }) + Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }) + delete process.env.TERM_PROGRAM const malicious = "name\x1b]8;;http://evil.example\x1b\\CLICK ME\x1b]8;;\x1b\\" const out = hyperlink(malicious, "https://tenant.ws.myaltimate.com/w/4242") // Exactly one real OSC 8 open + one real OSC 8 close — the malicious From 4c0b0b2979a69b93fc30a4e90ee1054654e22449 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 20:37:38 +0530 Subject: [PATCH 06/10] =?UTF-8?q?fix:=20address=20round-4=20review=20findi?= =?UTF-8?q?ngs=20on=20#1274=20=E2=80=94=20sanitize=20every=20raw=20name=20?= =?UTF-8?q?interpolation,=20not=20just=20hyperlink()'s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `hyperlink()`: sanitize `text` before the null-URL early return, not after — the null-URL branch previously returned the raw name unchanged (BYOK/unresolvable deployments), meaning callers relying on hyperlink() as their sanitization boundary got the raw name in exactly that case. - Validating this finding against the code showed the actual gap was much broader than the one call site flagged: every OTHER raw workspace-name interpolation in this file (~10 sites across the picker's non-current rows, the "Currently linked"/"Kept" messages, and every bind/create/ rebind success and ConflictError message) was never sanitized at all, since it never went through hyperlink() in the first place. Sanitized each — `currentName` and each picker row's `dm.name` once at their declaration/mapping, `created.datamate.name` once per function via a local, and each `res.binding.datamate_name` / `existing_datamate_name` call site individually (control flow there has multiple branches reassigning `res`, so hoisting one local was riskier than wrapping each use). - Test fixes: `link.test.ts`'s `hyperlink` describe block only cleared `TERM_PROGRAM` between tests, not the other terminalSupportsHyperlinks() signals (`WT_SESSION`/`KONSOLE_VERSION`/`VTE_VERSION`) its sibling block already isolated — an ambient one of those on the host/CI runner could make an "unsupported terminal" test spuriously pass. Also, the TTY restore helper used `Object.defineProperty(..., { value, configurable: true })`, which defaults every omitted attribute (enumerable, writable) to false — silently collapsing those flags from whatever the real descriptor had, rather than truly restoring it. Both fixed via two small shared helpers instead of duplicating cleanup per describe block. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 68 ++++++++++++++------- packages/opencode/test/cli/cmd/link.test.ts | 57 +++++++++++------ 2 files changed, 87 insertions(+), 38 deletions(-) diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index b0db3d4ac..fc3a39dfe 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -130,8 +130,16 @@ export function buildManageUrl(base: URL, workspaceId: number): string { * SGR code than OSC 8, so emitting it unconditionally would make the name * look clickable in terminals where it isn't. (cubic, PR #1274, rounds 2 + 3.) */ export function hyperlink(text: string, url: string | null): string { - if (!url || !text) return text + if (!text) return text const safeText = stripControlChars(text) + // Sanitize before checking `url` — the null-URL early return used to skip + // stripControlChars entirely, so a caller relying on hyperlink() as its + // sanitization boundary got the raw name whenever no manage URL existed + // (BYOK/unresolvable deployments). Every caller in this file now also + // sanitizes independently before calling this (defense in depth, not the + // sole boundary), but this fixes the function's own contract too. + // (CodeRabbit, PR #1274 round 4.) + if (!url) return safeText if (!process.stdout.isTTY) return safeText const OSC8 = "\x1b]8;;" const ST = "\x1b\\" @@ -221,7 +229,13 @@ export const LinkCommand = cmd({ ? projectNameFromRemote(identifier.repoRemote) : projectNameFromPath(identifier.projectPath) const currentId = existing?.datamate.id - const currentName = existing?.datamate.name + // Sanitized once here so every downstream display (the picker message, + // the "Kept" outro, hyperlink()'s own text) is covered — hyperlink() + // only sanitized its own `text` param, not the raw name reaching + // `prompts.outro`/`prompts.select`'s message directly. (CodeRabbit, + // PR #1274 round 4 — flagged one call site; the underlying gap was + // every raw name-interpolation in this file, not just that one.) + const currentName = existing ? stripControlChars(existing.datamate.name) : undefined // Only offer the browser-based handoff when the deployment supports it // (freemium only today). Enterprise / localhost / custom-domain callers @@ -274,12 +288,17 @@ export const LinkCommand = cmd({ ? "Creates a new workspace and repoints this project to it (no browser step)." : "No browser step; configure integrations later in the SaaS.", }, - ...list.map((dm) => ({ - value: String(dm.id), - label: - dm.id === currentId ? `● ${hyperlink(dm.name, currentManageUrl)}` : ` ${dm.name}`, - hint: dm.id === currentId ? "currently linked here" : undefined, - })), + ...list.map((dm) => { + // Every row's name is server-controlled (any workspace the account + // can see, not just ones this user created) — sanitize regardless + // of whether this row also goes through hyperlink() below. + const safeDmName = stripControlChars(dm.name) + return { + value: String(dm.id), + label: dm.id === currentId ? `● ${hyperlink(safeDmName, currentManageUrl)}` : ` ${safeDmName}`, + hint: dm.id === currentId ? "currently linked here" : undefined, + } + }), ] const pick = await prompts.select({ @@ -376,7 +395,7 @@ async function runBrowserHandoff( projectPath: res.binding.project_path, linkedAt: Date.now(), }, { awaitBackfill: true }) - bindSpin.stop(`Linked to "${res.binding.datamate_name}".`) + bindSpin.stop(`Linked to "${stripControlChars(res.binding.datamate_name)}".`) prompts.log.info("Saved memory blocks will sync to this workspace if memory is enabled for it.") const manageUrl = await manageUrlFor(res.binding.datamate_id) if (manageUrl) prompts.log.info(`Manage it at: ${manageUrl}`) @@ -384,8 +403,11 @@ async function runBrowserHandoff( } catch (err) { bindSpin.stop("Link failed.", 1) if (err instanceof ConflictError) { + const existingName = err.detail.existing_datamate_name + ? stripControlChars(err.detail.existing_datamate_name) + : "another workspace" prompts.log.error( - `This project is already linked to "${err.detail.existing_datamate_name ?? "another workspace"}". Workspace "${projectName}" was created but is not linked — re-run \`altimate-code link\` and pick a different action to switch, or delete the new workspace in the SaaS.`, + `This project is already linked to "${existingName}". Workspace "${projectName}" was created but is not linked — re-run \`altimate-code link\` and pick a different action to switch, or delete the new workspace in the SaaS.`, ) } else if (err instanceof NotFoundError) { prompts.log.error("Workspace not found — the tenant or workspace may have changed.") @@ -462,8 +484,11 @@ async function createThenBindOrRebind( // can pick from the list; if the pre-check missed it, this is the // authoritative signal — surface it and hint the picker. if (err instanceof ConflictError) { + const existingName = err.detail.existing_datamate_name + ? stripControlChars(err.detail.existing_datamate_name) + : "another workspace" prompts.log.error( - `This project is already linked to "${err.detail.existing_datamate_name ?? "another workspace"}". Re-run \`altimate-code link\` to switch to a different workspace.`, + `This project is already linked to "${existingName}". Re-run \`altimate-code link\` to switch to a different workspace.`, ) } else { prompts.log.error(err instanceof Error ? err.message : String(err)) @@ -471,7 +496,11 @@ async function createThenBindOrRebind( process.exitCode = 1 return } - spin.stop(`Workspace "${created.datamate.name}" created.`) + // Sanitized once — echoed back from the create-workspace API response + // (not the locally-typed `name` param), so it's technically server data + // even though it usually just round-trips the caller's own auto-name. + const safeCreatedName = stripControlChars(created.datamate.name) + spin.stop(`Workspace "${safeCreatedName}" created.`) // If the project was already linked, the new workspace exists but the // binding still points at the OLD workspace — rebind so the project is @@ -479,7 +508,7 @@ async function createThenBindOrRebind( // wrote the binding as part of the atomic create; we're done. if (existing) { const rebindSpin = prompts.spinner() - rebindSpin.start(`Repointing project at "${created.datamate.name}"...`) + rebindSpin.start(`Repointing project at "${safeCreatedName}"...`) try { await rebindByMatchedIdentifier({ identifier, @@ -487,11 +516,11 @@ async function createThenBindOrRebind( expectedCurrentDatamateId: existing.datamate.id, matchedBy: existing.matchedBy, }) - rebindSpin.stop(`Project is now linked to "${created.datamate.name}".`) + rebindSpin.stop(`Project is now linked to "${safeCreatedName}".`) } catch (err) { rebindSpin.stop("Could not repoint the project.", 1) prompts.log.error( - `Workspace "${created.datamate.name}" was CREATED but could not be linked to this project. ${err instanceof Error ? err.message : String(err)} — re-run \`altimate-code link\` to retry (or delete the workspace in the SaaS).`, + `Workspace "${safeCreatedName}" was CREATED but could not be linked to this project. ${err instanceof Error ? err.message : String(err)} — re-run \`altimate-code link\` to retry (or delete the workspace in the SaaS).`, ) process.exitCode = 1 return @@ -609,7 +638,7 @@ async function bindOrRebind( targetDatamateId, }) } - rebindSpin.stop(`Re-linked to "${res.binding.datamate_name}".`) + rebindSpin.stop(`Re-linked to "${stripControlChars(res.binding.datamate_name)}".`) } catch (retryErr) { rebindSpin.stop("Re-link failed.", 1) throw retryErr @@ -627,11 +656,8 @@ async function bindOrRebind( projectPath: res.binding.project_path, linkedAt: Date.now(), }, { awaitBackfill: true }) - spin.stop( - isRebind - ? `Re-linked to "${res.binding.datamate_name}".` - : `Linked to "${res.binding.datamate_name}".`, - ) + const safeResName = stripControlChars(res.binding.datamate_name) + spin.stop(isRebind ? `Re-linked to "${safeResName}".` : `Linked to "${safeResName}".`) prompts.log.info("Saved memory blocks will sync to this workspace if memory is enabled for it.") const manageUrl = await manageUrlFor(res.binding.datamate_id) if (manageUrl) prompts.log.info(`Manage it at: ${manageUrl}`) diff --git a/packages/opencode/test/cli/cmd/link.test.ts b/packages/opencode/test/cli/cmd/link.test.ts index 19f064de6..5f94c2f60 100644 --- a/packages/opencode/test/cli/cmd/link.test.ts +++ b/packages/opencode/test/cli/cmd/link.test.ts @@ -8,6 +8,30 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test" import { buildManageUrl, hyperlink, stripControlChars, terminalSupportsHyperlinks } from "../../../src/cli/cmd/link" +// Shared by both describe blocks below that exercise terminalSupportsHyperlinks +// (directly, or indirectly via hyperlink()). Object.defineProperty defaults +// omitted attributes (enumerable/writable) to false, so restoring via +// `{ value, configurable: true }` alone would silently collapse those flags +// from whatever the real descriptor had — capture and restore the full +// descriptor instead. (CodeRabbit, PR #1274 round 4.) The env vars cleared +// here are every signal terminalSupportsHyperlinks() reads — an ambient +// WT_SESSION/KONSOLE_VERSION/VTE_VERSION on the host or CI runner would +// otherwise make an "unsupported" test spuriously pass. +const TERMINAL_ENV_KEYS = ["TERM", "TERM_PROGRAM", "WT_SESSION", "KONSOLE_VERSION", "VTE_VERSION"] as const +const ORIGINAL_TTY_DESCRIPTOR = Object.getOwnPropertyDescriptor(process.stdout, "isTTY") + +function clearTerminalEnv() { + for (const key of TERMINAL_ENV_KEYS) delete process.env[key] +} + +function setTTY(value: boolean) { + Object.defineProperty(process.stdout, "isTTY", { value, configurable: true }) +} + +function restoreTTY() { + if (ORIGINAL_TTY_DESCRIPTOR) Object.defineProperty(process.stdout, "isTTY", ORIGINAL_TTY_DESCRIPTOR) +} + describe("stripControlChars", () => { test("removes C0 control bytes including ESC", () => { expect(stripControlChars("a\x1bb\x00c")).toBe("abc") @@ -33,23 +57,14 @@ describe("stripControlChars", () => { describe("terminalSupportsHyperlinks", () => { const ORIGINAL_ENV = { ...process.env } - const ORIGINAL_TTY = process.stdout.isTTY - beforeEach(() => { - for (const key of ["TERM", "TERM_PROGRAM", "WT_SESSION", "KONSOLE_VERSION", "VTE_VERSION"]) { - delete process.env[key] - } - }) + beforeEach(clearTerminalEnv) afterEach(() => { process.env = { ...ORIGINAL_ENV } - Object.defineProperty(process.stdout, "isTTY", { value: ORIGINAL_TTY, configurable: true }) + restoreTTY() }) - function setTTY(value: boolean) { - Object.defineProperty(process.stdout, "isTTY", { value, configurable: true }) - } - test("false when stdout is not a TTY, regardless of TERM_PROGRAM", () => { setTTY(false) process.env.TERM_PROGRAM = "iTerm.app" @@ -126,23 +141,31 @@ describe("buildManageUrl", () => { describe("hyperlink", () => { const ORIGINAL_ENV = { ...process.env } - const ORIGINAL_TTY = process.stdout.isTTY + + beforeEach(clearTerminalEnv) afterEach(() => { process.env = { ...ORIGINAL_ENV } - Object.defineProperty(process.stdout, "isTTY", { value: ORIGINAL_TTY, configurable: true }) + restoreTTY() }) test("returns text unchanged when url is null", () => { expect(hyperlink("anas-skill-test", null)).toBe("anas-skill-test") }) + test("sanitizes text even when url is null", () => { + const malicious = "name\x1b]8;;http://evil.example\x1b\\CLICK ME\x1b]8;;\x1b\\" + const out = hyperlink(malicious, null) + expect(out).not.toContain("\x1b") + expect(out).toBe("name]8;;http://evil.example\\CLICK ME]8;;\\") + }) + test("returns bare sanitized text with no escape bytes at all when stdout isn't a TTY", () => { // Even with a recognized TERM_PROGRAM — stdin can be a TTY (satisfying // the handler's interactive-input check) while stdout is redirected to // a file or piped, in which case no terminal is reading these bytes and // raw OSC 8 would land as literal junk in the captured output. - Object.defineProperty(process.stdout, "isTTY", { value: false, configurable: true }) + setTTY(false) process.env.TERM_PROGRAM = "iTerm.app" const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") expect(out).toBe("anas-skill-test") @@ -150,7 +173,7 @@ describe("hyperlink", () => { }) test("wraps text in OSC 8 with no underline on a TTY whose terminal isn't recognized", () => { - Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }) + setTTY(true) delete process.env.TERM_PROGRAM const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") expect(out).toBe("\x1b]8;;https://tenant.ws.myaltimate.com/w/4242\x1b\\anas-skill-test\x1b]8;;\x1b\\") @@ -158,7 +181,7 @@ describe("hyperlink", () => { }) test("wraps text in OSC 8 plus underline when the terminal is recognized as supporting", () => { - Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }) + setTTY(true) process.env.TERM_PROGRAM = "iTerm.app" const out = hyperlink("anas-skill-test", "https://tenant.ws.myaltimate.com/w/4242") expect(out).toBe( @@ -167,7 +190,7 @@ describe("hyperlink", () => { }) test("sanitizes an adversarial name so it cannot open a second, spoofed link", () => { - Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }) + setTTY(true) delete process.env.TERM_PROGRAM const malicious = "name\x1b]8;;http://evil.example\x1b\\CLICK ME\x1b]8;;\x1b\\" const out = hyperlink(malicious, "https://tenant.ws.myaltimate.com/w/4242") From e3dc9549cbb874c21c61331677f7d3778063fc3c Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 21:34:38 +0530 Subject: [PATCH 07/10] =?UTF-8?q?fix:=20address=20round-5=20cubic=20findin?= =?UTF-8?q?g=20on=20#1274=20=E2=80=94=20restoreTTY()=20was=20a=20silent=20?= =?UTF-8?q?no-op?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-4 fix for the TTY-descriptor-restoration finding introduced a new bug of its own: `process.stdout.isTTY` has no OWN property descriptor in the common case (stdout piped/redirected, which is how it always runs under a test runner) — Node only sets it as an own property when the stream genuinely is a TTY. So `ORIGINAL_TTY_DESCRIPTOR` was `undefined` in virtually every real test run, and `restoreTTY()`'s `if (descriptor)` guard made it a complete no-op: the property `setTTY()` added stayed shadowed on `process.stdout` for the rest of the process instead of being restored, contradicting the comment's stated intent. Verified empirically (bun -e) before fixing: process.stdout.isTTY genuinely has no own descriptor and isn't found anywhere on its prototype chain when not a real TTY. Fixed by deleting the property when there was no original descriptor to restore. Added a regression test, and confirmed by temporarily reverting the fix that the test actually fails without it (not vacuously green). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/test/cli/cmd/link.test.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/opencode/test/cli/cmd/link.test.ts b/packages/opencode/test/cli/cmd/link.test.ts index 5f94c2f60..c5bc44102 100644 --- a/packages/opencode/test/cli/cmd/link.test.ts +++ b/packages/opencode/test/cli/cmd/link.test.ts @@ -29,9 +29,29 @@ function setTTY(value: boolean) { } function restoreTTY() { + // `isTTY` is not an own property of process.stdout in the common case + // (stdout piped/redirected, as it always is under a test runner) — Node + // only sets it as an own property when the stream genuinely is a TTY. So + // ORIGINAL_TTY_DESCRIPTOR is `undefined` in virtually every real test run, + // and restoring by re-defining only when it's truthy was a no-op: the + // property setTTY() added stayed shadowed on process.stdout for the rest + // of the process. Delete it in that case instead of leaving it dangling. + // (cubic, PR #1274 round 5 — caught in the very helper meant to fix the + // previous round's descriptor-restoration finding.) if (ORIGINAL_TTY_DESCRIPTOR) Object.defineProperty(process.stdout, "isTTY", ORIGINAL_TTY_DESCRIPTOR) + else delete (process.stdout as { isTTY?: boolean }).isTTY } +describe("restoreTTY (test-helper regression)", () => { + test("actually removes the isTTY property setTTY() added, instead of leaving it dangling", () => { + const before = Object.getOwnPropertyDescriptor(process.stdout, "isTTY") + setTTY(true) + expect(Object.getOwnPropertyDescriptor(process.stdout, "isTTY")).toBeDefined() + restoreTTY() + expect(Object.getOwnPropertyDescriptor(process.stdout, "isTTY")).toEqual(before) + }) +}) + describe("stripControlChars", () => { test("removes C0 control bytes including ESC", () => { expect(stripControlChars("a\x1bb\x00c")).toBe("abc") From e8cc4378ddb717e8a8f219ec1da0923f0a363a1c Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 8 Sep 2026 21:43:11 +0530 Subject: [PATCH 08/10] fix: address round-6 cubic finding, decline incorrect Kilo finding on #1274 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - cubic (valid): the round-5 regression test didn't wrap its first assertion in try/finally, so a failure there would skip restoreTTY() and leak the mutated process.stdout.isTTY into every later test. Fixed. - Kilo (declined — verified factually wrong): claimed `isTTY` is "an inherited prototype property... regardless of whether stdout is a TTY" and that the `if (ORIGINAL_TTY_DESCRIPTOR)` branch in restoreTTY() is dead code, suggesting it be simplified to an unconditional `delete`. Checked empirically in a real pty (`tmux new-session ... bun -e`, because the sandboxed shell always pipes stdout) rather than trusting either the bot's claim or my own prior comment: when process.stdout genuinely IS a TTY, `isTTY` IS a real own property (`{value: true, writable: true, enumerable: true, configurable: true}`), not inherited, not undefined. Applying Kilo's suggested "fix" would have made restoreTTY() permanently delete the real isTTY property instead of restoring it whenever these tests run in an actual interactive terminal — a regression, not a fix. Left the conditional as-is; expanded the comment to document the verification so this doesn't get relitigated incorrectly again. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/test/cli/cmd/link.test.ts | 36 ++++++++++++++------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/packages/opencode/test/cli/cmd/link.test.ts b/packages/opencode/test/cli/cmd/link.test.ts index c5bc44102..c178a0ad3 100644 --- a/packages/opencode/test/cli/cmd/link.test.ts +++ b/packages/opencode/test/cli/cmd/link.test.ts @@ -29,15 +29,23 @@ function setTTY(value: boolean) { } function restoreTTY() { - // `isTTY` is not an own property of process.stdout in the common case - // (stdout piped/redirected, as it always is under a test runner) — Node - // only sets it as an own property when the stream genuinely is a TTY. So - // ORIGINAL_TTY_DESCRIPTOR is `undefined` in virtually every real test run, - // and restoring by re-defining only when it's truthy was a no-op: the - // property setTTY() added stayed shadowed on process.stdout for the rest - // of the process. Delete it in that case instead of leaving it dangling. - // (cubic, PR #1274 round 5 — caught in the very helper meant to fix the - // previous round's descriptor-restoration finding.) + // Whether `isTTY` is an own property of process.stdout genuinely depends + // on whether stdout IS a real TTY — it is NOT always inherited/undefined + // (a later review round claimed otherwise; verified wrong empirically, see + // below). Node backs `process.stdout` with different stream classes + // depending on what fd 1 actually is: a `tty.WriteStream` when it's a + // terminal (which sets `this.isTTY = true` as a genuine own instance + // property — confirmed via `Object.getOwnPropertyDescriptor` inside a real + // pty, e.g. `tmux new-session ... bun -e '...'`, where it returns + // `{value: true, writable: true, enumerable: true, configurable: true}`, + // not undefined), versus a plain stream with no `isTTY` at all when piped/ + // redirected (which is how it always runs under `bun test`/CI, hence + // ORIGINAL_TTY_DESCRIPTOR being undefined in THAT case specifically). + // So: re-define when there was a real descriptor to restore (interactive + // `bun test` run), delete when there wasn't (everywhere else) — both + // branches are reachable and necessary, not dead code. (cubic, PR #1274 + // round 5, on the previous version of this function that always no-op'd + // for the common non-TTY case.) if (ORIGINAL_TTY_DESCRIPTOR) Object.defineProperty(process.stdout, "isTTY", ORIGINAL_TTY_DESCRIPTOR) else delete (process.stdout as { isTTY?: boolean }).isTTY } @@ -46,8 +54,14 @@ describe("restoreTTY (test-helper regression)", () => { test("actually removes the isTTY property setTTY() added, instead of leaving it dangling", () => { const before = Object.getOwnPropertyDescriptor(process.stdout, "isTTY") setTTY(true) - expect(Object.getOwnPropertyDescriptor(process.stdout, "isTTY")).toBeDefined() - restoreTTY() + // If the assertion below throws, restoreTTY() must still run — otherwise + // this test's own process-global mutation leaks into every test after + // it. (cubic, PR #1274 round 6.) + try { + expect(Object.getOwnPropertyDescriptor(process.stdout, "isTTY")).toBeDefined() + } finally { + restoreTTY() + } expect(Object.getOwnPropertyDescriptor(process.stdout, "isTTY")).toEqual(before) }) }) From e016cc0258782b8b90a9debad3906118ba4377d4 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 9 Sep 2026 20:57:08 +0530 Subject: [PATCH 09/10] =?UTF-8?q?fix:=20address=20round-7=20multi-model=20?= =?UTF-8?q?review=20on=20#1274=20=E2=80=94=20third=20sanitization=20gap,?= =?UTF-8?q?=20wider=20hyperlink()=20contract,=20deduplicate=20URL-join=20a?= =?UTF-8?q?cross=20CLI/TUI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `bindOrRebind`'s `ConflictError` handler had a third, unsanitized `existing_datamate_name` interpolation — round 4's sanitization sweep fixed the other two occurrences of this exact pattern (in `runBrowserHandoff` and `createThenBindOrRebind`) but missed this one in a third function, making round 4's "every raw name-interpolation" claim inaccurate. Fixed, and swept the whole file again to confirm no others remain (all display sites go through `stripControlChars`; the three `recordApprovedBinding` cache-storage calls deliberately still store the raw value, which is correct — sanitization belongs at display time, not storage time). - `hyperlink()` now also validates `url` via the existing `isSafeHttpUrl`, not just sanitizing `text`. It's an exported function now (tests import it), so its contract is wider than its two in-file callers — both of which only ever pass a `buildManageUrl(...)`-derived trusted URL, but an external caller passing something unvalidated wouldn't get the same protection the `text` side already has. - Moved `buildManageUrl` (the URL-join helper, previously duplicated as three near-identical private copies across `link.ts`, `workspace.tsx`, and `workspace-sidebar.tsx`) to `browser-handoff.ts`, beside `resolveWorkspaceWebUrl` — a module all three files already import for that function, so this doesn't cross the deliberate CLI/TUI self-containment boundary the way `isSafeHttpUrl`'s split exists to avoid. Round 3's URL-join bug needed three separate edits because of this exact duplication; consolidating deletes the bug class instead of leaving three instances of it to individually stay in sync. Moved its tests to `browser-handoff.test.ts` alongside the function. - Fixed a stale/circular doc comment on `openManageUrl` that described itself via a "same behavior as WorkspaceLinkedDialog's open action" cross-reference — that action now just calls `openManageUrl` directly (round-1 refactor), making the comment confusing rather than useful. - Declined (per the review's own "possibly deliberate — ignore if so"): the differing "open in browser" wording across `WorkspaceLinkedDialog` and `AlreadyLinkedDialog` — one is a post-creation success card, the other a you're-already-linked prompt; the differing phrasing fits the differing context. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- .../src/altimate/workspace/browser-handoff.ts | 24 ++++++++++ packages/opencode/src/cli/cmd/link.ts | 44 +++++++------------ .../plugin/tui/altimate/workspace-sidebar.tsx | 7 ++- .../src/plugin/tui/altimate/workspace.tsx | 28 ++++-------- .../workspace/browser-handoff.test.ts | 27 ++++++++++++ packages/opencode/test/cli/cmd/link.test.ts | 34 +++++--------- 6 files changed, 90 insertions(+), 74 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/browser-handoff.ts b/packages/opencode/src/altimate/workspace/browser-handoff.ts index 7e815da52..e5405ae35 100644 --- a/packages/opencode/src/altimate/workspace/browser-handoff.ts +++ b/packages/opencode/src/altimate/workspace/browser-handoff.ts @@ -184,6 +184,30 @@ export function resolveWorkspaceWebUrl(altimateUrl: string, tenant: string): URL } } +/** Append ``/w/`` to ``base``'s pathname using real URL semantics, rather + * than string-concatenating ``toString()``. The dev-only + * ``ALTIMATE_WORKSPACE_WEB_URL`` override (above) can carry its own path/ + * query/fragment (e.g. a local dev server), and naive concatenation would + * land ``/w/`` inside the query string instead of the path — clears + * search/hash for the same reason. + * + * Lives beside ``resolveWorkspaceWebUrl`` (this module is already imported + * by both the CLI (``cli/cmd/link.ts``) and the TUI plugin + * (``plugin/tui/altimate/workspace.tsx`` / ``workspace-sidebar.tsx``) for + * that function, so sharing this one too doesn't cross the deliberate CLI/ + * TUI self-containment boundary the way importing FROM one side INTO the + * other would (see ``isSafeHttpUrl``'s split in ``link.ts`` for that + * reasoning). Previously duplicated as three near-identical private copies + * — one bug (missing this exact fix) needed three separate edits to close. + * (multi-model review, PR #1274 round 7.) */ +export function buildManageUrl(base: URL, workspaceId: number): string { + const u = new URL(base) + u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` + u.search = "" + u.hash = "" + return u.toString() +} + interface HandoffPending { state: string expectedTenant: string diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index fc3a39dfe..e9352611d 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -33,6 +33,7 @@ import { resolveProjectIdentifier, } from "@/altimate/workspace/detect" import { + buildManageUrl, openWorkspaceBrowserHandoff, resolveWorkspaceWebUrl, type HandoffResult, @@ -88,31 +89,6 @@ export function terminalSupportsHyperlinks(): boolean { return false } -/** Append ``/w/`` to ``base``'s pathname using real URL semantics, rather - * than string-concatenating ``toString()``. The dev-only - * ``ALTIMATE_WORKSPACE_WEB_URL`` override (resolveWorkspaceWebUrl) can carry - * its own path/query/fragment (e.g. a local dev server), and naive - * concatenation would land ``/w/`` inside the query string instead of the - * path — clears search/hash for the same reason. (CodeRabbit + cubic, PR #1274.) - * - * Both in-file callers (``currentManageUrl`` in the handler, and - * ``manageUrlFor`` below) now go through this one implementation. The - * identical ``workspace.tsx``/``workspace-sidebar.tsx`` copy - * (``joinManageUrlPath``) is deliberately NOT unified with this one across a - * shared module, though: same - * CLI/TUI self-containment reasoning as ``isSafeHttpUrl``'s split (see - * ``cli/cmd/link.ts``'s other comment on that). cubic suggested a shared - * module (PR #1274 round 3); declined for that reason — but *within* this - * file, keep it to one implementation (see ``manageUrlFor``'s comment on - * why two near-identical copies in the same file already drifted once). */ -export function buildManageUrl(base: URL, workspaceId: number): string { - const u = new URL(base) - u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` - u.search = "" - u.hash = "" - return u.toString() -} - /** Wrap ``text`` in an OSC 8 terminal hyperlink pointing at ``url``, or return * ``text`` unchanged when ``url`` is null. Unlike the TUI's `` (which * crashes in the current @opentui/solid JSX layer — see workspace-sidebar.tsx), @@ -139,7 +115,16 @@ export function hyperlink(text: string, url: string | null): string { // sanitizes independently before calling this (defense in depth, not the // sole boundary), but this fixes the function's own contract too. // (CodeRabbit, PR #1274 round 4.) - if (!url) return safeText + // + // Also validate `url` itself, not just `text` — hyperlink() is exported + // (tests import it directly), so its contract is wider than its two + // in-file callers, both of which only ever pass a `buildManageUrl(...)`- + // derived trusted URL. A hypothetical external caller passing something + // unvalidated (e.g. a raw `manage_url` straight from an API response) + // would otherwise defeat the escaping this function is careful about on + // the `text` side while doing nothing for `url`. (multi-model review, PR + // #1274 round 7.) + if (!url || !isSafeHttpUrl(url)) return safeText if (!process.stdout.isTTY) return safeText const OSC8 = "\x1b]8;;" const ST = "\x1b\\" @@ -665,9 +650,10 @@ async function bindOrRebind( } catch (err) { spin.stop(isRebind ? `Re-link failed.` : `Link failed.`, 1) if (err instanceof ConflictError) { - prompts.log.error( - `Already linked to "${err.detail.existing_datamate_name ?? "another workspace"}". Re-run \`altimate-code link\` to switch.`, - ) + const existingName = err.detail.existing_datamate_name + ? stripControlChars(err.detail.existing_datamate_name) + : "another workspace" + prompts.log.error(`Already linked to "${existingName}". Re-run \`altimate-code link\` to switch.`) } else if (err instanceof PreconditionFailedError) { prompts.log.error("Someone else re-linked this project — re-run and try again.") } else if (err instanceof NotFoundError) { diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index 6866edf1c..4cbcb9a91 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -10,10 +10,13 @@ import type { TuiPlugin, TuiPluginApi } from "@opencode-ai/plugin/tui" import type { BuiltinTuiPlugin } from "@opencode-ai/tui/builtins" import { createSignal, onCleanup, onMount, Show } from "solid-js" import { readLocalBinding, type CachedBinding } from "@/altimate/workspace/state" -import { resolveWorkspaceWebUrl } from "@/altimate/workspace/browser-handoff" +import { + buildManageUrl as joinManageUrlPath, + resolveWorkspaceWebUrl, +} from "@/altimate/workspace/browser-handoff" import { getResolvedWorkspaceId } from "@/altimate/workspace/session-context" import { AltimateApi } from "@/altimate/api/client" -import { openManageUrl, joinManageUrlPath } from "./workspace" +import { openManageUrl } from "./workspace" const id = "altimate:sidebar-workspace" diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 218052672..a9f9c90eb 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -39,6 +39,7 @@ import { type ProjectIdentifier, } from "@/altimate/workspace/api-client" import { + buildManageUrl as joinManageUrlPath, openWorkspaceBrowserHandoff, resolveWorkspaceWebUrl, type HandoffResult, @@ -241,21 +242,6 @@ function OfferDialog(props: OfferProps) { ) } -/** Append ``/w/`` to ``base``'s pathname using real URL semantics, rather - * than string-concatenating ``toString()``. The dev-only - * ``ALTIMATE_WORKSPACE_WEB_URL`` override (resolveWorkspaceWebUrl) can carry - * its own path/query/fragment (e.g. a local dev server), and naive - * concatenation would land ``/w/`` inside the query string instead of the - * path — clears search/hash for the same reason. (CodeRabbit + cubic on - * PR #1274's identical bug in cli/cmd/link.ts.) */ -export function joinManageUrlPath(base: URL, workspaceId: number): string { - const u = new URL(base) - u.pathname = `${u.pathname.replace(/\/+$/, "")}/w/${workspaceId}` - u.search = "" - u.hash = "" - return u.toString() -} - /** Build the SaaS manage-workspace URL for a bound workspace. Deterministic * from tenant + id, so any caller can construct it without an extra round-trip. * Returns null when the current deployment isn't the freemium web (BYOK or @@ -592,11 +578,13 @@ function isSafeHttpUrl(url: string): boolean { } } -/** Guarded ``open(url)`` for a workspace manage-URL, with the same - * refuse-and-toast / catch-and-toast behavior as ``WorkspaceLinkedDialog``'s - * "open" action below. Shared with ``workspace-sidebar.tsx`` (both live under - * this TUI plugin path — unlike ``isSafeHttpUrl``'s CLI/TUI split, there's no - * reason for these two call sites to diverge). */ +/** Guarded ``open(url)`` for a workspace manage-URL: refuses (and toasts) a + * non-http(s) URL before calling ``open()``, and toasts if ``open()`` itself + * fails. The single implementation every "open in browser" action in this + * TUI plugin calls — ``WorkspaceLinkedDialog``, ``AlreadyLinkedDialog``, and + * ``workspace-sidebar.tsx`` (all live under this same TUI plugin path — + * unlike ``isSafeHttpUrl``'s CLI/TUI split, there's no reason for these call + * sites to diverge). */ export function openManageUrl(api: TuiPluginApi, url: string) { if (!isSafeHttpUrl(url)) { api.ui.toast({ diff --git a/packages/opencode/test/altimate/workspace/browser-handoff.test.ts b/packages/opencode/test/altimate/workspace/browser-handoff.test.ts index 00ba3be32..f94ce5ffc 100644 --- a/packages/opencode/test/altimate/workspace/browser-handoff.test.ts +++ b/packages/opencode/test/altimate/workspace/browser-handoff.test.ts @@ -12,6 +12,7 @@ import { createServer, connect } from "node:net" import { AltimateApi } from "../../../src/altimate/api/client" import { + buildManageUrl, openWorkspaceBrowserHandoff, resolveWorkspaceWebUrl, runHandoffWithOpener, @@ -125,6 +126,32 @@ describe("resolveWorkspaceWebUrl", () => { }) }) +// Moved here from cli/cmd/link.test.ts (PR #1274 round 7) — buildManageUrl +// itself moved from a private cli/cmd/link.ts helper to live beside +// resolveWorkspaceWebUrl, since both the CLI and the TUI plugin already +// import this module for that function. Previously duplicated as three +// near-identical private copies (link.ts, workspace.tsx, workspace-sidebar.tsx); +// one bug (a naive string-concat URL join) needed three separate fixes to close. +describe("buildManageUrl", () => { + test("appends /w/ to a bare origin", () => { + expect(buildManageUrl(new URL("https://tenant.ws.myaltimate.com"), 4242)).toBe( + "https://tenant.ws.myaltimate.com/w/4242", + ) + }) + + test("joins via pathname, not string concatenation, when the base carries a query/fragment", () => { + // The dev-only ALTIMATE_WORKSPACE_WEB_URL override can be an arbitrary + // URL (e.g. a local dev server) — naive `toString() + "/w/id"` + // concatenation would land the path inside the query string instead. + const url = buildManageUrl(new URL("http://localhost:3003/base?x=1#frag"), 42) + expect(url).toBe("http://localhost:3003/base/w/42") + }) + + test("normalizes a trailing slash on the base path", () => { + expect(buildManageUrl(new URL("https://host/base/"), 7)).toBe("https://host/base/w/7") + }) +}) + // ───────────────────────────────────────────────────────────────────────────── // openWorkspaceBrowserHandoff — pre-flight failures (do not open a browser) // ───────────────────────────────────────────────────────────────────────────── diff --git a/packages/opencode/test/cli/cmd/link.test.ts b/packages/opencode/test/cli/cmd/link.test.ts index c178a0ad3..aff772d73 100644 --- a/packages/opencode/test/cli/cmd/link.test.ts +++ b/packages/opencode/test/cli/cmd/link.test.ts @@ -2,11 +2,14 @@ // Unit coverage for the pure-logic helpers in // packages/opencode/src/cli/cmd/link.ts that back the `altimate-code link` // picker's clickable-workspace-name affordance: control-char sanitization, -// terminal capability detection, URL joining, and the OSC 8 wrapper itself. -// The interactive `@clack/prompts` flow (LinkCommand.handler) needs a TTY -// and is covered by manual verification (PR #1274), not here. +// terminal capability detection, and the OSC 8 wrapper itself. URL joining +// (`buildManageUrl`) is tested in +// test/altimate/workspace/browser-handoff.test.ts, where the function now +// lives (shared with the TUI plugin). The interactive `@clack/prompts` flow +// (LinkCommand.handler) needs a TTY and is covered by manual verification +// (PR #1274), not here. import { afterEach, beforeEach, describe, expect, test } from "bun:test" -import { buildManageUrl, hyperlink, stripControlChars, terminalSupportsHyperlinks } from "../../../src/cli/cmd/link" +import { hyperlink, stripControlChars, terminalSupportsHyperlinks } from "../../../src/cli/cmd/link" // Shared by both describe blocks below that exercise terminalSupportsHyperlinks // (directly, or indirectly via hyperlink()). Object.defineProperty defaults @@ -153,25 +156,10 @@ describe("terminalSupportsHyperlinks", () => { }) }) -describe("buildManageUrl", () => { - test("appends /w/ to a bare origin", () => { - expect(buildManageUrl(new URL("https://tenant.ws.myaltimate.com"), 4242)).toBe( - "https://tenant.ws.myaltimate.com/w/4242", - ) - }) - - test("joins via pathname, not string concatenation, when the base carries a query/fragment", () => { - // The dev-only ALTIMATE_WORKSPACE_WEB_URL override can be an arbitrary - // URL (e.g. a local dev server) — naive `toString() + "/w/id"` - // concatenation would land the path inside the query string instead. - const url = buildManageUrl(new URL("http://localhost:3003/base?x=1#frag"), 42) - expect(url).toBe("http://localhost:3003/base/w/42") - }) - - test("normalizes a trailing slash on the base path", () => { - expect(buildManageUrl(new URL("https://host/base/"), 7)).toBe("https://host/base/w/7") - }) -}) +// buildManageUrl's own tests moved to +// test/altimate/workspace/browser-handoff.test.ts (PR #1274 round 7) — the +// function itself moved there too, since it's now shared by the CLI and the +// TUI plugin rather than a private cli/cmd/link.ts helper. describe("hyperlink", () => { const ORIGINAL_ENV = { ...process.env } From 5b278b431a676071ff93f11f7d4693cd9f4a6cae Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 10 Sep 2026 16:18:38 +0530 Subject: [PATCH 10/10] =?UTF-8?q?fix:=20address=20round-8=20bot=20findings?= =?UTF-8?q?=20on=20#1274=20=E2=80=94=20url=20control-byte=20bypass,=20thir?= =?UTF-8?q?d=20conflict-name=20duplicate,=20naming=20collision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `hyperlink()`: `isSafeHttpUrl(url)` only checks that `url` parses as http(s) via `new URL()` — it doesn't sanitize, and doesn't return the re-serialized/encoded form. Verified empirically that a string can contain a live ESC byte and still parse successfully as a valid https: URL, so a hypothetical external caller could pass a URL that both (a) passes `isSafeHttpUrl` and (b) still carries the raw control bytes that get interpolated directly — defeating the OSC 8 boundary the same way the `text` sanitization exists to prevent, just on the other side of the wrapper. Now rejects (falls back to plain text) rather than stripping, since a mangled URL is worse than no link. Added tests for both the plain non-http(s)-scheme case and this control-byte case. - Extracted `conflictExistingName()` — a third near-identical copy of the `existing_datamate_name ? stripControlChars(...) : "another workspace"` ternary had appeared in a third catch block (this file's own `buildManageUrl` duplication history predicted exactly this failure mode). All three call sites now share one implementation. - `workspace.tsx` imported the shared `buildManageUrl` under an alias (`as joinManageUrlPath`) solely to avoid colliding with its own local async wrapper of the same name — which made the *real* `buildManageUrl` invisible under that name inside the file, and left `workspace.tsx` and `workspace-sidebar.tsx` importing the same function under two different local names. Renamed the local wrapper to `resolveManageUrl` and import the shared function under its real name in both files. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JsVwcBQP6VUwqEej3vK2zG --- packages/opencode/src/cli/cmd/link.ts | 38 ++++++++++++++----- .../plugin/tui/altimate/workspace-sidebar.tsx | 7 +--- .../src/plugin/tui/altimate/workspace.tsx | 26 ++++++++----- packages/opencode/test/cli/cmd/link.test.ts | 24 ++++++++++++ 4 files changed, 71 insertions(+), 24 deletions(-) diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index e9352611d..5ad18b864 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -59,6 +59,17 @@ export function stripControlChars(text: string): string { return text.replace(/[\x00-\x1f\x7f-\x9f]/g, "") } +/** Sanitized display name for a ``ConflictError``'s existing-binding name, + * with a stable fallback when the server didn't send one. A third + * near-identical copy of this exact ternary appeared across three different + * catch blocks in this file before being extracted here — same drift risk + * ``buildManageUrl``'s move to ``browser-handoff.ts`` (see that function's + * comment) was extracted to avoid: a security-relevant pattern duplicated + * per call site only stays in sync by accident. (Kilo, PR #1274 round 8.) */ +function conflictExistingName(detail: { existing_datamate_name?: string | null }): string { + return detail.existing_datamate_name ? stripControlChars(detail.existing_datamate_name) : "another workspace" +} + /** Conservative allowlist of terminals known to render OSC 8 hyperlinks. * There's no capability query as reliable as opentui's device-attribute * detection (used by the TUI side) available to a plain CLI process, so this @@ -124,7 +135,20 @@ export function hyperlink(text: string, url: string | null): string { // would otherwise defeat the escaping this function is careful about on // the `text` side while doing nothing for `url`. (multi-model review, PR // #1274 round 7.) - if (!url || !isSafeHttpUrl(url)) return safeText + // + // isSafeHttpUrl only checks that `url` PARSES as http(s) via `new URL()` + // — it doesn't sanitize, and doesn't return the re-serialized/encoded + // form. `new URL()` itself percent-encodes control bytes when it builds + // its own `.toString()`, but that encoding never reaches the ORIGINAL + // `url` string this function actually interpolates below — a string can + // contain a live ESC byte and still parse successfully as a valid + // https: URL (verified: `new URL("https://evil.example/\x1b]8;;...")` + // does not throw). So `isSafeHttpUrl` returning true does not mean `url` + // is free of control bytes; reject it separately, the same way `text` is + // sanitized above — rejecting (falling back to plain text) rather than + // stripping, since a mangled URL is worse than no link at all. (cubic, + // PR #1274 round 8.) + if (!url || stripControlChars(url) !== url || !isSafeHttpUrl(url)) return safeText if (!process.stdout.isTTY) return safeText const OSC8 = "\x1b]8;;" const ST = "\x1b\\" @@ -388,9 +412,7 @@ async function runBrowserHandoff( } catch (err) { bindSpin.stop("Link failed.", 1) if (err instanceof ConflictError) { - const existingName = err.detail.existing_datamate_name - ? stripControlChars(err.detail.existing_datamate_name) - : "another workspace" + const existingName = conflictExistingName(err.detail) prompts.log.error( `This project is already linked to "${existingName}". Workspace "${projectName}" was created but is not linked — re-run \`altimate-code link\` and pick a different action to switch, or delete the new workspace in the SaaS.`, ) @@ -469,9 +491,7 @@ async function createThenBindOrRebind( // can pick from the list; if the pre-check missed it, this is the // authoritative signal — surface it and hint the picker. if (err instanceof ConflictError) { - const existingName = err.detail.existing_datamate_name - ? stripControlChars(err.detail.existing_datamate_name) - : "another workspace" + const existingName = conflictExistingName(err.detail) prompts.log.error( `This project is already linked to "${existingName}". Re-run \`altimate-code link\` to switch to a different workspace.`, ) @@ -650,9 +670,7 @@ async function bindOrRebind( } catch (err) { spin.stop(isRebind ? `Re-link failed.` : `Link failed.`, 1) if (err instanceof ConflictError) { - const existingName = err.detail.existing_datamate_name - ? stripControlChars(err.detail.existing_datamate_name) - : "another workspace" + const existingName = conflictExistingName(err.detail) prompts.log.error(`Already linked to "${existingName}". Re-run \`altimate-code link\` to switch.`) } else if (err instanceof PreconditionFailedError) { prompts.log.error("Someone else re-linked this project — re-run and try again.") diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index 4cbcb9a91..f00d8ab06 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -10,10 +10,7 @@ import type { TuiPlugin, TuiPluginApi } from "@opencode-ai/plugin/tui" import type { BuiltinTuiPlugin } from "@opencode-ai/tui/builtins" import { createSignal, onCleanup, onMount, Show } from "solid-js" import { readLocalBinding, type CachedBinding } from "@/altimate/workspace/state" -import { - buildManageUrl as joinManageUrlPath, - resolveWorkspaceWebUrl, -} from "@/altimate/workspace/browser-handoff" +import { buildManageUrl, resolveWorkspaceWebUrl } from "@/altimate/workspace/browser-handoff" import { getResolvedWorkspaceId } from "@/altimate/workspace/session-context" import { AltimateApi } from "@/altimate/api/client" import { openManageUrl } from "./workspace" @@ -71,7 +68,7 @@ function View(props: { api: TuiPluginApi }) { return } const base = await resolveManageBase() - setManageUrl(base ? joinManageUrlPath(base, b.datamateId) : null) + setManageUrl(base ? buildManageUrl(base, b.datamateId) : null) } finally { refreshInFlight = false } diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index a9f9c90eb..31cc3a706 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -39,7 +39,7 @@ import { type ProjectIdentifier, } from "@/altimate/workspace/api-client" import { - buildManageUrl as joinManageUrlPath, + buildManageUrl, openWorkspaceBrowserHandoff, resolveWorkspaceWebUrl, type HandoffResult, @@ -245,13 +245,21 @@ function OfferDialog(props: OfferProps) { /** Build the SaaS manage-workspace URL for a bound workspace. Deterministic * from tenant + id, so any caller can construct it without an extra round-trip. * Returns null when the current deployment isn't the freemium web (BYOK or - * unresolvable) — the confirmation dialog degrades to id-only in that case. */ -async function buildManageUrl(workspaceId: number): Promise { + * unresolvable) — the confirmation dialog degrades to id-only in that case. + * + * Named ``resolveManageUrl`` (not ``buildManageUrl``) so it doesn't collide + * with — and locally shadow the meaning of — the shared, imported + * ``buildManageUrl`` (browser-handoff.ts) this function delegates the + * actual join to. Before this rename, the import needed an alias + * (``buildManageUrl as joinManageUrlPath``) just to coexist with this + * function's own name, which made the *real* ``buildManageUrl`` invisible + * under that name inside this file. (Kilo, PR #1274 round 8.) */ +async function resolveManageUrl(workspaceId: number): Promise { try { const creds = await AltimateApi.getCredentials() const base = resolveWorkspaceWebUrl(creds.altimateUrl, creds.altimateInstanceName) if (!base) return null - return joinManageUrlPath(base, workspaceId) + return buildManageUrl(base, workspaceId) } catch { return null } @@ -300,8 +308,8 @@ function WorkspaceLinkedDialog(props: LinkedProps) { if (option.value === "open" && props.manageUrl) { // Guard before delegating to open() — a rogue manage_url with a // non-http protocol would otherwise dispatch to an unrelated OS - // scheme handler. buildManageUrl only ever emits http(s) URLs from - // resolveWorkspaceWebUrl, but the guard survives future changes. + // scheme handler. resolveManageUrl only ever emits http(s) URLs + // from resolveWorkspaceWebUrl, but the guard survives future changes. openManageUrl(props.api, props.manageUrl) } props.api.ui.dialog.clear() @@ -318,7 +326,7 @@ async function showLinkedConfirmation( workspaceId: number, workspaceName: string, ): Promise { - const manageUrl = await buildManageUrl(workspaceId) + const manageUrl = await resolveManageUrl(workspaceId) api.ui.dialog.replace(() => ( )) @@ -1144,7 +1152,7 @@ async function runFlow(api: TuiPluginApi, directory: string): Promise { const hasDrift = boundIdent != null && currentIdent != null && boundIdent !== currentIdent // Resolved before the dialog renders — see AlreadyLinkedDialog's comment // on why this can't be fetched async inside the dialog itself. - const manageUrl = await buildManageUrl(serverBinding.datamate.id) + const manageUrl = await resolveManageUrl(serverBinding.datamate.id) api.ui.dialog.replace(() => ( { const currentIdent = cachedMatchedBy === "remote" ? identifier.repoRemote : identifier.projectPath const hasDrift = cachedIdent !== "" && currentIdent != null && cachedIdent !== currentIdent - const manageUrl = await buildManageUrl(local.datamateId) + const manageUrl = await resolveManageUrl(local.datamateId) api.ui.dialog.replace(() => ( { expect(out.split("\x1b]8;;").length - 1).toBe(2) expect(out).toContain("name]8;;http://evil.example\\CLICK ME]8;;\\") }) + + test("refuses a non-http(s) url — no OSC 8 bytes, just the sanitized text", () => { + setTTY(true) + process.env.TERM_PROGRAM = "iTerm.app" + for (const url of ["file:///etc/passwd", "javascript:alert(1)", "ftp://host/path"]) { + const out = hyperlink("name", url) + expect(out).toBe("name") + expect(out).not.toContain("\x1b") + } + }) + + test("refuses a url that parses as http(s) but still carries a live control byte", () => { + // isSafeHttpUrl only checks that `new URL(url)` parses and the protocol + // is http(s) — it does not sanitize, and a string can contain a live + // ESC byte and still parse successfully. hyperlink() interpolates the + // ORIGINAL string, not new URL(url)'s re-serialized/encoded form, so + // that parse check alone doesn't guarantee `url` is safe to embed. + setTTY(true) + process.env.TERM_PROGRAM = "iTerm.app" + const maliciousUrl = "https://evil.example/\x1b]8;;http://spoofed.example\x1b\\CLICK\x1b]8;;\x1b\\" + const out = hyperlink("name", maliciousUrl) + expect(out).toBe("name") + expect(out).not.toContain("\x1b") + }) })