diff --git a/bin/butter b/bin/butter index 4bc0f53..4cdb13b 100755 --- a/bin/butter +++ b/bin/butter @@ -105,6 +105,10 @@ function clearCredentials() { } function openBrowser(url) { + // 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 // URL is host-derived (--host / BUTTERSTACK_HOST / config), so a malicious // or malformed host value can never be interpreted by a shell, only @@ -374,7 +378,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 9a70cf8..f2762e2 100644 --- a/test/butter.test.js +++ b/test/butter.test.js @@ -43,7 +43,8 @@ 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 }; + // 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]; else fullEnv[key] = value; @@ -577,3 +578,173 @@ test("#1938: a server with no investigation key is not reported as 'no investiga rmHome(home); } }); + +// 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; +} + +function stripAnsi(s) { + return s.replace(/\x1b\[[0-9;]*m/g, ""); +} + +// readAuthUrl's (\S+) swallows the trailing ANSI reset. Fine for `new URL()`, +// fatal for 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()); + + // 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}`); + + 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}`); + + // 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 { + 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); + + 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(); + 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); + } +}); + +// 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-")); + 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}`); + + assert.equal(out.split(authUrl).length - 1, 1, `URL should appear once:\n${out}`); + + // 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)); + } + 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); + } +});