From f7ff11b15b10d530e572afc19fb09571ef0132f3 Mon Sep 17 00:00:00 2001 From: Tommaso Fontana Date: Fri, 26 Jun 2026 14:28:44 +0200 Subject: [PATCH] fix(omp): address #3553 round-2 review feedback - strip IPv6 URL brackets before invoking ssh (ssh://[::1]/ -> ::1) - reject malformed/out-of-range ssh:// ports before connecting (prod:abc, host:65536) - search requests directory metadata only (skipDirectoryListing) instead of draining a remote ls it always rejects --- packages/coding-agent/CHANGELOG.md | 1 + .../__tests__/ssh-protocol.test.ts | 34 +++++++++++++++++++ .../src/internal-urls/ssh-protocol.ts | 26 +++++++++++--- .../coding-agent/src/internal-urls/types.ts | 7 ++++ packages/coding-agent/src/tools/search.ts | 1 + .../test/tools/search-internal-urls.test.ts | 23 ++++++++++++- 6 files changed, 87 insertions(+), 5 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index c8b3a6f84..1d86dd0e5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -79,6 +79,7 @@ - `ssh://` `search` now rejects a malformed or multi-range line selector (e.g. `:-10`, `:1-1:1-2`) instead of silently searching the whole remote file, matching `read`'s selector grammar, and validates the pattern against the native RE2 dialect for remote/virtual-only searches so unsupported regexes fail consistently with local search. - `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`). ## [16.1.19] - 2026-06-25 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 16db57843..eb182b4ed 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 @@ -216,4 +216,38 @@ describe("SshProtocolHandler", () => { mockHosts(); await expect(handler.resolve(parseInternalUrl("ssh://icaro:0/etc/hostname"))).rejects.toThrow(/port 0/); }); + + it("strips IPv6 URL brackets before building the ssh target", async () => { + mockHosts(); + const spy = mockReadBytes("ok\n"); + await handler.resolve(parseInternalUrl("ssh://[::1]/etc/hostname")); + expect(spy.mock.calls[0]?.[0]?.host).toBe("::1"); + }); + + it("matches a configured bracketed-colon alias instead of stripping it as IPv6", async () => { + mockHosts([{ name: "[prod:2222]", host: "prod.internal", _source: SOURCE }]); + const spy = mockReadBytes("ok\n"); + await handler.resolve(parseInternalUrl("ssh://%5Bprod%3A2222%5D/etc/hostname")); + expect(spy.mock.calls[0]?.[0]?.host).toBe("prod.internal"); + }); + + it("rejects a malformed or out-of-range ssh:// port before connecting", async () => { + mockHosts(); + await expect(handler.resolve(parseInternalUrl("ssh://prod:abc/etc"))).rejects.toThrow(/invalid host or port/); + await expect(handler.resolve(parseInternalUrl("ssh://prod:65536/etc"))).rejects.toThrow(/invalid host or port/); + }); + + it("skips the remote directory listing when skipDirectoryListing is set", async () => { + mockHosts(); + vi.spyOn(fileTransfer, "readRemoteFile").mockRejectedValue(new Error("Is a directory")); + vi.spyOn(fileTransfer, "statRemotePath").mockResolvedValue("directory"); + const listSpy = vi.spyOn(fileTransfer, "listRemoteDir").mockResolvedValue([]); + + const res = await handler.resolve(parseInternalUrl("ssh://h/etc"), { skipDirectoryListing: true }); + expect(res.isDirectory).toBe(true); + expect(listSpy).not.toHaveBeenCalled(); + + await handler.resolve(parseInternalUrl("ssh://h/etc")); + expect(listSpy).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/coding-agent/src/internal-urls/ssh-protocol.ts b/packages/coding-agent/src/internal-urls/ssh-protocol.ts index b1b3b470e..175ed876b 100644 --- a/packages/coding-agent/src/internal-urls/ssh-protocol.ts +++ b/packages/coding-agent/src/internal-urls/ssh-protocol.ts @@ -120,11 +120,25 @@ function formatHostIndex(hosts: readonly SSHHost[]): string { * an opaque OpenSSH destination so plain `~/.ssh/config` aliases work. */ async function resolveTarget(url: InternalUrl, cwd?: string): Promise { + // `parseInternalUrl` falls back to a lenient regex parse when WHATWG `new URL` + // rejects the input. For ssh:// that only happens on a malformed authority — an + // invalid or out-of-range port (`prod:abc`, `host:65536`) or a bad IPv6 literal — + // which would otherwise be mis-read as an opaque host and silently connect to the + // default port. Reject it before resolving. + if (!URL.canParse(url.href)) { + throw new Error(`ssh://: invalid host or port in "${url.href}"; use ssh://host[:1-65535]/`); + } + // WHATWG `hostname` is bracketed only for a *valid* IPv6 literal, so a bracketed + // host is unambiguously IPv6 — hand OpenSSH the bare address. Percent-encoded + // bracketed aliases (e.g. `%5Bprod%3A2222%5D`) keep their literal brackets in the + // decoded `rawHost`, so they are matched and forwarded verbatim, never stripped. const bareHost = url.hostname; const rawAuthority = url.rawHost || bareHost; if (!bareHost && !rawAuthority) { throw new Error("ssh:// requires a host: ssh:///"); } + const isIpv6Literal = bareHost.startsWith("[") && bareHost.endsWith("]"); + const sshHost = isIpv6Literal ? bareHost.slice(1, -1) : bareHost; const username = url.username || undefined; const port = url.port ? Number(url.port) : undefined; if (port === 0) { @@ -142,7 +156,7 @@ async function resolveTarget(url: InternalUrl, cwd?: string): Promise { - const content = formatDirListing(await listRemoteDir(target, remotePath, { signal })); + // `search`/`find` reject an ssh:// directory outright, so they pass `skipListing` + // to avoid draining a full remote `ls` we would only discard. + const content = skipListing ? "" : formatDirListing(await listRemoteDir(target, remotePath, { signal })); return { url: url.href, content, diff --git a/packages/coding-agent/src/internal-urls/types.ts b/packages/coding-agent/src/internal-urls/types.ts index edbf36d1b..b5b8dfaa9 100644 --- a/packages/coding-agent/src/internal-urls/types.ts +++ b/packages/coding-agent/src/internal-urls/types.ts @@ -100,6 +100,13 @@ export interface ResolveContext { localProtocolOptions?: LocalProtocolOptions; /** Calling session's loaded skills. Prefer this over process-global skill state. */ skills?: readonly Skill[]; + /** + * When set, handlers that would otherwise materialize an expensive directory + * listing (e.g. the ssh:// handler draining a full remote `ls`) instead return + * the directory shape (`isDirectory: true`) with empty content. `search`/`find` + * reject directory resources, so they never need the listing. + */ + skipDirectoryListing?: boolean; } /** diff --git a/packages/coding-agent/src/tools/search.ts b/packages/coding-agent/src/tools/search.ts index 96dc9f22d..32e540bc4 100644 --- a/packages/coding-agent/src/tools/search.ts +++ b/packages/coding-agent/src/tools/search.ts @@ -667,6 +667,7 @@ async function resolveInternalSearchInputs(opts: { signal: opts.signal, localProtocolOptions: opts.localProtocolOptions, skills: opts.skills, + skipDirectoryListing: true, }; for (let idx = 0; idx < paths.length; idx++) { diff --git a/packages/coding-agent/test/tools/search-internal-urls.test.ts b/packages/coding-agent/test/tools/search-internal-urls.test.ts index 84207949a..f16272041 100644 --- a/packages/coding-agent/test/tools/search-internal-urls.test.ts +++ b/packages/coding-agent/test/tools/search-internal-urls.test.ts @@ -1,7 +1,9 @@ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import * as capability from "@oh-my-pi/pi-coding-agent/capability"; +import type { CapabilityResult } from "@oh-my-pi/pi-coding-agent/capability/types"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { resetActiveSkillsForTests, setActiveSkills } from "@oh-my-pi/pi-coding-agent/extensibility/skills"; import { @@ -12,6 +14,7 @@ import { type ProtocolHandler, } from "@oh-my-pi/pi-coding-agent/internal-urls"; import { AgentRegistry } from "@oh-my-pi/pi-coding-agent/registry/agent-registry"; +import * as sshFileTransfer from "@oh-my-pi/pi-coding-agent/ssh/file-transfer"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { FindTool } from "@oh-my-pi/pi-coding-agent/tools/find"; import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; @@ -92,6 +95,7 @@ describe("SearchTool internal URL resolution", () => { LocalProtocolHandler.resetOverrideForTests(); InternalUrlRouter.resetForTests(); resetActiveSkillsForTests(); + vi.restoreAllMocks(); }); function createSession(overrides: Partial = {}): ToolSession { @@ -526,4 +530,21 @@ describe("SearchTool internal URL resolution", () => { /directory listing|cannot recurse/, ); }); + + it("rejects an ssh:// directory in search without draining a remote listing", async () => { + vi.spyOn(capability, "loadCapability").mockResolvedValue({ + items: [], + all: [], + warnings: [], + providers: [], + } as CapabilityResult); + vi.spyOn(sshFileTransfer, "readRemoteFile").mockRejectedValue(new Error("Is a directory")); + vi.spyOn(sshFileTransfer, "statRemotePath").mockResolvedValue("directory"); + const listSpy = vi.spyOn(sshFileTransfer, "listRemoteDir").mockResolvedValue([]); + const tool = new SearchTool(createSession()); + await expect(tool.execute("ssh-dir-search", { pattern: "x", paths: ["ssh://h/etc"] })).rejects.toThrow( + /directory listing|cannot recurse/, + ); + expect(listSpy).not.toHaveBeenCalled(); + }); });