[PPSC-927] fix(supply-chain): survive armis-cli upgrades in init wrappers#216
Merged
Merged
Conversation
…cli upgrades resolveCliPath embedded the fully symlink-resolved binary path, which on Homebrew is the version-pinned Cellar dir (e.g. .../Cellar/armis-cli/1.11.0/...). After `brew upgrade armis-cli` deleted that dir, every wrapped package manager (npm, pnpm, bun, pip, uv, poetry, npx) failed to run in new shells. - resolveCliPath now prefers the bare name `armis-cli` (re-resolved from PATH on every call) when on PATH, else the stable symlink path from filepath.Abs(os.Executable()) without EvalSymlinks. - Generated wrappers are now fail-closed: if armis-cli cannot be found at invocation time, the wrapper warns loudly on stderr and runs the real package manager un-wrapped so installs never silently break. - Add cliBinaryName const; add tests for the guard and resolveCliPath. Forward-only: existing users must re-run `armis-cli supply-chain init` once after upgrading to a build with this fix.
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
Total: 1
View all 1 findings
🟠 HIGH (1)
CWE-426 - Software and Data Integrity Failures (CWE-426
Location: internal/supplychain/shell.go:434
Untrusted Search Path): The program looks at the system's PATH variable and walks through every folder listed there. It collects the names of any executables it finds, such as "npm" or "pip". Later, when the tool creates shell wrapper functions, it uses those names directly in the commands it writes to your shell configuration files. Because the PATH contents are not checked or filtered, an attacker who can add a directory to your PATH (or place a malicious program in an existing PATH directory) can cause the wrapper to run their own program instead of the legitimate one. The wrapper will call the name it discovered, and the shell will resolve it using the same untrusted PATH, potentially executing the attacker‑controlled binary. In short, user‑controlled data (the PATH variable) flows into the part of the code that decides which program to run, without any validation, making it possible for an attacker to influence which executable is executed. No safeguards are present to stop this, so the vulnerability is real.
const maxScanPathResults = 128
// scanPathExecutables walks every directory on $PATH and returns the
// deduplicated, sorted set of entry names for which match(name) is true and
// (on Unix) the file carries at least one execute bit. It is the single place
// the PATH traversal, dedup, and execute-bit semantics live, shared by
// DetectPipVariants and DetectInstalledPMs so the two cannot drift apart.
// Returns nil when PATH is unset or nothing matches; callers decide their own
// fallback.
func scanPathExecutables(match func(name string) bool) []string {
pathEnv := os.Getenv("PATH")
if pathEnv == "" {
return nil
}
seen := make(map[string]bool)
const readDirChunk = 32 // entries per ReadDir call; small enough to avoid large allocs
for _, dir := range filepath.SplitList(pathEnv) {
if len(seen) >= maxScanPathResults {
break
}
CWEs: CWE-426: Untrusted Search Path
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Test Coverage Reporttotal: (statements) 72.1% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR improves supply-chain init’s injected shell wrappers so they remain functional across armis-cli upgrades (notably Homebrew Cellar path churn) and so wrappers don’t hard-fail package manager commands when armis-cli is missing.
Changes:
- Updated wrapper generation to avoid embedding symlink-resolved, version-pinned
armis-clipaths and instead prefer a PATH-resolvedarmis-clireference when available. - Added a “fail-closed” runtime guard in generated wrappers: if
armis-clican’t be located, emit a stderr warning and run the real package manager unwrapped. - Added unit tests for the new wrapper guard behavior and
resolveCliPathbehavior; documented the fix in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/supplychain/shell.go |
Changes wrapper generation + resolveCliPath to be upgrade-proof and add runtime guard/fallback behavior. |
internal/supplychain/shell_test.go |
Adds tests asserting guard/warning/fallback behavior and resolveCliPath selection logic. |
docs/CHANGELOG.md |
Documents the behavioral change and the need to re-run supply-chain init once after upgrading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+150
to
+158
| // The `command -v` 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. | ||
| // 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; command -v is used only for presence detection and its output is discarded | ||
| fmt.Fprintf(&b, | ||
| "function %s\n"+ | ||
| " if command -v %s >/dev/null 2>&1\n"+ | ||
| " command %s supply-chain wrap %s $argv\n"+ |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 05acf31. You're right that fish's command builtin has no POSIX -v; command -v armis-cli would error and make the if always take the else branch, silently disabling enforcement for every fish user even when armis-cli is present. Switched the fish guard to command -q (fish 3.0+), and dropped the now-redundant >/dev/null 2>&1 since -q emits no output. bash/zsh keep command -v (correct POSIX).
Comment on lines
+520
to
+522
| if !strings.Contains(wrapper, "if command -v") { | ||
| t.Errorf("fish wrapper missing `command -v` guard:\n%s", wrapper) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Done in 05acf31 — the fish wrapper now uses command -q and TestGenerateWrapper_FishContainsGuard asserts on if command -q accordingly (the posix test still asserts command -v, which stays correct for bash/zsh). Verified the generated fish output: if command -q 'armis-cli' with no redirect.
…lveCliPath The code-scanning check flags resolveCliPath for untrusted search path because it returns the bare name "armis-cli" (PATH-resolved by the shell). This is a false positive: cliBinaryName is a hardcoded constant (not user input), the generated wrapper already resolves the package manager itself by bare name (`command npm`), and supply-chain init configures the current user's own shell, whose $PATH is not a trust boundary for a local CLI. Add an armis:ignore matching the existing cwe:426 cwe:427 annotations on IsOnPath and DetectInstalledPMs in this file.
…apper guard fish's `command` builtin does not accept POSIX `-v`; `command -v` would error and make the guard always take the else branch, silently disabling enforcement for every fish user even when armis-cli is present. Switch the fish guard to `command -q` (fish 3.0+), which is the native presence check and emits no output (so the `>/dev/null 2>&1` redirect is no longer needed). bash/zsh keep `command -v`, which is correct POSIX. Update the fish test to assert `-q`. Addresses PR #216 review comments from copilot-pull-request-reviewer.
Comment on lines
+128
to
+137
| fmt.Fprintf(&b, | ||
| "%s() {\n"+ | ||
| " if 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, pm, pm, pm) |
Comment on lines
+157
to
+166
| fmt.Fprintf(&b, | ||
| "function %s\n"+ | ||
| " if 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, pm, pm, pm) |
Comment on lines
+522
to
+524
| if !strings.Contains(wrapper, "if command -q") { | ||
| t.Errorf("fish wrapper missing `command -q` guard:\n%s", wrapper) | ||
| } |
When resolveCliPath() falls back to an absolute path (armis-cli not on PATH at init time), the wrapper guard's bare `command -v`/`command -q` check is given a slash-containing argument. That is unspecified across shells and can report the binary missing even when it exists, making the wrapper silently take the `else` branch and bypass enforcement. Prepend an executable-path check that reliably handles the absolute-path case while keeping the PATH lookup for the bare-name case: POSIX: `if [ -x <abs> ] || command -v <abs> ...` fish: `if test -x <abs>; or command -q <abs>` Tests assert the `command -v`/`command -q` substring rather than the exact `if` prefix so the guard can be extended without breaking them, and a new test locks in the executable-path check for the absolute-path case. Addresses copilot-pull-request-reviewer feedback on PR #216.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
supply-chain initinjected shell wrappers that embedded a fully symlink-resolved binary path, which on Homebrew is the version-pinned Cellar dir (e.g..../Cellar/armis-cli/1.11.0/...). Afterbrew upgrade armis-clideleted that dir, the wrapperscommand '<dead-path>'failed with exit 127 — and because each wrapper shadows the real command, every wrapped package manager (npm, pnpm, bun, pip, uv, poetry, npx) broke in new shells untilinitwas re-run.Solution
resolveCliPathnow referencesarmis-cliby bare name (re-resolved from PATH on every call) when it is on PATH, falling back to the stable symlink path viafilepath.Abs(os.Executable())withoutEvalSymlinks. The generated wrappers are also now fail-closed: ifarmis-clicannot be found at invocation time, the wrapper prints a loud stderr warning that enforcement has lapsed and runs the real package manager un-wrapped, so installs never silently break. AcliBinaryNameconstant centralizes the literal.Testing
Automated Tests
Manual Testing
Built the binary and inspected
init --mode envoutput (bare'armis-cli'reference + guard). In a real bash subshell, simulated the post-upgrade state (armis-cli absent from PATH): the wrapper warned on stderr and still ran the realnpm(exit 0); with armis-cli present, it routed throughsupply-chain wrap. Fullgo test ./...green (2533 passed, 71.3% coverage), gofmt/go vet clean, AppSec scan_diff 0 findings.Reviewer Notes
Forward-only: this cannot repair an RC file already on disk. Existing users must re-run
armis-cli supply-chain initonce after upgrading to a build with this fix (noted in CHANGELOG).HasCurrentInjection's exact-match meansinitcorrectly detects the old block and re-injects.Checklist