From 4ba598d191042b43062ba5de478838b5684f6e59 Mon Sep 17 00:00:00 2001 From: EazyHood Date: Sat, 1 Aug 2026 22:16:14 -0500 Subject: [PATCH 1/2] fix(cli): stop cmd.exe mangling the OAuth URL on Windows `auth login` opened the browser through cmd: 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 'state' is not recognized as an internal or external command Two consequences. The browser receives a truncated authorize URL with no redirect_uri, state or scope, so login cannot complete. And the remaining query fragments are handed to cmd as commands to run. `stdio: "ignore"` means the user sees neither -- `calle auth login` appears to do nothing. The command-execution half is not exploitable here, since the URL comes from your own broker, but a URL should never reach a shell parser in the first place. Uses rundll32 url.dll,FileProtocolHandler instead, which takes the URL as a single argument and never parses it. macOS and Linux paths are unchanged. Also adds an 'error' listener to the child. Without one, a spawn failure emits an unhandled 'error' event, which is an uncaught exception -- failing to open a browser should not take the CLI down when the URL is printed anyway. Tests: two cases pinning the cmd behaviour and the argv-passing that replaces it, skipped off Windows. Full suite 113 passing. --- packages/cli/lib/cli.js | 17 +++++++- packages/cli/test/open-browser.test.js | 54 ++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 packages/cli/test/open-browser.test.js diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 4ac6d09..cab7310 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -919,9 +919,22 @@ export async function runCli(argv, deps = {}) { 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]; + // 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. + const [command, args] = + process.platform === "darwin" + ? ["open", [url]] + : process.platform === "win32" + ? ["rundll32", ["url.dll,FileProtocolHandler", url]] + : ["xdg-open", [url]]; 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..47cc98b --- /dev/null +++ b/packages/cli/test/open-browser.test.js @@ -0,0 +1,54 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import test from "node:test"; + +// 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 opener now uses rundll32, which takes the URL as one argument and never +// parses it. This test pins the underlying cmd behaviour so nobody reintroduces +// the old approach; it is skipped off Windows, where cmd does not exist. + +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("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")); +}); + +test("rundll32 receives the URL intact", { skip: !onWindows }, () => { + // Argument passing only -- `echo` stands in for rundll32 so nothing opens. + // The point is that the URL survives as a single argv entry. + const result = spawnSync( + process.execPath, + ["-e", "process.stdout.write(process.argv[1])", OAUTH_URL], + { encoding: "utf8" }, + ); + + assert.equal((result.stdout || "").trim(), OAUTH_URL); +}); From 217150028c16ca14f2fa2cea71e6a39c0020e8e8 Mon Sep 17 00:00:00 2001 From: EazyHood <209367218+EazyHood@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:47:37 -0500 Subject: [PATCH 2/2] fix(cli): name the Windows opener by its System32 path, and test the real path Addresses the review on #71. [P1] spawn("rundll32", ...) used an unqualified executable name, so the CreateProcess search order would look in the current working directory before System32 and a planted rundll32.exe in an untrusted checkout would run. The opener is now the fully qualified %SystemRoot%\System32\rundll32.exe. [P2] The opener selection moved into an exported browserOpenCommand(url, platform, env), and the tests now call it instead of demonstrating cmd.exe behaviour beside it. Putting cmd back makes 4 of the 7 fail, on every platform. [P2] Added the patch changeset for @call-e/cli. --- .changeset/lucky-poems-argue.md | 7 +++ packages/cli/lib/cli.js | 36 ++++++++++----- packages/cli/test/open-browser.test.js | 63 ++++++++++++++++++++------ 3 files changed, 80 insertions(+), 26 deletions(-) create mode 100644 .changeset/lucky-poems-argue.md 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 cab7310..753b9bf 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -914,22 +914,36 @@ 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"); - // 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. - const [command, args] = - process.platform === "darwin" - ? ["open", [url]] - : process.platform === "win32" - ? ["rundll32", ["url.dll,FileProtocolHandler", url]] - : ["xdg-open", [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 diff --git a/packages/cli/test/open-browser.test.js b/packages/cli/test/open-browser.test.js index 47cc98b..71082f7 100644 --- a/packages/cli/test/open-browser.test.js +++ b/packages/cli/test/open-browser.test.js @@ -2,6 +2,8 @@ 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" }) @@ -17,9 +19,8 @@ import test from "node:test"; // 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 opener now uses rundll32, which takes the URL as one argument and never -// parses it. This test pins the underlying cmd behaviour so nobody reintroduces -// the old approach; it is skipped off Windows, where cmd does not exist. +// 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" + @@ -27,6 +28,50 @@ const OAUTH_URL = 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(); @@ -40,15 +85,3 @@ test("cmd.exe mangles a URL containing &", { skip: !onWindows }, () => { ); assert.ok(received.startsWith("https://example.invalid/oauth/authorize?client_id=calle")); }); - -test("rundll32 receives the URL intact", { skip: !onWindows }, () => { - // Argument passing only -- `echo` stands in for rundll32 so nothing opens. - // The point is that the URL survives as a single argv entry. - const result = spawnSync( - process.execPath, - ["-e", "process.stdout.write(process.argv[1])", OAUTH_URL], - { encoding: "utf8" }, - ); - - assert.equal((result.stdout || "").trim(), OAUTH_URL); -});