diff --git a/.changeset/lucky-poems-argue.md b/.changeset/lucky-poems-argue.md new file mode 100644 index 0000000..9b074ca --- /dev/null +++ b/.changeset/lucky-poems-argue.md @@ -0,0 +1,7 @@ +--- +"@call-e/cli": patch +--- + +Open the browser without cmd.exe on Windows during `auth login`. + +`cmd /c start "" ` let cmd parse the OAuth URL, and `&` is a command separator there, so the URL was truncated at the first `&` (losing `redirect_uri`, `state` and `scope`) and the rest of the query string was run as shell commands. The opener is now `rundll32`, which takes the URL as a single argument, named by its fully qualified `%SystemRoot%\System32` path so the executable is not resolved through the current working directory. diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 4ac6d09..753b9bf 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -914,14 +914,41 @@ async function handleCallCommand({ command, positional, options, config, deps, s } } +// Picks the program that opens a URL in the user's browser. +// +// Windows is handled without cmd.exe on purpose. `cmd /c start "" ` lets cmd +// parse the URL, and `&` is a command separator there -- an OAuth URL is truncated +// at the first `&` (losing redirect_uri, state and scope) and the remaining query +// fragments are run as shell commands. rundll32 takes the URL as a single argument +// and never parses it. +// +// The Windows opener is named by its fully qualified path. An unqualified +// "rundll32" would be looked up with the CreateProcess search order, which checks +// the current directory before System32, so running `calle auth login` from an +// untrusted checkout holding a planted rundll32.exe would execute it. +// https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa +// +// Exported so the tests can assert the exact executable and argv the CLI uses. +export function browserOpenCommand(url, platform = process.platform, env = process.env) { + if (platform === "darwin") return ["open", [url]]; + if (platform !== "win32") return ["xdg-open", [url]]; + + const systemRoot = env.SystemRoot || env.SYSTEMROOT || env.windir || "C:\\Windows"; + const rundll32 = `${systemRoot.replace(/[\\/]+$/, "")}\\System32\\rundll32.exe`; + return [rundll32, ["url.dll,FileProtocolHandler", url]]; +} + export async function runCli(argv, deps = {}) { const stdout = deps.stdout || ((text) => process.stdout.write(text)); const stderr = deps.stderr || ((text) => process.stderr.write(`${text}\n`)); const openBrowser = deps.openBrowser || (async (url) => { const { spawn } = await import("node:child_process"); - const command = process.platform === "darwin" ? "open" : process.platform === "win32" ? "cmd" : "xdg-open"; - const args = process.platform === "win32" ? ["/c", "start", "", url] : [url]; + const [command, args] = browserOpenCommand(url, process.platform, process.env); const child = spawn(command, args, { detached: true, stdio: "ignore" }); + // Without a listener an 'error' event (a missing opener, say) becomes an + // uncaught exception. Failing to open a browser should not take the CLI + // down -- the URL is printed for the user either way. + child.on("error", () => {}); child.unref(); }); diff --git a/packages/cli/test/open-browser.test.js b/packages/cli/test/open-browser.test.js new file mode 100644 index 0000000..71082f7 --- /dev/null +++ b/packages/cli/test/open-browser.test.js @@ -0,0 +1,87 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import test from "node:test"; + +import { browserOpenCommand } from "../lib/cli.js"; + +// Regression: `auth login` opened the browser through cmd.exe on Windows. +// +// spawn("cmd", ["/c", "start", "", url], { detached: true, stdio: "ignore" }) +// +// cmd treats `&` as a command separator, and an OAuth URL is full of them. +// Measured with the real URL shape: +// +// in ...authorize?client_id=calle&redirect_uri=http://localhost:9999/cb&state=abc&scope=mcp +// out ...authorize?client_id=calle +// err 'redirect_uri' is not recognized as an internal or external command +// +// So the browser got a truncated URL -- no redirect_uri, no state, no scope, so +// login could not complete -- and the rest of the query string was executed as +// shell commands. `stdio: "ignore"` meant the user saw none of it. +// +// The assertions below run against browserOpenCommand, the function the CLI +// actually calls, so putting cmd.exe back makes them fail on every platform. + +const OAUTH_URL = + "https://example.invalid/oauth/authorize" + + "?client_id=calle&redirect_uri=http://localhost:9999/cb&state=abc123&scope=mcp"; + +const onWindows = process.platform === "win32"; + +test("windows opens the URL with System32's rundll32, by absolute path", () => { + const [command, args] = browserOpenCommand(OAUTH_URL, "win32", { SystemRoot: "C:\\Windows" }); + + assert.equal(command, "C:\\Windows\\System32\\rundll32.exe"); + assert.deepEqual(args, ["url.dll,FileProtocolHandler", OAUTH_URL]); +}); + +test("windows never routes the URL through a shell", () => { + const [command, args] = browserOpenCommand(OAUTH_URL, "win32", { SystemRoot: "C:\\Windows" }); + + assert.doesNotMatch(command, /(^|[\\/])cmd(\.exe)?$/i); + assert.ok(!args.includes("/c"), "no cmd switch in the argv"); + assert.ok(!args.includes("start"), "no start builtin in the argv"); +}); + +test("windows names the opener by a qualified path, not a bare executable", () => { + const [command] = browserOpenCommand(OAUTH_URL, "win32", { SystemRoot: "C:\\Windows" }); + + // A bare "rundll32" would be resolved with the CreateProcess search order, + // which looks in the current directory before System32. + assert.ok(/^[A-Za-z]:\\/.test(command), `expected an absolute path, got ${command}`); + assert.match(command, /\\System32\\rundll32\.exe$/i); +}); + +test("windows takes the system directory from the environment", () => { + const [command] = browserOpenCommand(OAUTH_URL, "win32", { SystemRoot: "D:\\WINNT" }); + + assert.equal(command, "D:\\WINNT\\System32\\rundll32.exe"); +}); + +test("the URL travels as one argv entry, so nothing can split it", () => { + const [, args] = browserOpenCommand(OAUTH_URL, "win32", { SystemRoot: "C:\\Windows" }); + const urlArgs = args.filter((arg) => arg.includes("example.invalid")); + + assert.equal(urlArgs.length, 1); + assert.equal(urlArgs[0], OAUTH_URL); + assert.ok(urlArgs[0].includes("&scope=mcp"), "the query survives whole"); +}); + +test("macos and linux keep their openers", () => { + assert.deepEqual(browserOpenCommand(OAUTH_URL, "darwin", {}), ["open", [OAUTH_URL]]); + assert.deepEqual(browserOpenCommand(OAUTH_URL, "linux", {}), ["xdg-open", [OAUTH_URL]]); +}); + +test("cmd.exe mangles a URL containing &", { skip: !onWindows }, () => { + const result = spawnSync("cmd", ["/c", "echo", OAUTH_URL], { encoding: "utf8" }); + const received = (result.stdout || "").trim(); + + // This is the bug, asserted so its absence would be noticed: cmd stops at the + // first `&` and treats what follows as separate commands. + assert.ok( + !received.includes("scope=mcp"), + "expected cmd to truncate the URL -- if this now passes the whole URL, " + + "the platform changed and the comment above needs revisiting", + ); + assert.ok(received.startsWith("https://example.invalid/oauth/authorize?client_id=calle")); +});