diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 3fd64411..3e22d3ed 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -17,20 +17,10 @@ import ( "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" "github.com/tracebloc/cli/internal/doctor" + "github.com/tracebloc/cli/internal/installer" "github.com/tracebloc/cli/internal/ui" ) -// installerURL is the single source of truth for the installer script URL. -// Everything that downloads or points at the installer (installCmd here, -// prepareHostInstallerCmd in prepare_host.go) derives from this so a URL change -// updates every path at once. -const installerURL = "https://tracebloc.io/i.sh" - -// installCmd is the one-line installer we point people at when there's no -// secure environment on this machine, or a component needs reinstalling. Kept in -// one place so every remedy says the same thing. -const installCmd = "bash <(curl -fsSL " + installerURL + ")" - // doctorRunFn is a test seam over doctor.Run (the cluster-side probe). Tests // inject a fixed []doctor.Result so the roll-up + render can be exercised with a // controlled mix without standing up a fake cluster. @@ -133,7 +123,7 @@ func runClusterDoctor( return &exitError{code: exitChecksFailed, err: nil} case errors.As(werr, &ue): p.Newline() - p.Errorf("This CLI is out of date — update it: %s", installCmd) + p.Errorf("This CLI is out of date — update it: %s", installer.Cmd) return &exitError{code: exitChecksFailed, err: nil} case errors.As(werr, &ae): tok = tokenServerErr // tracebloc answered, just not with 200 @@ -172,7 +162,7 @@ func runClusterDoctor( p.Newline() noteSessionProblem(p, tok) p.Errorf("No secure environment on this machine yet.") - p.Hintf(" Set one up: %s", installCmd) + p.Hintf(" Set one up: %s", installer.Cmd) return &exitError{code: earlyExitCode(tok), err: nil} } cs, err := newClientsetFn(resolved) @@ -195,7 +185,7 @@ func runClusterDoctor( p.Newline() noteSessionProblem(p, tok) p.Errorf("No secure environment on this machine yet.") - p.Hintf(" Set one up: %s", installCmd) + p.Hintf(" Set one up: %s", installer.Cmd) renderDetailsIfVerbose(p, resolved, results) return &exitError{code: earlyExitCode(tok), err: nil} } @@ -372,7 +362,7 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready case by["Pod health"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment isn't running.", - fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installCmd, launcher())} + fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installer.Cmd, launcher())} case by["Pod health"].Status == doctor.StatusWarn && strings.HasPrefix(by["Pod health"].Detail, "could not list pods"): // checkPods returns StatusWarn for TWO different situations: pods stuck // Pending (below) AND a failure to list pods at all (e.g. RBAC, doctor.go diff --git a/internal/cli/prepare_host.go b/internal/cli/prepare_host.go index cdfd8052..9e453b8a 100644 --- a/internal/cli/prepare_host.go +++ b/internal/cli/prepare_host.go @@ -12,6 +12,8 @@ import ( "time" "github.com/spf13/cobra" + + "github.com/tracebloc/cli/internal/installer" ) // prepareHostUserRe validates the researcher username before we pass it to the @@ -20,58 +22,25 @@ 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}$`) -// 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 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 (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 download uses `--tlsv1.2` — the TLS 1.2 floor scripts/install.sh enforces -// on every security-sensitive fetch — so this privileged installer download can -// never negotiate a weaker protocol (Bugbot #397). The temp file is removed on -// exit. The URL comes from installerURL (doctor.go) 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 --tlsv1.2 ` + installerURL + ` -o "$tmp" -` + 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") +// installer.Script for why we download-then-execute rather than pipe. +var prepareHostInstallerCmd = installer.Script("prepare-host", "") // prepareHostManualHint is the copy-pasteable command we show if the automated -// run fails. Built from installCmd (doctor.go) — the single shared bootstrap -// idiom — so a URL/idiom change updates every hint at once (Bugbot #394); we -// only append the prepare-host subcommand. installCmd uses process substitution -// (bash <(curl …)), which keeps stdin on the terminal for interactive prompts. -// When a researcher username was given we prefix TB_PREPARE_USER= so a -// copy-pasted retry still grants access — otherwise the manual fallback would -// silently do less than the original request (Bugbot #394). +// run fails. It's built from installer.Script — the same single bootstrap idiom +// we just executed — so the hint is byte-identical to the command that failed, +// and a URL/idiom change updates every hint at once (Bugbot #394, cli#396). +// When a researcher username was given we carry TB_PREPARE_USER= into the +// installer so a copy-pasted retry still grants access — otherwise the manual +// fallback would silently do less than the original request (Bugbot #394). func prepareHostManualHint(user string) string { - if user != "" { - return "TB_PREPARE_USER=" + user + " " + installCmd + " prepare-host" + if user == "" { + return prepareHostInstallerCmd } - return installCmd + " prepare-host" + return installer.Script("prepare-host", "TB_PREPARE_USER="+user) } // prepareHostEnv is the child's environment: the parent's, but with any ambient @@ -110,7 +79,7 @@ func prepareHostEnv(user string) []string { // that traps signals. We rely on the default SIGKILL rather than a custom // SIGINT-only Cancel (which a privileged child could ignore, hanging Wait). func prepareHostCmd(ctx context.Context) *exec.Cmd { - c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) // #nosec G204 -- argv is compile-time constant: literal "bash" -c installerRunScript("prepare-host"), built only from the installerURL const; no runtime input. + c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) // #nosec G204 -- argv is compile-time constant: literal "bash" -c installer.Script("prepare-host", ""), built only from the installer.URL const; no runtime input. c.WaitDelay = 5 * time.Second return c } diff --git a/internal/cli/prepare_host_test.go b/internal/cli/prepare_host_test.go index 1da7d945..0e3ffcd0 100644 --- a/internal/cli/prepare_host_test.go +++ b/internal/cli/prepare_host_test.go @@ -6,6 +6,8 @@ import ( "os/exec" "strings" "testing" + + "github.com/tracebloc/cli/internal/installer" ) // A failed download must abort rather than run an empty script: with the old @@ -28,15 +30,15 @@ func TestPrepareHostCmdFailsClosedOnDownloadError(t *testing.T) { // Every curl in the shared installer script must pin the TLS 1.2 floor // (--tlsv1.2), matching scripts/install.sh, so this privileged download can never // negotiate a weaker protocol. Guards both the upgrade (no subcommand) and -// prepare-host paths since they share installerRunScript (Bugbot #397). +// prepare-host paths since they share installer.Script (Bugbot #397). func TestInstallerRunScriptPinsTLSFloor(t *testing.T) { for _, sub := range []string{"", "prepare-host"} { - script := installerRunScript(sub) + script := installer.Script(sub, "") if !strings.Contains(script, "curl") { - t.Fatalf("installerRunScript(%q) must curl the installer; got: %q", sub, script) + t.Fatalf("installer.Script(%q) must curl the installer; got: %q", sub, script) } if !strings.Contains(script, "--tlsv1.2") { - t.Errorf("installerRunScript(%q) must pin --tlsv1.2 on the download (matches install.sh); got: %q", sub, script) + t.Errorf("installer.Script(%q) must pin --tlsv1.2 on the download (matches install.sh); got: %q", sub, script) } } } @@ -172,6 +174,15 @@ func TestPrepareHostManualHint_CarriesUser(t *testing.T) { } } +// The no-username manual hint must be the EXACT command we just tried, so a user +// pasting it reproduces the automated run rather than a lookalike that could +// drift from it (cli#396). +func TestPrepareHostManualHint_MatchesTheCommandWeRan(t *testing.T) { + if got := prepareHostManualHint(""); got != prepareHostInstallerCmd { + t.Errorf("manual hint = %q, want the command we executed %q", got, prepareHostInstallerCmd) + } +} + // prepare-host shells out to bash/curl and readies a Unix host, so it must be // guarded on Windows (a no-op-with-explanation, not a cryptic missing-bash // failure) — mirrors upgrade's Windows handling (Bugbot #394). diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 686c1057..e7489817 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -7,6 +7,8 @@ import ( "runtime" "github.com/spf13/cobra" + + "github.com/tracebloc/cli/internal/installer" ) // upgradeCmdName is the command that re-runs the installer; the update nudge @@ -18,12 +20,12 @@ 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. -// 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). +// We download-then-execute the installer (installer.Cmd, the shared bootstrap +// idiom) 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. Both the URL and the idiom come +// from internal/installer so they can't drift from the other installer paths +// (Bugbot #397, cli#396). // // 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 @@ -47,16 +49,17 @@ func upgradePlanFor(goos string) upgradePlan { if goos == "windows" { return upgradePlan{exec: false, manual: upgradeInstallerCmdWindows} } - // 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. + // Download-then-execute the verified installer (installer.Cmd, shared with + // prepare-host and every printed remedy): 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. exec and manual are deliberately the SAME string — if the + // run fails, the command we hand the user is byte-identical to the one that + // just failed. return upgradePlan{ exec: true, name: "bash", - args: []string{"-c", installerRunScript("")}, - manual: installCmd, + args: []string{"-c", installer.Cmd}, + manual: installer.Cmd, } } @@ -120,7 +123,7 @@ 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. ctx := cmd.Context() - c := exec.CommandContext(ctx, plan.name, plan.args...) // #nosec G204 -- upgradePlanFor(runtime.GOOS) yields compile-time constants: "bash" -c installerRunScript(""); only the GOOS branch varies, no user input. + c := exec.CommandContext(ctx, plan.name, plan.args...) // #nosec G204 -- upgradePlanFor(runtime.GOOS) yields compile-time constants: "bash" -c installer.Cmd; only the GOOS branch varies, no user input. 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 diff --git a/internal/cli/upgrade_test.go b/internal/cli/upgrade_test.go index 59843896..5a0cab65 100644 --- a/internal/cli/upgrade_test.go +++ b/internal/cli/upgrade_test.go @@ -4,6 +4,8 @@ import ( "bytes" "strings" "testing" + + "github.com/tracebloc/cli/internal/installer" ) // TestUpgradeCmd_Metadata pins the command's shape without running it (RunE @@ -50,8 +52,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 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). +// `curl | bash` (which would steal the installer's stdin), and reuses +// installer.Cmd for the manual hint so the URL and idiom have one source +// (Bugbot #397, cli#396). func TestUpgradePlanFor_PerOS(t *testing.T) { win := upgradePlanFor("windows") if win.exec { @@ -83,9 +86,11 @@ func TestUpgradePlanFor_PerOS(t *testing.T) { if !strings.Contains(joined, "i.sh") { t.Errorf("%s upgrade must run i.sh: %q", goos, joined) } - // 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) + // Manual hint reuses installer.Cmd (single source for URL *and* idiom), not + // a re-hardcoded command — and it's the same string we exec, so the hint we + // print after a failed run is exactly what failed (cli#396). + if p.manual != installer.Cmd { + t.Errorf("%s manual hint = %q, want installer.Cmd %q", goos, p.manual, installer.Cmd) } } } diff --git a/internal/cluster/discover.go b/internal/cluster/discover.go index 257e6ed6..39c1d882 100644 --- a/internal/cluster/discover.go +++ b/internal/cluster/discover.go @@ -12,6 +12,8 @@ import ( "k8s.io/client-go/kubernetes" "gopkg.in/yaml.v3" + + "github.com/tracebloc/cli/internal/installer" ) // ErrNoParentRelease is the sentinel for DiscoverParentRelease's "the namespace @@ -128,9 +130,9 @@ func DiscoverParentRelease(ctx context.Context, cs kubernetes.Interface, namespa "%w in namespace %q. "+ "If your client runs in another namespace, pass --namespace; "+ "if this cluster has no tracebloc client yet, run the installer: "+ - "bash <(curl -fsSL https://tracebloc.io/i.sh). "+ + "%s. "+ "Diagnose with `tracebloc doctor`.", - ErrNoParentRelease, namespace, + ErrNoParentRelease, namespace, installer.Cmd, ) case 1: // happy path diff --git a/internal/cluster/discover_test.go b/internal/cluster/discover_test.go index e7860e58..fdfe878f 100644 --- a/internal/cluster/discover_test.go +++ b/internal/cluster/discover_test.go @@ -12,6 +12,8 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" k8stesting "k8s.io/client-go/testing" + + "github.com/tracebloc/cli/internal/installer" ) // jobsManagerDeployment builds the minimal Deployment the chart @@ -186,7 +188,7 @@ func TestDiscoverParentRelease_NoReleaseFound(t *testing.T) { // The error message has to be customer-actionable. Pin the // key remediation phrase so a future refactor that loses it // (or worse, replaces it with a stack trace) fails this test. - for _, want := range []string{"no tracebloc client found", "--namespace", "https://tracebloc.io/i.sh", "tracebloc doctor"} { + for _, want := range []string{"no tracebloc client found", "--namespace", installer.URL, "tracebloc doctor"} { if !strings.Contains(err.Error(), want) { t.Errorf("expected error to mention %q, got: %s", want, err) } diff --git a/internal/installer/installer.go b/internal/installer/installer.go new file mode 100644 index 00000000..c48aae09 --- /dev/null +++ b/internal/installer/installer.go @@ -0,0 +1,76 @@ +// Package installer holds the ONE bootstrap idiom for the official tracebloc +// installer: its URL, and the shell command that downloads and runs it. +// +// Every place in the CLI that runs the installer or prints a copy-paste hint for +// it derives from here — `tracebloc upgrade`, `tracebloc prepare-host`, the +// doctor remedies, and the cluster-discovery error. That's deliberate: the same +// string is both executed and printed, so the command we tell a user to run is +// byte-identical to the one we just tried on their behalf, and a URL or idiom +// change lands everywhere at once (Bugbot #394/#397, cli#396). +package installer + +// URL is the single source of truth for the installer script's location. +const URL = "https://tracebloc.io/i.sh" + +// Cmd is the bare bootstrap — download the installer and run it, no subcommand. +// This is the copy-paste one-liner every remedy prints, and what `tracebloc +// upgrade` executes. +var Cmd = Script("", "") + +// Script builds the one-line bash program that downloads the cosign-verified +// installer to a private temp file and runs THAT file, optionally with a +// subcommand (e.g. "prepare-host") and an environment assignment applied to the +// installer itself (e.g. "TB_PREPARE_USER=alice"). +// +// Every part of the shape below is load-bearing. It is both executed (via +// `bash -c`) and printed for a human to paste, so it has to hold up in both. +// +// - A downloaded FILE, run as `bash "$tmp"` — never `curl … | bash` and never +// `bash <(curl …)`. The two rejected forms fail for DIFFERENT reasons, worth +// keeping apart. `curl … | bash` steals stdin: the inner bash reads its +// *program* from the pipe, so the installer's stdin is no longer the terminal +// and any interactive prompt (sign-in, or which non-admin user gets runtime +// access) gets EOF. `bash <(curl …)` does NOT steal stdin — process +// substitution hands bash a `/dev/fd/N` *filename* to read the program from, +// so stdin stays the TTY; that's exactly why it was the original choice, and +// its only fatal flaw is fail-open (next bullet). Running a downloaded file +// gets both properties at once: stdin stays the TTY AND curl's exit is checked. +// +// - `set -e` + `curl -o`, so a failed download (network/DNS/HTTP) aborts with +// curl's real exit status. This is the load-bearing reason to reject BOTH +// pipe/substitution forms — and the ONLY thing wrong with `bash <(curl …)`: +// `curl | bash` and `bash <(curl …)` alike leave bash reading an empty script +// and exiting 0, so we'd report success while nothing ran. Downloading first +// also means a truncated mid-stream download is never partially executed. +// +// - Wrapped in a subshell `( … )`. This is what makes the string safe to PASTE, +// not just to exec: pasted into the user's interactive shell, a bare `set -e` +// would arm errexit on that shell and the failed download would close their +// terminal, and the bare `trap … EXIT` would outlive the command. Scoping +// both to a subshell keeps the fail-closed status (the subshell's exit status +// is the command's) without touching the shell the user is sitting in. +// +// - Written on ONE line, `;`-joined. Copy-pasting a multi-line block is +// unreliable — notably PowerShell, where a multi-line paste can execute +// bottom-up, running the installer before the download. A single line pastes +// the same way everywhere. This rules out the multi-line form for anything we +// print, which is why the printed and executed strings are one and the same. +// +// - `--tlsv1.2` pins the TLS floor scripts/install.sh enforces on every +// security-sensitive fetch, so this privileged download can never negotiate a +// weaker protocol (Bugbot #397). +// +// env is applied to the inner `bash "$tmp"` rather than to the whole command +// because a subshell cannot take a variable assignment — `VAR=x (…)` is a bash +// syntax error — and the installer is what needs to see it anyway. +func Script(subcommand, env string) string { + run := `bash "$tmp"` + if env != "" { + run = env + " " + run + } + if subcommand != "" { + run += " " + subcommand + } + return `(set -e; tmp="$(mktemp)"; trap 'rm -f "$tmp"' EXIT; ` + + `curl -fsSL --tlsv1.2 ` + URL + ` -o "$tmp"; ` + run + `)` +} diff --git a/internal/installer/installer_test.go b/internal/installer/installer_test.go new file mode 100644 index 00000000..5274ed04 --- /dev/null +++ b/internal/installer/installer_test.go @@ -0,0 +1,98 @@ +package installer + +import ( + "os/exec" + "strings" + "testing" +) + +// The idiom is both executed and printed for a human to paste, so it has to be +// ONE line. A multi-line block pastes unreliably — notably PowerShell, where a +// multi-line paste can run bottom-up and execute the installer before the +// download. Guard every shape we generate (cli#396). +func TestScriptIsSingleLine(t *testing.T) { + for _, tc := range []struct{ sub, env string }{ + {"", ""}, + {"prepare-host", ""}, + {"prepare-host", "TB_PREPARE_USER=alice"}, + } { + if got := Script(tc.sub, tc.env); strings.Contains(got, "\n") { + t.Errorf("Script(%q,%q) must be a single line (paste-safe), got:\n%q", tc.sub, tc.env, got) + } + } +} + +// Structural guards on the shared idiom. Each is load-bearing; see Script's doc. +func TestScriptShape(t *testing.T) { + s := Cmd + checks := []struct { + want, why string + }{ + {"set -e", "fail closed: a non-zero curl must abort, not run an empty script"}, + {"curl", "download the installer"}, + {"-o ", "download to a FILE so curl's exit is checked (not piped)"}, + {"--tlsv1.2", "pin the TLS 1.2 floor on this privileged download (Bugbot #397)"}, + {`bash "$tmp"`, "run the downloaded file, keeping stdin on the TTY"}, + {URL, "point at the single-source installer URL"}, + } + for _, c := range checks { + if !strings.Contains(s, c.want) { + t.Errorf("Cmd missing %q — %s; got: %q", c.want, c.why, s) + } + } + // Must NOT pipe or process-substitute the script into bash: both steal the + // installer's stdin AND fail open (bash reads an empty script and exits 0 when + // curl fails). These are exactly the shapes cli#396 replaced. + for _, bad := range []string{"| bash", "|bash", "<(", "curl -fsSL " + URL + ")"} { + if strings.Contains(s, bad) { + t.Errorf("Cmd must not contain %q (steals stdin / fails open); got: %q", bad, s) + } + } + // A subshell wrapper is what makes it paste-safe: set -e / trap stay scoped to + // the subshell instead of arming errexit on the user's interactive shell. + if !strings.HasPrefix(s, "(") || !strings.HasSuffix(s, ")") { + t.Errorf("Cmd must be wrapped in a subshell so set -e/trap don't leak into the pasting shell; got: %q", s) + } +} + +// Executable proof of the two properties that matter, run against a bogus +// endpoint so no network is touched: (1) a failed download makes the command +// exit non-zero — the whole point of the change, since the old `bash <(curl …)` +// / `curl | bash` shapes exited 0 on a curl failure; and (2) pasting it into an +// interactive-style shell does NOT arm errexit or leave a trap behind on the +// caller's shell (the subshell contains both). We swap only the endpoint in the +// real generated string, so the shell shape under test is exactly what ships. +func TestScriptFailsClosedWithoutLeakingErrexit(t *testing.T) { + if _, err := exec.LookPath("bash"); err != nil { + t.Skip("bash not available") + } + // http://127.0.0.1:1 — a port nothing listens on: curl fails fast, offline. + badCmd := strings.Replace(Cmd, URL, "http://127.0.0.1:1/i.sh", 1) + if !strings.Contains(badCmd, "127.0.0.1:1") { + t.Fatalf("could not substitute endpoint in %q", Cmd) + } + + // The pasted command, then two lines that only run if it did NOT kill the + // shell and did NOT leave errexit armed. The final `false` would abort an + // errexit-armed shell before the last echo. + script := badCmd + "\n" + + `echo "SURVIVED status=$?"` + "\n" + + `false; echo "NO-ERREXIT-LEAK"` + "\n" + + `trap -p EXIT | grep -q "rm -f" && echo "TRAP-LEAKED" || echo "NO-TRAP-LEAK"` + + out, _ := exec.Command("bash", "-c", script).CombinedOutput() + got := string(out) + if !strings.Contains(got, "SURVIVED status=") { + t.Fatalf("pasting the command killed the shell — set -e/trap leaked; output:\n%s", got) + } + // The failed download must have produced a non-zero status inside the subshell. + if strings.Contains(got, "SURVIVED status=0") { + t.Errorf("command exited 0 on a failed download — NOT fail-closed; output:\n%s", got) + } + if !strings.Contains(got, "NO-ERREXIT-LEAK") { + t.Errorf("errexit leaked into the pasting shell; output:\n%s", got) + } + if !strings.Contains(got, "NO-TRAP-LEAK") { + t.Errorf("the EXIT trap leaked into the pasting shell; output:\n%s", got) + } +}