From da2bccfdf95114b3e49108417d9dc58bf6a6744a Mon Sep 17 00:00:00 2001 From: Tommaso Fontana Date: Fri, 26 Jun 2026 16:46:55 +0200 Subject: [PATCH] fix(omp): reject ssh:// URL passwords and empty userinfo (#3553 review) resolveTarget now rejects a non-empty password (ssh://user:pass@host, ssh://:pw@host) since ssh:// uses key/agent auth (BatchMode), and a literal empty username (ssh://@host) that previously fell through to the bare-host fallback and could match a configured host. Both use the same decoded-authority comparison as the empty-port guard, so a percent-encoded alias like %40prod (decodes to @prod) is still matched. --- .../src/internal-urls/__tests__/ssh-protocol.test.ts | 8 ++++++++ .../coding-agent/src/internal-urls/ssh-protocol.ts | 12 ++++++++++++ 2 files changed, 20 insertions(+) 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 9ca417e2a..e571f42f8 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 @@ -260,6 +260,14 @@ describe("SshProtocolHandler", () => { await expect(handler.resolve(parseInternalUrl("ssh://u%2Dname@prod:/etc/hosts"))).rejects.toThrow(/empty port/); }); + it("rejects ssh:// password and empty-username userinfo before matching a host", async () => { + mockHosts([{ name: "prod", host: "10.0.0.5", _source: SOURCE }]); + await expect(handler.resolve(parseInternalUrl("ssh://user:pass@prod/etc/hosts"))).rejects.toThrow(/password/); + await expect(handler.resolve(parseInternalUrl("ssh://:pw@prod/etc/hosts"))).rejects.toThrow(/password/); + await expect(handler.resolve(parseInternalUrl("ssh://@prod/etc/hosts"))).rejects.toThrow(/empty username/); + await expect(handler.resolve(parseInternalUrl("ssh://@prod:22/etc/hosts"))).rejects.toThrow(/empty username/); + }); + it("matches a configured colon-suffixed alias via %3A instead of treating it as an empty port", async () => { mockHosts([{ name: "prod:", host: "prod.internal", _source: SOURCE }]); const spy = mockReadBytes("ok\n"); diff --git a/packages/coding-agent/src/internal-urls/ssh-protocol.ts b/packages/coding-agent/src/internal-urls/ssh-protocol.ts index 4a6525f14..f8672d1f4 100644 --- a/packages/coding-agent/src/internal-urls/ssh-protocol.ts +++ b/packages/coding-agent/src/internal-urls/ssh-protocol.ts @@ -137,6 +137,11 @@ async function resolveTarget(url: InternalUrl, cwd?: string): Promise/"); } + if (url.password) { + throw new Error( + "ssh://: password authentication is not supported; ssh:// uses key/agent auth — drop the ':' from the URL", + ); + } const isIpv6Literal = bareHost.startsWith("[") && bareHost.endsWith("]"); const sshHost = isIpv6Literal ? bareHost.slice(1, -1) : bareHost; const username = url.username || undefined; @@ -162,6 +167,13 @@ async function resolveTarget(url: InternalUrl, cwd?: string): Promise/ or drop the colon`); } + // A literal but empty userinfo (`ssh://@host`) sets username to "" — WHATWG drops + // the `@` from hostname, but rawHost keeps the leading `@`. A percent-encoded + // alias like `%40prod` decodes to `@prod` in rawHost too, but its hostname keeps + // `%40`, so the reconstruction is `@@prod` and is left alone. + if (username === undefined && url.rawHost === `@${decodeOr(bareHost)}${port !== undefined ? `:${port}` : ""}`) { + throw new Error(`ssh://: empty username in "${url.href}"; drop the leading '@' or provide a username before it`); + } const items = await loadConfiguredHosts(cwd); // A literal user/port in the URL is an authority override. A configured alias