From 176f15dced452fda13831f2652265760f87d5d6e Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Fri, 24 Jul 2026 14:54:02 +0200 Subject: [PATCH] fix: resolve Bugbot findings on promotion PR #397 - upgrade: reuse prepareHostInstallerCmd / installerURL (download-then-exec) instead of curl|bash, so stdin stays on the TTY and the URL has one source - upgrade: treat cancelled context / bash exit 130 as a quiet interrupt (exit 130), matching prepare-host Co-Authored-By: Claude Opus 4.8 --- internal/cli/prepare_host.go | 54 +++++++++++++++++++------------ internal/cli/prepare_host_test.go | 8 ++--- internal/cli/upgrade.go | 34 ++++++++++++------- internal/cli/upgrade_test.go | 20 +++++++++--- 4 files changed, 74 insertions(+), 42 deletions(-) diff --git a/internal/cli/prepare_host.go b/internal/cli/prepare_host.go index 6f1757f..2a00d77 100644 --- a/internal/cli/prepare_host.go +++ b/internal/cli/prepare_host.go @@ -20,29 +20,41 @@ import ( // early with a clear error rather than a confusing failure deep in the installer). var prepareHostUserRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,31}$`) -// prepareHostInstallerCmd runs the official installer's admin-only prepare-host -// step. Like `tracebloc upgrade`, this deliberately delegates to the verified -// installer (cosign-checked) instead of re-implementing any privileged host prep -// in the CLI — the privileged surface stays in one audited place. +// installerRunScript builds the bash program that downloads the cosign-verified +// installer to a temp file and runs THAT — optionally with a subcommand (e.g. +// "prepare-host"). Shared by `tracebloc upgrade` (no subcommand, full install) +// and `tracebloc prepare-host`, so both stay on one download-then-execute idiom. // -// We download the installer to a temp file and run THAT, rather than -// `curl | bash -s`. Two reasons, both Bugbot #394: -// - stdin: with `curl | bash -s`, the inner bash reads its *program* from the +// We run a downloaded FILE rather than `curl … | bash`. Two reasons, both Bugbot +// (#394, #397): +// - stdin: with `curl | bash`, the inner bash reads its *program* from the // pipe, so the installer's stdin is no longer the terminal. Any interactive -// prompt in prepare-host (e.g. which non-admin user gets runtime access) -// would get EOF. Running a downloaded file leaves stdin on the TTY. +// prompt (sign-in, or which non-admin user gets runtime access) would get +// EOF. Running a downloaded file leaves stdin on the TTY. // - fail-closed: `set -e` + `curl -o` makes a failed download (network/DNS/HTTP // error) abort with a non-zero status instead of silently running nothing. // (`curl | bash` swallowed this — bash read empty stdin and exited 0.) // // The temp file is removed on exit. The URL comes from installerURL (doctor.go) -// so the automated download can't drift from the manual hint / other bootstrap -// copy (Bugbot #394). -const prepareHostInstallerCmd = `set -e +// so every installer path shares one source and can't drift (Bugbot #394/#397). +func installerRunScript(subcommand string) string { + run := `bash "$tmp"` + if subcommand != "" { + run += " " + subcommand + } + return `set -e tmp="$(mktemp)" trap 'rm -f "$tmp"' EXIT curl -fsSL ` + installerURL + ` -o "$tmp" -bash "$tmp" prepare-host` +` + run +} + +// prepareHostInstallerCmd runs the official installer's admin-only prepare-host +// step. Like `tracebloc upgrade`, this deliberately delegates to the verified +// installer (cosign-checked) instead of re-implementing any privileged host prep +// in the CLI — the privileged surface stays in one audited place. See +// installerRunScript for why we download-then-execute rather than pipe. +var prepareHostInstallerCmd = installerRunScript("prepare-host") // prepareHostManualHint is the copy-pasteable command we show if the automated // run fails. Built from installCmd (doctor.go) — the single shared bootstrap @@ -100,13 +112,13 @@ func prepareHostCmd(ctx context.Context) *exec.Cmd { return c } -// prepareHostInterrupted reports whether the installer run ended because the user -// aborted, so the caller can exit quietly (130) instead of framing it as a failed -// install. ctx.Err() catches a cancel the signal handler already propagated — but -// on a terminal Ctrl-C the child can die and c.Run() can return BEFORE -// NotifyContext flips ctx.Err() (a race), so also treat bash's 130 (128+SIGINT) -// exit as an interrupt (Bugbot #394). -func prepareHostInterrupted(ctx context.Context, runErr error) bool { +// installerRunInterrupted reports whether an installer run (upgrade or +// prepare-host) ended because the user aborted, so the caller can exit quietly +// (130) instead of framing it as a failed install. ctx.Err() catches a cancel +// the signal handler already propagated — but on a terminal Ctrl-C the child can +// die and c.Run() can return BEFORE NotifyContext flips ctx.Err() (a race), so +// also treat bash's 130 (128+SIGINT) exit as an interrupt (Bugbot #394/#397). +func installerRunInterrupted(ctx context.Context, runErr error) bool { if ctx.Err() != nil { return true } @@ -189,7 +201,7 @@ host, so it's safe to run on a shared machine. Safe to re-run.`, // User aborted (Ctrl-C) or the parent context was cancelled: exit // quietly with 130 like the other cancellable paths, not a scary // "prepare-host didn't complete — retry" (Bugbot #394). - if prepareHostInterrupted(ctx, err) { + if installerRunInterrupted(ctx, err) { return &exitError{code: exitInterrupted} } return &exitError{code: exitFailure, err: fmt.Errorf("prepare-host didn't complete (%w). You can run the installer directly:\n %s", err, prepareHostManualHint(user))} diff --git a/internal/cli/prepare_host_test.go b/internal/cli/prepare_host_test.go index a8ee58e..799995a 100644 --- a/internal/cli/prepare_host_test.go +++ b/internal/cli/prepare_host_test.go @@ -54,11 +54,11 @@ func TestPrepareHostCmdStaysInForegroundGroup(t *testing.T) { // A user abort must be detected as an interrupt even when NotifyContext hasn't // flipped ctx.Err() yet — bash exits 130 on SIGINT and c.Run() can return first // (Bugbot #394). A genuine failure (exit 1) must NOT be treated as an interrupt. -func TestPrepareHostInterrupted(t *testing.T) { +func TestInstallerRunInterrupted(t *testing.T) { // Cancelled context → interrupt regardless of the run error. ctx, cancel := context.WithCancel(context.Background()) cancel() - if !prepareHostInterrupted(ctx, errors.New("boom")) { + if !installerRunInterrupted(ctx, errors.New("boom")) { t.Error("a cancelled context must be treated as an interrupt") } @@ -68,13 +68,13 @@ func TestPrepareHostInterrupted(t *testing.T) { // Live context + bash exit 130 (128+SIGINT) → interrupt (the Ctrl-C race). if err := exec.Command("bash", "-c", "exit 130").Run(); err == nil { t.Fatal("expected a non-nil error from exit 130") - } else if !prepareHostInterrupted(context.Background(), err) { + } else if !installerRunInterrupted(context.Background(), err) { t.Error("exit 130 with a live context must be treated as an interrupt") } // Live context + a normal failure (exit 1) → NOT an interrupt. if err := exec.Command("bash", "-c", "exit 1").Run(); err == nil { t.Fatal("expected a non-nil error from exit 1") - } else if prepareHostInterrupted(context.Background(), err) { + } else if installerRunInterrupted(context.Background(), err) { t.Error("exit 1 must NOT be treated as an interrupt") } } diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 75d7d9f..d0e27b8 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -18,17 +18,18 @@ const upgradeCmdName = "upgrade" // installer ourselves: it re-downloads + cosign-verifies the release, replaces // the CLI, and upgrades the secure environment's services to match — so we never // re-implement (and risk diverging from) the installer's signature verification. -// The short `curl … | bash` form (not `bash <(curl …)`) works from any shell -// (see the shell-safety fix in the docs). +// We download-then-execute the installer (installerRunScript, shared with +// prepare-host) rather than `curl … | bash`: piping makes the inner bash read +// its program from the pipe, stealing the installer's stdin so its interactive +// prompts (sign-in, etc.) can't read the TTY. The URL is derived from +// installerURL (doctor.go) so it can't drift from the other installer paths +// (Bugbot #397). // // Windows is different: we do NOT self-exec there. A running .exe is locked, so // install.ps1's Move-Item can't overwrite the very binary we're running, and // install.ps1 is CLI-only (no environment). So on Windows we print the command // for the user to run in a fresh shell instead of pretending to self-update. -const ( - upgradeInstallerCmdUnix = "curl -fsSL https://tracebloc.io/i.sh | bash" - upgradeInstallerCmdWindows = "irm https://github.com/tracebloc/cli/releases/latest/download/install.ps1 | iex" -) +const upgradeInstallerCmdWindows = "irm https://github.com/tracebloc/cli/releases/latest/download/install.ps1 | iex" // upgradePlan is how `upgrade` proceeds on a given OS: either exec the installer // (Unix) or just show the user a command to run (Windows). manual is the @@ -46,14 +47,16 @@ func upgradePlanFor(goos string) upgradePlan { if goos == "windows" { return upgradePlan{exec: false, manual: upgradeInstallerCmdWindows} } - // `-o pipefail` so a failed `curl` fails the whole pipeline: without it the - // trailing `bash` exits 0 on empty stdin and we'd report a successful upgrade - // having installed nothing. + // Download-then-execute the verified installer (installerRunScript, shared + // with prepare-host): its `set -e`+`curl -o` fails closed on a bad download, + // and running a file (not a pipe) keeps the installer's stdin on the TTY. The + // manual hint reuses installCmd (doctor.go), the shared bootstrap idiom, so + // the URL has a single source. return upgradePlan{ exec: true, name: "bash", - args: []string{"-o", "pipefail", "-c", upgradeInstallerCmdUnix}, - manual: upgradeInstallerCmdUnix, + args: []string{"-c", installerRunScript("")}, + manual: installCmd, } } @@ -102,9 +105,16 @@ Safe to run anytime; safe to re-run.`, // Stream the installer straight to the user's terminal, and keep // stdin wired so its interactive prompts (sign-in, etc.) still work. - c := exec.CommandContext(cmd.Context(), plan.name, plan.args...) + ctx := cmd.Context() + c := exec.CommandContext(ctx, plan.name, plan.args...) c.Stdin, c.Stdout, c.Stderr = os.Stdin, os.Stdout, os.Stderr if err := c.Run(); err != nil { + // User aborted (Ctrl-C) or the parent context was cancelled: exit + // quietly with 130 like prepare-host, not a scary "upgrade didn't + // complete — retry" (Bugbot #397). + if installerRunInterrupted(ctx, err) { + return &exitError{code: exitInterrupted} + } return &exitError{code: exitFailure, err: fmt.Errorf( "upgrade didn't complete (%w). You can run the installer directly:\n %s", err, plan.manual)} diff --git a/internal/cli/upgrade_test.go b/internal/cli/upgrade_test.go index ed5e845..61f41d0 100644 --- a/internal/cli/upgrade_test.go +++ b/internal/cli/upgrade_test.go @@ -49,7 +49,9 @@ func TestUpgradeCmd_HelpMentionsVerified(t *testing.T) { // TestUpgradePlanFor_PerOS: Windows must NOT self-exec (a running .exe is locked // and install.ps1 is CLI-only) — it only prints the manual command. Unix runs -// the installer with pipefail so a failed curl fails the whole pipeline. +// the verified installer via the shared download-then-execute script, never +// `curl | bash` (which would steal the installer's stdin), and reuses installCmd +// for the manual hint so the URL has one source (Bugbot #397). func TestUpgradePlanFor_PerOS(t *testing.T) { win := upgradePlanFor("windows") if win.exec { @@ -68,14 +70,22 @@ func TestUpgradePlanFor_PerOS(t *testing.T) { t.Errorf("%s upgrade should exec bash, got exec=%v name=%q", goos, p.exec, p.name) } joined := strings.Join(p.args, " ") - if !strings.Contains(joined, "-o pipefail") { - t.Errorf("%s upgrade must set pipefail so a failed curl fails the run: %q", goos, joined) + // Download-then-execute, not `curl | bash`: piping steals the installer's + // stdin so its interactive prompts can't read the TTY (Bugbot #397). + if strings.Contains(joined, "| bash") || strings.Contains(joined, "|bash") { + t.Errorf("%s upgrade must not pipe the installer into bash (steals its stdin): %q", goos, joined) + } + // `set -e` + `curl -o ` fails closed on a bad download instead of + // running an empty script and reporting a phantom success. + if !strings.Contains(joined, "set -e") || !strings.Contains(joined, "curl") || !strings.Contains(joined, "-o ") { + t.Errorf("%s upgrade must download the installer to a file (set -e + curl -o): %q", goos, joined) } if !strings.Contains(joined, "i.sh") { t.Errorf("%s upgrade must run i.sh: %q", goos, joined) } - if p.manual != upgradeInstallerCmdUnix { - t.Errorf("%s manual hint = %q, want %q", goos, p.manual, upgradeInstallerCmdUnix) + // Manual hint reuses installCmd (single URL source), not a re-hardcoded URL. + if p.manual != installCmd { + t.Errorf("%s manual hint = %q, want installCmd %q", goos, p.manual, installCmd) } } }