diff --git a/README.md b/README.md index 39d0b1f8..faebcf37 100644 --- a/README.md +++ b/README.md @@ -449,6 +449,12 @@ MoshCode resolves the latest GitHub release and drops the binary in goes through your distro's package manager and will ask for sudo (on macOS it delegates to the App Store). +MoshCode asks for that password **before** starting the work rather than letting +the installer stop for it partway through โ€” which matters most in `moshcode +update`, where tailscale is one step in a long unattended run and the prompt +would otherwise land where nobody is watching. Nothing is asked when the plan has +no privileged step in it, when a credential is already cached, or on macOS. + Top-level passthrough preserves stdin, stdout, stderr, environment variables, the current directory, and the native exit result. That keeps JSON pipelines usable: diff --git a/bin/moshcode.mjs b/bin/moshcode.mjs index 48c0019a..924e1d9c 100755 --- a/bin/moshcode.mjs +++ b/bin/moshcode.mjs @@ -43,6 +43,7 @@ import { renderMarkdown, renderScriptVerb, suggest, wantsHelp, withoutHelp, } from "../src/help.mjs"; import { moshcodeVersion } from "../src/ui.mjs"; +import { needsRootHere, primeEscalation } from "../src/escalate.mjs"; const HERE = path.dirname(fileURLToPath(import.meta.url)); const EXAMPLE = path.join(HERE, "..", "examples", "alive.mosh"); @@ -445,6 +446,9 @@ async function main() { } const { install, desc, bin } = entry; console.log(`๐ŸŽธ installing ${target} โ€” ${desc}\n$ ${install.cmd} ${install.args.join(" ")}\n`); + // Ask for the password before the installer starts, not after it has spent a + // minute refreshing package lists and then stopped to wait on one. + if (needsRootHere(entry)) primeEscalation({ what: target }); const result = await runCmd(install.cmd, install.args); if (!result.ok) { console.error(`install failed: ${result.error?.message || result.error || "unknown error"}`); diff --git a/src/escalate.mjs b/src/escalate.mjs index 5a21a88b..726290b5 100644 --- a/src/escalate.mjs +++ b/src/escalate.mjs @@ -27,6 +27,22 @@ function defaultProbe(tool) { return spawnSync("sh", ["-c", `command -v ${tool}`], { stdio: "ignore" }).status === 0; } +/** + * How to ask a helper to cache a credential without running anything real. + * + * `sudo -v` exists for exactly this: validate, refresh the timestamp, run no + * command. doas has no equivalent flag, so it gets the smallest possible real + * command instead โ€” the point is only to make it prompt. + */ +function primeArgs(tool) { + return tool === "sudo" ? ["-v"] : ["true"]; +} + +/** `-n` is "never prompt" in both, so a zero exit means a credential is ready. */ +function alreadyCached(tool, spawn) { + return spawn(tool, ["-n", "true"], { stdio: "ignore" })?.status === 0; +} + /** * Which escalation helper this machine has, honouring an explicit override. * Returns null when there is none โ€” a container running as a non-root user @@ -42,6 +58,82 @@ export function findEscalator({ env = process.env, probe = defaultProbe } = {}) return null; } +/** + * Does installing or upgrading this entry need root *on this machine*? + * + * A spec says `needsRoot: true` when it always does, or `{ except: [...] }` when + * a platform is the exception. tailscale is the reason for the second form: its + * script goes through the distro package manager on Linux and delegates to the + * App Store on macOS, where nothing escalates. Without the distinction, every + * mac running `moshcode update` would be asked for a password by a step that + * never wanted one โ€” which is the same bug as prompting halfway through, just + * earlier and more annoying. + */ +export function needsRootHere(entry, platform = process.platform) { + const spec = entry?.needsRoot; + if (!spec) return false; + if (spec === true) return true; + if (Array.isArray(spec)) return spec.includes(platform); + if (Array.isArray(spec.except)) return !spec.except.includes(platform); + return false; +} + +/** + * Ask for the password now, for a step that will need root later. + * + * Some installers escalate on their own partway through their own work โ€” + * tailscale's goes through the distro package manager, so it calls sudo after + * refreshing package lists. That is fine when it is the only thing running and + * miserable inside `moshcode update`, which walks a plan: moshcode itself, then + * every installed engine, then every tool. The operator sees a long stream of + * downloads scroll past, looks away, and comes back to a run that has been + * parked on a password prompt โ€” or worse, to sudo's own timeout having failed + * the step. The work is not interactive, so nobody is watching the one moment + * that is. + * + * So we prompt before starting instead. sudo caches the credential against the + * terminal, and every installer we hand off to inherits that same terminal, so + * the escalation they do later finds it already there and never asks. + * + * Returns `{ primed, tool, reason }` and never throws. `primed: false` is not + * fatal anywhere it is called: the caller carries on and the installer prompts + * whenever it was going to, which is exactly the old behaviour. Being unable to + * ask early is a missed convenience, not a reason to refuse to install. + */ +export function primeEscalation({ + what = "this", + env = process.env, + isTTY = Boolean(process.stdin?.isTTY && process.stdout?.isTTY), + spawn = spawnSync, + probe = defaultProbe, + out = console.log, + getuid = typeof process.getuid === "function" ? process.getuid : null, +} = {}) { + // Already root โ€” nothing to ask for, and nothing to ask with. + if (getuid && getuid() === 0) return { primed: true, tool: null, reason: "already-root" }; + // No terminal means no prompt. Warming a credential here would either fail or + // hang a CI job on a password nobody can type, which is the thing this exists + // to prevent rather than to cause. + if (!isTTY) return { primed: false, tool: null, reason: "no-tty" }; + + const tool = findEscalator({ env, probe }); + if (!tool) return { primed: false, tool: null, reason: "no-escalator" }; + + // Silence is the right outcome when a credential is already cached, or when + // this operator's rule is NOPASSWD. Printing "asking for your password" and + // then not asking reads as a bug. + if (alreadyCached(tool, spawn)) return { primed: true, tool, reason: "cached" }; + + out(`ยท ${what} needs root partway through โ€” asking ${tool} for your password now, so it doesn't stop halfway.`); + const result = spawn(tool, primeArgs(tool), { stdio: "inherit" }); + if (result?.error) return { primed: false, tool, reason: "spawn-failed" }; + // A non-zero exit is a wrong password, a cancelled prompt, or an operator who + // is not in sudoers. All three mean "carry on unprimed" rather than "stop": + // the installer may well not need root on this machine at all. + if (result?.status !== 0) return { primed: false, tool, reason: "declined" }; + return { primed: true, tool, reason: "prompted" }; +} + /** * Re-run this CLI's own argv under the escalation helper. * diff --git a/src/tools.mjs b/src/tools.mjs index 1225797a..24185ad0 100644 --- a/src/tools.mjs +++ b/src/tools.mjs @@ -118,6 +118,17 @@ export const TOOLS = { // which means it needs root โ€” it finds sudo/doas itself and may prompt for a // password (stdio is inherited, so the prompt works). On macOS the same // script delegates to the App Store. + // + // `needsRoot` is what lets us get that prompt out of the way before the work + // starts rather than partway through it. It says nothing about how the + // escalation happens โ€” the vendor script still does its own โ€” only that one + // is coming, which is all primeEscalation needs to know. `tailscale update` + // needs root for the same reason, so it covers both directions. + // + // macOS is the exception, and the same line above says why: there the script + // delegates to the App Store, which does its own authorisation. Asking for a + // sudo password there would be a prompt for a step that never escalates. + needsRoot: { except: ["darwin"] }, install: { cmd: "sh", args: ["-c", "curl -fsSL https://tailscale.com/install.sh | sh"] }, // Native updater on Linux (v1.36+) and Windows. macOS updates come from the // App Store, so there it fails with tailscale's own message rather than diff --git a/src/tui.mjs b/src/tui.mjs index 71ce719f..1896f9d2 100644 --- a/src/tui.mjs +++ b/src/tui.mjs @@ -26,6 +26,7 @@ import { cryptoCommand } from "./crypto.mjs"; import { gamesCommand } from "./games.mjs"; import { canOpenBrowser, openBrowser } from "./open-url.mjs"; import { shellInvocation } from "./shell.mjs"; +import { needsRootHere, primeEscalation } from "./escalate.mjs"; import { banner, hr, acid, ash, bone, dim, ok, err, warn, info, moshcodeVersion } from "./ui.mjs"; import { CORE_CLI_COMMAND_NAMES } from "./cli-schema.mjs"; import { RENAMED_COMMANDS, findPitCommand, pitHelpModel, renderPitCommand, suggest, wantsHelp } from "./help.mjs"; @@ -550,6 +551,9 @@ function installTarget(key) { const target = (Object.hasOwn(ENGINES, key) && ENGINES[key]) || (Object.hasOwn(TOOLS, key) && TOOLS[key]); if (!target) { console.log(err(`unknown engine or tool "${key}"`)); return resolve(); } console.log(info(`installing ${key}: ${target.install.cmd} ${target.install.args.join(" ")}`)); + // Before the rule, so the prompt reads as the pit asking rather than as + // something the installer's output scrolled into view. + if (needsRootHere(target)) primeEscalation({ what: key, out: (s) => console.log(info(s.replace(/^ยท /, ""))) }); console.log(hr()); const child = spawn(target.install.cmd, target.install.args, { stdio: "inherit" }); child.on("error", (e) => { diff --git a/src/upgrade.mjs b/src/upgrade.mjs index 932757bc..65b36bc9 100644 --- a/src/upgrade.mjs +++ b/src/upgrade.mjs @@ -6,6 +6,7 @@ import path from "node:path"; import fs from "node:fs"; import { ENGINES, engineStatus, exitReason, ranOk, resolveEngine, upgradeSpec, runCmd } from "./engines.mjs"; import { TOOLS, resolveTool, toolStatus, toolUpgradeSpec } from "./tools.mjs"; +import { needsRootHere, primeEscalation } from "./escalate.mjs"; // Self-upgrade re-runs the moshcode installer's `update` path. Defaults to the // GitHub-hosted install.sh (always live); override with MOSHCODE_INSTALL_URL. @@ -84,6 +85,7 @@ export function planUpgrade(targets = []) { // is something other than the installer, so a fallback can never repeat // the command that just failed. fallback: installed && upgradeSpec(ENGINES[key]) !== ENGINES[key].install ? ENGINES[key].install : null, + needsRoot: needsRootHere(ENGINES[key]), installed, }); }; @@ -98,6 +100,7 @@ export function planUpgrade(targets = []) { kind: "tool", spec: installed ? toolUpgradeSpec(TOOLS[key]) : TOOLS[key].install, fallback: installed && toolUpgradeSpec(TOOLS[key]) !== TOOLS[key].install ? TOOLS[key].install : null, + needsRoot: needsRootHere(TOOLS[key]), installed, }); }; @@ -167,6 +170,17 @@ export async function runUpgrade(targets = [], io = {}) { const exec = io.runCmd || runCmd; + // Get the password prompt out of the way before the first download rather than + // somewhere in the middle of the plan. A plan is long and unattended by + // design; the one interactive moment in it should not be buried where nobody + // is looking. Skipped entirely when nothing in the plan needs root, so the + // common `moshcode update` never asks. + const rootItems = items.filter((it) => it.needsRoot); + if (rootItems.length) { + const prime = io.primeEscalation || primeEscalation; + prime({ what: rootItems.map((it) => it.label).join(", "), out: log }); + } + const results = []; const attempt = async (name, spec, note) => { log(`\nโฌ† upgrading ${name}${note ? ` ${note}` : ""} โ€” ${spec.cmd} ${spec.args.join(" ")}`); diff --git a/test/escalate-prime.test.mjs b/test/escalate-prime.test.mjs new file mode 100644 index 00000000..17184f09 --- /dev/null +++ b/test/escalate-prime.test.mjs @@ -0,0 +1,165 @@ +// Asking for the password up front. +// +// Some installers escalate on their own partway through their own work โ€” +// tailscale's goes through the distro package manager, so it calls sudo after +// refreshing package lists. Inside `moshcode update`, which walks a plan of +// moshcode + every engine + every tool, that prompt lands somewhere in the +// middle of a long unattended stream and parks the whole run. +// +// What these pin is the ordering the operator experiences: one prompt before any +// work, and silence when there is nothing to ask for. Every input is injected, +// because none of these cases โ€” no tty, no sudo, a cancelled prompt, being root +// โ€” can be produced from inside a test process. +import assert from "node:assert/strict"; +import test from "node:test"; + +import { needsRootHere, primeEscalation } from "../src/escalate.mjs"; +import { TOOLS } from "../src/tools.mjs"; + +const has = (...names) => (tool) => names.includes(tool); +const collect = () => { + const lines = []; + return { lines, out: (s) => lines.push(String(s)) }; +}; + +// The stub answers two different questions โ€” "is a credential already cached" +// (`-n true`) and "prompt now" โ€” so it branches on the args rather than +// counting calls, which would break the moment an implementation detail moved. +function fakeSudo({ cached = false, status = 0, error = null } = {}) { + const calls = []; + const spawn = (cmd, argv, opts) => { + calls.push({ cmd, argv, opts }); + if (argv[0] === "-n") return { status: cached ? 0 : 1 }; + return error ? { error } : { status }; + }; + return { calls, spawn }; +} + +test("a spec with no needsRoot never triggers a prompt", () => { + assert.equal(needsRootHere({}, "linux"), false); + assert.equal(needsRootHere(undefined, "linux"), false); + assert.equal(needsRootHere(TOOLS.gh, "linux"), false); +}); + +test("needsRoot: true means every platform", () => { + assert.equal(needsRootHere({ needsRoot: true }, "linux"), true); + assert.equal(needsRootHere({ needsRoot: true }, "darwin"), true); +}); + +test("an except list spares the platform that does not escalate", () => { + // tailscale's script uses the distro package manager on Linux and delegates to + // the App Store on macOS, where nothing asks for a password. Prompting there + // would be a password request for a step that never escalates. + assert.equal(needsRootHere(TOOLS.tailscale, "linux"), true); + assert.equal(needsRootHere(TOOLS.tailscale, "darwin"), false); +}); + +test("an explicit platform list is the other direction", () => { + assert.equal(needsRootHere({ needsRoot: ["linux"] }, "linux"), true); + assert.equal(needsRootHere({ needsRoot: ["linux"] }, "win32"), false); +}); + +test("prompts once, before the work, with sudo's validate-only flag", () => { + const { calls, spawn } = fakeSudo(); + const { lines, out } = collect(); + + const result = primeEscalation({ + what: "tailscale", env: {}, isTTY: true, probe: has("sudo"), spawn, out, getuid: () => 1000, + }); + + assert.deepEqual(result, { primed: true, tool: "sudo", reason: "prompted" }); + // `-v` refreshes the timestamp and runs nothing, which is what makes this safe + // to do before an install that might turn out not to need root at all. + assert.deepEqual(calls.map((c) => c.argv), [["-n", "true"], ["-v"]]); + // The prompt needs the terminal or there is nowhere to type. + assert.equal(calls[1].opts.stdio, "inherit"); + assert.match(lines.join("\n"), /tailscale needs root partway through/); +}); + +test("doas gets a real command, because it has no -v", () => { + const { calls, spawn } = fakeSudo(); + const result = primeEscalation({ + what: "tailscale", env: {}, isTTY: true, probe: has("doas"), spawn, out: () => {}, getuid: () => 1000, + }); + + assert.equal(result.tool, "doas"); + assert.deepEqual(calls.map((c) => c.argv), [["-n", "true"], ["true"]]); +}); + +test("says nothing when a credential is already cached", () => { + const { calls, spawn } = fakeSudo({ cached: true }); + const { lines, out } = collect(); + + const result = primeEscalation({ + what: "tailscale", env: {}, isTTY: true, probe: has("sudo"), spawn, out, getuid: () => 1000, + }); + + assert.deepEqual(result, { primed: true, tool: "sudo", reason: "cached" }); + // Announcing a password prompt and then not showing one reads as a bug. This + // is also the NOPASSWD case. + assert.deepEqual(lines, []); + assert.equal(calls.length, 1); +}); + +test("root has nothing to ask for", () => { + const result = primeEscalation({ + env: {}, isTTY: true, probe: has("sudo"), + spawn: () => assert.fail("must not shell out when already root"), + out: () => assert.fail("must not print"), + getuid: () => 0, + }); + + assert.deepEqual(result, { primed: true, tool: null, reason: "already-root" }); +}); + +test("no tty means no prompt โ€” a CI job must not stall on one", () => { + const result = primeEscalation({ + env: {}, isTTY: false, probe: has("sudo"), + spawn: () => assert.fail("must not prompt without a terminal"), + out: () => {}, getuid: () => 1000, + }); + + assert.deepEqual(result, { primed: false, tool: null, reason: "no-tty" }); +}); + +test("no escalator on the box is not an error", () => { + const result = primeEscalation({ + env: {}, isTTY: true, probe: has(), + spawn: () => assert.fail("nothing to spawn"), + out: () => {}, getuid: () => 1000, + }); + + assert.deepEqual(result, { primed: false, tool: null, reason: "no-escalator" }); +}); + +test("MOSHCODE_ESCALATOR is honoured here too", () => { + const { calls, spawn } = fakeSudo(); + const result = primeEscalation({ + what: "tailscale", env: { MOSHCODE_ESCALATOR: "doas" }, isTTY: true, + probe: has("sudo", "doas"), spawn, out: () => {}, getuid: () => 1000, + }); + + assert.equal(result.tool, "doas"); + assert.equal(calls[0].cmd, "doas"); +}); + +test("a cancelled or refused prompt carries on unprimed rather than failing the install", () => { + const { spawn } = fakeSudo({ status: 1 }); + const result = primeEscalation({ + what: "tailscale", env: {}, isTTY: true, probe: has("sudo"), spawn, out: () => {}, getuid: () => 1000, + }); + + // A wrong password, a cancelled prompt, and an operator who is not in sudoers + // all land here. The installer may not need root on this machine, so refusing + // must not decide that on its behalf. + assert.deepEqual(result, { primed: false, tool: "sudo", reason: "declined" }); +}); + +test("a failed spawn degrades instead of throwing", () => { + const { spawn } = fakeSudo({ error: new Error("ENOENT") }); + const result = primeEscalation({ + what: "tailscale", env: {}, isTTY: true, probe: has("sudo"), spawn, out: () => {}, getuid: () => 1000, + }); + + assert.deepEqual(result, { primed: false, tool: "sudo", reason: "spawn-failed" }); +}); diff --git a/test/upgrade-prime-sudo.test.mjs b/test/upgrade-prime-sudo.test.mjs new file mode 100644 index 00000000..823b0492 --- /dev/null +++ b/test/upgrade-prime-sudo.test.mjs @@ -0,0 +1,133 @@ +// `moshcode update` walks a plan: moshcode itself, then every installed engine, +// then every tool. It is long and nobody watches it. A tool whose installer +// escalates on its own โ€” tailscale goes through the distro package manager โ€” +// would otherwise stop for a password somewhere in the middle of that stream. +// +// So the ordering is the contract: the prompt comes before the first hand-off, +// or it is not worth having. These assert on one interleaved event log rather +// than on two separate call counts, because "was it asked for" is not the +// question โ€” "was it asked for first" is. +import assert from "node:assert/strict"; +import { chmodSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import test from "node:test"; + +import { planUpgrade, runUpgrade } from "../src/upgrade.mjs"; +import { TOOLS } from "../src/tools.mjs"; + +/** + * Make exactly `names` look installed and nothing else. + * + * PATH is replaced rather than prepended: which targets are installed is what + * decides the plan, so a real gh or tailscale on the developer's machine would + * otherwise leak in and change what is under test. binDirs is stubbed off for + * the same reason โ€” it searches outside PATH by design. + */ +async function withOnly(names, fn) { + const dir = mkdtempSync(path.join(tmpdir(), "moshcode-prime-")); + for (const name of names) { + const file = path.join(dir, name); + writeFileSync(file, "#!/bin/sh\nexit 0\n"); + chmodSync(file, 0o755); + } + const beforePath = process.env.PATH; + const beforeBinDirs = Object.values(TOOLS).map((tool) => [tool, tool.binDirs]); + process.env.PATH = dir; + for (const [tool] of beforeBinDirs) delete tool.binDirs; + try { return await fn(); } + finally { + process.env.PATH = beforePath; + for (const [tool, binDirs] of beforeBinDirs) if (binDirs) tool.binDirs = binDirs; + } +} + +/** + * Force an entry to need root regardless of platform, and put it back. + * + * tailscale's real declaration spares macOS, where its script delegates to the + * App Store. That is correct behaviour and it would also make the ordering tests + * below silently vacuous on a mac โ€” no privileged item in the plan, so nothing + * to prime, so the assertion that priming happens first never runs. Pinning the + * flag keeps these about ordering, and the platform rule is asserted on its own + * in escalate-prime.test.mjs. + */ +async function withNeedsRoot(entry, fn) { + const before = entry.needsRoot; + entry.needsRoot = true; + // Awaited, not just returned: a sync finally would restore the flag the moment + // fn() handed back its promise, which happens before the plan is even built. + // It passes either way today only because planUpgrade runs before runUpgrade's + // first await โ€” an accident, not something to rest a test on. + try { return await fn(); } + finally { entry.needsRoot = before; } +} + +test("the plan records which items need root", async () => { + await withOnly(["tailscale", "gh"], () => { + const { items } = planUpgrade(["tools"]); + const byKey = Object.fromEntries(items.map((it) => [it.key, it])); + // Platform-dependent by design: macOS gets its packages from the App Store, + // which does its own authorisation. + assert.equal(byKey.tailscale.needsRoot, process.platform !== "darwin"); + // Everything else lands in ~/.local/bin and must never trigger a prompt. + assert.equal(byKey.gh.needsRoot, false); + }); +}); + +test("asks for the password before the first hand-off, not partway through", async () => { + const events = []; + await withNeedsRoot(TOOLS.tailscale, () => withOnly(["tailscale"], () => runUpgrade(["tailscale"], { + log: () => {}, + rule: () => {}, + primeEscalation: ({ what }) => { events.push(`prime:${what}`); return { primed: true }; }, + runCmd: (cmd, args) => { events.push(`run:${cmd} ${args.join(" ")}`); return { ok: true, code: 0 }; }, + }))); + + assert.equal(events[0], "prime:tailscale", `expected priming first, got ${JSON.stringify(events)}`); + // And it is `tailscale update` that follows โ€” the native updater needs root + // for the same reason the installer does. + assert.match(events[1], /^run:tailscale update/); +}); + +test("a plan with nothing privileged in it never asks", async () => { + const events = []; + await withOnly(["gh"], () => runUpgrade(["gh"], { + log: () => {}, + rule: () => {}, + primeEscalation: () => assert.fail("must not ask for a password to upgrade gh"), + runCmd: (cmd, args) => { events.push(`run:${cmd} ${args.join(" ")}`); return { ok: true, code: 0 }; }, + })); + + assert.equal(events.length, 1); +}); + +test("one prompt covers a plan with several privileged items", async () => { + const primes = []; + await withNeedsRoot(TOOLS.tailscale, () => withOnly(["tailscale"], () => runUpgrade(["tailscale", "gh"], { + log: () => {}, + rule: () => {}, + primeEscalation: ({ what }) => { primes.push(what); return { primed: true }; }, + runCmd: () => ({ ok: true, code: 0 }), + }))); + + // sudo caches against the terminal, so asking once is enough no matter how + // many privileged steps follow. + assert.equal(primes.length, 1); + assert.equal(primes[0], "tailscale"); +}); + +test("a declined prompt does not cancel the run", async () => { + const events = []; + const results = await withNeedsRoot(TOOLS.tailscale, () => withOnly(["tailscale"], () => runUpgrade(["tailscale"], { + log: () => {}, + rule: () => {}, + primeEscalation: () => ({ primed: false, reason: "declined" }), + runCmd: (cmd, args) => { events.push(`run:${cmd} ${args.join(" ")}`); return { ok: true, code: 0 }; }, + }))); + + // The installer may not need root on this machine at all, and it prompts for + // itself if it does. Refusing early must not decide that for it. + assert.equal(events.length, 1); + assert.equal(results.filter((r) => r.ok).length, 1); +});