diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f124f83f7..771575158 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -18,6 +18,8 @@ - `ssh://` host names containing URL-reserved characters (e.g. `prod:2222`, `alice@prod`) are now percent-encoded in the host index and autocomplete and matched against the decoded authority, so configured aliases resolve correctly (a literal `user@`/`:port` in the URL is still rejected as an override on a configured host). - `ssh://` `read`/`search`/`write` now reject an explicit `:0` port (e.g. `ssh://host:0/path`) before connecting, instead of silently dropping it and connecting on the default SSH port; a path-less `ssh://host:2222` also no longer mistakes its authority port for a read selector. - `ssh://` now strips IPv6 URL brackets before invoking OpenSSH (so `ssh://[::1]/path` targets `::1` instead of failing to resolve), rejects a malformed or out-of-range port (`ssh://host:abc`, `ssh://host:65536`) before connecting instead of treating the bad authority as an opaque default-port host, and `search` no longer drains a remote directory listing only to reject it (it requests directory metadata via `skipDirectoryListing`). +- `ssh://` `read`/`search`/`write` now reject a URL query string or fragment (e.g. `ssh://host/tmp/a?draft`, `ssh://host/tmp/a#draft`) instead of silently operating on the path truncated before the `?`/`#`; a literal `?`/`#` in a remote filename must be percent-encoded (`%3F`/`%23`). +- `ssh://` `read`/`search`/`write` now restrict transfers to remotes whose login shell is `sh`, `bash`, or `zsh`. A non-POSIX login shell (fish, csh/tcsh) can't parse the POSIX transfer snippets — and csh/tcsh apply `!` history expansion to the command line — so it is refused with a clear error (use the `ssh` tool) instead of running a broken remote command; host-shell detection no longer misclassifies `fish`/`csh`/`tcsh` as `sh`. ## [16.1.23] - 2026-06-26 diff --git a/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts b/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts index 596baebd7..f69c1f421 100644 --- a/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts +++ b/packages/coding-agent/src/internal-urls/__tests__/ssh-protocol.test.ts @@ -316,4 +316,16 @@ describe("SshProtocolHandler", () => { await handler.resolve(parseInternalUrl("ssh://h/etc")); expect(listSpy).toHaveBeenCalledTimes(1); }); + + it("rejects ssh:// URL queries and fragments instead of operating on the truncated path", async () => { + mockHosts(); + // `?`/`#` are URL delimiters, so the query/fragment is stripped from the path; + // `ssh://h/tmp/a?draft` would otherwise read/write `/tmp/a`, the wrong file. + await expect(handler.resolve(parseInternalUrl("ssh://h/tmp/a?draft"))).rejects.toThrow(/quer/i); + await expect(handler.resolve(parseInternalUrl("ssh://h/tmp/a#draft"))).rejects.toThrow(/fragment/i); + // A literal `?` in a filename must be percent-encoded (`%3F`) and is then accepted. + const spy = mockReadBytes("ok\n"); + await handler.resolve(parseInternalUrl("ssh://h/tmp/a%3Fdraft")); + expect(spy.mock.calls[0]?.[1]).toBe("/tmp/a?draft"); + }); }); diff --git a/packages/coding-agent/src/internal-urls/ssh-protocol.ts b/packages/coding-agent/src/internal-urls/ssh-protocol.ts index c3389489a..3cd2b205a 100644 --- a/packages/coding-agent/src/internal-urls/ssh-protocol.ts +++ b/packages/coding-agent/src/internal-urls/ssh-protocol.ts @@ -70,6 +70,20 @@ function decodeUtf8Text(bytes: Uint8Array): string | null { * the non-special `ssh` scheme. */ function remotePathFromUrl(url: InternalUrl): string { + // `?`/`#` are URL delimiters, so parseInternalUrl strips them from the path + // (`ssh://h/tmp/a?draft` → `/tmp/a`). Reject the unsupported suffix instead of + // silently operating on the truncated path; a literal `?`/`#` in a filename + // must be percent-encoded (`%3F`/`%23`). + if (url.search) { + throw new Error( + `ssh:// does not support URL query strings; percent-encode a literal '?' as %3F in the path: ${url.href}`, + ); + } + if (url.hash) { + throw new Error( + `ssh:// does not support URL fragments; percent-encode a literal '#' as %23 in the path: ${url.href}`, + ); + } const raw = url.rawPathname ?? url.pathname; let decoded: string; try { diff --git a/packages/coding-agent/src/prompts/tools/read.md b/packages/coding-agent/src/prompts/tools/read.md index 61a42e5f9..070439bd2 100644 --- a/packages/coding-agent/src/prompts/tools/read.md +++ b/packages/coding-agent/src/prompts/tools/read.md @@ -69,7 +69,7 @@ For `.sqlite`, `.sqlite3`, `.db`, `.db3`: All URI schemes take the same line selectors. `artifact://` recovers full output a bash/eval/tool result spilled or truncated. `history://` = agent transcript; bare `history://` lists agents. -`ssh://host/` reads a remote text file (UTF-8, ≤1 MiB) or lists a directory one level deep, on a pre-configured SSH host or `~/.ssh/config` alias; `ssh://host/` lists the remote root and bare `ssh://` lists the configured hosts. Files are also writable via `write` and searchable via `search`; a directory only lists (`search` refuses a directory, `write` refuses to overwrite one). A literal `:` in the remote path must be percent-encoded as `%3A`, since a trailing `:sel` is read as a line selector. POSIX remotes only (Linux/macOS/BSD); a Windows host is rejected — use the `ssh` tool there. +`ssh://host/` reads a remote text file (UTF-8, ≤1 MiB) or lists a directory one level deep, on a pre-configured SSH host or `~/.ssh/config` alias; `ssh://host/` lists the remote root and bare `ssh://` lists the configured hosts. Files are also writable via `write` and searchable via `search`; a directory only lists (`search` refuses a directory, `write` refuses to overwrite one). A literal `:`, `?`, or `#` in the remote path must be percent-encoded (`%3A`/`%3F`/`%23`) — a trailing `:sel` is read as a line selector, and `?`/`#` start a URL query/fragment. Requires a POSIX login shell (`sh`/`bash`/`zsh`); a Windows host or a non-POSIX shell (fish, csh/tcsh) is rejected — use the `ssh` tool there. - Line ranges go in the selector: `path="src/foo.ts:50-200"`. diff --git a/packages/coding-agent/src/ssh/__tests__/connection-manager-args.test.ts b/packages/coding-agent/src/ssh/__tests__/connection-manager-args.test.ts index a79b2796d..be6c61319 100644 --- a/packages/coding-agent/src/ssh/__tests__/connection-manager-args.test.ts +++ b/packages/coding-agent/src/ssh/__tests__/connection-manager-args.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it } from "bun:test"; -import { buildRemoteCommand, type SSHConnectionTarget } from "../connection-manager"; -import { buildSshTarget } from "../utils"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import { getRemoteHostDir } from "@oh-my-pi/pi-utils"; +import { buildRemoteCommand, getHostInfo, type SSHConnectionTarget, type SSHHostShell } from "../connection-manager"; +import { buildSshTarget, sanitizeHostName } from "../utils"; const TARGET: SSHConnectionTarget = { name: "h", host: "h" }; @@ -36,3 +39,31 @@ describe("buildSshTarget argument-injection guard", () => { ); }); }); + +describe("ssh host shell classification", () => { + it("treats fish/csh/tcsh as non-POSIX (unknown) and keeps real sh-family as sh", async () => { + // parseHostInfo re-runs parseShell on the stored shell field, so getHostInfo + // exercises the classifier through a public seam. The ensurePosixRemote + // whitelist then refuses anything that isn't sh/bash/zsh. + const cases: Array<[string, SSHHostShell]> = [ + ["/usr/bin/fish", "unknown"], + ["/bin/csh", "unknown"], + ["/bin/tcsh", "unknown"], + ["/bin/dash", "sh"], + ["/bin/sh", "sh"], + ["/usr/bin/bash", "bash"], + ["/usr/bin/zsh", "zsh"], + ]; + for (const [shellValue, expected] of cases) { + const name = `omp-shellclf-${crypto.randomUUID()}`; + const file = path.join(getRemoteHostDir(), `${sanitizeHostName(name)}.json`); + await Bun.write(file, JSON.stringify({ version: 3, os: "linux", shell: shellValue, compatEnabled: false })); + try { + const info = await getHostInfo(name); + expect(info?.shell).toBe(expected); + } finally { + await fs.promises.rm(file, { force: true }); + } + } + }); +}); diff --git a/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts b/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts index cae09d50a..4bfeb5782 100644 --- a/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts +++ b/packages/coding-agent/src/ssh/__tests__/file-transfer-posix-guard.test.ts @@ -26,4 +26,43 @@ describe("ssh file-transfer POSIX guard", () => { expect(ensureConnectionSpy).toHaveBeenCalled(); expect(ensureHostInfoSpy).toHaveBeenCalled(); }); + + it("rejects a non-POSIX login shell (csh/tcsh/fish classify as non-sh) before any transfer", async () => { + // csh/tcsh history-expand `!`, and fish can't parse our POSIX source; all + // classify as a non-sh shell, so the guard must refuse them before any spawn. + vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined); + vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({ + version: 3, + os: "linux", + shell: "unknown", + compatEnabled: false, + }); + const target: SSHConnectionTarget = { name: "fishbox", host: "fishbox" }; + await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/non-POSIX login shell/); + await expect(writeRemoteFile(target, "/tmp/x", new Uint8Array([1]), {})).rejects.toThrow(/non-POSIX login shell/); + }); + + it("allows a POSIX login shell (sh/bash/zsh) to run the transfer commands directly", async () => { + // A POSIX login shell runs our snippets verbatim; the guard must let it through. + // Reject at buildRemoteCommand to capture the command before any real ssh spawn. + vi.spyOn(connectionManager, "ensureConnection").mockResolvedValue(undefined); + vi.spyOn(connectionManager, "ensureHostInfo").mockResolvedValue({ + version: 3, + os: "linux", + shell: "sh", + compatEnabled: false, + }); + const buildSpy = vi + .spyOn(connectionManager, "buildRemoteCommand") + .mockRejectedValue(new Error("stop-before-spawn")); + const target: SSHConnectionTarget = { name: "shbox", host: "shbox" }; + + await expect(readRemoteFile(target, "/etc/hosts", { maxBytes: 1024 })).rejects.toThrow(/stop-before-spawn/); + await expect(writeRemoteFile(target, "/tmp/x", new Uint8Array([1]), {})).rejects.toThrow(/stop-before-spawn/); + + // Reached buildRemoteCommand → the guard allowed the POSIX shell. Commands are + // sent verbatim (no `sh -c` wrapper); write keeps its stdin staging. + expect(buildSpy.mock.calls[0]?.[1]).toContain("head -c 1025"); + expect(buildSpy.mock.calls[1]?.[2]).toMatchObject({ allowStdin: true }); + }); }); diff --git a/packages/coding-agent/src/ssh/connection-manager.ts b/packages/coding-agent/src/ssh/connection-manager.ts index 878b3d08b..852ce2820 100644 --- a/packages/coding-agent/src/ssh/connection-manager.ts +++ b/packages/coding-agent/src/ssh/connection-manager.ts @@ -32,7 +32,7 @@ export interface SSHHostInfo { const CONTROL_DIR = getSshControlDir(); const CONTROL_PATH = path.join(CONTROL_DIR, "%C.sock"); const HOST_INFO_DIR = getRemoteHostDir(); -const HOST_INFO_VERSION = 2; +const HOST_INFO_VERSION = 3; const activeHosts = new Map(); const pendingConnections = new Map>(); @@ -153,7 +153,11 @@ function parseShell(value: unknown): SSHHostShell | null { if (normalized.includes("zsh")) return "zsh"; if (normalized.includes("pwsh") || normalized.includes("powershell")) return "powershell"; if (normalized.includes("cmd.exe") || normalized === "cmd") return "cmd"; - if (normalized.endsWith("sh") || normalized.includes("/sh")) return "sh"; + // Only genuine POSIX sh-family by basename — fish/csh/tcsh also end in "sh" + // but are non-POSIX (csh/tcsh history-expand `!`), so they fall through to + // "unknown" and are refused by the ssh:// transfer guard. + const base = normalized.slice(normalized.lastIndexOf("/") + 1); + if (base === "sh" || base === "dash" || base === "ash" || base === "ksh" || base === "mksh") return "sh"; return "unknown"; } @@ -295,18 +299,9 @@ async function probeHostInfo(host: SSHConnectionTarget): Promise { os = "linux"; } - let shell: SSHHostShell = "unknown"; - if (shellLower.includes("bash")) { - shell = "bash"; - } else if (shellLower.includes("zsh")) { - shell = "zsh"; - } else if (shellLower.includes("pwsh") || shellLower.includes("powershell")) { - shell = "powershell"; - } else if (shellLower.includes("cmd.exe") || shellLower === "cmd") { - shell = "cmd"; - } else if (shellLower.endsWith("sh") || shellLower.includes("/sh")) { - shell = "sh"; - } else if (os === "windows" && !shellLower) { + // Reuse parseShell so probe-time and cached classification stay identical. + let shell = parseShell(shellLower) ?? "unknown"; + if (shell === "unknown" && os === "windows" && !shellLower) { shell = "cmd"; } diff --git a/packages/coding-agent/src/ssh/file-transfer.ts b/packages/coding-agent/src/ssh/file-transfer.ts index 74c974404..96aa3aee7 100644 --- a/packages/coding-agent/src/ssh/file-transfer.ts +++ b/packages/coding-agent/src/ssh/file-transfer.ts @@ -14,8 +14,12 @@ import { quotePosixPath } from "./utils"; const DEFAULT_TIMEOUT_MS = 30_000; /** - * Ensure the ControlMaster connection and reject non-POSIX (Windows) remotes, - * which the POSIX `head`/`cat`/`mv`/`ls` commands in this module cannot drive. + * Ensure the ControlMaster connection and restrict transfers to remotes whose + * *login* shell runs our POSIX snippets directly. OpenSSH hands each command to + * `$SHELL -c`, so the login shell must be POSIX: Windows (cmd/powershell) can't + * drive `head`/`cat`/`mv`, and csh/tcsh apply `!` history expansion to the + * command line. `ensureHostInfo` classifies those (and fish) as a non-sh shell, + * so accept only sh, bash, and zsh; anything else is refused here. */ async function ensurePosixRemote(target: SSHConnectionTarget): Promise { await ensureConnection(target); @@ -25,6 +29,11 @@ async function ensurePosixRemote(target: SSHConnectionTarget): Promise { `ssh://: ${target.name} is a Windows host; ssh:// supports POSIX remotes only (head/cat/mv) — use the ssh tool for Windows hosts`, ); } + if (info.shell !== "sh" && info.shell !== "bash" && info.shell !== "zsh") { + throw new Error( + `ssh://: ${target.name} uses a non-POSIX login shell (${info.shell}); ssh:// read/write needs sh, bash, or zsh — use the ssh tool for this host`, + ); + } } export interface RemoteFileReadOptions {