From 2acb8f33eec9162de4974f0a08216cbfbf77e6f5 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 11:06:49 +0200 Subject: [PATCH 1/8] [PPSC-920] feat(supply-chain): switch init PM detection from lockfile-based to PATH-based MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/cmd/cmdutil/failon.go | 42 +++++ internal/cmd/cmdutil/failon_test.go | 77 ++++++++ internal/cmd/cmdutil/output.go | 113 +++++++++++ internal/cmd/cmdutil/output_test.go | 278 ++++++++++++++++++++++++++++ internal/cmd/cmdutil/theme.go | 72 +++++++ internal/cmd/supply_chain_wrap.go | 30 +-- internal/supplychain/shell.go | 128 +++++++++++-- 7 files changed, 713 insertions(+), 27 deletions(-) create mode 100644 internal/cmd/cmdutil/failon.go create mode 100644 internal/cmd/cmdutil/failon_test.go create mode 100644 internal/cmd/cmdutil/output.go create mode 100644 internal/cmd/cmdutil/output_test.go create mode 100644 internal/cmd/cmdutil/theme.go diff --git a/internal/cmd/cmdutil/failon.go b/internal/cmd/cmdutil/failon.go new file mode 100644 index 00000000..0bf202e2 --- /dev/null +++ b/internal/cmd/cmdutil/failon.go @@ -0,0 +1,42 @@ +package cmdutil + +import ( + "fmt" + "strings" +) + +// ValidSeverities contains the valid severity level strings for the --fail-on flag. +var ValidSeverities = []string{"INFO", "LOW", "MEDIUM", "HIGH", "CRITICAL"} + +// ValidateFailOn checks that every entry of severities is a recognized severity +// level and normalizes it to uppercase in place. ShouldFail matches severities +// exactly, so normalization here is what lets a lowercase "medium" trip the gate +// on a "MEDIUM" finding. +func ValidateFailOn(severities []string) error { + validSet := make(map[string]bool) + for _, s := range ValidSeverities { + validSet[s] = true + } + + for i, sev := range severities { + // Normalize to uppercase for case-insensitive matching + upper := strings.ToUpper(sev) + if !validSet[upper] { + return fmt.Errorf("invalid severity level %q: must be one of %v", sev, ValidSeverities) + } + // Update the slice with normalized value + severities[i] = upper + } + return nil +} + +// 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. +func GetFailOn(failOn []string) ([]string, error) { + if err := ValidateFailOn(failOn); err != nil { + return nil, err + } + return failOn, nil +} diff --git a/internal/cmd/cmdutil/failon_test.go b/internal/cmd/cmdutil/failon_test.go new file mode 100644 index 00000000..cf9a90f1 --- /dev/null +++ b/internal/cmd/cmdutil/failon_test.go @@ -0,0 +1,77 @@ +package cmdutil + +import "testing" + +func TestValidateFailOn(t *testing.T) { + tests := []struct { + name string + severities []string + wantErr bool + }{ + { + name: "valid single severity", + severities: []string{"CRITICAL"}, + wantErr: false, + }, + { + name: "valid multiple severities", + severities: []string{"HIGH", "CRITICAL"}, + wantErr: false, + }, + { + name: "valid all severities", + severities: []string{"INFO", "LOW", "MEDIUM", "HIGH", "CRITICAL"}, + wantErr: false, + }, + { + name: "valid severity lowercase", + severities: []string{"high"}, + wantErr: false, + }, + { + name: "invalid severity unknown", + severities: []string{"INVALID"}, + wantErr: true, + }, + { + name: "invalid mixed valid and invalid", + severities: []string{"HIGH", "invalid"}, + wantErr: true, + }, + { + name: "empty slice is valid", + severities: []string{}, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateFailOn(tt.severities) + if (err != nil) != tt.wantErr { + t.Errorf("ValidateFailOn(%v) error = %v, wantErr %v", tt.severities, err, tt.wantErr) + } + }) + } +} + +func TestGetFailOn(t *testing.T) { + t.Run("returns normalized severities", func(t *testing.T) { + // GetFailOn is pure: it validates and uppercase-normalizes the slice it is + // given. A lowercase entry must come back uppercased so ShouldFail (exact + // match) can trip the CI gate. + result, err := GetFailOn([]string{"high", "CRITICAL"}) + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + if len(result) != 2 || result[0] != "HIGH" || result[1] != "CRITICAL" { + t.Errorf("Expected [HIGH CRITICAL], got %v", result) + } + }) + + t.Run("returns error for invalid severity", func(t *testing.T) { + if _, err := GetFailOn([]string{"invalid"}); err == nil { + t.Error("Expected error for invalid severity") + } + }) +} diff --git a/internal/cmd/cmdutil/output.go b/internal/cmd/cmdutil/output.go new file mode 100644 index 00000000..6a5e19d9 --- /dev/null +++ b/internal/cmd/cmdutil/output.go @@ -0,0 +1,113 @@ +// Package cmdutil holds command plumbing shared across the cmd package and its +// subpackages (e.g. supplychain). It exists to break what would otherwise be an +// import cycle: cmd imports its command subpackages to wire them into the root +// command, so those subpackages cannot import cmd back. Helpers both sides need +// live here, the one package both can import. +package cmdutil + +import ( + "io" + "os" + + "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/output" + "github.com/spf13/cobra" +) + +// OutputConfig holds the resolved output configuration for scan commands. +type OutputConfig struct { + // Writer is the destination for formatted output (stdout or file). + Writer io.Writer + // Format is the resolved output format (may differ from flag if auto-detected). + Format string + // cleanup is called to close the file and reset state. Always call this via defer. + cleanup func() + // cleaned tracks whether cleanup has already been called (for idempotency) + cleaned bool +} + +// Cleanup closes the output file (if any) and restores color state. +// This method is idempotent and safe to call multiple times. +// Called via defer in scan commands; the defer ensures cleanup on all exit paths. +func (c *OutputConfig) Cleanup() { + if c.cleaned { + return + } + c.cleaned = true + c.cleanup() +} + +// ResolveOutput determines the output writer and format for scan results. +// It handles: +// - Auto-detecting format from file extension (if --format not explicitly set) +// - Creating the output file (with proper directory creation) +// - Disabling colors when writing to file (unless --color=always) +// +// The returned OutputConfig.cleanup function MUST be called via defer to: +// - Close the output file (if writing to file) +// - Reset the outputToFile state for proper cleanup +// +// Example usage: +// +// cfg, err := cmdutil.ResolveOutput(cmd, outputFile, format, colorFlag) +// if err != nil { +// return err +// } +// defer cfg.Cleanup() +// // use cfg.Writer and cfg.Format +func ResolveOutput(cmd *cobra.Command, outputPath, formatFlag, colorFlag string) (*OutputConfig, error) { + cfg := &OutputConfig{ + Writer: os.Stdout, + Format: formatFlag, + cleanup: func() {}, // no-op by default + } + + if outputPath == "" { + return cfg, nil + } + + // Auto-detect format from extension if user hasn't explicitly set --format. + // Use cmd.Flag() instead of cmd.Flags().Changed() because --format is a + // persistent flag on the root command, and Flags() doesn't include inherited flags. + fmtFlag := cmd.Flag("format") + formatExplicitlySet := fmtFlag != nil && fmtFlag.Changed + if !formatExplicitlySet { + if detected := output.FormatFromExtension(outputPath); detected != "" { + cfg.Format = detected + } + } + + // Capture previous state for restoration on error or cleanup + prevOutputToFile := cli.GetOutputToFile() + colorMode := cli.ColorMode(colorFlag) + + // Disable colors when writing to file (unless --color=always) + if colorMode != cli.ColorModeAlways { + cli.SetOutputToFile(true) + cli.InitColors(colorMode) // Pass actual mode, not hardcoded Auto + output.SyncColors() + } + + // armis:ignore cwe:22 reason:outputPath is from --output flag; user-controlled CLI arg for their own files + fileOutput, err := output.NewFileOutput(outputPath) // armis:ignore cwe:73 reason:outputPath from --output flag; user's own files + if err != nil { + // Restore previous state on error + cli.SetOutputToFile(prevOutputToFile) + cli.InitColors(colorMode) + output.SyncColors() + return nil, err + } + + cfg.Writer = fileOutput.Writer() + cfg.cleanup = func() { + // Restore previous outputToFile state and re-sync colors + cli.SetOutputToFile(prevOutputToFile) + cli.InitColors(colorMode) + output.SyncColors() + if cerr := fileOutput.Close(); cerr != nil { + cli.PrintWarningf("failed to close output file: %v", cerr) + } + } + + return cfg, nil +} diff --git a/internal/cmd/cmdutil/output_test.go b/internal/cmd/cmdutil/output_test.go new file mode 100644 index 00000000..6c5e2620 --- /dev/null +++ b/internal/cmd/cmdutil/output_test.go @@ -0,0 +1,278 @@ +package cmdutil + +import ( + "os" + "path/filepath" + "testing" + + "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/spf13/cobra" +) + +// Output helper test format constants to satisfy goconst linter +const ( + ohFormatHuman = "human" + ohFormatJSON = "json" + ohFormatSARIF = "sarif" + ohFormatJUnit = "junit" +) + +func TestResolveOutput(t *testing.T) { + // Helper to create a minimal cobra command with format flag + newTestCmd := func() *cobra.Command { + cmd := &cobra.Command{Use: "test"} + cmd.Flags().String("format", ohFormatHuman, "output format") + return cmd + } + + t.Run("returns stdout when no output path", func(t *testing.T) { + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, "", ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + if cfg.Writer != os.Stdout { + t.Error("expected Writer to be os.Stdout when outputPath is empty") + } + if cfg.Format != ohFormatHuman { + t.Errorf("expected Format = %q, got %q", ohFormatHuman, cfg.Format) + } + }) + + t.Run("auto-detects JSON format from extension", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "results.json") + + cmd := newTestCmd() + // Don't mark format as changed - let auto-detection kick in + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + if cfg.Format != ohFormatJSON { + t.Errorf("expected Format = %q (auto-detected), got %q", ohFormatJSON, cfg.Format) + } + }) + + t.Run("auto-detects SARIF format from extension", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "results.sarif") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + if cfg.Format != ohFormatSARIF { + t.Errorf("expected Format = %q (auto-detected), got %q", ohFormatSARIF, cfg.Format) + } + }) + + t.Run("auto-detects JUnit format from .xml extension", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "results.xml") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + if cfg.Format != ohFormatJUnit { + t.Errorf("expected Format = %q (auto-detected), got %q", ohFormatJUnit, cfg.Format) + } + }) + + t.Run("explicit format flag overrides auto-detection", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "results.json") + + cmd := newTestCmd() + // Simulate user explicitly setting --format=sarif + if err := cmd.Flags().Set("format", ohFormatSARIF); err != nil { + t.Fatalf("failed to set flag: %v", err) + } + + cfg, err := ResolveOutput(cmd, outputPath, ohFormatSARIF, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + if cfg.Format != ohFormatSARIF { + t.Errorf("expected Format = %q (explicit), got %q", ohFormatSARIF, cfg.Format) + } + }) + + t.Run("creates output file", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.txt") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + // Verify file was created + if _, err := os.Stat(outputPath); os.IsNotExist(err) { + t.Error("expected output file to be created") + } + + // Verify we can write to it + _, err = cfg.Writer.Write([]byte("test content")) + if err != nil { + t.Errorf("Write() error = %v", err) + } + }) + + t.Run("creates parent directories", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "nested", "dir", "output.txt") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + // Verify nested directories and file were created + if _, err := os.Stat(outputPath); os.IsNotExist(err) { + t.Error("expected output file with nested directories to be created") + } + }) + + t.Run("cleanup resets outputToFile state", func(t *testing.T) { + // Force colors in auto mode so behavior depends on correct state reset + t.Setenv("CLICOLOR_FORCE", "1") + + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.txt") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + + // Call cleanup + cfg.Cleanup() + + // Verify state was reset by checking colors work in auto mode + // With CLICOLOR_FORCE=1 and outputToFile=false, colors should be enabled + cli.InitColors(cli.ColorModeAuto) + if !cli.ColorsEnabled() { + t.Error("expected outputToFile state to be reset after cleanup (colors should be enabled in auto mode with CLICOLOR_FORCE=1)") + } + }) + + t.Run("color=always skips color disabling", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.txt") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "always") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + // With --color=always, colors should remain enabled even when writing to file + cli.InitColors(cli.ColorModeAlways) + if !cli.ColorsEnabled() { + t.Error("expected colors to remain enabled with --color=always") + } + }) + + t.Run("returns error for invalid path", func(t *testing.T) { + // Create a file (not a directory) to trigger ENOTDIR when MkdirAll tries + // to create a directory at this path - portable across all platforms + tmpDir := t.TempDir() + blockingFile := filepath.Join(tmpDir, "notadir") + if err := os.WriteFile(blockingFile, []byte("x"), 0600); err != nil { + t.Fatalf("failed to create blocking file: %v", err) + } + // Try to create output file inside the file (ENOTDIR) + outputPath := filepath.Join(blockingFile, "output.txt") + + cmd := newTestCmd() + _, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err == nil { + t.Error("expected error for invalid path") + } + }) + + t.Run("resets state on error", func(t *testing.T) { + // Force colors in auto mode so behavior depends on correct state reset + t.Setenv("CLICOLOR_FORCE", "1") + + // Create a file (not a directory) to trigger ENOTDIR when MkdirAll tries + // to create a directory at this path - portable across all platforms + tmpDir := t.TempDir() + blockingFile := filepath.Join(tmpDir, "notadir") + if err := os.WriteFile(blockingFile, []byte("x"), 0600); err != nil { + t.Fatalf("failed to create blocking file: %v", err) + } + outputPath := filepath.Join(blockingFile, "output.txt") + + cmd := newTestCmd() + _, _ = ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + + // Verify state was reset by checking colors are enabled in auto mode + // With CLICOLOR_FORCE=1 and outputToFile=false, colors should be enabled + cli.InitColors(cli.ColorModeAuto) + if !cli.ColorsEnabled() { + t.Error("expected outputToFile state to be reset after error (colors should be enabled in auto mode with CLICOLOR_FORCE=1)") + } + }) + + t.Run("unrecognized extension keeps original format", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "results.txt") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + defer cfg.Cleanup() + + // .txt is not recognized, so format should stay as ohFormatHuman + if cfg.Format != ohFormatHuman { + t.Errorf("expected Format = %q (unrecognized extension), got %q", ohFormatHuman, cfg.Format) + } + }) + + t.Run("cleanup is idempotent", func(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.txt") + + cmd := newTestCmd() + cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") + if err != nil { + t.Fatalf("ResolveOutput() error = %v", err) + } + + // Call cleanup multiple times - should not panic or error + cfg.Cleanup() + cfg.Cleanup() + cfg.Cleanup() + + // Verify the file was properly closed (we can write a new file at the same path) + // #nosec G304 -- test code using controlled temp directory path + fo, err := os.Create(outputPath) + if err != nil { + t.Fatalf("failed to create file after cleanup: %v", err) + } + _ = fo.Close() + }) +} diff --git a/internal/cmd/cmdutil/theme.go b/internal/cmd/cmdutil/theme.go new file mode 100644 index 00000000..65b5a642 --- /dev/null +++ b/internal/cmd/cmdutil/theme.go @@ -0,0 +1,72 @@ +package cmdutil + +import ( + "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/charmbracelet/huh" + "github.com/charmbracelet/lipgloss" +) + +// Brand colors matching internal/output/styles.go. The exported colors are +// referenced by interactive flows outside this package (install, uninstall); +// the unexported ones are used only by armisTheme below. +var ( + BrandAccent = lipgloss.AdaptiveColor{Light: "#7c3aed", Dark: "#7c3aed"} // purple-600 (Armis brand) + BrandSuccess = lipgloss.AdaptiveColor{Light: "#16A34A", Dark: "#22C55E"} // green-600/500 (completion ✓) + BrandError = lipgloss.AdaptiveColor{Light: "#DC2626", Dark: "#EF4444"} // red-600/500 + BrandMuted = lipgloss.AdaptiveColor{Light: "#4B5563", Dark: "#6B7280"} // gray-600/500 + BrandSeparator = lipgloss.AdaptiveColor{Light: "#C4B5FD", Dark: "#4C1D95"} // purple-300/900 (title underline) + BrandWarn = lipgloss.AdaptiveColor{Light: "#D97706", Dark: "#F59E0B"} // amber-600/500 + + brandSelected = lipgloss.AdaptiveColor{Light: "#059669", Dark: "#34D399"} // emerald-600/400 (multi-select [+]) + brandBright = lipgloss.AdaptiveColor{Light: "#1F2937", Dark: "#FFFFFF"} // gray-800/white + brandBorder = lipgloss.AdaptiveColor{Light: "#D1D5DB", Dark: "#374151"} // gray-300/700 (buttons) + brandPanelBorder = lipgloss.AdaptiveColor{Light: "#6366F1", Dark: "#818CF8"} // indigo-500/400 (interactive panels) + brandDim = lipgloss.AdaptiveColor{Light: "#9CA3AF", Dark: "#4B5563"} // gray-400/600 +) + +func armisTheme() *huh.Theme { + t := huh.ThemeBase() + + t.Focused.Base = t.Focused.Base.BorderForeground(brandPanelBorder) + t.Focused.Card = t.Focused.Base + t.Focused.Title = t.Focused.Title.Foreground(brandBright).Bold(true) + t.Focused.NoteTitle = t.Focused.NoteTitle.Foreground(BrandAccent).Bold(true).MarginBottom(1) + t.Focused.Description = t.Focused.Description.Foreground(BrandMuted) + t.Focused.ErrorIndicator = t.Focused.ErrorIndicator.Foreground(BrandError) + t.Focused.ErrorMessage = t.Focused.ErrorMessage.Foreground(BrandError) + t.Focused.SelectSelector = t.Focused.SelectSelector.Foreground(BrandAccent) + t.Focused.NextIndicator = t.Focused.NextIndicator.Foreground(BrandAccent) + t.Focused.PrevIndicator = t.Focused.PrevIndicator.Foreground(BrandAccent) + t.Focused.Option = t.Focused.Option.Foreground(brandBright) + t.Focused.MultiSelectSelector = t.Focused.MultiSelectSelector.Foreground(BrandAccent) + t.Focused.SelectedOption = t.Focused.SelectedOption.Foreground(brandSelected) + t.Focused.SelectedPrefix = lipgloss.NewStyle().Foreground(brandSelected).SetString("[+] ") + t.Focused.UnselectedPrefix = lipgloss.NewStyle().Foreground(brandDim).SetString("[ ] ") + t.Focused.UnselectedOption = t.Focused.UnselectedOption.Foreground(brandBright) + t.Focused.FocusedButton = t.Focused.FocusedButton.Foreground(lipgloss.Color("#FFFFFF")).Background(BrandAccent).Bold(true) + t.Focused.BlurredButton = t.Focused.BlurredButton.Foreground(brandBright).Background(brandBorder) + + t.Focused.TextInput.Cursor = t.Focused.TextInput.Cursor.Foreground(BrandAccent) + t.Focused.TextInput.Placeholder = t.Focused.TextInput.Placeholder.Foreground(brandDim) + t.Focused.TextInput.Prompt = t.Focused.TextInput.Prompt.Foreground(BrandAccent) + + t.Blurred = t.Focused + t.Blurred.Base = t.Blurred.Base.BorderStyle(lipgloss.HiddenBorder()) + t.Blurred.Card = t.Blurred.Base + t.Blurred.NextIndicator = lipgloss.NewStyle() + t.Blurred.PrevIndicator = lipgloss.NewStyle() + + t.Group.Title = t.Focused.Title + t.Group.Description = t.Focused.Description + + return t +} + +// GetInstallTheme returns the Armis-branded huh theme, or the plain base theme +// when colors are disabled (NO_COLOR, non-TTY, --color=never). +func GetInstallTheme() *huh.Theme { + if !cli.ColorsEnabled() { + return huh.ThemeBase() + } + return armisTheme() +} diff --git a/internal/cmd/supply_chain_wrap.go b/internal/cmd/supply_chain_wrap.go index 5e0e0ebd..7cc6120c 100644 --- a/internal/cmd/supply_chain_wrap.go +++ b/internal/cmd/supply_chain_wrap.go @@ -33,6 +33,7 @@ const ( // scattering the literals across the file. const ( pmNPM = "npm" + pmNPX = "npx" pmPNPM = "pnpm" pmBun = "bun" pmYarn = "yarn" @@ -59,7 +60,7 @@ func init() { } var allowedPMs = map[string]bool{ - pmNPM: true, pmPNPM: true, pmBun: true, pmYarn: true, + pmNPM: true, pmNPX: true, pmPNPM: true, pmBun: true, pmYarn: true, pmPip: true, pmUV: true, pmPoetry: true, pmPipenv: true, pmPDM: true, pmMaven: true, pmGradle: true, } @@ -82,7 +83,7 @@ func runSupplyChainWrap(cmd *cobra.Command, args []string) error { canonical := canonicalPM(pmName) if !allowedPMs[canonical] { - return fmt.Errorf("unsupported package manager: %s (allowed: npm, pnpm, bun, yarn, pip, uv, poetry, pipenv, pdm, mvn, gradle)", pmName) + return fmt.Errorf("unsupported package manager: %s (allowed: npm, npx, pnpm, bun, yarn, pip, uv, poetry, pipenv, pdm, mvn, gradle)", pmName) } if os.Getenv(envSCActive) == "1" { @@ -128,7 +129,7 @@ func runProxyWrap(cmd *cobra.Command, pmName string, pmArgs []string) error { // pip and uv resolve from the PyPI Simple API, a different protocol from the // npm registry, so the proxy must run in PyPI mode (PEP 691/700 JSON file - // filtering). All other proxied PMs (npm/pnpm/bun/yarn) speak the npm registry. + // filtering). All other proxied PMs (npm/npx/pnpm/bun/yarn) speak the npm registry. mode := supplychain.ModeNPM switch canonicalPM(pmName) { case pmPip, pmUV: @@ -195,6 +196,8 @@ func execPM(pm string, args []string, extraEnv []string) (int, error) { switch pm { case pmNPM: pmName = pmNPM + case pmNPX: + pmName = pmNPX case pmPNPM: pmName = pmPNPM case pmBun: @@ -218,15 +221,13 @@ func execPM(pm string, args []string, extraEnv []string) (int, error) { default: // Versioned pip variants (pip3, pip3.11, pip3.12) must execute the exact // binary the user invoked so the install lands in that interpreter's - // environment. IsPipVariant enforces a strict pattern (letters, digits, a - // single dotted numeric suffix), so the value reaching exec.LookPath is - // still a bounded, shell-metacharacter-free name rather than arbitrary - // user input — preserving the CWE-426 guarantee the literal cases provide. - if supplychain.IsPipVariant(pm) { - pmName = pm - break + // environment. CanonicalPipVariant reconstructs the name from its parsed + // numeric components so no taint flows from pm into pmName. + canonical, ok := supplychain.CanonicalPipVariant(pm) + if !ok { + return 1, fmt.Errorf("unsupported package manager: %s (allowed: npm, npx, pnpm, bun, yarn, pip, uv, poetry, pipenv, pdm, mvn, gradle)", pm) } - return 1, fmt.Errorf("unsupported package manager: %s (allowed: npm, pnpm, bun, yarn, pip, uv, poetry, pipenv, pdm, mvn, gradle)", pm) + pmName = canonical } // armis:ignore cwe:426 cwe:427 reason:pmName is one of the hardcoded string literals selected by the switch above, never the user argument; resolving the user's own PM from PATH is the point of a transparent wrapper @@ -903,6 +904,13 @@ func pmToEcosystem(pm string) supplychain.Ecosystem { switch pm { case pmNPM: return supplychain.EcosystemNPM + case pmNPX: + // npx is the npm package runner, not a distinct ecosystem: it resolves from + // the npm registry and has no lockfile of its own. Mapping it to + // EcosystemNPM lets the config "ecosystems" scoping gate treat npx exactly + // like npm — `ecosystems: [npm]` enforces both, and scoping npm out + // (e.g. `ecosystems: [pip]`) passes npx through too, so the two never diverge. + return supplychain.EcosystemNPM case pmPNPM: return supplychain.EcosystemPNPM case pmBun: diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 82e69471..00e1af0f 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -8,6 +8,7 @@ import ( "regexp" "runtime" "slices" + "strconv" "strings" ) @@ -283,6 +284,19 @@ func HasInjection(path string) bool { return strings.Contains(string(content), markerStart) } +// HasCurrentInjection reports whether path already contains the exact wrapper +// block that would be written for the given shell and PMs. Unlike HasInjection, +// which only checks for the marker, this verifies the content matches so callers +// can skip the prompt when nothing would change (same binary path, same PM set). +func HasCurrentInjection(path, shell string, pms []string) bool { + // armis:ignore cwe:22 cwe:73 reason:path is a shell RC file under the current user's own $HOME (see DetectShells); reading the user's RC file to check injection status is safe + content, err := os.ReadFile(path) //nolint:gosec // user's own RC file + if err != nil { + return false + } + return strings.Contains(string(content), GenerateWrapper(shell, pms)) +} + func fileExists(path string) bool { _, err := os.Stat(path) return err == nil @@ -303,42 +317,86 @@ func IsPipVariant(name string) bool { return pipExecutable.MatchString(name) } -// DetectPipVariants scans $PATH for pip executables (pip, pip3, pip3.11, …) and -// returns a deduplicated, sorted list of the command names found. pip installs -// under several names depending on how Python was set up, and a shell wrapper -// only shadows the exact name the user types, so all present variants must be -// wrapped. Falls back to ["pip"] when none are found or PATH is unset. -func DetectPipVariants() []string { +// 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]+))?)?$`) + +// CanonicalPipVariant reconstructs a pip variant command name from its numeric +// components, breaking any taint that may be associated with the caller's input +// string. It returns ("", false) when name does not match the pip variant +// pattern. The returned name is built entirely from integer literals and +// strconv-formatted ints, so it is safe to pass directly to exec.LookPath +// without a CWE-426 taint concern. +func CanonicalPipVariant(name string) (string, bool) { + m := pipVariant.FindStringSubmatch(name) + if m == nil { + return "", false + } + // m[1] is the "3" or "3.NN" suffix (may be empty for bare "pip"). + // m[2] is the minor version digits (may be empty). + if m[1] == "" { + return "pip", true + } + if m[2] == "" { + return "pip3", true + } + minor, err := strconv.Atoi(m[2]) + if err != nil { + return "", false + } + return fmt.Sprintf("pip3.%d", minor), true +} + +// maxScanPathResults caps the number of distinct matches scanPathExecutables +// will collect. In normal use the supported PM set is ~15 names, so 128 is +// far above any realistic ceiling while still bounding memory when $PATH +// contains unusual directories. +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 []string{"pip"} + return nil } seen := make(map[string]bool) for _, dir := range filepath.SplitList(pathEnv) { + if len(seen) >= maxScanPathResults { + break + } entries, err := os.ReadDir(dir) if err != nil { continue } for _, entry := range entries { + if len(seen) >= maxScanPathResults { + break + } if entry.IsDir() { continue } name := entry.Name() - if !pipExecutable.MatchString(name) { + if !match(name) { continue } - // On Unix, a pip-named entry on PATH with no execute bit (a stray data + // On Unix, a matching entry on PATH with no execute bit (a stray data // file or a non-exec script) would yield a wrapper that later fails at // exec.LookPath with a confusing error, so require at least one execute - // bit before treating it as a real pip command. Info() reports the - // entry's own mode (lstat semantics); a symlink to a real pip keeps its + // bit before treating it as a real command. Info() reports the entry's + // own mode (lstat semantics); a symlink to a real binary keeps its // 0o777 link bits and so still passes, matching what the user can run. // // Skip this check on Windows: there is no execute-bit concept there // (executability is governed by file extension via PATHEXT), and // os.FileMode.Perm never sets 0o111, so the filter would reject every - // real pip and collapse detection to the ["pip"] fallback. + // real binary and collapse detection to nothing. if runtime.GOOS != goosWindows { info, err := entry.Info() if err != nil || info.Mode().Perm()&0o111 == 0 { @@ -350,13 +408,51 @@ func DetectPipVariants() []string { } if len(seen) == 0 { - return []string{"pip"} + return nil } - variants := make([]string, 0, len(seen)) + names := make([]string, 0, len(seen)) for name := range seen { - variants = append(variants, name) + names = append(names, name) + } + slices.Sort(names) + return names +} + +// DetectPipVariants scans $PATH for pip executables (pip, pip3, pip3.11, …) and +// returns a deduplicated, sorted list of the command names found. pip installs +// under several names depending on how Python was set up, and a shell wrapper +// only shadows the exact name the user types, so all present variants must be +// wrapped. Falls back to ["pip"] when none are found or PATH is unset. +func DetectPipVariants() []string { + variants := scanPathExecutables(pipExecutable.MatchString) + if len(variants) == 0 { + return []string{"pip"} } - slices.Sort(variants) return variants } + +// DetectInstalledPMs scans $PATH for every supported package manager actually +// installed and returns the deduplicated, sorted set of command names found. +// names is the list of fixed package-manager command names to look for (npm, +// pnpm, bun, mvn, …); pip variants (pip, pip3, pip3.12) are matched separately +// via the pip pattern, since pip installs under several interpreter-specific +// names and each must be wrapped independently. +// +// `supply-chain init` uses this rather than CWD lockfile detection because the +// shell wrappers it injects are global — they shadow the package-manager command +// in every directory, not just the project init happened to run in. Per-project +// enforcement is still decided dynamically at wrap time (the ecosystems config +// and policy are re-read on each invocation), so the wrapper set should reflect +// what is installed on the machine, not which lockfiles sit in one directory. +// Returns nil when nothing is found or PATH is unset; the caller supplies the +// fallback. +func DetectInstalledPMs(names []string) []string { + want := make(map[string]bool, len(names)) + for _, n := range names { + want[n] = true + } + return scanPathExecutables(func(name string) bool { + return want[name] || pipExecutable.MatchString(name) + }) +} From 6b8d454f531c462cc4f300a78c8f12e325a184f4 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 11:23:23 +0200 Subject: [PATCH 2/8] =?UTF-8?q?[PPSC-920]=20feat(supply-chain):=20PATH-bas?= =?UTF-8?q?ed=20init=20detection=20=E2=80=94=20remaining=20cmd=20refactor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- README.md | 2 +- docs/CHANGELOG.md | 5 +- internal/cmd/install_interactive.go | 21 +- internal/cmd/install_theme.go | 67 ------ internal/cmd/output_helper.go | 108 --------- internal/cmd/output_helper_test.go | 278 ---------------------- internal/cmd/root.go | 29 --- internal/cmd/root_test.go | 80 +------ internal/cmd/scan_image.go | 5 +- internal/cmd/scan_repo.go | 5 +- internal/cmd/supply_chain.go | 4 +- internal/cmd/supply_chain_check.go | 9 +- internal/cmd/supply_chain_check_test.go | 2 +- internal/cmd/supply_chain_init.go | 270 +++++++++++++-------- internal/cmd/supply_chain_init_test.go | 237 +++++++++++++++--- internal/cmd/supply_chain_test.go | 19 +- internal/cmd/supply_chain_wrap_pm_test.go | 10 +- internal/cmd/uninstall.go | 19 +- internal/supplychain/shell_test.go | 25 ++ 19 files changed, 461 insertions(+), 734 deletions(-) delete mode 100644 internal/cmd/install_theme.go delete mode 100644 internal/cmd/output_helper.go delete mode 100644 internal/cmd/output_helper_test.go diff --git a/README.md b/README.md index 8db03ca6..e345c6fd 100644 --- a/README.md +++ b/README.md @@ -464,7 +464,7 @@ The `supply-chain` command enforces a minimum **release age** on your dependenci No Armis Cloud authentication is required: `supply-chain` queries public registries (npm, PyPI, Maven Central) directly. -**Supported ecosystems:** npm, pnpm, bun, yarn (Node); pip, uv, poetry, pipenv, pdm (Python); Maven, Gradle (Java). +**Supported ecosystems:** npm, npx, pnpm, bun, yarn (Node); pip, uv, poetry, pipenv, pdm (Python); Maven, Gradle (Java). ### Audit a lockfile (CI) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f275c14d..a7e466af 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,8 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `supply-chain` command for enforcing package release-age policies, defending against supply-chain attacks (typosquatting, compromised maintainers, dependency confusion) by flagging or blocking packages published more recently than a configurable threshold (default 72h). No Armis Cloud authentication required — queries public registries directly. (#206, #210, #211) - - Supports 11 package managers across three ecosystems: npm, pnpm, bun, yarn (Node); pip, uv, poetry, pipenv, pdm (Python); Maven, Gradle (Java). + - Supports 12 package managers across three ecosystems: npm, npx, pnpm, bun, yarn (Node); pip, uv, poetry, pipenv, pdm (Python); Maven, Gradle (Java). - Node package managers and pip/uv use a transparent registry proxy that filters out too-young versions during install; poetry, pipenv, pdm, Maven, and Gradle use a pre-install lockfile audit that blocks the build before execution. + - `npx` is wrapped alongside `npm` (it ships with npm and resolves from the same registry), so ad-hoc `npx ` runs are filtered through the same proxy. Enforcement applies to packages npx fetches from the registry; a package already in the npx cache or a binary already in `node_modules/.bin` runs without a registry round-trip and is not re-checked. The sibling runners `pnpm dlx` and `yarn dlx` are already covered as subcommands of the existing pnpm/yarn wrappers; `bunx` (a separate binary) is not yet wrapped. - `supply-chain check` audits lockfiles in CI; `supply-chain init`/`uninit` set up local shell enforcement; `supply-chain status` reports the active policy and detected ecosystems. - Configurable via `.armis-supply-chain.yaml` (`min-age`, `exclusions`, `ecosystems`, `fail-open`); per-invocation bypass via `ARMIS_SUPPLY_CHAIN_SKIP`; master kill switch via `ARMIS_SUPPLY_CHAIN=off`. - Gradle lockfile staleness detection (warns when `build.gradle` is newer than `gradle.lockfile`), Maven `pom.xml` partial-coverage notice (direct dependencies only), and a warning for unrecognized ecosystem names in the config. @@ -20,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `supply-chain init`: now wraps every supported package manager found on your `PATH` instead of only the ones with a lockfile in the current directory. The injected shell functions are global (they apply in every directory), so detecting from the current project's lockfiles left gaps — e.g. running `init` in a Go repo wrapped only `npm`/`npx`, so a later `pip install` in a Python project ran unenforced. Detection is now machine-wide; per-project enforcement is still decided dynamically at install time from the nearest `.armis-supply-chain.yaml` (the `ecosystems` scope and policy are re-read on each install), so wrapping a package manager never forces enforcement where the project hasn't opted in. When no supported package manager is on `PATH`, `init` still falls back to wrapping `npm`/`npx`. + ### Deprecated ### Removed diff --git a/internal/cmd/install_interactive.go b/internal/cmd/install_interactive.go index b22804a7..883a2338 100644 --- a/internal/cmd/install_interactive.go +++ b/internal/cmd/install_interactive.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/install" "github.com/ArmisSecurity/armis-cli/internal/progress" "github.com/charmbracelet/huh" @@ -14,7 +15,7 @@ import ( ) func runInteractiveInstall(force bool) error { - theme := getInstallTheme() + theme := cmdutil.GetInstallTheme() accessible := !cli.ColorsEnabled() fmt.Fprintln(os.Stderr, "") @@ -22,8 +23,8 @@ func runInteractiveInstall(force bool) error { fmt.Fprintln(os.Stderr, " Armis AppSec MCP Server Setup") fmt.Fprintln(os.Stderr, " ─────────────────────────────") } else { - titleStyle := lipgloss.NewStyle().Bold(true).Foreground(brandAccent) - borderStyle := lipgloss.NewStyle().Foreground(brandSeparator) + titleStyle := lipgloss.NewStyle().Bold(true).Foreground(cmdutil.BrandAccent) + borderStyle := lipgloss.NewStyle().Foreground(cmdutil.BrandSeparator) fmt.Fprintf(os.Stderr, " %s\n", titleStyle.Render("Armis AppSec MCP Server Setup")) fmt.Fprintf(os.Stderr, " %s\n", borderStyle.Render("─────────────────────────────")) } @@ -52,9 +53,9 @@ func runInteractiveInstall(force bool) error { if accessible { successMark, failMark, warnMark = "[OK]", "[FAIL]", "[WARN]" } else { - successMark = lipgloss.NewStyle().Foreground(brandSuccess).Render("✓") - failMark = lipgloss.NewStyle().Foreground(brandError).Render("✗") - warnMark = lipgloss.NewStyle().Foreground(brandWarn).Render("⚠") + successMark = lipgloss.NewStyle().Foreground(cmdutil.BrandSuccess).Render("✓") + failMark = lipgloss.NewStyle().Foreground(cmdutil.BrandError).Render("✗") + warnMark = lipgloss.NewStyle().Foreground(cmdutil.BrandWarn).Render("⚠") } fmt.Fprintln(os.Stderr, "") @@ -219,7 +220,7 @@ func runInteractiveInstall(force bool) error { fmt.Fprintln(os.Stderr, " Run 'armis-cli hook init' in other repos for pre-commit coverage.") } } else { - dimStyle := lipgloss.NewStyle().Foreground(brandMuted) + dimStyle := lipgloss.NewStyle().Foreground(cmdutil.BrandMuted) fmt.Fprintln(os.Stderr, dimStyle.Render(" Next steps:")) fmt.Fprintln(os.Stderr, dimStyle.Render(" Restart your editors to activate the MCP server.")) if installPreCommit { @@ -347,7 +348,7 @@ func validateAndReport(clientID, clientSecret string, accessible bool) error { if accessible { fmt.Fprintln(os.Stderr, "[FAIL]") } else { - fmt.Fprintln(os.Stderr, lipgloss.NewStyle().Foreground(brandError).Render("✗")) + fmt.Fprintln(os.Stderr, lipgloss.NewStyle().Foreground(cmdutil.BrandError).Render("✗")) } for _, line := range strings.Split(err.Error(), "\n") { fmt.Fprintf(os.Stderr, " %s\n", line) @@ -358,7 +359,7 @@ func validateAndReport(clientID, clientSecret string, accessible bool) error { if accessible { fmt.Fprintln(os.Stderr, "[OK]") } else { - fmt.Fprintln(os.Stderr, lipgloss.NewStyle().Foreground(brandSuccess).Render("✓")) + fmt.Fprintln(os.Stderr, lipgloss.NewStyle().Foreground(cmdutil.BrandSuccess).Render("✓")) } fmt.Fprintln(os.Stderr, "") return nil @@ -477,7 +478,7 @@ func offerHookSetup(theme *huh.Theme, accessible bool, hasClaude bool) ([]instal if accessible { fmt.Fprintln(os.Stderr, " No hook-capable AI clients detected. Skipping hook setup.") } else { - dimStyle := lipgloss.NewStyle().Foreground(brandMuted) + dimStyle := lipgloss.NewStyle().Foreground(cmdutil.BrandMuted) fmt.Fprintln(os.Stderr, dimStyle.Render(" No hook-capable AI clients detected. Skipping hook setup.")) } } diff --git a/internal/cmd/install_theme.go b/internal/cmd/install_theme.go deleted file mode 100644 index f8934e5d..00000000 --- a/internal/cmd/install_theme.go +++ /dev/null @@ -1,67 +0,0 @@ -package cmd - -import ( - "github.com/ArmisSecurity/armis-cli/internal/cli" - "github.com/charmbracelet/huh" - "github.com/charmbracelet/lipgloss" -) - -// Brand colors matching internal/output/styles.go -var ( - brandAccent = lipgloss.AdaptiveColor{Light: "#7c3aed", Dark: "#7c3aed"} // purple-600 (Armis brand) - brandSuccess = lipgloss.AdaptiveColor{Light: "#16A34A", Dark: "#22C55E"} // green-600/500 (completion ✓) - brandSelected = lipgloss.AdaptiveColor{Light: "#059669", Dark: "#34D399"} // emerald-600/400 (multi-select [+]) - brandError = lipgloss.AdaptiveColor{Light: "#DC2626", Dark: "#EF4444"} // red-600/500 - brandMuted = lipgloss.AdaptiveColor{Light: "#4B5563", Dark: "#6B7280"} // gray-600/500 - brandBright = lipgloss.AdaptiveColor{Light: "#1F2937", Dark: "#FFFFFF"} // gray-800/white - brandBorder = lipgloss.AdaptiveColor{Light: "#D1D5DB", Dark: "#374151"} // gray-300/700 (buttons) - brandSeparator = lipgloss.AdaptiveColor{Light: "#C4B5FD", Dark: "#4C1D95"} // purple-300/900 (title underline) - brandPanelBorder = lipgloss.AdaptiveColor{Light: "#6366F1", Dark: "#818CF8"} // indigo-500/400 (interactive panels) - brandDim = lipgloss.AdaptiveColor{Light: "#9CA3AF", Dark: "#4B5563"} // gray-400/600 - brandWarn = lipgloss.AdaptiveColor{Light: "#D97706", Dark: "#F59E0B"} // amber-600/500 -) - -func armisTheme() *huh.Theme { - t := huh.ThemeBase() - - t.Focused.Base = t.Focused.Base.BorderForeground(brandPanelBorder) - t.Focused.Card = t.Focused.Base - t.Focused.Title = t.Focused.Title.Foreground(brandBright).Bold(true) - t.Focused.NoteTitle = t.Focused.NoteTitle.Foreground(brandAccent).Bold(true).MarginBottom(1) - t.Focused.Description = t.Focused.Description.Foreground(brandMuted) - t.Focused.ErrorIndicator = t.Focused.ErrorIndicator.Foreground(brandError) - t.Focused.ErrorMessage = t.Focused.ErrorMessage.Foreground(brandError) - t.Focused.SelectSelector = t.Focused.SelectSelector.Foreground(brandAccent) - t.Focused.NextIndicator = t.Focused.NextIndicator.Foreground(brandAccent) - t.Focused.PrevIndicator = t.Focused.PrevIndicator.Foreground(brandAccent) - t.Focused.Option = t.Focused.Option.Foreground(brandBright) - t.Focused.MultiSelectSelector = t.Focused.MultiSelectSelector.Foreground(brandAccent) - t.Focused.SelectedOption = t.Focused.SelectedOption.Foreground(brandSelected) - t.Focused.SelectedPrefix = lipgloss.NewStyle().Foreground(brandSelected).SetString("[+] ") - t.Focused.UnselectedPrefix = lipgloss.NewStyle().Foreground(brandDim).SetString("[ ] ") - t.Focused.UnselectedOption = t.Focused.UnselectedOption.Foreground(brandBright) - t.Focused.FocusedButton = t.Focused.FocusedButton.Foreground(lipgloss.Color("#FFFFFF")).Background(brandAccent).Bold(true) - t.Focused.BlurredButton = t.Focused.BlurredButton.Foreground(brandBright).Background(brandBorder) - - t.Focused.TextInput.Cursor = t.Focused.TextInput.Cursor.Foreground(brandAccent) - t.Focused.TextInput.Placeholder = t.Focused.TextInput.Placeholder.Foreground(brandDim) - t.Focused.TextInput.Prompt = t.Focused.TextInput.Prompt.Foreground(brandAccent) - - t.Blurred = t.Focused - t.Blurred.Base = t.Blurred.Base.BorderStyle(lipgloss.HiddenBorder()) - t.Blurred.Card = t.Blurred.Base - t.Blurred.NextIndicator = lipgloss.NewStyle() - t.Blurred.PrevIndicator = lipgloss.NewStyle() - - t.Group.Title = t.Focused.Title - t.Group.Description = t.Focused.Description - - return t -} - -func getInstallTheme() *huh.Theme { - if !cli.ColorsEnabled() { - return huh.ThemeBase() - } - return armisTheme() -} diff --git a/internal/cmd/output_helper.go b/internal/cmd/output_helper.go deleted file mode 100644 index eb6252c4..00000000 --- a/internal/cmd/output_helper.go +++ /dev/null @@ -1,108 +0,0 @@ -package cmd - -import ( - "io" - "os" - - "github.com/ArmisSecurity/armis-cli/internal/cli" - "github.com/ArmisSecurity/armis-cli/internal/output" - "github.com/spf13/cobra" -) - -// OutputConfig holds the resolved output configuration for scan commands. -type OutputConfig struct { - // Writer is the destination for formatted output (stdout or file). - Writer io.Writer - // Format is the resolved output format (may differ from flag if auto-detected). - Format string - // cleanup is called to close the file and reset state. Always call this via defer. - cleanup func() - // cleaned tracks whether cleanup has already been called (for idempotency) - cleaned bool -} - -// Cleanup closes the output file (if any) and restores color state. -// This method is idempotent and safe to call multiple times. -// Called via defer in scan commands; the defer ensures cleanup on all exit paths. -func (c *OutputConfig) Cleanup() { - if c.cleaned { - return - } - c.cleaned = true - c.cleanup() -} - -// ResolveOutput determines the output writer and format for scan results. -// It handles: -// - Auto-detecting format from file extension (if --format not explicitly set) -// - Creating the output file (with proper directory creation) -// - Disabling colors when writing to file (unless --color=always) -// -// The returned OutputConfig.cleanup function MUST be called via defer to: -// - Close the output file (if writing to file) -// - Reset the outputToFile state for proper cleanup -// -// Example usage: -// -// cfg, err := ResolveOutput(cmd, outputFile, format, colorFlag) -// if err != nil { -// return err -// } -// defer cfg.Cleanup() -// // use cfg.Writer and cfg.Format -func ResolveOutput(cmd *cobra.Command, outputPath, formatFlag, colorFlag string) (*OutputConfig, error) { - cfg := &OutputConfig{ - Writer: os.Stdout, - Format: formatFlag, - cleanup: func() {}, // no-op by default - } - - if outputPath == "" { - return cfg, nil - } - - // Auto-detect format from extension if user hasn't explicitly set --format. - // Use cmd.Flag() instead of cmd.Flags().Changed() because --format is a - // persistent flag on the root command, and Flags() doesn't include inherited flags. - fmtFlag := cmd.Flag("format") - formatExplicitlySet := fmtFlag != nil && fmtFlag.Changed - if !formatExplicitlySet { - if detected := output.FormatFromExtension(outputPath); detected != "" { - cfg.Format = detected - } - } - - // Capture previous state for restoration on error or cleanup - prevOutputToFile := cli.GetOutputToFile() - colorMode := cli.ColorMode(colorFlag) - - // Disable colors when writing to file (unless --color=always) - if colorMode != cli.ColorModeAlways { - cli.SetOutputToFile(true) - cli.InitColors(colorMode) // Pass actual mode, not hardcoded Auto - output.SyncColors() - } - - // armis:ignore cwe:22 reason:outputPath is from --output flag; user-controlled CLI arg for their own files - fileOutput, err := output.NewFileOutput(outputPath) // armis:ignore cwe:73 reason:outputPath from --output flag; user's own files - if err != nil { - // Restore previous state on error - cli.SetOutputToFile(prevOutputToFile) - cli.InitColors(colorMode) - output.SyncColors() - return nil, err - } - - cfg.Writer = fileOutput.Writer() - cfg.cleanup = func() { - // Restore previous outputToFile state and re-sync colors - cli.SetOutputToFile(prevOutputToFile) - cli.InitColors(colorMode) - output.SyncColors() - if cerr := fileOutput.Close(); cerr != nil { - cli.PrintWarningf("failed to close output file: %v", cerr) - } - } - - return cfg, nil -} diff --git a/internal/cmd/output_helper_test.go b/internal/cmd/output_helper_test.go deleted file mode 100644 index 39681175..00000000 --- a/internal/cmd/output_helper_test.go +++ /dev/null @@ -1,278 +0,0 @@ -package cmd - -import ( - "os" - "path/filepath" - "testing" - - "github.com/ArmisSecurity/armis-cli/internal/cli" - "github.com/spf13/cobra" -) - -// Output helper test format constants to satisfy goconst linter -const ( - ohFormatHuman = "human" - ohFormatJSON = "json" - ohFormatSARIF = "sarif" - ohFormatJUnit = "junit" -) - -func TestResolveOutput(t *testing.T) { - // Helper to create a minimal cobra command with format flag - newTestCmd := func() *cobra.Command { - cmd := &cobra.Command{Use: "test"} - cmd.Flags().String("format", ohFormatHuman, "output format") - return cmd - } - - t.Run("returns stdout when no output path", func(t *testing.T) { - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, "", ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - if cfg.Writer != os.Stdout { - t.Error("expected Writer to be os.Stdout when outputPath is empty") - } - if cfg.Format != ohFormatHuman { - t.Errorf("expected Format = %q, got %q", ohFormatHuman, cfg.Format) - } - }) - - t.Run("auto-detects JSON format from extension", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "results.json") - - cmd := newTestCmd() - // Don't mark format as changed - let auto-detection kick in - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - if cfg.Format != ohFormatJSON { - t.Errorf("expected Format = %q (auto-detected), got %q", ohFormatJSON, cfg.Format) - } - }) - - t.Run("auto-detects SARIF format from extension", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "results.sarif") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - if cfg.Format != ohFormatSARIF { - t.Errorf("expected Format = %q (auto-detected), got %q", ohFormatSARIF, cfg.Format) - } - }) - - t.Run("auto-detects JUnit format from .xml extension", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "results.xml") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - if cfg.Format != ohFormatJUnit { - t.Errorf("expected Format = %q (auto-detected), got %q", ohFormatJUnit, cfg.Format) - } - }) - - t.Run("explicit format flag overrides auto-detection", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "results.json") - - cmd := newTestCmd() - // Simulate user explicitly setting --format=sarif - if err := cmd.Flags().Set("format", ohFormatSARIF); err != nil { - t.Fatalf("failed to set flag: %v", err) - } - - cfg, err := ResolveOutput(cmd, outputPath, ohFormatSARIF, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - if cfg.Format != ohFormatSARIF { - t.Errorf("expected Format = %q (explicit), got %q", ohFormatSARIF, cfg.Format) - } - }) - - t.Run("creates output file", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "output.txt") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - // Verify file was created - if _, err := os.Stat(outputPath); os.IsNotExist(err) { - t.Error("expected output file to be created") - } - - // Verify we can write to it - _, err = cfg.Writer.Write([]byte("test content")) - if err != nil { - t.Errorf("Write() error = %v", err) - } - }) - - t.Run("creates parent directories", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "nested", "dir", "output.txt") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - // Verify nested directories and file were created - if _, err := os.Stat(outputPath); os.IsNotExist(err) { - t.Error("expected output file with nested directories to be created") - } - }) - - t.Run("cleanup resets outputToFile state", func(t *testing.T) { - // Force colors in auto mode so behavior depends on correct state reset - t.Setenv("CLICOLOR_FORCE", "1") - - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "output.txt") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - - // Call cleanup - cfg.Cleanup() - - // Verify state was reset by checking colors work in auto mode - // With CLICOLOR_FORCE=1 and outputToFile=false, colors should be enabled - cli.InitColors(cli.ColorModeAuto) - if !cli.ColorsEnabled() { - t.Error("expected outputToFile state to be reset after cleanup (colors should be enabled in auto mode with CLICOLOR_FORCE=1)") - } - }) - - t.Run("color=always skips color disabling", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "output.txt") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "always") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - // With --color=always, colors should remain enabled even when writing to file - cli.InitColors(cli.ColorModeAlways) - if !cli.ColorsEnabled() { - t.Error("expected colors to remain enabled with --color=always") - } - }) - - t.Run("returns error for invalid path", func(t *testing.T) { - // Create a file (not a directory) to trigger ENOTDIR when MkdirAll tries - // to create a directory at this path - portable across all platforms - tmpDir := t.TempDir() - blockingFile := filepath.Join(tmpDir, "notadir") - if err := os.WriteFile(blockingFile, []byte("x"), 0600); err != nil { - t.Fatalf("failed to create blocking file: %v", err) - } - // Try to create output file inside the file (ENOTDIR) - outputPath := filepath.Join(blockingFile, "output.txt") - - cmd := newTestCmd() - _, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err == nil { - t.Error("expected error for invalid path") - } - }) - - t.Run("resets state on error", func(t *testing.T) { - // Force colors in auto mode so behavior depends on correct state reset - t.Setenv("CLICOLOR_FORCE", "1") - - // Create a file (not a directory) to trigger ENOTDIR when MkdirAll tries - // to create a directory at this path - portable across all platforms - tmpDir := t.TempDir() - blockingFile := filepath.Join(tmpDir, "notadir") - if err := os.WriteFile(blockingFile, []byte("x"), 0600); err != nil { - t.Fatalf("failed to create blocking file: %v", err) - } - outputPath := filepath.Join(blockingFile, "output.txt") - - cmd := newTestCmd() - _, _ = ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - - // Verify state was reset by checking colors are enabled in auto mode - // With CLICOLOR_FORCE=1 and outputToFile=false, colors should be enabled - cli.InitColors(cli.ColorModeAuto) - if !cli.ColorsEnabled() { - t.Error("expected outputToFile state to be reset after error (colors should be enabled in auto mode with CLICOLOR_FORCE=1)") - } - }) - - t.Run("unrecognized extension keeps original format", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "results.txt") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - defer cfg.Cleanup() - - // .txt is not recognized, so format should stay as ohFormatHuman - if cfg.Format != ohFormatHuman { - t.Errorf("expected Format = %q (unrecognized extension), got %q", ohFormatHuman, cfg.Format) - } - }) - - t.Run("cleanup is idempotent", func(t *testing.T) { - tmpDir := t.TempDir() - outputPath := filepath.Join(tmpDir, "output.txt") - - cmd := newTestCmd() - cfg, err := ResolveOutput(cmd, outputPath, ohFormatHuman, "auto") - if err != nil { - t.Fatalf("ResolveOutput() error = %v", err) - } - - // Call cleanup multiple times - should not panic or error - cfg.Cleanup() - cfg.Cleanup() - cfg.Cleanup() - - // Verify the file was properly closed (we can write a new file at the same path) - // #nosec G304 -- test code using controlled temp directory path - fo, err := os.Create(outputPath) - if err != nil { - t.Fatalf("failed to create file after cleanup: %v", err) - } - _ = fo.Close() - }) -} diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 9d42cb32..bbf8ab60 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "os" - "strings" "sync" "time" @@ -319,31 +318,3 @@ func validatePageLimit(limit int) error { } return nil } - -// validSeverities contains the valid severity level strings for the --fail-on flag. -var validSeverities = []string{"INFO", "LOW", "MEDIUM", "HIGH", "CRITICAL"} - -func validateFailOn(severities []string) error { - validSet := make(map[string]bool) - for _, s := range validSeverities { - validSet[s] = true - } - - for i, sev := range severities { - // Normalize to uppercase for case-insensitive matching - upper := strings.ToUpper(sev) - if !validSet[upper] { - return fmt.Errorf("invalid severity level %q: must be one of %v", sev, validSeverities) - } - // Update the slice with normalized value - severities[i] = upper - } - return nil -} - -func getFailOn() ([]string, error) { - if err := validateFailOn(failOn); err != nil { - return nil, err - } - return failOn, nil -} diff --git a/internal/cmd/root_test.go b/internal/cmd/root_test.go index e24c5d18..1111e5fa 100644 --- a/internal/cmd/root_test.go +++ b/internal/cmd/root_test.go @@ -244,83 +244,9 @@ func TestGetPageLimit(t *testing.T) { }) } -func TestValidateFailOn(t *testing.T) { - tests := []struct { - name string - severities []string - wantErr bool - }{ - { - name: "valid single severity", - severities: []string{"CRITICAL"}, - wantErr: false, - }, - { - name: "valid multiple severities", - severities: []string{"HIGH", "CRITICAL"}, - wantErr: false, - }, - { - name: "valid all severities", - severities: []string{"INFO", "LOW", "MEDIUM", "HIGH", "CRITICAL"}, - wantErr: false, - }, - { - name: "valid severity lowercase", - severities: []string{"high"}, - wantErr: false, - }, - { - name: "invalid severity unknown", - severities: []string{"INVALID"}, - wantErr: true, - }, - { - name: "invalid mixed valid and invalid", - severities: []string{"HIGH", "invalid"}, - wantErr: true, - }, - { - name: "empty slice is valid", - severities: []string{}, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateFailOn(tt.severities) - if (err != nil) != tt.wantErr { - t.Errorf("validateFailOn(%v) error = %v, wantErr %v", tt.severities, err, tt.wantErr) - } - }) - } -} - -func TestGetFailOn(t *testing.T) { - t.Run("returns valid severities", func(t *testing.T) { - failOn = []string{"HIGH", "CRITICAL"} - defer func() { failOn = []string{"CRITICAL"} }() - - result, err := getFailOn() - if err != nil { - t.Fatalf("Expected no error, got %v", err) - } - if len(result) != 2 || result[0] != "HIGH" || result[1] != "CRITICAL" { - t.Errorf("Expected [HIGH CRITICAL], got %v", result) - } - }) - - t.Run("returns error for invalid severity", func(t *testing.T) { - failOn = []string{"invalid"} - defer func() { failOn = []string{"CRITICAL"} }() - - _, err := getFailOn() - if err == nil { - t.Error("Expected error for invalid severity") - } - }) -} +// Fail-on validation moved to internal/cmd/cmdutil (ValidateFailOn / GetFailOn); +// its unit tests live in cmdutil/failon_test.go. The supply-chain CI-gate +// regression that exercises the end-to-end path stays in supply_chain_test.go. func TestExecute(t *testing.T) { err := Execute() diff --git a/internal/cmd/scan_image.go b/internal/cmd/scan_image.go index 1fcb3d15..d9e78546 100644 --- a/internal/cmd/scan_image.go +++ b/internal/cmd/scan_image.go @@ -7,6 +7,7 @@ import ( "github.com/ArmisSecurity/armis-cli/internal/api" "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/model" "github.com/ArmisSecurity/armis-cli/internal/output" "github.com/ArmisSecurity/armis-cli/internal/scan" @@ -63,7 +64,7 @@ var scanImageCmd = &cobra.Command{ return err } - failOnSeverities, err := getFailOn() + failOnSeverities, err := cmdutil.GetFailOn(failOn) if err != nil { return err } @@ -126,7 +127,7 @@ var scanImageCmd = &cobra.Command{ } // Resolve output destination and format (handles file creation, format auto-detection, colors) - outputCfg, err := ResolveOutput(cmd, outputFile, format, colorFlag) + outputCfg, err := cmdutil.ResolveOutput(cmd, outputFile, format, colorFlag) if err != nil { return err } diff --git a/internal/cmd/scan_repo.go b/internal/cmd/scan_repo.go index bdc2507e..0893bf2f 100644 --- a/internal/cmd/scan_repo.go +++ b/internal/cmd/scan_repo.go @@ -9,6 +9,7 @@ import ( "github.com/ArmisSecurity/armis-cli/internal/api" "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/output" "github.com/ArmisSecurity/armis-cli/internal/scan" "github.com/ArmisSecurity/armis-cli/internal/scan/repo" @@ -64,7 +65,7 @@ var scanRepoCmd = &cobra.Command{ return err } - failOnSeverities, err := getFailOn() + failOnSeverities, err := cmdutil.GetFailOn(failOn) if err != nil { return err } @@ -164,7 +165,7 @@ var scanRepoCmd = &cobra.Command{ } // Resolve output destination and format (handles file creation, format auto-detection, colors) - outputCfg, err := ResolveOutput(cmd, outputFile, format, colorFlag) + outputCfg, err := cmdutil.ResolveOutput(cmd, outputFile, format, colorFlag) if err != nil { return err } diff --git a/internal/cmd/supply_chain.go b/internal/cmd/supply_chain.go index 9d347e41..2be74f05 100644 --- a/internal/cmd/supply_chain.go +++ b/internal/cmd/supply_chain.go @@ -46,7 +46,9 @@ are flagged or blocked to prevent supply chain attacks via typosquatting, compromised maintainer accounts, or dependency confusion. Supported ecosystems: - Node: npm, pnpm, bun, yarn (transparent proxy enforcement) + Node: npm, npx, pnpm, bun, yarn (transparent proxy enforcement) + npx enforces packages it fetches; a package already in the npx cache + or a binary in node_modules/.bin runs without a registry round-trip. Python: pip, uv (transparent proxy); poetry, pipenv, pdm (pre-install block) Java: maven (pom.xml), gradle (gradle.lockfile) (pre-install block) diff --git a/internal/cmd/supply_chain_check.go b/internal/cmd/supply_chain_check.go index c9d9591a..f16db496 100644 --- a/internal/cmd/supply_chain_check.go +++ b/internal/cmd/supply_chain_check.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/model" "github.com/ArmisSecurity/armis-cli/internal/output" "github.com/ArmisSecurity/armis-cli/internal/supplychain" @@ -172,7 +173,7 @@ func runSupplyChainCheck(cmd *cobra.Command, args []string) error { Summary: buildSummary(findings), } - outputCfg, err := ResolveOutput(cmd, outputFile, format, colorFlag) + outputCfg, err := cmdutil.ResolveOutput(cmd, outputFile, format, colorFlag) if err != nil { return err } @@ -190,11 +191,11 @@ func runSupplyChainCheck(cmd *cobra.Command, args []string) error { return fmt.Errorf("formatting output: %w", err) } - // Use getFailOn() (not the raw failOn global) so --fail-on is validated and - // case-normalized to uppercase. ShouldFail matches severities exactly, so a + // Use cmdutil.GetFailOn (not the raw failOn global) so --fail-on is validated + // and case-normalized to uppercase. ShouldFail matches severities exactly, so a // lowercase "medium" would otherwise never match a "MEDIUM" finding and the // CI gate would silently pass. The scan commands already route through here. - failOnSeverities, err := getFailOn() + failOnSeverities, err := cmdutil.GetFailOn(failOn) if err != nil { return err } diff --git a/internal/cmd/supply_chain_check_test.go b/internal/cmd/supply_chain_check_test.go index 0a0b2833..38c3fd5f 100644 --- a/internal/cmd/supply_chain_check_test.go +++ b/internal/cmd/supply_chain_check_test.go @@ -241,7 +241,7 @@ func TestRunSupplyChainCheck_EcosystemScopeSkips(t *testing.T) { scLockfile = "package-lock.json" scAll = true // skip base-lockfile git detection scMinAge = "72h" - format = ohFormatJSON + format = "json" cmd := newWrapTestCmd() // a command with a live context cmd.Flags().StringVar(&scMinAge, "min-age", "72h", "") diff --git a/internal/cmd/supply_chain_init.go b/internal/cmd/supply_chain_init.go index 44d7cd2f..2620eaac 100644 --- a/internal/cmd/supply_chain_init.go +++ b/internal/cmd/supply_chain_init.go @@ -5,9 +5,11 @@ import ( "fmt" "io" "os" + "sort" "strings" "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/output" "github.com/ArmisSecurity/armis-cli/internal/supplychain" "github.com/charmbracelet/huh" @@ -25,11 +27,14 @@ var scInitCmd = &cobra.Command{ Short: "Set up local package age enforcement", Long: `Configure your shell to enforce package release age policies during installations. -This wraps your package manager (auto-detected from lockfiles) so that armis-cli -can enforce age policies on package installations. Node PMs (npm, pnpm, bun, yarn) -and pip/uv use a transparent proxy that filters registry responses. poetry, pipenv, -pdm, mvn, and gradle use a pre-install check that blocks the build if violations -are found. +This wraps every supported package manager found on your PATH so that armis-cli +can enforce age policies on package installations. The shell functions are global +(they apply in every directory), so init wraps what is installed on the machine, +not just what a single project uses; whether enforcement actually runs is decided +per-project at install time from the nearest .armis-supply-chain.yaml. Node PMs +(npm, npx, pnpm, bun, yarn) and pip/uv use a transparent proxy that filters +registry responses. poetry, pipenv, pdm, mvn, and gradle use a pre-install check +that blocks the build if violations are found. Four modes are available: rc — Inject shell functions into ~/.bashrc / ~/.zshrc (default, interactive) @@ -80,15 +85,15 @@ func runSupplyChainInit(_ *cobra.Command, _ []string) error { // until that file exists, so it also skips PM detection. return runInitConfig() case "env", "rc": - pms, detected := detectWrappablePMs() + pms, installed := detectWrappablePMs() if len(pms) == 0 { - // Lockfiles were detected, but the config's `ecosystems` scope excludes - // every one — there is nothing in scope to wrap. Report it plainly - // instead of falling back to npm: wrapping an ecosystem the user - // deliberately scoped enforcement away from would modify their RC files - // (rc) or emit an eval they didn't ask for (env), contradicting init's - // stated "only wraps in-scope package managers" behavior. - return reportNothingInScope(detected) + // Package managers were found on PATH, but the config's `ecosystems` + // scope excludes every one — there is nothing in scope to wrap. Report + // it plainly instead of falling back to npm: wrapping an ecosystem the + // user deliberately scoped enforcement away from would modify their RC + // files (rc) or emit an eval they didn't ask for (env), contradicting + // init's stated "only wraps in-scope package managers" behavior. + return reportNothingInScope(installed) } if scInitMode == "rc" { return runInitRC(pms) @@ -99,57 +104,69 @@ func runSupplyChainInit(_ *cobra.Command, _ []string) error { } } -// reportNothingInScope explains that lockfiles were found but the config's -// `ecosystems` scope excludes all of them, so init has nothing to set up. It -// returns nil (not an error): scoping enforcement away from every detected -// ecosystem is a legitimate user choice, so exiting 0 with guidance is the -// correct, CI-friendly outcome. -func reportNothingInScope(detected []supplychain.DetectedEcosystem) error { +// reportNothingInScope explains that package managers were found on PATH but the +// config's `ecosystems` scope excludes all of them, so init has nothing to set +// up. It returns nil (not an error): scoping enforcement away from every +// installed ecosystem is a legitimate user choice, so exiting 0 with guidance is +// the correct, CI-friendly outcome. installed is the raw list of PM command +// names found on PATH (e.g. ["npm", "pip3"]). +func reportNothingInScope(installed []string) error { s := output.GetStyles() - seen := make(map[string]bool) - names := make([]string, 0, len(detected)) - for _, e := range detected { - n := string(e.Ecosystem) - if n == "" || seen[n] { - continue - } - seen[n] = true - names = append(names, n) - } - fmt.Fprintf(os.Stderr, "%s No package managers in scope to wrap.\n\n", s.WarningText.Render("⚠")) fmt.Fprintf(os.Stderr, "%s\n", s.MutedText.Render(fmt.Sprintf( - "Detected %s, but the `ecosystems` scope in %s excludes all of them.", - strings.Join(names, ", "), supplychain.ConfigFileName))) + "Found %s on PATH, but the `ecosystems` scope in %s excludes all of them.", + strings.Join(installed, ", "), supplychain.ConfigFileName))) fmt.Fprintf(os.Stderr, "%s\n", s.MutedText.Render( "Add one of them to that list (or remove the `ecosystems` key to enforce all) to set up enforcement.")) return nil } -// detectWrappablePMs returns the package managers init should wrap, plus the -// raw list of ecosystems that were detected. The two return values let the -// caller tell apart two cases that both yield an empty PM list: +// allSupportedPMs is the set of fixed package-manager command names init looks +// for on PATH. pip is intentionally omitted: its variants (pip, pip3, pip3.12) +// are matched by pattern inside DetectInstalledPMs, since each interpreter ships +// its own pip binary. npx is omitted too — it is not detected directly but paired +// with npm below, mirroring how the wrap gate treats it as part of the npm +// ecosystem. +var allSupportedPMs = []string{ + pmNPM, pmPNPM, pmBun, pmYarn, + pmUV, pmPoetry, pmPipenv, pmPDM, + pmMaven, pmGradle, +} + +// detectWrappablePMs returns the package managers init should wrap, plus the raw +// list of package-manager commands found on PATH. Detection is PATH-based, not +// lockfile-based: the shell functions init injects are global (they shadow the +// command in every directory), so the wrapper set should reflect what is +// installed on the machine rather than which lockfiles happen to sit in the +// current directory. Per-project enforcement is still decided dynamically at wrap +// time from the nearest config and policy. // -// - No lockfiles at all (detected is empty): DetectEcosystems errored, so we -// fall back to npm and the returned PM slice is non-empty. -// - Lockfiles present but all scoped out (detected is non-empty, PM slice -// empty): the config's `ecosystems` key excludes every detected ecosystem. -// The caller reports this instead of wrapping something out of scope. -func detectWrappablePMs() (pms []string, detected []supplychain.DetectedEcosystem) { - ecosystems, err := supplychain.DetectEcosystems(".") - if err != nil { - // DetectEcosystems errors when no supported lockfile is present, or when - // a lockfile exists but can't be stat'd (permissions/I/O). Either way, - // default to npm so the generated wrapper still protects the most common - // package manager rather than silently wrapping nothing. detected stays - // empty, signaling "no lockfiles" rather than "scoped out". - return []string{pmNPM}, nil +// The two return values let the caller tell apart two cases that both yield an +// empty PM list: +// +// - Nothing installed (installed is empty): no supported PM is on PATH, so we +// fall back to npm (and its runner npx) and the returned PM slice is +// non-empty. +// - PMs installed but all scoped out (installed is non-empty, PM slice empty): +// the config's `ecosystems` key excludes every installed ecosystem. The +// caller reports this instead of wrapping something out of scope. +func detectWrappablePMs() (pms []string, installed []string) { + installed = supplychain.DetectInstalledPMs(allSupportedPMs) + if len(installed) == 0 { + // No supported package manager is on PATH. Default to npm (and its runner + // npx) so the generated wrapper still protects the most common package + // manager rather than silently wrapping nothing — and so a one-off + // `npx ` in a machine where npm was not on the scanned PATH (e.g. a + // minimal CI image generating an eval block) is still guarded. installed + // stays empty, signaling "nothing found" rather than "scoped out". + return []string{pmNPM, pmNPX}, nil } // Honor the config's "ecosystems" scope so init only wraps the package - // managers the user opted to enforce. Loaded best-effort: a nil config (none - // found, or load error) enforces everything, matching the check/wrap gates. + // managers the user opted to enforce. Loaded best-effort from the current + // directory upward: a nil config (none found, or load error) enforces + // everything, matching the check/wrap gates. var cfg *supplychain.Config if dir := supplychain.FindConfigDir("."); dir != "" { cfg, _ = supplychain.LoadConfig(dir) @@ -165,58 +182,90 @@ func detectWrappablePMs() (pms []string, detected []supplychain.DetectedEcosyste pms = append(pms, pm) } - for _, e := range ecosystems { + for _, pm := range installed { + // Map each command back to its ecosystem so the config scope applies. pip + // variants (pip3, pip3.12) canonicalize to the pip ecosystem; an unknown + // name (shouldn't happen — installed comes from our own scan) maps to "" and + // is enforced by default. + eco := pmToEcosystem(canonicalPM(pm)) // armis:ignore cwe:476 reason:EnforcesEcosystem has an explicit nil-receiver guard (returns true when c==nil), so calling it on a nil cfg is safe by design - if !cfg.EnforcesEcosystem(e.Ecosystem) { - continue - } - pm := ecosystemToPM(e.Ecosystem) - // A bare requirements.txt is installed with pip, which can be present - // under several names (pip, pip3, pip3.12). Wrap every variant on PATH - // so enforcement holds regardless of which one the user invokes. - if pm == pmPip { - for _, variant := range supplychain.DetectPipVariants() { - addPM(variant) - } + if eco != "" && !cfg.EnforcesEcosystem(eco) { continue } addPM(pm) } - // An empty pms here means every detected ecosystem was scoped out (or none - // mapped to a PM). We deliberately do NOT fall back to npm: the caller uses - // the non-empty `ecosystems` to report that nothing is in scope, rather than - // wrapping a package manager the user asked enforcement to skip. - return pms, ecosystems + // npx ships with npm and resolves from the same registry, so wherever npm is + // wrapped its runner should be too: `npx ` downloads and executes a + // package on demand, exactly the supply-chain vector the proxy guards. Pair it + // here (after scoping) so it inherits npm's in-scope decision — when npm was + // excluded by the `ecosystems` config it stays out, keeping the two in lockstep. + if seen[pmNPM] { + addPM(pmNPX) + } + + // An empty pms here means every installed PM was scoped out. We deliberately do + // NOT fall back to npm: the caller uses the non-empty `installed` list to report + // that nothing is in scope, rather than wrapping a package manager the user + // asked enforcement to skip. + return pms, installed } -func ecosystemToPM(eco supplychain.Ecosystem) string { - switch eco { - case supplychain.EcosystemNPM: - return pmNPM - case supplychain.EcosystemPNPM: - return pmPNPM - case supplychain.EcosystemBun: - return pmBun - case supplychain.EcosystemYarn: - return pmYarn - case supplychain.EcosystemPip: - return pmPip - case supplychain.EcosystemPoetry: - return pmPoetry - case supplychain.EcosystemPipfile: - return pmPipenv - case supplychain.EcosystemPDM: - return pmPDM - case supplychain.EcosystemUV: - return pmUV - case supplychain.EcosystemMaven: - return pmMaven - case supplychain.EcosystemGradle: - return pmGradle - default: - return "" +// summarizeDetectedPMs renders the wrapped package managers as a compact, +// human-readable summary for the init preview. It exists to answer the obvious +// question the raw wrapper block raises — "were these detected, or did init just +// add everything it supports?" — by surfacing exactly what was found on PATH: +// +// - pip's interpreter-specific variants (pip3, pip3.11, pip3.12, …) collapse +// into a single "pip (N variants)" entry. A multi-Python machine can expose a +// dozen of these; listing each by name would bury the summary even though they +// all resolve to PyPI and share one policy. The verbatim wrapper block below +// still names every variant, so no information is lost — this is the digest. +// - npx is annotated "(paired with npm)" rather than listed as a plain find, +// because it is the one entry init adds without detecting it: it ships with +// npm and is wrapped wherever npm is in scope (see detectWrappablePMs). Making +// that explicit keeps the summary honest about what was on PATH vs. inferred. +// +// Remaining names are shown as-is, bolded, in sorted order so the line is stable +// regardless of PATH scan order or where npx was appended. +func summarizeDetectedPMs(s *output.Styles, pms []string) string { + var others []string + pipVariants := 0 + hasNPX := false + + for _, pm := range pms { + switch { + case pm == pmNPX: + hasNPX = true + case supplychain.IsPipVariant(pm): + pipVariants++ + default: + others = append(others, pm) + } + } + sort.Strings(others) + + parts := make([]string, 0, len(others)+2) + for _, pm := range others { + parts = append(parts, s.Bold.Render(pm)) } + if pipVariants > 0 { + label := s.Bold.Render(pmPip) + // Only flag the variant count when there is more than one binary; a lone + // "pip" needs no "(1 variant)" noise. + if pipVariants > 1 { + label += s.MutedText.Render(fmt.Sprintf(" (%d variants)", pipVariants)) + } + parts = append(parts, label) + } + if hasNPX { + parts = append(parts, s.Bold.Render(pmNPX)+s.MutedText.Render(" (paired with npm)")) + } + + // Keep pip and npx at the end of the line: pip carries a parenthetical and npx + // is the inferred entry, so trailing them reads more naturally than interleaving + // by alphabetical position. + return strings.Join(parts, ", ") } // promptYesNo asks the user a yes/no question and reports their answer. @@ -257,7 +306,7 @@ func confirmInteractive(prompt string, defaultYes bool) bool { Negative("No"). Value(&confirmed), ), - ).WithTheme(getInstallTheme()).WithAccessible(!cli.ColorsEnabled()) + ).WithTheme(cmdutil.GetInstallTheme()).WithAccessible(!cli.ColorsEnabled()) if err := form.Run(); err != nil { return false @@ -368,12 +417,39 @@ func runInitRC(pms []string) error { return fmt.Errorf("no supported shells detected (bash, zsh, or fish)") } + // Short-circuit: if every detected shell already has the exact wrapper we + // would inject (same binary path, same PM set), nothing needs to change. + if !scInitDryRun { + alreadyDone := true + for _, sh := range shells { + if !supplychain.HasCurrentInjection(sh.RCFile, sh.Name, pms) { + alreadyDone = false + break + } + } + if alreadyDone { + fmt.Fprintf(os.Stderr, "%s Supply chain wrappers already configured — nothing to do.\n", s.SuccessText.Render(output.IconSuccess)) + fmt.Fprintf(os.Stderr, "%s %s\n", s.MutedText.Render("Undo: "), s.Bold.Render("armis-cli supply-chain uninit")) + return nil + } + } + fmt.Fprintf(os.Stderr, "%s ", s.MutedText.Render("Detected shell(s):")) names := make([]string, 0, len(shells)) for _, sh := range shells { names = append(names, s.Bold.Render(sh.Name)+" ("+sh.RCFile+")") } - fmt.Fprintf(os.Stderr, "%s\n\n", strings.Join(names, ", ")) + fmt.Fprintf(os.Stderr, "%s\n", strings.Join(names, ", ")) + + // Mirror the shell line with a package-manager line so the user can see what + // init found on their PATH before the wrapper preview, rather than inferring it + // from the function block. This is the answer to "did we detect these or just + // add everything we support?": only PMs actually on PATH appear, npx is marked + // as paired (it ships with npm; it is not detected), and pip's interpreter + // variants are folded into one entry so a multi-Python machine does not bury + // the summary under a dozen near-identical names. + fmt.Fprintf(os.Stderr, "%s %s\n\n", + s.MutedText.Render("Detected package manager(s):"), summarizeDetectedPMs(s, pms)) // Preview each distinct wrapper. bash/zsh share the posix wrapper while fish // uses different syntax, so group shells by the wrapper they produce to keep @@ -382,6 +458,7 @@ func runInitRC(pms []string) error { var order []string shellsByWrapper := make(map[string][]string) for _, sh := range shells { + // armis:ignore cwe:78 cwe:77 reason:pms flows through sanitizePMNames inside GenerateWrapper (^[a-z][a-z0-9-]*(\.[0-9]+)?$), so no shell metacharacter can reach the generated wrapper string w := supplychain.GenerateWrapper(sh.Name, pms) if _, seen := shellsByWrapper[w]; !seen { order = append(order, w) @@ -405,6 +482,7 @@ func runInitRC(pms []string) error { } } + // armis:ignore cwe:78 cwe:77 reason:pms flows through sanitizePMNames inside InjectFunctions/GenerateWrapper (^[a-z][a-z0-9-]*(\.[0-9]+)?$), so no shell metacharacter can reach the generated wrapper or RC file modified, err := supplychain.InjectFunctions(shells, pms) if err != nil { return err diff --git a/internal/cmd/supply_chain_init_test.go b/internal/cmd/supply_chain_init_test.go index 6ff19106..fd5c2e99 100644 --- a/internal/cmd/supply_chain_init_test.go +++ b/internal/cmd/supply_chain_init_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/ArmisSecurity/armis-cli/internal/output" "github.com/ArmisSecurity/armis-cli/internal/supplychain" ) @@ -82,77 +83,235 @@ func (c *countingReader) Read(p []byte) (int, error) { return n, err } -func TestDetectWrappablePMs_DefaultsToNpm(t *testing.T) { - // In a directory with no lockfiles, DetectEcosystems errors; detectWrappablePMs - // must fall back to npm rather than silently wrapping nothing. +// seedPMsOnPath creates a fresh dir, writes an executable stub for each given +// package-manager command name, and points $PATH at only that dir for the test. +// detectWrappablePMs scans $PATH (not lockfiles), so this is how a test controls +// which package managers it "finds installed". Restricting PATH to the seeded +// dir keeps detection deterministic — a real npm/pip on the developer's machine +// can't leak into the result. +func seedPMsOnPath(t *testing.T, names ...string) { + t.Helper() dir := t.TempDir() - cwd, err := os.Getwd() - if err != nil { - t.Fatalf("getwd: %v", err) + for _, name := range names { + // 0o755 so the Unix execute-bit filter in scanPathExecutables accepts it; + // the bit is ignored on Windows, where the file's mere presence suffices. + if err := os.WriteFile(filepath.Join(dir, name), []byte{}, 0o755); err != nil { //nolint:gosec // test stub on an isolated PATH + t.Fatalf("seed %s: %v", name, err) + } } - defer os.Chdir(cwd) //nolint:errcheck - if err := os.Chdir(dir); err != nil { - t.Fatalf("chdir: %v", err) + t.Setenv("PATH", dir) +} + +func TestDetectWrappablePMs_DefaultsToNpm(t *testing.T) { + // With no supported package manager on PATH, detectWrappablePMs must fall back + // to npm (and its runner npx) rather than silently wrapping nothing — a one-off + // `npx ` is exactly the case worth guarding even on a bare machine. + seedPMsOnPath(t) // empty PATH dir: nothing installed + chdirTemp(t) // isolated cwd so no stray config scopes the result + + pms, installed := detectWrappablePMs() + if len(pms) != 2 || pms[0] != "npm" || pms[1] != "npx" { + t.Errorf("detectWrappablePMs() = %v, want [npm npx]", pms) + } + // Nothing on PATH means installed is empty — this is how the caller + // distinguishes "nothing found" (npm fallback) from "scoped out" (nothing in + // scope). + if len(installed) != 0 { + t.Errorf("detectWrappablePMs() installed = %v, want empty (nothing on PATH)", installed) } +} - pms, detected := detectWrappablePMs() - if len(pms) != 1 || pms[0] != "npm" { - t.Errorf("detectWrappablePMs() = %v, want [npm]", pms) +// containsPM reports whether name is in the PM slice. Used by the npx pairing +// tests, which care about presence/absence rather than slice position. +func containsPM(pms []string, name string) bool { + for _, p := range pms { + if p == name { + return true + } } - // No lockfiles means DetectEcosystems errored, so the detected list is empty — - // this is how the caller distinguishes "no lockfiles" (npm fallback) from - // "scoped out" (nothing in scope). - if len(detected) != 0 { - t.Errorf("detectWrappablePMs() detected = %v, want empty (no lockfiles)", detected) + return false +} + +func TestDetectWrappablePMs_PairsNpxWithNpm(t *testing.T) { + // npm on PATH puts the npm ecosystem in scope; npx must be wrapped alongside it + // so ad-hoc `npx ` runs are filtered through the same proxy. + seedPMsOnPath(t, pmNPM) + chdirTemp(t) + + pms, _ := detectWrappablePMs() + if !containsPM(pms, pmNPM) || !containsPM(pms, pmNPX) { + t.Errorf("detectWrappablePMs() = %v, want both npm and npx", pms) } } -func TestDetectWrappablePMs_HonorsEcosystemScope(t *testing.T) { - // Two lockfiles are present (npm + pnpm) but the config scopes enforcement to - // pnpm only, so init must wrap only pnpm. +func TestDetectWrappablePMs_NpxNotPairedWithoutNpm(t *testing.T) { + // A machine with poetry but no npm never wraps npm, so npx must not appear: + // the runner is paired only where npm itself is in scope. + seedPMsOnPath(t, pmPoetry) + chdirTemp(t) + + pms, _ := detectWrappablePMs() + if containsPM(pms, pmNPX) { + t.Errorf("detectWrappablePMs() = %v, must not contain npx without npm", pms) + } +} + +func TestDetectWrappablePMs_NpxAbsentWhenNpmScopedOut(t *testing.T) { + // npx is paired AFTER ecosystem scoping, so it must inherit npm's exclusion: + // with npm and pnpm both on PATH but the config scoping enforcement to pnpm + // only, npm is out of scope — and npx must follow it out, never wrapped on its + // own. This pins the invariant explicitly; TestDetectWrappablePMs_HonorsEcosystemScope + // only covers it implicitly via a length check that predates npx. + seedPMsOnPath(t, pmNPM, pmPNPM) dir := chdirTemp(t) - for _, f := range []string{"package-lock.json", "pnpm-lock.yaml"} { - if err := os.WriteFile(filepath.Join(dir, f), []byte("{}"), 0o600); err != nil { - t.Fatal(err) - } + if err := os.WriteFile(filepath.Join(dir, supplychain.ConfigFileName), + []byte("version: 1\necosystems:\n - pnpm\n"), 0o600); err != nil { + t.Fatal(err) + } + + pms, _ := detectWrappablePMs() + if containsPM(pms, pmNPX) { + t.Errorf("detectWrappablePMs() = %v, must not contain npx when npm is scoped out", pms) } + if !containsPM(pms, pmPNPM) { + t.Errorf("detectWrappablePMs() = %v, want pnpm (the in-scope PM)", pms) + } +} + +func TestDetectWrappablePMs_HonorsEcosystemScope(t *testing.T) { + // npm and pnpm are both on PATH but the config scopes enforcement to pnpm only, + // so init must wrap only pnpm. + seedPMsOnPath(t, pmNPM, pmPNPM) + dir := chdirTemp(t) if err := os.WriteFile(filepath.Join(dir, supplychain.ConfigFileName), []byte("version: 1\necosystems:\n - pnpm\n"), 0o600); err != nil { t.Fatal(err) } - pms, detected := detectWrappablePMs() + pms, installed := detectWrappablePMs() if len(pms) != 1 || pms[0] != "pnpm" { t.Errorf("detectWrappablePMs() = %v, want [pnpm] (npm excluded by config scope)", pms) } - // Both lockfiles are still reported as detected; scoping only narrows the PM - // list, not what was found on disk. - if len(detected) != 2 { - t.Errorf("detectWrappablePMs() detected = %v, want 2 ecosystems (npm + pnpm)", detected) + // Both PMs are still reported as installed; scoping only narrows the wrapped + // list, not what was found on PATH. + if len(installed) != 2 { + t.Errorf("detectWrappablePMs() installed = %v, want 2 PMs (npm + pnpm)", installed) } } // TestDetectWrappablePMs_AllScopedOut covers the case the npm fallback used to -// mask: lockfiles exist but the config scopes enforcement away from every one. -// The PM list must come back empty (so the caller can report "nothing in -// scope") while the detected list still names what was found. +// mask: package managers are installed but the config scopes enforcement away +// from every one. The PM list must come back empty (so the caller can report +// "nothing in scope") while the installed list still names what was found. func TestDetectWrappablePMs_AllScopedOut(t *testing.T) { + seedPMsOnPath(t, pmNPM) dir := chdirTemp(t) - if err := os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte("{}"), 0o600); err != nil { - t.Fatal(err) - } - // Scope enforcement to pip only; the sole detected ecosystem (npm) is excluded. + // Scope enforcement to pip only; the sole installed PM (npm) is excluded. if err := os.WriteFile(filepath.Join(dir, supplychain.ConfigFileName), []byte("version: 1\necosystems:\n - pip\n"), 0o600); err != nil { t.Fatal(err) } - pms, detected := detectWrappablePMs() + pms, installed := detectWrappablePMs() if len(pms) != 0 { t.Errorf("detectWrappablePMs() = %v, want empty (npm scoped out, no npm fallback)", pms) } - if len(detected) != 1 || detected[0].Ecosystem != supplychain.EcosystemNPM { - t.Errorf("detectWrappablePMs() detected = %v, want [npm]", detected) + if len(installed) != 1 || installed[0] != pmNPM { + t.Errorf("detectWrappablePMs() installed = %v, want [npm]", installed) + } +} + +// TestDetectWrappablePMs_WrapsPipWhenInstalled is the regression for the bug that +// motivated PATH-based detection: pip on PATH must be wrapped even when the cwd +// has no Python lockfile (e.g. running init from a Go repo). Lockfile-based +// detection used to fall back to npm-only here, silently leaving every later +// `pip install` in any directory unguarded. +func TestDetectWrappablePMs_WrapsPipWhenInstalled(t *testing.T) { + seedPMsOnPath(t, pmPip) + chdirTemp(t) // no lockfile, no config — the exact "Go repo" scenario + + pms, _ := detectWrappablePMs() + if !containsPM(pms, pmPip) { + t.Errorf("detectWrappablePMs() = %v, want pip wrapped (it is on PATH)", pms) + } +} + +// TestDetectWrappablePMs_WrapsAllInstalled verifies init wraps every supported +// PM found on PATH, mixing Node and Python tools, regardless of the cwd's +// lockfiles. npx is paired in because npm is present. +func TestDetectWrappablePMs_WrapsAllInstalled(t *testing.T) { + seedPMsOnPath(t, pmNPM, pmYarn, pmPip, pmPoetry) + chdirTemp(t) + + pms, _ := detectWrappablePMs() + for _, want := range []string{pmNPM, pmNPX, pmYarn, pmPip, pmPoetry} { + if !containsPM(pms, want) { + t.Errorf("detectWrappablePMs() = %v, want %q wrapped", pms, want) + } + } +} + +// TestDetectWrappablePMs_WrapsPipVariants verifies that versioned pip binaries +// (pip3, pip3.12) are each wrapped: a shell function only shadows the exact name +// the user types, so every interpreter's pip must get its own wrapper. +func TestDetectWrappablePMs_WrapsPipVariants(t *testing.T) { + seedPMsOnPath(t, "pip3", "pip3.12") + chdirTemp(t) + + pms, _ := detectWrappablePMs() + for _, want := range []string{"pip3", "pip3.12"} { + if !containsPM(pms, want) { + t.Errorf("detectWrappablePMs() = %v, want pip variant %q wrapped", pms, want) + } + } +} + +// TestSummarizeDetectedPMs covers the init preview summary line. forceNoColor +// (from supply_chain_wrap_summary_test.go, same package) pins the plain style +// set so the rendered string can be matched without ANSI escapes. +func TestSummarizeDetectedPMs(t *testing.T) { + forceNoColor(t) + s := output.GetStyles() + + tests := []struct { + name string + pms []string + want string + }{ + { + // The exact shape that prompted the change: many pip variants must + // collapse to one "pip (N variants)" entry, and npx must be marked as + // paired rather than listed as a bare detection. + name: "pip variants collapse and npx is annotated", + pms: []string{"bun", "npm", "pnpm", "poetry", "uv", "pip", "pip3", "pip3.10", "pip3.11", "pip3.12", "npx"}, + want: "bun, npm, pnpm, poetry, uv, pip (5 variants), npx (paired with npm)", + }, + { + // A single pip binary carries no variant count — "(1 variant)" would be + // noise. + name: "single pip has no variant count", + pms: []string{"npm", "pip", "npx"}, + want: "npm, pip, npx (paired with npm)", + }, + { + // No npm means no npx; non-pip names sort alphabetically. + name: "no npx without npm", + pms: []string{"poetry", "bun"}, + want: "bun, poetry", + }, + { + name: "pip only", + pms: []string{"pip3", "pip3.12"}, + want: "pip (2 variants)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := summarizeDetectedPMs(s, tt.pms); got != tt.want { + t.Errorf("summarizeDetectedPMs(%v) = %q, want %q", tt.pms, got, tt.want) + } + }) } } diff --git a/internal/cmd/supply_chain_test.go b/internal/cmd/supply_chain_test.go index 4637ea00..0369f8ed 100644 --- a/internal/cmd/supply_chain_test.go +++ b/internal/cmd/supply_chain_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/model" "github.com/ArmisSecurity/armis-cli/internal/output" ) @@ -40,10 +41,12 @@ func TestSupplyChainUnknownSubcommand(t *testing.T) { } // TestSupplyChainCheckFailOnCaseInsensitive is the regression test for the -// CI-gate bypass: `supply-chain check` routes --fail-on through getFailOn(), -// which uppercases and validates it. A lowercase "medium" must therefore trip -// the gate on a MEDIUM finding (ShouldFail matches severities exactly), and an -// invalid value must be rejected rather than silently ignored. +// CI-gate bypass: `supply-chain check` routes --fail-on through +// cmdutil.GetFailOn, which uppercases and validates it. A lowercase "medium" +// must therefore trip the gate on a MEDIUM finding (ShouldFail matches +// severities exactly), and an invalid value must be rejected rather than +// silently ignored. It feeds the failOn global (the bound flag value) through +// the same call the command makes, end-to-end into output.ShouldFail. func TestSupplyChainCheckFailOnCaseInsensitive(t *testing.T) { medium := &model.ScanResult{ Findings: []model.Finding{{Severity: model.SeverityMedium}}, @@ -51,9 +54,9 @@ func TestSupplyChainCheckFailOnCaseInsensitive(t *testing.T) { t.Run("lowercase fail-on still fails the gate", func(t *testing.T) { failOn = []string{"medium"} - normalized, err := getFailOn() + normalized, err := cmdutil.GetFailOn(failOn) if err != nil { - t.Fatalf("getFailOn rejected a valid lowercase severity: %v", err) + t.Fatalf("GetFailOn rejected a valid lowercase severity: %v", err) } if !output.ShouldFail(medium, normalized) { t.Error("lowercase --fail-on medium should fail on a MEDIUM finding after normalization") @@ -62,8 +65,8 @@ func TestSupplyChainCheckFailOnCaseInsensitive(t *testing.T) { t.Run("invalid fail-on is rejected", func(t *testing.T) { failOn = []string{"banana"} - if _, err := getFailOn(); err == nil { - t.Error("getFailOn should reject an invalid severity, not silently ignore it") + if _, err := cmdutil.GetFailOn(failOn); err == nil { + t.Error("GetFailOn should reject an invalid severity, not silently ignore it") } }) diff --git a/internal/cmd/supply_chain_wrap_pm_test.go b/internal/cmd/supply_chain_wrap_pm_test.go index f25370d4..c80b3cb1 100644 --- a/internal/cmd/supply_chain_wrap_pm_test.go +++ b/internal/cmd/supply_chain_wrap_pm_test.go @@ -56,6 +56,9 @@ func TestRequiresPreInstallBlock(t *testing.T) { {pmPip, false}, {pmUV, false}, {pmNPM, false}, + // npx is the npm runner: like npm it uses the transparent proxy, never the + // pre-install lockfile audit (it has no lockfile of its own). + {pmNPX, false}, {pmPNPM, false}, {pmBun, false}, {pmYarn, false}, @@ -81,10 +84,13 @@ func TestPmToEcosystem(t *testing.T) { {pmMaven, supplychain.EcosystemMaven}, {pmGradle, supplychain.EcosystemGradle}, // pmToEcosystem maps every supported PM to its ecosystem — the proxied - // ones (npm/pnpm/bun/yarn/pip/uv) as well as the pre-install ones — so the + // ones (npm/npx/pnpm/bun/yarn/pip/uv) as well as the pre-install ones — so the // config "ecosystems" scoping gate can classify any wrapped PM. Pass the // canonical name; a versioned pip variant resolves to pip via canonicalPM. {pmNPM, supplychain.EcosystemNPM}, + // npx maps to the npm ecosystem so the config "ecosystems" scoping gate + // treats it exactly like npm (scoping npm in/out includes npx too). + {pmNPX, supplychain.EcosystemNPM}, {pmPNPM, supplychain.EcosystemPNPM}, {pmBun, supplychain.EcosystemBun}, {pmYarn, supplychain.EcosystemYarn}, @@ -116,6 +122,8 @@ func TestRegistryEnvForPM(t *testing.T) { wantVal string }{ {pmNPM, "npm_config_registry", url}, + // npx resolves from the npm registry, so it gets the same env override as npm. + {pmNPX, "npm_config_registry", url}, {pmPNPM, "npm_config_registry", url}, {pmBun, "BUN_CONFIG_REGISTRY", url}, {pmYarn, "YARN_NPM_REGISTRY_SERVER", url}, diff --git a/internal/cmd/uninstall.go b/internal/cmd/uninstall.go index b6e5a47d..4da9d97d 100644 --- a/internal/cmd/uninstall.go +++ b/internal/cmd/uninstall.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/ArmisSecurity/armis-cli/internal/cli" + "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/install" "github.com/charmbracelet/lipgloss" "github.com/spf13/cobra" @@ -67,11 +68,11 @@ func uninstallAll(u *install.Uninstaller, keepCreds, force bool) error { var titleStyle, separatorStyle, successMark, warnMark, dimStyle lipgloss.Style if styled { - titleStyle = lipgloss.NewStyle().Bold(true).Foreground(brandAccent) - separatorStyle = lipgloss.NewStyle().Foreground(brandSeparator) - successMark = lipgloss.NewStyle().Foreground(brandSuccess) - warnMark = lipgloss.NewStyle().Foreground(brandWarn) - dimStyle = lipgloss.NewStyle().Foreground(brandMuted) + titleStyle = lipgloss.NewStyle().Bold(true).Foreground(cmdutil.BrandAccent) + separatorStyle = lipgloss.NewStyle().Foreground(cmdutil.BrandSeparator) + successMark = lipgloss.NewStyle().Foreground(cmdutil.BrandSuccess) + warnMark = lipgloss.NewStyle().Foreground(cmdutil.BrandWarn) + dimStyle = lipgloss.NewStyle().Foreground(cmdutil.BrandMuted) } if !force { @@ -194,10 +195,10 @@ func uninstallTargets(u *install.Uninstaller, targets []string) error { var successMark, failMark, warnMark, dimStyle lipgloss.Style if styled { - successMark = lipgloss.NewStyle().Foreground(brandSuccess) - failMark = lipgloss.NewStyle().Foreground(brandError) - warnMark = lipgloss.NewStyle().Foreground(brandWarn) - dimStyle = lipgloss.NewStyle().Foreground(brandMuted) + successMark = lipgloss.NewStyle().Foreground(cmdutil.BrandSuccess) + failMark = lipgloss.NewStyle().Foreground(cmdutil.BrandError) + warnMark = lipgloss.NewStyle().Foreground(cmdutil.BrandWarn) + dimStyle = lipgloss.NewStyle().Foreground(cmdutil.BrandMuted) } printSuccess := func(msg string) { diff --git a/internal/supplychain/shell_test.go b/internal/supplychain/shell_test.go index 75254b04..99b0a8d1 100644 --- a/internal/supplychain/shell_test.go +++ b/internal/supplychain/shell_test.go @@ -275,6 +275,31 @@ func TestHasInjection(t *testing.T) { } } +func TestHasCurrentInjection(t *testing.T) { + tmpDir := t.TempDir() + rcFile := filepath.Join(tmpDir, ".bashrc") + + os.WriteFile(rcFile, []byte("# empty\n"), 0o644) //nolint:errcheck,gosec + + // Before injection, neither HasInjection nor HasCurrentInjection match. + if HasCurrentInjection(rcFile, "bash", []string{"npm"}) { + t.Error("should return false for clean file") + } + + shells := []Shell{{Name: "bash", RCFile: rcFile}} + InjectFunctions(shells, []string{"npm"}) //nolint:errcheck,gosec + + // After injection the exact wrapper is present. + if !HasCurrentInjection(rcFile, "bash", []string{"npm"}) { + t.Error("should return true after injecting npm wrapper for bash") + } + + // A different PM set does not match the already-injected block. + if HasCurrentInjection(rcFile, "bash", []string{"npm", "pnpm"}) { + t.Error("should return false when PM set differs from what was injected") + } +} + func TestEvalCommand(t *testing.T) { cmd := EvalCommand([]string{"npm"}) if !strings.Contains(cmd, markerStart) { From 5401a14153908e2910fa8444fc5bf9494aafec64 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 11:33:51 +0200 Subject: [PATCH 3/8] [PPSC-920] fix(lint): address CI golangci-lint v2.10.1 failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- internal/cmd/scan_repo_test.go | 2 +- internal/supplychain/shell.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cmd/scan_repo_test.go b/internal/cmd/scan_repo_test.go index 2805808e..54c2b380 100644 --- a/internal/cmd/scan_repo_test.go +++ b/internal/cmd/scan_repo_test.go @@ -61,7 +61,7 @@ func TestScanRepoRunE_SuccessfulScan(t *testing.T) { tenantID = testTenantID clientID = "" clientSecret = "" - format = "json" + format = agentFormatJSON colorFlag = testColorNever themeFlag = themeAuto noUpdateCheck = true diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 00e1af0f..11b07eb8 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -298,7 +298,7 @@ func HasCurrentInjection(path, shell string, pms []string) bool { } func fileExists(path string) bool { - _, err := os.Stat(path) + _, err := os.Stat(path) //nolint:gosec // path is a shell RC file under the user's own $HOME (see DetectShells) return err == nil } From 50569bc20c7fec76f6c71ec450b6c01e966eb8cf Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 11:39:41 +0200 Subject: [PATCH 4/8] [PPSC-920] fix(supply-chain): address PR review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- internal/cmd/supply_chain_init.go | 2 +- internal/supplychain/shell.go | 59 ++++++++++++++++++++++--------- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/internal/cmd/supply_chain_init.go b/internal/cmd/supply_chain_init.go index 2620eaac..c408fbda 100644 --- a/internal/cmd/supply_chain_init.go +++ b/internal/cmd/supply_chain_init.go @@ -449,7 +449,7 @@ func runInitRC(pms []string) error { // variants are folded into one entry so a multi-Python machine does not bury // the summary under a dozen near-identical names. fmt.Fprintf(os.Stderr, "%s %s\n\n", - s.MutedText.Render("Detected package manager(s):"), summarizeDetectedPMs(s, pms)) + s.MutedText.Render("Package manager(s) to wrap:"), summarizeDetectedPMs(s, pms)) // Preview each distinct wrapper. bash/zsh share the posix wrapper while fish // uses different syntax, so group shells by the wrapper they produce to keep diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 11b07eb8..0d354373 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -4,6 +4,7 @@ package supplychain import ( "fmt" "os" + "os/exec" "path/filepath" "regexp" "runtime" @@ -383,6 +384,14 @@ func scanPathExecutables(match func(name string) bool) []string { continue } name := entry.Name() + // On Windows, executables carry extensions from PATHEXT (typically + // .exe, .cmd, .bat). Strip any known extension so that pip.exe and + // pip3.cmd match the same patterns as bare pip / pip3 on Unix. + if runtime.GOOS == goosWindows { + if ext := filepath.Ext(name); ext != "" { + name = strings.TrimSuffix(name, ext) + } + } if !match(name) { continue } @@ -432,27 +441,43 @@ func DetectPipVariants() []string { return variants } -// DetectInstalledPMs scans $PATH for every supported package manager actually -// installed and returns the deduplicated, sorted set of command names found. -// names is the list of fixed package-manager command names to look for (npm, -// pnpm, bun, mvn, …); pip variants (pip, pip3, pip3.12) are matched separately -// via the pip pattern, since pip installs under several interpreter-specific -// names and each must be wrapped independently. +// DetectInstalledPMs returns the deduplicated, sorted set of supported package +// managers found on $PATH. // -// `supply-chain init` uses this rather than CWD lockfile detection because the -// shell wrappers it injects are global — they shadow the package-manager command -// in every directory, not just the project init happened to run in. Per-project -// enforcement is still decided dynamically at wrap time (the ecosystems config -// and policy are re-read on each invocation), so the wrapper set should reflect -// what is installed on the machine, not which lockfiles sit in one directory. +// Fixed names (npm, pnpm, bun, yarn, uv, poetry, pipenv, pdm, mvn, gradle) are +// resolved via exec.LookPath — a single stat per name, no directory enumeration. +// Pip variants (pip, pip3, pip3.11, …) are found via scanPathExecutables because +// they install under interpreter-specific names that must be enumerated. +// +// `supply-chain init` uses PATH-based detection rather than CWD lockfile +// detection because the injected shell functions are global — they shadow the +// command in every directory, not just the project init ran in. // Returns nil when nothing is found or PATH is unset; the caller supplies the // fallback. func DetectInstalledPMs(names []string) []string { - want := make(map[string]bool, len(names)) + seen := make(map[string]bool) + + // Fixed-name PMs: exec.LookPath is O(PATH-dirs) with early-exit and no + // ReadDir — strictly cheaper than directory enumeration for known names. for _, n := range names { - want[n] = true + if _, err := exec.LookPath(n); err == nil { + seen[n] = true + } + } + + // Pip variants require enumeration because the names are not fixed + // (pip3.11, pip3.12, …). scanPathExecutables handles dedup and execute-bit. + for _, v := range scanPathExecutables(pipExecutable.MatchString) { + seen[v] = true + } + + if len(seen) == 0 { + return nil + } + result := make([]string, 0, len(seen)) + for name := range seen { + result = append(result, name) } - return scanPathExecutables(func(name string) bool { - return want[name] || pipExecutable.MatchString(name) - }) + slices.Sort(result) + return result } From e29c3c7e2342c9bcd9cbb2b4f54e80adcef8e456 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 11:44:39 +0200 Subject: [PATCH 5/8] [PPSC-920] fix(security): suppress CWE-426 FP in DetectInstalledPMs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/supplychain/shell.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 0d354373..253d5d40 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -459,7 +459,10 @@ func DetectInstalledPMs(names []string) []string { // Fixed-name PMs: exec.LookPath is O(PATH-dirs) with early-exit and no // ReadDir — strictly cheaper than directory enumeration for known names. + // The returned path is discarded (_); only `n` (the hardcoded input name) + // is added to `seen`, so no attacker-controlled path reaches any sink. for _, n := range names { + // armis:ignore cwe:426 cwe:427 reason:the resolved path is intentionally discarded; only the hardcoded input name n is used, so no untrusted path flows to any execution sink if _, err := exec.LookPath(n); err == nil { seen[n] = true } From 9c20d9386e17c22878df715c309214e866de1fd0 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 12:33:27 +0200 Subject: [PATCH 6/8] [PPSC-920] fix(supply-chain): address PR review comments and Windows CI failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- internal/cmd/supply_chain_init.go | 4 +++- internal/cmd/supply_chain_init_test.go | 33 ++++++++++++++++++++++---- internal/supplychain/shell.go | 10 ++++---- internal/supplychain/shell_test.go | 24 ++++++++++++++----- 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/internal/cmd/supply_chain_init.go b/internal/cmd/supply_chain_init.go index c408fbda..8ba85336 100644 --- a/internal/cmd/supply_chain_init.go +++ b/internal/cmd/supply_chain_init.go @@ -200,7 +200,9 @@ func detectWrappablePMs() (pms []string, installed []string) { // package on demand, exactly the supply-chain vector the proxy guards. Pair it // here (after scoping) so it inherits npm's in-scope decision — when npm was // excluded by the `ecosystems` config it stays out, keeping the two in lockstep. - if seen[pmNPM] { + // Guard with a PATH check: if npx is not installed, wrapping it would shadow + // "command not found" with an Armis wrapper error. + if seen[pmNPM] && len(supplychain.DetectInstalledPMs([]string{pmNPX})) > 0 { addPM(pmNPX) } diff --git a/internal/cmd/supply_chain_init_test.go b/internal/cmd/supply_chain_init_test.go index fd5c2e99..4e4cfe9e 100644 --- a/internal/cmd/supply_chain_init_test.go +++ b/internal/cmd/supply_chain_init_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "runtime" "strings" "testing" @@ -89,14 +90,23 @@ func (c *countingReader) Read(p []byte) (int, error) { // which package managers it "finds installed". Restricting PATH to the seeded // dir keeps detection deterministic — a real npm/pip on the developer's machine // can't leak into the result. +// +// On Windows, exec.LookPath only resolves files matching PATHEXT (.exe, .cmd, +// etc.), so stubs are written with a ".exe" suffix. scanPathExecutables strips +// known PATHEXT extensions before applying the match, so pip3.12.exe is still +// recognized as "pip3.12" rather than losing the ".12" suffix. func seedPMsOnPath(t *testing.T, names ...string) { t.Helper() dir := t.TempDir() for _, name := range names { + fname := name + if runtime.GOOS == "windows" { + fname = name + ".exe" + } // 0o755 so the Unix execute-bit filter in scanPathExecutables accepts it; // the bit is ignored on Windows, where the file's mere presence suffices. - if err := os.WriteFile(filepath.Join(dir, name), []byte{}, 0o755); err != nil { //nolint:gosec // test stub on an isolated PATH - t.Fatalf("seed %s: %v", name, err) + if err := os.WriteFile(filepath.Join(dir, fname), []byte{}, 0o755); err != nil { //nolint:gosec // test stub on an isolated PATH + t.Fatalf("seed %s: %v", fname, err) } } t.Setenv("PATH", dir) @@ -133,9 +143,9 @@ func containsPM(pms []string, name string) bool { } func TestDetectWrappablePMs_PairsNpxWithNpm(t *testing.T) { - // npm on PATH puts the npm ecosystem in scope; npx must be wrapped alongside it - // so ad-hoc `npx ` runs are filtered through the same proxy. - seedPMsOnPath(t, pmNPM) + // npm and npx both on PATH: npx must be wrapped alongside npm so ad-hoc + // `npx ` runs are filtered through the same proxy. + seedPMsOnPath(t, pmNPM, pmNPX) chdirTemp(t) pms, _ := detectWrappablePMs() @@ -156,6 +166,19 @@ func TestDetectWrappablePMs_NpxNotPairedWithoutNpm(t *testing.T) { } } +func TestDetectWrappablePMs_NpxNotWrappedWhenAbsentFromPath(t *testing.T) { + // npm on PATH but npx not installed: the pairing guard must prevent wrapping a + // missing npx binary. Unconditionally wrapping it would shadow "command not found" + // with an Armis wrapper error. + seedPMsOnPath(t, pmNPM) + chdirTemp(t) + + pms, _ := detectWrappablePMs() + if containsPM(pms, pmNPX) { + t.Errorf("detectWrappablePMs() = %v, must not wrap npx when it is not on PATH", pms) + } +} + func TestDetectWrappablePMs_NpxAbsentWhenNpmScopedOut(t *testing.T) { // npx is paired AFTER ecosystem scoping, so it must inherit npm's exclusion: // with npm and pnpm both on PATH but the config scoping enforcement to pnpm diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 253d5d40..9d2b02e5 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -385,11 +385,13 @@ func scanPathExecutables(match func(name string) bool) []string { } name := entry.Name() // On Windows, executables carry extensions from PATHEXT (typically - // .exe, .cmd, .bat). Strip any known extension so that pip.exe and - // pip3.cmd match the same patterns as bare pip / pip3 on Unix. + // .exe, .cmd, .bat, .com). Strip only those known executable extensions + // so that pip.exe and pip3.cmd match the same patterns as bare pip / + // pip3 on Unix. filepath.Ext must NOT be used here — it returns the last + // dot-separated suffix, so pip3.12 would lose ".12" and match as "pip3". if runtime.GOOS == goosWindows { - if ext := filepath.Ext(name); ext != "" { - name = strings.TrimSuffix(name, ext) + if ext := strings.ToLower(filepath.Ext(name)); ext == ".exe" || ext == ".cmd" || ext == ".bat" || ext == ".com" { + name = strings.TrimSuffix(name, filepath.Ext(name)) } } if !match(name) { diff --git a/internal/supplychain/shell_test.go b/internal/supplychain/shell_test.go index 99b0a8d1..32e7763d 100644 --- a/internal/supplychain/shell_test.go +++ b/internal/supplychain/shell_test.go @@ -353,8 +353,12 @@ func TestDetectPipVariants(t *testing.T) { t.Run("finds and sorts variants on PATH", func(t *testing.T) { dir := t.TempDir() for _, name := range []string{pipExe, "pip3", "pip3.12"} { - if err := os.WriteFile(filepath.Join(dir, name), []byte{}, 0o755); err != nil { //nolint:gosec - t.Fatalf("seed %s: %v", name, err) + fname := name + if runtime.GOOS == goosWindows { + fname = name + ".exe" + } + if err := os.WriteFile(filepath.Join(dir, fname), []byte{}, 0o755); err != nil { //nolint:gosec + t.Fatalf("seed %s: %v", fname, err) } } t.Setenv("PATH", dir) @@ -369,8 +373,12 @@ func TestDetectPipVariants(t *testing.T) { t.Run("ignores lookalike commands", func(t *testing.T) { dir := t.TempDir() for _, name := range []string{pipExe, "pipx", "pipenv", "pip-compile"} { - if err := os.WriteFile(filepath.Join(dir, name), []byte{}, 0o755); err != nil { //nolint:gosec - t.Fatalf("seed %s: %v", name, err) + fname := name + if runtime.GOOS == goosWindows { + fname = name + ".exe" + } + if err := os.WriteFile(filepath.Join(dir, fname), []byte{}, 0o755); err != nil { //nolint:gosec + t.Fatalf("seed %s: %v", fname, err) } } t.Setenv("PATH", dir) @@ -384,8 +392,12 @@ func TestDetectPipVariants(t *testing.T) { t.Run("deduplicates across PATH dirs", func(t *testing.T) { dir1, dir2 := t.TempDir(), t.TempDir() - os.WriteFile(filepath.Join(dir1, pipExe), []byte{}, 0o755) //nolint:errcheck,gosec - os.WriteFile(filepath.Join(dir2, pipExe), []byte{}, 0o755) //nolint:errcheck,gosec + fname := pipExe + if runtime.GOOS == goosWindows { + fname = pipExe + ".exe" + } + os.WriteFile(filepath.Join(dir1, fname), []byte{}, 0o755) //nolint:errcheck,gosec + os.WriteFile(filepath.Join(dir2, fname), []byte{}, 0o755) //nolint:errcheck,gosec t.Setenv("PATH", dir1+string(os.PathListSeparator)+dir2) got := DetectPipVariants() From 692fcb9a05ec1bff1f763dc4a29f7317b9912af1 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 12:43:18 +0200 Subject: [PATCH 7/8] [PPSC-920] fix(supply-chain): address second round of Copilot review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- internal/cmd/supply_chain_init.go | 6 +- internal/cmd/supply_chain_init_test.go | 4 +- internal/supplychain/shell.go | 95 +++++++++++++++++--------- 3 files changed, 69 insertions(+), 36 deletions(-) diff --git a/internal/cmd/supply_chain_init.go b/internal/cmd/supply_chain_init.go index 8ba85336..1be82a37 100644 --- a/internal/cmd/supply_chain_init.go +++ b/internal/cmd/supply_chain_init.go @@ -201,8 +201,10 @@ func detectWrappablePMs() (pms []string, installed []string) { // here (after scoping) so it inherits npm's in-scope decision — when npm was // excluded by the `ecosystems` config it stays out, keeping the two in lockstep. // Guard with a PATH check: if npx is not installed, wrapping it would shadow - // "command not found" with an Armis wrapper error. - if seen[pmNPM] && len(supplychain.DetectInstalledPMs([]string{pmNPX})) > 0 { + // "command not found" with an Armis wrapper error. Use IsOnPath (single + // exec.LookPath call) rather than DetectInstalledPMs which also enumerates + // pip variants — unnecessary overhead for a single fixed-name check. + if seen[pmNPM] && supplychain.IsOnPath(pmNPX) { addPM(pmNPX) } diff --git a/internal/cmd/supply_chain_init_test.go b/internal/cmd/supply_chain_init_test.go index 4e4cfe9e..4e2b9520 100644 --- a/internal/cmd/supply_chain_init_test.go +++ b/internal/cmd/supply_chain_init_test.go @@ -263,7 +263,9 @@ func TestDetectWrappablePMs_WrapsPipWhenInstalled(t *testing.T) { // PM found on PATH, mixing Node and Python tools, regardless of the cwd's // lockfiles. npx is paired in because npm is present. func TestDetectWrappablePMs_WrapsAllInstalled(t *testing.T) { - seedPMsOnPath(t, pmNPM, pmYarn, pmPip, pmPoetry) + // npx is seeded alongside npm: it is paired with npm but only when present + // on PATH, so the test must reflect that both are actually installed. + seedPMsOnPath(t, pmNPM, pmNPX, pmYarn, pmPip, pmPoetry) chdirTemp(t) pms, _ := detectWrappablePMs() diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 9d2b02e5..6356a6bf 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -345,6 +345,11 @@ func CanonicalPipVariant(name string) (string, bool) { if err != nil { return "", false } + // Reject non-canonical minor versions (e.g. "011"): reconstructing the name + // from the integer would produce a different command than the user invoked. + if strconv.Itoa(minor) != m[2] { + return "", false + } return fmt.Sprintf("pip3.%d", minor), true } @@ -368,54 +373,69 @@ func scanPathExecutables(match func(name string) bool) []string { } 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 } - entries, err := os.ReadDir(dir) + // Stream directory entries in chunks so we stop reading as soon as + // maxScanPathResults is reached, without loading the full listing into + // memory. This bounds both CPU and memory for large PATH entries like + // /usr/bin or network mounts. + f, err := os.Open(dir) //nolint:gosec // dir comes from PATH, not user input if err != nil { continue } - for _, entry := range entries { + for { if len(seen) >= maxScanPathResults { break } - if entry.IsDir() { - continue - } - name := entry.Name() - // On Windows, executables carry extensions from PATHEXT (typically - // .exe, .cmd, .bat, .com). Strip only those known executable extensions - // so that pip.exe and pip3.cmd match the same patterns as bare pip / - // pip3 on Unix. filepath.Ext must NOT be used here — it returns the last - // dot-separated suffix, so pip3.12 would lose ".12" and match as "pip3". - if runtime.GOOS == goosWindows { - if ext := strings.ToLower(filepath.Ext(name)); ext == ".exe" || ext == ".cmd" || ext == ".bat" || ext == ".com" { - name = strings.TrimSuffix(name, filepath.Ext(name)) + entries, err := f.ReadDir(readDirChunk) + for _, entry := range entries { + if len(seen) >= maxScanPathResults { + break } - } - if !match(name) { - continue - } - // On Unix, a matching entry on PATH with no execute bit (a stray data - // file or a non-exec script) would yield a wrapper that later fails at - // exec.LookPath with a confusing error, so require at least one execute - // bit before treating it as a real command. Info() reports the entry's - // own mode (lstat semantics); a symlink to a real binary keeps its - // 0o777 link bits and so still passes, matching what the user can run. - // - // Skip this check on Windows: there is no execute-bit concept there - // (executability is governed by file extension via PATHEXT), and - // os.FileMode.Perm never sets 0o111, so the filter would reject every - // real binary and collapse detection to nothing. - if runtime.GOOS != goosWindows { - info, err := entry.Info() - if err != nil || info.Mode().Perm()&0o111 == 0 { + if entry.IsDir() { + continue + } + name := entry.Name() + // On Windows, executables carry extensions from PATHEXT (typically + // .exe, .cmd, .bat, .com). Strip only those known executable extensions + // so that pip.exe and pip3.cmd match the same patterns as bare pip / + // pip3 on Unix. filepath.Ext must NOT be used here — it returns the last + // dot-separated suffix, so pip3.12 would lose ".12" and match as "pip3". + if runtime.GOOS == goosWindows { + if ext := strings.ToLower(filepath.Ext(name)); ext == ".exe" || ext == ".cmd" || ext == ".bat" || ext == ".com" { + name = strings.TrimSuffix(name, filepath.Ext(name)) + } + } + if !match(name) { continue } + // On Unix, a matching entry on PATH with no execute bit (a stray data + // file or a non-exec script) would yield a wrapper that later fails at + // exec.LookPath with a confusing error, so require at least one execute + // bit before treating it as a real command. Info() reports the entry's + // own mode (lstat semantics); a symlink to a real binary keeps its + // 0o777 link bits and so still passes, matching what the user can run. + // + // Skip this check on Windows: there is no execute-bit concept there + // (executability is governed by file extension via PATHEXT), and + // os.FileMode.Perm never sets 0o111, so the filter would reject every + // real binary and collapse detection to nothing. + if runtime.GOOS != goosWindows { + info, err := entry.Info() + if err != nil || info.Mode().Perm()&0o111 == 0 { + continue + } + } + seen[name] = true + } + if err != nil { // io.EOF or real error — either way, done with this dir + break } - seen[name] = true } + f.Close() //nolint:errcheck,gosec // read-only, close error is not actionable } if len(seen) == 0 { @@ -486,3 +506,12 @@ func DetectInstalledPMs(names []string) []string { slices.Sort(result) return result } + +// IsOnPath reports whether the named fixed-binary is present on $PATH. +// It uses exec.LookPath (one stat per PATH dir, no directory enumeration) and +// discards the resolved path so no untrusted value flows to any execution sink. +// armis:ignore cwe:426 cwe:427 reason:resolved path is intentionally discarded; only the hardcoded input name is used +func IsOnPath(name string) bool { + _, err := exec.LookPath(name) + return err == nil +} From 922cf64210d5a1360bdd1b1900216bc8f75b8220 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Mon, 8 Jun 2026 13:57:10 +0200 Subject: [PATCH 8/8] [PPSC-920] fix(lint): lift loop condition in scanPathExecutables (QF1006) Replace `for { if condition { break } ... }` with the idiomatic `for len(seen) < maxScanPathResults { ... }` to satisfy staticcheck QF1006. --- internal/supplychain/shell.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/supplychain/shell.go b/internal/supplychain/shell.go index 6356a6bf..cd0b7f42 100644 --- a/internal/supplychain/shell.go +++ b/internal/supplychain/shell.go @@ -386,10 +386,7 @@ func scanPathExecutables(match func(name string) bool) []string { if err != nil { continue } - for { - if len(seen) >= maxScanPathResults { - break - } + for len(seen) < maxScanPathResults { entries, err := f.ReadDir(readDirChunk) for _, entry := range entries { if len(seen) >= maxScanPathResults {