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.
This commit is contained in:
Sahil Bansal
2026-07-15 09:11:52 +05:30
committed by GitHub
parent dc0e3202cc
commit 46803f8e70
5 changed files with 76 additions and 19 deletions
+30 -12
View File
@@ -8,6 +8,8 @@ import (
"strings" "strings"
"github.com/safedep/dry/log" "github.com/safedep/dry/log"
"github.com/safedep/pmg/internal/shim"
"github.com/safedep/pmg/internal/ui"
) )
type ProtectionTestCase struct { type ProtectionTestCase struct {
@@ -34,7 +36,10 @@ func ProtectionTestCases() []ProtectionTestCase {
} }
func RunProtectionCheck(tc ProtectionTestCase, pmgBinary string) CheckResult { 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{ return CheckResult{
Status: StatusWarn, Status: StatusWarn,
Message: fmt.Sprintf("%s not available — skipping protection test for %s", tc.PackageManager, tc.Package), 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.Dir = tmpDir
cmd.Env = env cmd.Env = env
_, runErr := cmd.CombinedOutput() output, runErr := cmd.CombinedOutput()
return evaluateProtectionResult(tc.PackageManager, tc.Package, runErr) return evaluateProtectionResult(tc.PackageManager, tc.Package, string(output), runErr)
} }
func setupVenv(baseDir string) (string, error) { func setupVenv(baseDir string) (string, error) {
@@ -96,22 +101,35 @@ func prependPath(env []string, dir string) []string {
return result return result
} }
func evaluateProtectionResult(pm string, pkg string, err error) CheckResult { func evaluateProtectionResult(pm string, pkg string, output string, err error) CheckResult {
if err != nil { if err == nil {
if isExecutableNotFound(err) { return CheckResult{
return CheckResult{ Status: StatusFail,
Status: StatusWarn, Message: fmt.Sprintf("Failed to block %s/%s — package was installed instead of blocked", pm, pkg),
Message: fmt.Sprintf("%s not available — skipping protection test for %s", 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{ return CheckResult{
Status: StatusPass, Status: StatusPass,
Message: fmt.Sprintf("Malicious package blocked (%s/%s)", pm, pkg), Message: fmt.Sprintf("Malicious package blocked (%s/%s)", pm, pkg),
} }
} }
return CheckResult{ return CheckResult{
Status: StatusFail, Status: StatusWarn,
Message: fmt.Sprintf("Failed to block %s/%s — package was installed instead of blocked", pm, pkg), Message: fmt.Sprintf("Install failed without a malware block (%v)", err),
} }
} }
+37 -4
View File
@@ -8,6 +8,7 @@ import (
"strings" "strings"
"testing" "testing"
"github.com/safedep/pmg/internal/ui"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
) )
@@ -17,13 +18,15 @@ func TestEvaluateProtectionResult(t *testing.T) {
name string name string
pm string pm string
pkg string pkg string
output string
err error err error
wantStatus CheckStatus wantStatus CheckStatus
}{ }{
{ {
name: "blocked", name: "blocked with headline in output",
pm: "npm", pm: "npm",
pkg: "safedep-test-pkg@0.1.3", pkg: "safedep-test-pkg@0.1.3",
output: "✗ " + ui.MalwareBlockedHeadline + "\n safedep-test-pkg@0.1.3\n",
err: fmt.Errorf("exit status 1"), err: fmt.Errorf("exit status 1"),
wantStatus: StatusPass, wantStatus: StatusPass,
}, },
@@ -31,25 +34,55 @@ func TestEvaluateProtectionResult(t *testing.T) {
name: "not blocked", name: "not blocked",
pm: "npm", pm: "npm",
pkg: "safedep-test-pkg@0.1.3", pkg: "safedep-test-pkg@0.1.3",
output: "",
err: nil, err: nil,
wantStatus: StatusFail, wantStatus: StatusFail,
}, },
{ {
name: "pm not found", name: "pmg binary not found",
pm: "npm", pm: "npm",
pkg: "safedep-test-pkg@0.1.3", 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, wantStatus: StatusWarn,
}, },
} }
for _, tt := range tests { for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { 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) 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) { func TestProtectionTestCases(t *testing.T) {
cases := ProtectionTestCases() cases := ProtectionTestCases()
require.GreaterOrEqual(t, len(cases), 2) require.GreaterOrEqual(t, len(cases), 2)
+1 -1
View File
@@ -36,7 +36,7 @@ func (p ProxyPresenter) BlockMessage(reason proxy.BlockReason, blockCtx *proxy.B
var message string var message string
switch reason { switch reason {
case proxy.BlockReasonMalware, proxy.BlockReasonUserDeclined: case proxy.BlockReasonMalware, proxy.BlockReasonUserDeclined:
prefix := "Malicious package blocked" prefix := MalwareBlockedHeadline
if reason == proxy.BlockReasonUserDeclined { if reason == proxy.BlockReasonUserDeclined {
prefix = "Installation blocked by user" prefix = "Installation blocked by user"
} }
+7 -1
View File
@@ -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) { func printMalwareBlockSection(data *ReportData) {
if len(data.BlockedPackages) == 0 { if len(data.BlockedPackages) == 0 {
return return
} }
fmt.Println() 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) printMaliciousPackagesList(data.BlockedPackages)
fmt.Println() fmt.Println()
} }
+1 -1
View File
@@ -70,7 +70,7 @@ func blockWithExit(config *BlockConfig, exit bool) error {
// already shown to the user in verbose mode as part of the reporting. // already shown to the user in verbose mode as part of the reporting.
if verbosityLevel != VerbosityLevelVerbose { if verbosityLevel != VerbosityLevelVerbose {
fmt.Println() 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 { if config.ShowReference {
printMaliciousPackagesList(config.MalwarePackages) printMaliciousPackagesList(config.MalwarePackages)