From 46803f8e70fdd8fdc6192a37480588567114c7e7 Mon Sep 17 00:00:00 2001 From: Sahil Bansal Date: Wed, 15 Jul 2026 09:11:52 +0530 Subject: [PATCH] fix(doctor): stop npm protection passing when the real package manager is absent (#378) `pmg setup doctor` reported npm protection as OK on a machine with no npm installed. Two compounding defects: 1. The availability gate used a plain `exec.LookPath`, which resolves the PMG shim on PATH rather than the real binary, so the "skip" branch never fired. It now uses `shim.ResolveRealBinary` (PATH with shim dirs stripped), matching the runner, so a missing real binary correctly yields WARN "not available". 2. The result was inferred purely from a non-zero exit, so PackageManagerNotFound (exit 127) and other failures were misread as a successful block. The check now requires PMG's block headline in the captured output before reporting PASS; other non-zero exits report WARN with the error surfaced. The block headline is extracted into `ui.MalwareBlockedHeadline` so the doctor's marker stays in sync with what PMG prints across its block-output sites. --- internal/doctor/checks.go | 42 ++++++++++++++++++++++++---------- internal/doctor/checks_test.go | 41 +++++++++++++++++++++++++++++---- internal/ui/proxy_presenter.go | 2 +- internal/ui/report.go | 8 ++++++- internal/ui/ui.go | 2 +- 5 files changed, 76 insertions(+), 19 deletions(-) diff --git a/internal/doctor/checks.go b/internal/doctor/checks.go index 824fdba..7550f3b 100644 --- a/internal/doctor/checks.go +++ b/internal/doctor/checks.go @@ -8,6 +8,8 @@ import ( "strings" "github.com/safedep/dry/log" + "github.com/safedep/pmg/internal/shim" + "github.com/safedep/pmg/internal/ui" ) type ProtectionTestCase struct { @@ -34,7 +36,10 @@ func ProtectionTestCases() []ProtectionTestCase { } func RunProtectionCheck(tc ProtectionTestCase, pmgBinary string) CheckResult { - if _, err := exec.LookPath(tc.PackageManager); err != nil { + // Resolve the real package manager the same way the runner does — PATH with + // PMG shim dirs stripped. A plain exec.LookPath would find the PMG shim on + // PATH and report the manager as available even when the real binary is absent + if _, err := shim.ResolveRealBinary(tc.PackageManager); err != nil { return CheckResult{ Status: StatusWarn, Message: fmt.Sprintf("%s not available — skipping protection test for %s", tc.PackageManager, tc.Package), @@ -72,8 +77,8 @@ func RunProtectionCheck(tc ProtectionTestCase, pmgBinary string) CheckResult { cmd.Dir = tmpDir cmd.Env = env - _, runErr := cmd.CombinedOutput() - return evaluateProtectionResult(tc.PackageManager, tc.Package, runErr) + output, runErr := cmd.CombinedOutput() + return evaluateProtectionResult(tc.PackageManager, tc.Package, string(output), runErr) } func setupVenv(baseDir string) (string, error) { @@ -96,22 +101,35 @@ func prependPath(env []string, dir string) []string { return result } -func evaluateProtectionResult(pm string, pkg string, err error) CheckResult { - if err != nil { - if isExecutableNotFound(err) { - return CheckResult{ - Status: StatusWarn, - Message: fmt.Sprintf("%s not available — skipping protection test for %s", pm, pkg), - } +func evaluateProtectionResult(pm string, pkg string, output string, err error) CheckResult { + if err == nil { + return CheckResult{ + Status: StatusFail, + Message: fmt.Sprintf("Failed to block %s/%s — package was installed instead of blocked", pm, pkg), } + } + + if isExecutableNotFound(err) { + return CheckResult{ + Status: StatusWarn, + Message: fmt.Sprintf("%s not available — skipping protection test for %s", pm, pkg), + } + } + + // A non-zero exit alone is not proof of a block: PackageManagerNotFound, + // proxy/CA setup failures, and config errors all exit non-zero too. Require + // PMG's block headline in the output before declaring protection working. + // The headline is emitted only for malware blocks, not cooldown-only blocks. + if strings.Contains(output, ui.MalwareBlockedHeadline) { return CheckResult{ Status: StatusPass, Message: fmt.Sprintf("Malicious package blocked (%s/%s)", pm, pkg), } } + return CheckResult{ - Status: StatusFail, - Message: fmt.Sprintf("Failed to block %s/%s — package was installed instead of blocked", pm, pkg), + Status: StatusWarn, + Message: fmt.Sprintf("Install failed without a malware block (%v)", err), } } diff --git a/internal/doctor/checks_test.go b/internal/doctor/checks_test.go index 95dc08c..a718830 100644 --- a/internal/doctor/checks_test.go +++ b/internal/doctor/checks_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/safedep/pmg/internal/ui" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -17,13 +18,15 @@ func TestEvaluateProtectionResult(t *testing.T) { name string pm string pkg string + output string err error wantStatus CheckStatus }{ { - name: "blocked", + name: "blocked with headline in output", pm: "npm", pkg: "safedep-test-pkg@0.1.3", + output: "✗ " + ui.MalwareBlockedHeadline + "\n safedep-test-pkg@0.1.3\n", err: fmt.Errorf("exit status 1"), wantStatus: StatusPass, }, @@ -31,25 +34,55 @@ func TestEvaluateProtectionResult(t *testing.T) { name: "not blocked", pm: "npm", pkg: "safedep-test-pkg@0.1.3", + output: "", err: nil, wantStatus: StatusFail, }, { - name: "pm not found", + name: "pmg binary not found", pm: "npm", pkg: "safedep-test-pkg@0.1.3", - err: &exec.Error{Name: "npm", Err: exec.ErrNotFound}, + err: &exec.Error{Name: "pmg", Err: exec.ErrNotFound}, + wantStatus: StatusWarn, + }, + { + name: "non-zero exit without block headline is inconclusive", + pm: "npm", + pkg: "safedep-test-pkg@0.1.3", + output: "npm is not installed\n", + err: fmt.Errorf("exit status 127"), wantStatus: StatusWarn, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := evaluateProtectionResult(tt.pm, tt.pkg, tt.err) + result := evaluateProtectionResult(tt.pm, tt.pkg, tt.output, tt.err) assert.Equal(t, tt.wantStatus, result.Status) }) } } +func TestRunProtectionCheckSkipsWhenOnlyShimOnPath(t *testing.T) { + tmp := t.TempDir() + // ~/.pmg/bin suffix is stripped by FilterPMGFromPath, so a shim here must + // not be mistaken for a real npm binary. + shimDir := filepath.Join(tmp, ".pmg", "bin") + require.NoError(t, os.MkdirAll(shimDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(shimDir, "npm"), []byte("#!/bin/sh\n"), 0o755)) + + t.Setenv("PATH", shimDir) + + tc := ProtectionTestCase{ + PackageManager: "npm", + Package: "safedep-test-pkg@0.1.3", + InstallArgs: []string{"npm", "install", "safedep-test-pkg@0.1.3"}, + } + + result := RunProtectionCheck(tc, filepath.Join(tmp, "nonexistent-pmg")) + assert.Equal(t, StatusWarn, result.Status) + assert.Contains(t, result.Message, "not available") +} + func TestProtectionTestCases(t *testing.T) { cases := ProtectionTestCases() require.GreaterOrEqual(t, len(cases), 2) diff --git a/internal/ui/proxy_presenter.go b/internal/ui/proxy_presenter.go index 2f46ffe..e128b7c 100644 --- a/internal/ui/proxy_presenter.go +++ b/internal/ui/proxy_presenter.go @@ -36,7 +36,7 @@ func (p ProxyPresenter) BlockMessage(reason proxy.BlockReason, blockCtx *proxy.B var message string switch reason { case proxy.BlockReasonMalware, proxy.BlockReasonUserDeclined: - prefix := "Malicious package blocked" + prefix := MalwareBlockedHeadline if reason == proxy.BlockReasonUserDeclined { prefix = "Installation blocked by user" } diff --git a/internal/ui/report.go b/internal/ui/report.go index 6f46cbe..e15cf26 100644 --- a/internal/ui/report.go +++ b/internal/ui/report.go @@ -139,13 +139,19 @@ func Report(data *ReportData) { } } +// MalwareBlockedHeadline is the headline printed when a malicious package is +// blocked. Exported so out-of-process consumers (e.g. `pmg setup doctor`) can +// detect a genuine block from captured output instead of inferring it from a +// non-zero exit code, which any failure would also produce. +const MalwareBlockedHeadline = "Malicious package blocked" + func printMalwareBlockSection(data *ReportData) { if len(data.BlockedPackages) == 0 { return } fmt.Println() - fmt.Printf("%s %s\n", Colors.Red("✗"), Colors.Red("Malicious package blocked")) + fmt.Printf("%s %s\n", Colors.Red("✗"), Colors.Red(MalwareBlockedHeadline)) printMaliciousPackagesList(data.BlockedPackages) fmt.Println() } diff --git a/internal/ui/ui.go b/internal/ui/ui.go index e8f1640..0088f56 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -70,7 +70,7 @@ func blockWithExit(config *BlockConfig, exit bool) error { // already shown to the user in verbose mode as part of the reporting. if verbosityLevel != VerbosityLevelVerbose { fmt.Println() - fmt.Printf("%s %s\n", Colors.Red("✗"), Colors.Red("Malicious package blocked")) + fmt.Printf("%s %s\n", Colors.Red("✗"), Colors.Red(MalwareBlockedHeadline)) if config.ShowReference { printMaliciousPackagesList(config.MalwarePackages)