diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 21f733f3..4d22cad4 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `supply-chain init`: the injected shell wrappers no longer break after an `armis-cli` upgrade. The wrapper now references `armis-cli` by bare name (resolved from `PATH` on every call) when it is on `PATH`, falling back to the stable symlink path otherwise — previously it embedded the fully symlink-resolved binary path, which on Homebrew was the version-pinned Cellar directory (e.g. `…/Cellar/armis-cli/1.11.0/…`). After `brew upgrade armis-cli` deleted that directory, every wrapped package manager (npm, pnpm, bun, pip, uv, poetry, npx) failed to run in new shells. The wrappers are also now fail-closed: if `armis-cli` cannot be found at invocation time, the wrapper prints a loud warning to stderr that enforcement has lapsed and runs the real package manager un-wrapped, so installs never silently break. Wrappers injected before this fix must be refreshed by re-running `armis-cli supply-chain init` once. + ### Security --- diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index cd0b7f42..8dbe1200 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -18,6 +18,12 @@ const ( markerEnd = "# <<< armis-cli supply-chain <<<" ) +// cliBinaryName is the armis-cli executable name. resolveCliPath embeds it in +// the generated wrappers (preferring this bare, PATH-resolved name so upgrades +// stay transparent) and uses it to probe $PATH; centralizing the literal keeps +// the two in sync. +const cliBinaryName = "armis-cli" + // goosWindows is runtime.GOOS on Windows. Centralized so the several // platform guards across this package (and its tests) share one literal. const goosWindows = "windows" @@ -114,8 +120,28 @@ func generatePosixWrapper(pms []string, cli string) string { var b strings.Builder b.WriteString(markerStart + "\n") for _, pm := range sanitizePMNames(pms) { - // armis:ignore cwe:78 reason:pm is constrained to ^[a-z][a-z0-9-]*(\.[0-9]+)?$ by sanitizePMNames, so any dot is followed only by digits (not a shell metacharacter); safeCli is shellQuote-escaped - fmt.Fprintf(&b, "%s() {\n command %s supply-chain wrap %s \"$@\"\n}\n", pm, safeCli, pm) + // The guard makes the wrapper fail-closed: if armis-cli is not resolvable + // at invocation time (e.g. a stale absolute path left by a package-manager + // upgrade), the wrapper warns loudly on stderr and runs the real package + // manager un-wrapped rather than failing the command outright. + // + // Two checks, because resolveCliPath() may embed either a bare name or an + // absolute path: `[ -x %s ]` reliably accepts an executable at an absolute + // path (POSIX `command -v` with a slash-containing argument is unspecified + // and some shells report it missing even when it exists), while `command -v` + // handles the bare-name PATH lookup. For a bare name `[ -x ]` checks the CWD + // (normally absent) and falls through to the PATH lookup. + // armis:ignore cwe:78 reason:pm is constrained to ^[a-z][a-z0-9-]*(\.[0-9]+)?$ by sanitizePMNames, so any dot is followed only by digits (not a shell metacharacter); safeCli is shellQuote-escaped; both `[ -x ]` and `command -v` are used only for presence detection and their output is discarded + fmt.Fprintf(&b, + "%s() {\n"+ + " if [ -x %s ] || command -v %s >/dev/null 2>&1; then\n"+ + " command %s supply-chain wrap %s \"$@\"\n"+ + " else\n"+ + " printf '[armis] armis-cli not found - running %s WITHOUT supply-chain enforcement\\n' >&2\n"+ + " command %s \"$@\"\n"+ + " fi\n"+ + "}\n", + pm, safeCli, safeCli, safeCli, pm, pm, pm) } b.WriteString(markerEnd + "\n") return b.String() @@ -128,8 +154,29 @@ func generateFishWrapper(pms []string, cli string) string { var b strings.Builder b.WriteString(markerStart + "\n") for _, pm := range sanitizePMNames(pms) { - // armis:ignore cwe:78 reason:pm is constrained to ^[a-z][a-z0-9-]*(\.[0-9]+)?$ by sanitizePMNames, so any dot is followed only by digits (not a shell metacharacter); safeCli is shellQuote-escaped - fmt.Fprintf(&b, "function %s\n command %s supply-chain wrap %s $argv\nend\n", pm, safeCli, pm) + // The guard makes the wrapper fail-closed: if armis-cli is not resolvable + // at invocation time (e.g. a stale absolute path left by a package-manager + // upgrade), the wrapper warns loudly on stderr and runs the real package + // manager un-wrapped rather than failing the command outright. + // + // Two checks, because resolveCliPath() may embed either a bare name or an + // absolute path: `test -x %s` reliably accepts an executable at an absolute + // path (fish's `command -q` with a slash-containing argument is unspecified + // across versions and can report it missing even when it exists), while + // `command -q`/`--query` (fish 3.0+) handles the bare-name PATH lookup and + // emits no output. For a bare name `test -x` checks the CWD (normally + // absent) and falls through to the PATH lookup. + // armis:ignore cwe:78 reason:pm is constrained to ^[a-z][a-z0-9-]*(\.[0-9]+)?$ by sanitizePMNames, so any dot is followed only by digits (not a shell metacharacter); safeCli is shellQuote-escaped; both `test -x` and `command -q` are used only for presence detection and their output is discarded + fmt.Fprintf(&b, + "function %s\n"+ + " if test -x %s; or command -q %s\n"+ + " command %s supply-chain wrap %s $argv\n"+ + " else\n"+ + " printf '[armis] armis-cli not found - running %s WITHOUT supply-chain enforcement\\n' >&2\n"+ + " command %s $argv\n"+ + " end\n"+ + "end\n", + pm, safeCli, safeCli, safeCli, pm, pm, pm) } b.WriteString(markerEnd + "\n") return b.String() @@ -139,20 +186,37 @@ func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" } +// resolveCliPath returns the most upgrade-proof reference to the armis-cli +// binary to embed in the generated shell wrapper functions. +// +// Priority order: +// 1. The bare name "armis-cli" when it is resolvable on $PATH. The shell +// re-resolves the name on every invocation, so a package-manager upgrade +// (e.g. `brew upgrade armis-cli`, which moves the binary into a new +// versioned directory) is transparent — the wrapper never embeds a +// version-pinned path that a later upgrade would delete. +// 2. Otherwise the stable absolute path from filepath.Abs(os.Executable()). +// We deliberately do NOT call filepath.EvalSymlinks here: on a Homebrew +// install os.Executable() reports the stable /opt/homebrew/bin/armis-cli +// symlink, and resolving it would pin the wrapper to the versioned Cellar +// path (…/Cellar/armis-cli//bin/armis-cli) that `brew upgrade` +// removes, breaking every wrapped package manager. +// 3. The literal "armis-cli" if os.Executable() fails — better a PATH-resolved +// name than nothing. func resolveCliPath() string { + // armis:ignore cwe:426 cwe:427 reason:cliBinaryName is the hardcoded literal "armis-cli", not user input; supply-chain init configures the current user's own interactive shell, whose $PATH is not a trust boundary for a local CLI. The generated wrapper already resolves the package manager itself by bare name (e.g. `command npm`), so embedding the bare armis-cli name adds no search-path exposure the shell does not already have — an attacker able to write to a $PATH dir could shadow npm/pip directly. + if IsOnPath(cliBinaryName) { + return cliBinaryName + } exe, err := os.Executable() if err != nil { - return "armis-cli" + return cliBinaryName } abs, err := filepath.Abs(exe) if err != nil { return exe } - resolved, err := filepath.EvalSymlinks(abs) - if err != nil { - return abs - } - return resolved + return abs } func InjectFunctions(shells []Shell, pms []string) ([]string, error) { diff --git a/internal/supplychain/shell_test.go b/internal/supplychain/shell_test.go index 32e7763d..0e662539 100644 --- a/internal/supplychain/shell_test.go +++ b/internal/supplychain/shell_test.go @@ -493,3 +493,125 @@ func TestDetectShells(t *testing.T) { } }) } + +// TestGenerateWrapper_PosixContainsGuard verifies the bash/zsh wrapper is +// fail-closed: it guards the armis-cli invocation with a `command -v` PATH lookup +// (alongside an `[ -x ]` absolute-path check), warns on stderr when the binary is +// missing, and falls back to running the real package manager so an install never +// silently breaks after an armis-cli upgrade. +// +// The assertion matches the `command -v` substring rather than the exact `if` +// prefix so the guard can be extended (e.g. with the `[ -x … ]` path check) without +// breaking this test, as long as the fail-closed PATH lookup remains. +func TestGenerateWrapper_PosixContainsGuard(t *testing.T) { + wrapper := GenerateWrapper("bash", []string{"npm"}) + + if !strings.Contains(wrapper, "command -v") { + t.Errorf("posix wrapper missing `command -v` guard:\n%s", wrapper) + } + if !strings.Contains(wrapper, "armis-cli not found") { + t.Errorf("posix wrapper missing the missing-binary warning:\n%s", wrapper) + } + if !strings.Contains(wrapper, `command npm "$@"`) { + t.Errorf("posix wrapper missing the un-wrapped fallback `command npm \"$@\"`:\n%s", wrapper) + } +} + +// TestGenerateWrapper_FishContainsGuard is the fish-syntax counterpart to +// TestGenerateWrapper_PosixContainsGuard. The guard uses fish-native `command -q` +// (not POSIX `command -v`, which fish's `command` builtin does not accept — using +// it would make the guard always fail and silently disable enforcement for fish). +// +// The assertion matches the `command -q` substring rather than the exact `if` +// prefix so the guard can be extended (e.g. with the `test -x …` path check) without +// breaking this test, as long as the fish-native presence check remains. +func TestGenerateWrapper_FishContainsGuard(t *testing.T) { + wrapper := GenerateWrapper("fish", []string{"npm"}) + + if !strings.Contains(wrapper, "command -q") { + t.Errorf("fish wrapper missing `command -q` guard:\n%s", wrapper) + } + if !strings.Contains(wrapper, "armis-cli not found") { + t.Errorf("fish wrapper missing the missing-binary warning:\n%s", wrapper) + } + if !strings.Contains(wrapper, "command npm $argv") { + t.Errorf("fish wrapper missing the un-wrapped fallback `command npm $argv`:\n%s", wrapper) + } +} + +// TestGenerateWrapper_AbsolutePathGuardChecksExecutable verifies that when +// resolveCliPath falls back to an absolute path (armis-cli not on PATH at init +// time), the guard includes an executable-path check on that path. A bare +// `command -v`/`command -q` with a slash-containing argument is unspecified across +// shells and can report the binary missing even when it exists, which would make +// the wrapper always take the `else` branch and silently bypass enforcement. The +// `[ -x … ]` / `test -x …` check makes the absolute-path case reliable. +func TestGenerateWrapper_AbsolutePathGuardChecksExecutable(t *testing.T) { + const absPath = "/opt/homebrew/bin/armis-cli" + quoted := shellQuote(absPath) + + posix := generatePosixWrapper([]string{"npm"}, absPath) + if !strings.Contains(posix, "[ -x "+quoted+" ]") { + t.Errorf("posix wrapper missing `[ -x ]` executable check for an absolute CLI path:\n%s", posix) + } + // The PATH lookup must remain alongside the path check for the bare-name case. + if !strings.Contains(posix, "command -v "+quoted) { + t.Errorf("posix wrapper missing `command -v` PATH lookup alongside the path check:\n%s", posix) + } + + fish := generateFishWrapper([]string{"npm"}, absPath) + if !strings.Contains(fish, "test -x "+quoted) { + t.Errorf("fish wrapper missing `test -x ` executable check for an absolute CLI path:\n%s", fish) + } + if !strings.Contains(fish, "command -q "+quoted) { + t.Errorf("fish wrapper missing `command -q` PATH lookup alongside the path check:\n%s", fish) + } +} + +// TestGenerateWrapper_WarningReferencesPMName verifies the fallback warning names +// the package manager the user actually invoked (not a hard-coded "npm"), for +// every shell and including versioned pip variants. +func TestGenerateWrapper_WarningReferencesPMName(t *testing.T) { + for _, shell := range []string{"bash", "zsh", "fish"} { + for _, pm := range []string{"npm", "pip3.12", "poetry"} { + wrapper := GenerateWrapper(shell, []string{pm}) + want := "running " + pm + " WITHOUT supply-chain enforcement" + if !strings.Contains(wrapper, want) { + t.Errorf("%s wrapper for %q missing PM name in warning %q:\n%s", shell, pm, want, wrapper) + } + } + } +} + +// TestResolveCliPath_PrefersBareNameWhenOnPath verifies that when armis-cli is +// resolvable on $PATH, the wrapper embeds the bare name so the shell re-resolves +// it on every call — surviving package-manager upgrades that move the binary. +func TestResolveCliPath_PrefersBareNameWhenOnPath(t *testing.T) { + dir := t.TempDir() + fname := cliBinaryName + if runtime.GOOS == goosWindows { + fname = cliBinaryName + ".exe" + } + if err := os.WriteFile(filepath.Join(dir, fname), []byte{}, 0o755); err != nil { //nolint:gosec + t.Fatalf("seed %s: %v", cliBinaryName, err) + } + t.Setenv("PATH", dir) + + if got := resolveCliPath(); got != cliBinaryName { + t.Errorf("resolveCliPath() = %q, want %q (bare name when on PATH)", got, cliBinaryName) + } +} + +// TestResolveCliPath_FallsBackToAbsWhenNotOnPath verifies that when armis-cli is +// not on $PATH, resolveCliPath returns an absolute path (or the bare-name +// fallback) — but never resolves symlinks, so it cannot return a version-pinned +// Cellar-style path that an upgrade would delete. +func TestResolveCliPath_FallsBackToAbsWhenNotOnPath(t *testing.T) { + // Non-empty but empty dir: armis-cli is not resolvable on PATH. + t.Setenv("PATH", t.TempDir()) + + got := resolveCliPath() + if got != cliBinaryName && !filepath.IsAbs(got) { + t.Errorf("resolveCliPath() = %q, want an absolute path or the %q fallback", got, cliBinaryName) + } +}