From f72ed0ceb75ebea0aa31c937f24f26c4ff87e467 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Fri, 18 Sep 2026 22:12:11 -0400 Subject: [PATCH 1/3] fix(cli): stop `npm test` from opening real browser tabs The two auth-login tests spawn the real binary, and `auth login` calls openBrowser(), which shells out to `open ` on macOS. Every `npm test` run therefore opened two browser tabs pointing at throwaway loopback servers that close seconds later, leaving dead tabs on the developer's machine. Three runs during one session left six. BUTTERSTACK_NO_BROWSER makes openBrowser() print the URL rather than launch anything, and the test harness sets it for every invocation in buildEnv(). The flag is generally useful beyond tests: any non-interactive driver of `auth login` (CI, a container, a remote shell) wants the URL, not a spawn attempt against a browser that isn't there. No credentials were ever at risk - HOME is a per-test temp directory and the flow completes against a local listener, never a real account - but the tabs are noise a test suite has no business creating. Adds a regression test asserting the binary honors the flag and still prints the URL, so the printing half (which the harness's readAuthUrl depends on) cannot be removed with it. Co-Authored-By: Claude Opus 5 --- bin/butter | 11 +++++++++++ test/butter.test.js | 44 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/bin/butter b/bin/butter index 4bc0f53..4d6e54e 100755 --- a/bin/butter +++ b/bin/butter @@ -105,6 +105,17 @@ function clearCredentials() { } function openBrowser(url) { + // BUTTERSTACK_NO_BROWSER exists for anything that drives `auth login` + // without a human at the keyboard. The two auth-login tests spawn the real + // binary against a throwaway loopback server, so every `npm test` run used + // to fire `open` twice and leave real browser tabs pointing at ports that + // close seconds later. Printing the URL is the useful half; launching a + // browser is the half that only makes sense interactively. + if (process.env.BUTTERSTACK_NO_BROWSER) { + console.log(`\nPlease visit this URL to authenticate:\n ${colors.accent}${url}${colors.reset}\n`); + return; + } + // spawn with an argument array, not a shell string built via exec(): the // URL is host-derived (--host / BUTTERSTACK_HOST / config), so a malicious // or malformed host value can never be interpreted by a shell, only diff --git a/test/butter.test.js b/test/butter.test.js index 9a70cf8..3c8fad2 100644 --- a/test/butter.test.js +++ b/test/butter.test.js @@ -43,7 +43,12 @@ function writeCredentials(home, { host, token = "test-token" }) { // explicit null/undefined value meaning "unset this variable" -- needed // for the "no BUTTERSTACK_HOST at all" case. function buildEnv(env) { - const fullEnv = { ...process.env, HOME: env.home }; + // Never launch a real browser from a test. The auth-login cases spawn the + // actual binary, which calls openBrowser() -> `open ` on macOS, so + // every `npm test` run left tabs pointing at loopback ports that close + // seconds later. Set before the per-test overrides so a test could still + // opt out deliberately. + const fullEnv = { ...process.env, BUTTERSTACK_NO_BROWSER: "1", HOME: env.home }; for (const [key, value] of Object.entries(env.overrides || {})) { if (value === null || value === undefined) delete fullEnv[key]; else fullEnv[key] = value; @@ -577,3 +582,40 @@ test("#1938: a server with no investigation key is not reported as 'no investiga rmHome(home); } }); + +// Regression guard for the browser-tab leak: `npm test` spawns the real binary +// for the auth-login cases, and openBrowser() shells out to `open` on macOS. +// Every run used to leave two tabs pointing at loopback ports that close +// seconds later. buildEnv() now sets BUTTERSTACK_NO_BROWSER for all test +// invocations; this asserts the binary actually honors it. +test("auth login honors BUTTERSTACK_NO_BROWSER and prints the URL instead", async () => { + const home = mkHome(); + const { server, port } = await startCaptureServer(); + try { + const child = spawn( + process.execPath, + [BUTTER_BIN, "auth", "login", "--host", `http://127.0.0.1:${port}`], + { env: buildEnv({ home }) } + ); + + const authUrl = await readAuthUrl(child); + assert.ok(authUrl.startsWith("http://127.0.0.1:"), "the URL must still be printed for the user"); + + let stdout = ""; + child.stdout.on("data", (c) => (stdout += c)); + + const parsed = new URL(authUrl); + await hitCallback( + `http://127.0.0.1:${parsed.searchParams.get("port")}/callback?code=c&state=${parsed.searchParams.get("state")}` + ); + await waitForExit(child); + + assert.ok( + !stdout.includes("Could not automatically open browser"), + "the no-browser path must not fall through to the spawn-error branch" + ); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); From 4b3e06beced1b57fe6757ad7f5cedf7ec6175a84 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Fri, 18 Sep 2026 22:24:58 -0400 Subject: [PATCH 2/3] fix(cli): don't claim to open a browser when we aren't, and test the output Follow-up within the same PR. Running the flag path by hand showed the output was wrong in two ways: Visit... no. It said: Opening your browser for one-click authorization... URL: http://127.0.0.1:57713/cli/auth?... Please visit this URL to authenticate: http://127.0.0.1:57713/cli/auth?... The line claimed a browser was opening while BUTTERSTACK_NO_BROWSER was deliberately suppressing exactly that, and the URL was printed twice - once by authLogin and again by openBrowser's suppression branch. Now authLogin says "Visit this URL to authorize:" when the flag is set, and openBrowser prints nothing, because the caller has already printed the URL. The first version of this change was only tested for what it did NOT do (no browser spawned). That missed both defects, since neither is about spawning. Three tests now cover the rendered output itself: - the URL appears exactly once, and "Opening your browser" does not - exactly one intro line and one URL line, and the URL line matches exactly - the interactive path: "Opening your browser" IS printed, the no-browser prompt is not, and the browser really receives the URL that was displayed That last one runs the branch a real user hits. It puts a shim named `open` (or `xdg-open`) first on PATH which records its argv, so the real branch executes and nothing launches - the one way to cover it without reintroducing the tab leak this PR exists to fix. Two test-side details worth knowing: readAuthUrl matches with (\S+) against coloured output, so the URL it returns carries a trailing ANSI reset code (harmless for `new URL()`, fatal for string comparison - hence cleanUrl), and openBrowser spawns without waiting, so the shim can still be writing after the CLI has exited - hence the poll. 18 tests, 0 failures, stable across three consecutive runs. Co-Authored-By: Claude Opus 5 --- bin/butter | 17 +++-- test/butter.test.js | 152 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 7 deletions(-) diff --git a/bin/butter b/bin/butter index 4d6e54e..19d16bc 100755 --- a/bin/butter +++ b/bin/butter @@ -109,12 +109,11 @@ function openBrowser(url) { // without a human at the keyboard. The two auth-login tests spawn the real // binary against a throwaway loopback server, so every `npm test` run used // to fire `open` twice and leave real browser tabs pointing at ports that - // close seconds later. Printing the URL is the useful half; launching a - // browser is the half that only makes sense interactively. - if (process.env.BUTTERSTACK_NO_BROWSER) { - console.log(`\nPlease visit this URL to authenticate:\n ${colors.accent}${url}${colors.reset}\n`); - return; - } + // close seconds later. + // + // Nothing to print here: authLogin already wrote the URL, and adds its own + // "Visit this URL to authorize:" line when this flag is set. + if (process.env.BUTTERSTACK_NO_BROWSER) return; // spawn with an argument array, not a shell string built via exec(): the // URL is host-derived (--host / BUTTERSTACK_HOST / config), so a malicious @@ -385,7 +384,11 @@ async function authLogin(args) { authUrl += `&scope=${encodeURIComponent(requestedScope)}`; } - console.log(`Opening your browser for one-click authorization...`); + if (process.env.BUTTERSTACK_NO_BROWSER) { + console.log(`Visit this URL to authorize:`); + } else { + console.log(`Opening your browser for one-click authorization...`); + } console.log(`${colors.dim}URL: ${authUrl}${colors.reset}\n`); openBrowser(authUrl); }); diff --git a/test/butter.test.js b/test/butter.test.js index 3c8fad2..cd0e077 100644 --- a/test/butter.test.js +++ b/test/butter.test.js @@ -588,6 +588,102 @@ test("#1938: a server with no investigation key is not reported as 'no investiga // Every run used to leave two tabs pointing at loopback ports that close // seconds later. buildEnv() now sets BUTTERSTACK_NO_BROWSER for all test // invocations; this asserts the binary actually honors it. +// Collects everything the child writes to stdout until it exits, so a test can +// assert on the whole rendered block rather than the first line readAuthUrl +// happened to match. +function captureStdout(child) { + let out = ""; + child.stdout.on("data", (c) => (out += c)); + return () => out; +} + +// ANSI colour codes make exact-match assertions unreadable; the codes are not +// what these tests are about. +function stripAnsi(s) { + return s.replace(/\x1b\[[0-9;]*m/g, ""); +} + +// readAuthUrl matches with (\S+) against coloured output, and ANSI escape +// characters are non-space, so the URL it returns has a trailing reset code +// glued to it. Harmless for `new URL(...)` (it lands inside the last query +// value) but fatal to any exact string comparison. +function cleanUrl(u) { + return stripAnsi(u); +} + +test("auth login prints the URL exactly once, and does not claim to open a browser", async () => { + const home = mkHome(); + const { server, port } = await startCaptureServer(); + try { + const child = spawn( + process.execPath, + [BUTTER_BIN, "auth", "login", "--host", `http://127.0.0.1:${port}`], + { env: buildEnv({ home }) } + ); + const readOut = captureStdout(child); + + const authUrl = cleanUrl(await readAuthUrl(child)); + const parsed = new URL(authUrl); + await hitCallback( + `http://127.0.0.1:${parsed.searchParams.get("port")}/callback?code=c&state=${parsed.searchParams.get("state")}` + ); + await waitForExit(child); + + const out = stripAnsi(readOut()); + + // The URL appeared twice before this was fixed: once from authLogin's + // "URL:" line and again from openBrowser's fallback message. + const occurrences = out.split(authUrl).length - 1; + assert.equal(occurrences, 1, `the auth URL should appear exactly once, saw ${occurrences}:\n${out}`); + + // Claiming to open a browser while deliberately not opening one is a lie + // the user can see. + assert.ok( + !out.includes("Opening your browser"), + `must not claim to open a browser when BUTTERSTACK_NO_BROWSER is set:\n${out}` + ); + assert.ok(out.includes("Visit this URL to authorize:"), `expected the no-browser prompt:\n${out}`); + + // That fallback belongs to the spawn-failure path, which is not this one. + assert.ok(!out.includes("Could not automatically open browser"), out); + assert.ok(!out.includes("Please visit this URL to authenticate"), out); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + +test("the no-browser prompt replaces the browser line rather than adding to it", async () => { + const home = mkHome(); + const { server, port } = await startCaptureServer(); + try { + const child = spawn( + process.execPath, + [BUTTER_BIN, "auth", "login", "--host", `http://127.0.0.1:${port}`], + { env: buildEnv({ home }) } + ); + const readOut = captureStdout(child); + const authUrl = cleanUrl(await readAuthUrl(child)); + const parsed = new URL(authUrl); + await hitCallback( + `http://127.0.0.1:${parsed.searchParams.get("port")}/callback?code=c&state=${parsed.searchParams.get("state")}` + ); + await waitForExit(child); + + const lines = stripAnsi(readOut()).split("\n").map((l) => l.trim()).filter(Boolean); + + // Exactly one line introduces the URL, and exactly one line carries it. + const intro = lines.filter((l) => /^Visit this URL to authorize:$/.test(l)); + const urlLines = lines.filter((l) => l.startsWith("URL: ")); + assert.equal(intro.length, 1, `expected one intro line, got ${intro.length}:\n${lines.join("\n")}`); + assert.equal(urlLines.length, 1, `expected one URL line, got ${urlLines.length}:\n${lines.join("\n")}`); + assert.equal(urlLines[0], `URL: ${authUrl}`); + } finally { + await stopCaptureServer(server); + rmHome(home); + } +}); + test("auth login honors BUTTERSTACK_NO_BROWSER and prints the URL instead", async () => { const home = mkHome(); const { server, port } = await startCaptureServer(); @@ -619,3 +715,59 @@ test("auth login honors BUTTERSTACK_NO_BROWSER and prints the URL instead", asyn rmHome(home); } }); + +// The interactive path, exercised without launching anything real: a shim +// named `open` (macOS) / `xdg-open` (linux) is placed first on PATH and +// records its argv. This is the only case that covers the branch a user +// actually hits, and it asserts both halves of it - the line that claims a +// browser is opening, and the browser actually being handed the URL. +test("without the flag, auth login says it is opening a browser and hands it the URL", async () => { + const home = mkHome(); + const shimDir = fs.mkdtempSync(path.join(os.tmpdir(), "butter-shim-")); + const argvLog = path.join(shimDir, "argv.txt"); + const shimName = process.platform === "darwin" ? "open" : "xdg-open"; + fs.writeFileSync( + path.join(shimDir, shimName), + `#!/bin/sh\nprintf '%s\\n' "$1" >> ${JSON.stringify(argvLog)}\n`, + { mode: 0o755 } + ); + + const { server, port } = await startCaptureServer(); + try { + const child = spawn(process.execPath, [BUTTER_BIN, "auth", "login", "--host", `http://127.0.0.1:${port}`], { + env: { + ...buildEnv({ home, overrides: { BUTTERSTACK_NO_BROWSER: null } }), + PATH: `${shimDir}:${process.env.PATH}` + } + }); + const readOut = captureStdout(child); + + const authUrl = cleanUrl(await readAuthUrl(child)); + const parsed = new URL(authUrl); + await hitCallback( + `http://127.0.0.1:${parsed.searchParams.get("port")}/callback?code=c&state=${parsed.searchParams.get("state")}` + ); + await waitForExit(child); + + const out = stripAnsi(readOut()); + assert.ok(out.includes("Opening your browser"), `expected the browser line:\n${out}`); + assert.ok(!out.includes("Visit this URL to authorize:"), `no-browser prompt must not appear:\n${out}`); + + // Still exactly once, on this path too. + assert.equal(out.split(authUrl).length - 1, 1, `URL should appear once:\n${out}`); + + // And the browser really was invoked, with the same URL the user was shown. + // openBrowser() spawns asynchronously and does not wait, so the shim can + // still be writing when the CLI process has already exited. + for (let i = 0; i < 50 && !fs.existsSync(argvLog); i++) { + await new Promise((r) => setTimeout(r, 20)); + } + assert.ok(fs.existsSync(argvLog), "the browser helper should have been spawned"); + const handed = stripAnsi(fs.readFileSync(argvLog, "utf-8")).trim(); + assert.equal(handed, authUrl, "the browser must receive the URL that was printed"); + } finally { + await stopCaptureServer(server); + fs.rmSync(shimDir, { recursive: true, force: true }); + rmHome(home); + } +}); From 315ab18e6dc5f4435a972e2653673a620fc3145a Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Fri, 18 Sep 2026 22:30:47 -0400 Subject: [PATCH 3/3] chore(cli): trim comments 42 comment lines removed. The assertion messages already say what each check is for; the comments were restating them. Co-Authored-By: Claude Opus 5 --- bin/butter | 10 ++-------- test/butter.test.js | 45 +++++++++++---------------------------------- 2 files changed, 13 insertions(+), 42 deletions(-) diff --git a/bin/butter b/bin/butter index 19d16bc..4cdb13b 100755 --- a/bin/butter +++ b/bin/butter @@ -105,14 +105,8 @@ function clearCredentials() { } function openBrowser(url) { - // BUTTERSTACK_NO_BROWSER exists for anything that drives `auth login` - // without a human at the keyboard. The two auth-login tests spawn the real - // binary against a throwaway loopback server, so every `npm test` run used - // to fire `open` twice and leave real browser tabs pointing at ports that - // close seconds later. - // - // Nothing to print here: authLogin already wrote the URL, and adds its own - // "Visit this URL to authorize:" line when this flag is set. + // For non-interactive callers (tests, CI, containers). authLogin has already + // printed the URL, so there is nothing to add here. if (process.env.BUTTERSTACK_NO_BROWSER) return; // spawn with an argument array, not a shell string built via exec(): the diff --git a/test/butter.test.js b/test/butter.test.js index cd0e077..f2762e2 100644 --- a/test/butter.test.js +++ b/test/butter.test.js @@ -43,11 +43,7 @@ function writeCredentials(home, { host, token = "test-token" }) { // explicit null/undefined value meaning "unset this variable" -- needed // for the "no BUTTERSTACK_HOST at all" case. function buildEnv(env) { - // Never launch a real browser from a test. The auth-login cases spawn the - // actual binary, which calls openBrowser() -> `open ` on macOS, so - // every `npm test` run left tabs pointing at loopback ports that close - // seconds later. Set before the per-test overrides so a test could still - // opt out deliberately. + // No real browser from a test. Set before the overrides so a test can opt out. const fullEnv = { ...process.env, BUTTERSTACK_NO_BROWSER: "1", HOME: env.home }; for (const [key, value] of Object.entries(env.overrides || {})) { if (value === null || value === undefined) delete fullEnv[key]; @@ -583,30 +579,21 @@ test("#1938: a server with no investigation key is not reported as 'no investiga } }); -// Regression guard for the browser-tab leak: `npm test` spawns the real binary -// for the auth-login cases, and openBrowser() shells out to `open` on macOS. -// Every run used to leave two tabs pointing at loopback ports that close -// seconds later. buildEnv() now sets BUTTERSTACK_NO_BROWSER for all test -// invocations; this asserts the binary actually honors it. -// Collects everything the child writes to stdout until it exits, so a test can -// assert on the whole rendered block rather than the first line readAuthUrl -// happened to match. +// buildEnv sets BUTTERSTACK_NO_BROWSER for every test; this pins that the +// binary honors it. +// Buffers the child's full stdout, so tests can assert on the whole block. function captureStdout(child) { let out = ""; child.stdout.on("data", (c) => (out += c)); return () => out; } -// ANSI colour codes make exact-match assertions unreadable; the codes are not -// what these tests are about. function stripAnsi(s) { return s.replace(/\x1b\[[0-9;]*m/g, ""); } -// readAuthUrl matches with (\S+) against coloured output, and ANSI escape -// characters are non-space, so the URL it returns has a trailing reset code -// glued to it. Harmless for `new URL(...)` (it lands inside the last query -// value) but fatal to any exact string comparison. +// readAuthUrl's (\S+) swallows the trailing ANSI reset. Fine for `new URL()`, +// fatal for string comparison. function cleanUrl(u) { return stripAnsi(u); } @@ -631,20 +618,17 @@ test("auth login prints the URL exactly once, and does not claim to open a brows const out = stripAnsi(readOut()); - // The URL appeared twice before this was fixed: once from authLogin's - // "URL:" line and again from openBrowser's fallback message. + // Printed twice before the fix: authLogin's line, then openBrowser's. const occurrences = out.split(authUrl).length - 1; assert.equal(occurrences, 1, `the auth URL should appear exactly once, saw ${occurrences}:\n${out}`); - // Claiming to open a browser while deliberately not opening one is a lie - // the user can see. assert.ok( !out.includes("Opening your browser"), `must not claim to open a browser when BUTTERSTACK_NO_BROWSER is set:\n${out}` ); assert.ok(out.includes("Visit this URL to authorize:"), `expected the no-browser prompt:\n${out}`); - // That fallback belongs to the spawn-failure path, which is not this one. + // Those belong to the spawn-failure path, not this one. assert.ok(!out.includes("Could not automatically open browser"), out); assert.ok(!out.includes("Please visit this URL to authenticate"), out); } finally { @@ -672,7 +656,6 @@ test("the no-browser prompt replaces the browser line rather than adding to it", const lines = stripAnsi(readOut()).split("\n").map((l) => l.trim()).filter(Boolean); - // Exactly one line introduces the URL, and exactly one line carries it. const intro = lines.filter((l) => /^Visit this URL to authorize:$/.test(l)); const urlLines = lines.filter((l) => l.startsWith("URL: ")); assert.equal(intro.length, 1, `expected one intro line, got ${intro.length}:\n${lines.join("\n")}`); @@ -716,11 +699,8 @@ test("auth login honors BUTTERSTACK_NO_BROWSER and prints the URL instead", asyn } }); -// The interactive path, exercised without launching anything real: a shim -// named `open` (macOS) / `xdg-open` (linux) is placed first on PATH and -// records its argv. This is the only case that covers the branch a user -// actually hits, and it asserts both halves of it - the line that claims a -// browser is opening, and the browser actually being handed the URL. +// The branch a real user hits. A shim `open`/`xdg-open` first on PATH records +// its argv, so the real path runs and nothing launches. test("without the flag, auth login says it is opening a browser and hands it the URL", async () => { const home = mkHome(); const shimDir = fs.mkdtempSync(path.join(os.tmpdir(), "butter-shim-")); @@ -753,12 +733,9 @@ test("without the flag, auth login says it is opening a browser and hands it the assert.ok(out.includes("Opening your browser"), `expected the browser line:\n${out}`); assert.ok(!out.includes("Visit this URL to authorize:"), `no-browser prompt must not appear:\n${out}`); - // Still exactly once, on this path too. assert.equal(out.split(authUrl).length - 1, 1, `URL should appear once:\n${out}`); - // And the browser really was invoked, with the same URL the user was shown. - // openBrowser() spawns asynchronously and does not wait, so the shim can - // still be writing when the CLI process has already exited. + // openBrowser spawns without waiting, so the shim may still be writing. for (let i = 0; i < 50 && !fs.existsSync(argvLog); i++) { await new Promise((r) => setTimeout(r, 20)); }