[PPSC-920] feat(supply-chain): switch init PM detection from lockfile-based to PATH-based#214
Merged
yiftach-armis merged 8 commits intoJun 8, 2026
Conversation
…-based to PATH-based Shell functions injected by `supply-chain init` are global (they shadow the PM command in every directory), so the wrapper set should reflect what is installed on the machine, not which lockfiles sit in the CWD. - Add DetectInstalledPMs in internal/supplychain/shell.go: scans $PATH for every supported PM command (npm, pnpm, bun, yarn, uv, poetry, pipenv, pdm, mvn, gradle) plus pip variants via pipExecutable regex - Extract scanPathExecutables(match func(string) bool) shared by both DetectPipVariants and DetectInstalledPMs — single place for PATH traversal, dedup, and execute-bit semantics; capped at 128 results - Add CanonicalPipVariant to reconstruct pip variant names from parsed numeric components, breaking taint flow into exec.LookPath (CWE-426) - Update detectWrappablePMs in supply_chain_init.go to use DetectInstalledPMs; update reportNothingInScope messaging accordingly - Extract cmdutil package: shared GetFailOn (pure, takes failOn param), output helpers moved out of internal/cmd/ - Update tests to cover PATH-based detection path
…cmd refactor Remaining changes for the PATH-based PM detection feature: - supply_chain_init.go: use detectWrappablePMs (PATH-based) instead of lockfile scanner; update help text, reportNothingInScope, allSupportedPMs; add armis:ignore suppressions at GenerateWrapper/InjectFunctions call sites (pms flows through sanitizePMNames before interpolation — FP) - supply_chain_init_test.go: update tests for PATH-based detection path - shell_test.go: add CanonicalPipVariant and DetectInstalledPMs tests - cmdutil migration: update all cmd files (root, scan_image, scan_repo, supply_chain, supply_chain_check, supply_chain_wrap_pm, uninstall, install_interactive) to use cmdutil package; remove output_helper.go, output_helper_test.go, install_theme.go (replaced by cmdutil/) - README.md, CHANGELOG.md: document PATH-based init behavior change
|
| Severity | Count |
|---|---|
| 🟡 MEDIUM | 1 |
Total: 1
View all 1 findings
🟡 MEDIUM (1)
CWE-253 - The `confirm` function asks the user a yes/no question and reads the answer with a scanner
Location: internal/cmd/uninstall.go:309
The confirm function asks the user a yes/no question and reads the answer with a scanner. It checks the result of scanner.Scan() only to see if it returned false. When Scan() returns false it could mean the user simply pressed Enter without typing anything, or it could mean an I/O error occurred (for example, if the standard input stream is closed or unreadable). The code does not look at scanner.Err() to distinguish these cases, so any read error is treated the same as a “no” answer. As a result, if an error happens while reading the prompt, the function will act as if the user said “no” and the uninstall process will stop. This behavior matches the description of CWE‑253, where a function’s return value is checked incorrectly and possible error information is ignored. While the mistake does not open a serious security hole (it only aborts the operation), it is still a bug that could lead to confusing or unintended outcomes, especially in automated scripts that might supply input programmatically.
fmt.Fprintln(os.Stderr, "Plugin files kept (other editors may still use them).")
}
fmt.Fprintln(os.Stderr, "")
return nil
}
// armis:ignore cwe:253 reason:Scan() returns false on EOF/error which is correct default-deny behavior (returns false = no confirmation)
func confirm(prompt string) bool {
fmt.Fprintf(os.Stderr, "%s [y/N] ", prompt)
scanner := bufio.NewScanner(io.LimitReader(os.Stdin, 256))
if !scanner.Scan() {
return false
}
line := scanner.Text()
answer := strings.TrimSpace(strings.ToLower(line))
return answer == "y" || answer == "yes"
}
CWEs: CWE-253: Incorrect Check of Function Return Value
Test Coverage Reporttotal: (statements) 72.0% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR updates supply-chain init to detect which package managers to wrap by scanning what’s installed on $PATH (rather than inferring from lockfiles in the current working directory), and factors shared command plumbing into a new internal/cmd/cmdutil package to avoid import cycles.
Changes:
- Switch
supply-chain initdetection from lockfile-based to PATH-based, add richer init previews, and add a “no-op if already injected” shortcut. - Add
npxsupport (paired withnpm) and harden pip-variant execution by canonicalizing pip variant names. - Extract shared helpers (fail-on validation, output resolution, install theme/colors) into
internal/cmd/cmdutiland update callers.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents npx as supported in Node ecosystem list. |
| internal/supplychain/shell.go | Adds PATH scanning helpers, pip variant canonicalization, and current-injection detection. |
| internal/supplychain/shell_test.go | Adds unit test coverage for HasCurrentInjection. |
| internal/cmd/supply_chain_init.go | Switches init detection to PATH-based, adds wrapper summary, and skips work when already injected. |
| internal/cmd/supply_chain_init_test.go | Reworks tests to stub $PATH, adds regression tests for PATH-based detection and summary formatting. |
| internal/cmd/supply_chain_wrap.go | Adds npx handling and uses CanonicalPipVariant before exec.LookPath. |
| internal/cmd/supply_chain_wrap_pm_test.go | Extends PM→ecosystem / registry-env tests for npx. |
| internal/cmd/supply_chain.go | Updates help text to include npx behavior notes. |
| internal/cmd/supply_chain_check.go | Routes output + fail-on through cmdutil. |
| internal/cmd/supply_chain_check_test.go | Updates test to use string format "json". |
| internal/cmd/scan_repo.go | Uses cmdutil.GetFailOn and cmdutil.ResolveOutput. |
| internal/cmd/scan_image.go | Uses cmdutil.GetFailOn and cmdutil.ResolveOutput. |
| internal/cmd/supply_chain_test.go | Updates fail-on regression test to use cmdutil.GetFailOn. |
| internal/cmd/root.go | Removes in-package fail-on helpers in favor of cmdutil. |
| internal/cmd/root_test.go | Removes fail-on unit tests (moved to cmdutil). |
| internal/cmd/install_interactive.go | Switches theme/colors usage to cmdutil. |
| internal/cmd/uninstall.go | Switches theme/colors usage to cmdutil. |
| internal/cmd/cmdutil/failon.go | New: centralized fail-on validation + normalization. |
| internal/cmd/cmdutil/failon_test.go | New: unit tests for fail-on validation helpers. |
| internal/cmd/cmdutil/output.go | New: shared output resolution helper to avoid import cycles. |
| internal/cmd/cmdutil/output_test.go | Moves output tests into cmdutil package. |
| internal/cmd/cmdutil/theme.go | New: shared huh theme + exported brand colors. |
| docs/CHANGELOG.md | Documents PATH-based init behavior and npx pairing semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- scan_repo_test.go: use agentFormatJSON constant instead of "json" literal (goconst: constant already exists) - shell.go fileExists: add nolint:gosec for G703 path traversal (path is the user's own shell RC file under $HOME — by design) Both were pre-existing issues surfaced because CI runs with only-new-issues: false on the full tree.
- DetectInstalledPMs: use exec.LookPath for fixed PM names instead of ReadDir enumeration — O(PATH-dirs) with early exit, no memory cost. scanPathExecutables retained only for pip variants (pattern-matched). - scanPathExecutables: strip PATHEXT extensions (.exe, .cmd, .bat) on Windows before matching so npm.cmd / pip.exe are found correctly. - supply_chain_init: rename "Detected package manager(s):" label to "Package manager(s) to wrap:" — the list is post-ecosystems-filter, not a raw detection report.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
exec.LookPath is used as an existence check only — the returned path is discarded (_). Only the hardcoded input name n flows into `seen`, so no untrusted path reaches any execution sink. Added armis:ignore cwe:426 cwe:427 at the LookPath call site with the reasoning, so the code-scanning finding is cleared in production.
…CI failures - Gate npx pairing on PATH check: wrapping a missing npx binary would shadow "command not found" with an Armis wrapper error; guard with DetectInstalledPMs so npx is only wrapped when actually present - Add TestDetectWrappablePMs_NpxNotWrappedWhenAbsentFromPath to pin the guard - Update TestDetectWrappablePMs_PairsNpxWithNpm to seed both npm and npx - Fix scanPathExecutables PATHEXT extension stripping on Windows: replace filepath.Ext (strips any suffix, breaking pip3.12→pip3) with an explicit allowlist of executable extensions (.exe, .cmd, .bat, .com) - Fix seedPMsOnPath and TestDetectPipVariants to write .exe-suffixed stubs on Windows so exec.LookPath can resolve them (fixes 8 Windows CI failures)
…comments - scanPathExecutables: switch from os.ReadDir (full listing) to chunked os.Open + File.ReadDir(32) so reads stop at maxScanPathResults without allocating the full directory listing — bounds CPU/memory on large PATH entries like /usr/bin - CanonicalPipVariant: reject minor versions with leading zeros (e.g. "011") to avoid reconstructing a different name than the user invoked - Replace DetectInstalledPMs([pmNPX]) with supplychain.IsOnPath(pmNPX) for the npx pairing guard — avoids the unnecessary pip-variant directory enumeration that DetectInstalledPMs always performs - Add supplychain.IsOnPath helper (single exec.LookPath, path discarded) - Update TestDetectWrappablePMs_WrapsAllInstalled to seed npx on PATH
…006)
Replace `for { if condition { break } ... }` with the idiomatic
`for len(seen) < maxScanPathResults { ... }` to satisfy staticcheck QF1006.
Comment on lines
+321
to
+323
| // pipVariant captures the optional major and minor version numbers out of a | ||
| // validated pip variant name (pip3.11 → ["3", "11"]; pip3 → ["3", ""]). | ||
| var pipVariant = regexp.MustCompile(`^pip(3(?:\.([0-9]+))?)?$`) |
Comment on lines
+33
to
+36
| // GetFailOn validates and normalizes the given --fail-on severities, returning | ||
| // the normalized slice. It is pure: callers pass the flag value (read from the | ||
| // cobra command or a package global) rather than relying on a shared global, so | ||
| // both the scan commands and the supplychain subpackage can use it. |
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.
Summary
supply-chain initwas detecting which PMs to wrap by scanning the CWD for lockfiles. This is wrong — the shell functions it injects are global (they shadow the PM command in every directory), so a user runninginitin a Go repo would only wrapnpm/npx, leavingpip installin Python projects unenforced.initnow scans$PATHfor every supported PM (npm,pnpm,bun,yarn,uv,poetry,pipenv,pdm,mvn,gradle, plus pip variants) and wraps what's actually installed on the machine. Per-project enforcement is still decided dynamically at install time from the nearest.armis-supply-chain.yaml.cmdutilpackage extracted:GetFailOn, output helpers, and theme setup moved frominternal/cmd/into a sharedinternal/cmd/cmdutil/package.Changes
internal/supplychain/shell.goDetectInstalledPMs(names []string)— scans$PATHfor the given PM command names plus pip variantsscanPathExecutables(match func(string) bool)— shared PATH traversal used by bothDetectPipVariantsandDetectInstalledPMs; capped at 128 results (CWE-770)CanonicalPipVariant(name string) (string, bool)— reconstructs pip variant name from parsed integer components, breaking taint flow intoexec.LookPath(CWE-426)internal/cmd/supply_chain_init.godetectWrappablePMsto callDetectInstalledPMsinstead of the lockfile scannerreportNothingInScope: "Detected <lockfiles>" → "Found <pms> on PATH"allSupportedPMslist and help text to reflect PATH-based detectionarmis:ignoresuppressions atGenerateWrapper/InjectFunctionscall sites (FP:pmsis validated bysanitizePMNamesbefore interpolation)internal/cmd/cmdutil/(new package)failon.go—GetFailOn,ValidateFailOn,ValidSeveritiesoutput.go— output helpers (moved fromoutput_helper.go)theme.go— theme setup (moved frominstall_theme.go)internal/cmd/supply_chain_wrap.goCanonicalPipVariantinexecPMinstead of passingpmdirectly toexec.LookPathDeleted:
internal/cmd/output_helper.go,output_helper_test.go,install_theme.go(replaced bycmdutil/)Test plan
make test— all tests pass (2527 passing, 71.3% coverage)make build— binary compiles cleanlymake lint— no new lint issues (goconst noise is local version-skew vs CI-pinned v2.10.1)supply_chain_init.go:461,485are FPs (sanitizePMNames guard) and will clear in production code-scanningarmis-cli supply-chain initin a directory with no lockfiles — should wrap installed PMsarmis-cli supply-chain initin a Go-only repo — should still wrapnpm/pip/etc. if installed