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
2026-06-26 20:41:35 +02:00
parent 73c1b33fd9
commit f7ff11b15b
6 changed files with 87 additions and 5 deletions
@@ -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

@@ -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);
+	});
 });
@@ -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<SSHConnectionTarget> {
+	// `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]/<absolute-path>`);
+	}
+	// 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://<host>/<absolute-path>");
 	}
+	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<SSHConnect
 			);
 		}
 		const name = `${username ? `${username}@` : ""}${bareHost}${port !== undefined ? `:${port}` : ""}`;
-		return { name, host: bareHost, username, port };
+		return { name, host: sshHost, username, port };
 	}

 	// No explicit user/port: match the full decoded authority against a
@@ -159,7 +173,7 @@ async function resolveTarget(url: InternalUrl, cwd?: string): Promise<SSHConnect
 		};
 	}
 	// Opaque OpenSSH destination (plain ~/.ssh/config alias, or any resolvable host).
-	return { name: rawAuthority, host: rawAuthority };
+	return { name: rawAuthority, host: isIpv6Literal ? sshHost : rawAuthority };
 }

 /** Format a one-level remote directory listing — mirrors buildDirectoryResource's plain `name/` lines. */
@@ -203,7 +217,8 @@ export class SshProtocolHandler implements ProtocolHandler {
 			} catch {
 				// Re-stat failed too (host/connection issue) — the original read error is clearer.
 			}
-			if (kind === "directory") return this.#resolveDirectory(target, remotePath, url, context?.signal);
+			if (kind === "directory")
+				return this.#resolveDirectory(target, remotePath, url, context?.signal, context?.skipDirectoryListing);
 			throw err;
 		}
 		if (fileResult.truncated) {
@@ -233,8 +248,11 @@ export class SshProtocolHandler implements ProtocolHandler {
 		remotePath: string,
 		url: InternalUrl,
 		signal?: AbortSignal,
+		skipListing?: boolean,
 	): Promise<InternalResource> {
-		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,
@@ -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;
 }

 /**
@@ -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++) {
@@ -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> = {}): 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<unknown>);
+		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();
+	});
 });