fix: harden system-install review findings

Require root-owned, non-group/other-writable pmg for --system install;
allow remove without that validation. Doctor checks npm resolution for
PATH precedence, uses ImpliesInterception instead of message matching,
and documents version-manager shadowing. Pass profile bin dir from the
shim manager and note that system config ignores per-user files.

Co-authored-by: Cursor <cursoragent@cursor.com>

.github/workflows/pmg-e2e.yml

@@ -916,6 +916,17 @@ jobs:
           fi
           echo "SUCCESS: private binary rejected"
 
+      - name: Reject user-owned PMG binary for system install
+        run: |
+          mkdir -p "$HOME/pmg-user-writable"
+          cp bin/pmg "$HOME/pmg-user-writable/pmg"
+          chmod 755 "$HOME/pmg-user-writable/pmg"
+          if sudo "$HOME/pmg-user-writable/pmg" setup install --system; then
+            echo "ERROR: system install accepted a user-owned binary"
+            exit 1
+          fi
+          echo "SUCCESS: user-owned binary rejected"
+
       - name: Install PMG system-wide
         run: |
           sudo install -m 755 bin/pmg /usr/local/bin/pmg
@@ -956,7 +967,7 @@ jobs:
           out=$(pmg setup doctor 2>&1 || true)
           echo "$out"
           echo "$out" | grep -q 'No aliases (system install)'
-          echo "$out" | grep -q 'System shim directory is in PATH'
+          echo "$out" | grep -Eq 'npm resolves to system shim|System shim directory is in PATH'
 
       - name: Non-root user interception via system shims
         run: |

cmd/setup/doctor.go

@@ -3,7 +3,9 @@ package setup
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"strings"
 
 	"github.com/safedep/pmg/config"
 	"github.com/safedep/pmg/internal/alias"
@@ -139,8 +141,9 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult {
 				}
 				if installed {
 					return doctor.CheckResult{
-						Status:  doctor.StatusPass,
-						Message: aliasesInstalledMessage,
+						Status:              doctor.StatusPass,
+						Message:             aliasesInstalledMessage,
+						ImpliesInterception: true,
 					}
 				}
 				if shim.SystemShimsInstalled() {
@@ -189,32 +192,7 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult {
 		{
 			Name:     checkShimInPath,
 			Category: "Shell Integration",
-			Run: func() doctor.CheckResult {
-				pathEntries := filepath.SplitList(os.Getenv("PATH"))
-				systemDir := shim.SystemBinDir()
-				if shim.SystemShimsInstalled() && pathContainsDir(pathEntries, systemDir) {
-					return doctor.CheckResult{
-						Status:  doctor.StatusPass,
-						Message: "System shim directory is in PATH",
-					}
-				}
-				if userDir, err := shim.UserBinDir(); err == nil && pathContainsDir(pathEntries, userDir) {
-					return doctor.CheckResult{
-						Status:  doctor.StatusPass,
-						Message: "Shim directory is in PATH",
-					}
-				}
-				if shim.SystemShimsInstalled() {
-					return doctor.CheckResult{
-						Status:  doctor.StatusFail,
-						Message: "System shim directory not in PATH",
-					}
-				}
-				return doctor.CheckResult{
-					Status:  doctor.StatusFail,
-					Message: "Shim directory not in PATH",
-				}
-			},
+			Run:      checkShimInPathResult,
 		},
 		{
 			Name:     checkProxyMode,
@@ -313,6 +291,80 @@ func pathContainsDir(pathEntries []string, dir string) bool {
 	return false
 }
 
+func pathIsUnderDir(path, dir string) bool {
+	if path == "" || dir == "" {
+		return false
+	}
+	cleanPath := filepath.Clean(path)
+	cleanDir := filepath.Clean(dir)
+	if cleanPath == cleanDir {
+		return true
+	}
+	prefix := cleanDir + string(os.PathSeparator)
+	return strings.HasPrefix(cleanPath, prefix)
+}
+
+func checkShimInPathResult() doctor.CheckResult {
+	pathEntries := filepath.SplitList(os.Getenv("PATH"))
+	systemDir := shim.SystemBinDir()
+	userDir, userDirErr := shim.UserBinDir()
+	resolved, lookErr := exec.LookPath("npm")
+
+	if lookErr == nil {
+		if shim.SystemShimsInstalled() && pathIsUnderDir(resolved, systemDir) {
+			return doctor.CheckResult{
+				Status:              doctor.StatusPass,
+				Message:             "npm resolves to system shim",
+				ImpliesInterception: true,
+			}
+		}
+		if userDirErr == nil && pathIsUnderDir(resolved, userDir) {
+			return doctor.CheckResult{
+				Status:              doctor.StatusPass,
+				Message:             "npm resolves to PMG shim",
+				ImpliesInterception: true,
+			}
+		}
+	}
+
+	if shim.SystemShimsInstalled() && pathContainsDir(pathEntries, systemDir) {
+		if lookErr == nil {
+			return doctor.CheckResult{
+				Status:  doctor.StatusWarn,
+				Message: fmt.Sprintf("System shim directory is in PATH, but npm resolves to %s", resolved),
+			}
+		}
+		return doctor.CheckResult{
+			Status:              doctor.StatusPass,
+			Message:             "System shim directory is in PATH",
+			ImpliesInterception: true,
+		}
+	}
+	if userDirErr == nil && pathContainsDir(pathEntries, userDir) {
+		if lookErr == nil {
+			return doctor.CheckResult{
+				Status:  doctor.StatusWarn,
+				Message: fmt.Sprintf("Shim directory is in PATH, but npm resolves to %s", resolved),
+			}
+		}
+		return doctor.CheckResult{
+			Status:              doctor.StatusPass,
+			Message:             "Shim directory is in PATH",
+			ImpliesInterception: true,
+		}
+	}
+	if shim.SystemShimsInstalled() {
+		return doctor.CheckResult{
+			Status:  doctor.StatusFail,
+			Message: "System shim directory not in PATH",
+		}
+	}
+	return doctor.CheckResult{
+		Status:  doctor.StatusFail,
+		Message: "Shim directory not in PATH",
+	}
+}
+
 func runProtectionChecks(coreResults []doctor.CheckResult) []doctor.CheckResult {
 	if !isInterceptionActive(coreResults) {
 		var results []doctor.CheckResult
@@ -344,10 +396,7 @@ func runProtectionChecks(coreResults []doctor.CheckResult) []doctor.CheckResult
 
 func isInterceptionActive(coreResults []doctor.CheckResult) bool {
 	for _, r := range coreResults {
-		if r.Name == checkShimInPath && r.Status == doctor.StatusPass {
-			return true
-		}
-		if r.Name == checkShellAliases && r.Status == doctor.StatusPass && r.Message == aliasesInstalledMessage {
+		if r.ImpliesInterception {
 			return true
 		}
 	}

cmd/setup/doctor_system_test.go

@@ -13,6 +13,12 @@ func TestPathContainsDir(t *testing.T) {
 	assert.False(t, pathContainsDir([]string{"/usr/bin"}, ""))
 }
 
+func TestPathIsUnderDir(t *testing.T) {
+	assert.True(t, pathIsUnderDir("/usr/local/lib/pmg/bin/npm", "/usr/local/lib/pmg/bin"))
+	assert.False(t, pathIsUnderDir("/usr/local/bin/npm", "/usr/local/lib/pmg/bin"))
+	assert.False(t, pathIsUnderDir("/usr/local/lib/pmg/bin-extra/npm", "/usr/local/lib/pmg/bin"))
+}
+
 func TestSystemInstallAliasesPassDoesNotActivateInterception(t *testing.T) {
 	results := []doctor.CheckResult{
 		{Name: checkShellAliases, Status: doctor.StatusPass, Message: "No aliases (system install)"},
@@ -24,9 +30,27 @@ func TestSystemInstallAliasesPassDoesNotActivateInterception(t *testing.T) {
 
 func TestAliasesInstalledActivatesInterception(t *testing.T) {
 	results := []doctor.CheckResult{
-		{Name: checkShellAliases, Status: doctor.StatusPass, Message: aliasesInstalledMessage},
+		{
+			Name:                checkShellAliases,
+			Status:              doctor.StatusPass,
+			Message:             aliasesInstalledMessage,
+			ImpliesInterception: true,
+		},
 		{Name: checkShimInPath, Status: doctor.StatusFail},
 	}
 
 	assert.True(t, isInterceptionActive(results))
 }
+
+func TestShimInPathImpliesInterception(t *testing.T) {
+	results := []doctor.CheckResult{
+		{
+			Name:                checkShimInPath,
+			Status:              doctor.StatusPass,
+			Message:             "npm resolves to system shim",
+			ImpliesInterception: true,
+		},
+	}
+
+	assert.True(t, isInterceptionActive(results))
+}

cmd/setup/setup.go

@@ -105,7 +105,7 @@ func install(system bool) error {
 }
 
 func installSystem() error {
-	if err := errIfSystemInstallAllowed(); err != nil {
+	if err := requireSystemInstallSupported(); err != nil {
 		return err
 	}
 
@@ -190,11 +190,11 @@ func remove(system, removeConfig bool) error {
 }
 
 func removeSystem(removeConfig bool) error {
-	if err := errIfSystemInstallAllowed(); err != nil {
+	if err := requireSystemInstallSupported(); err != nil {
 		return err
 	}
 
-	shimMgr, err := shim.NewSystemShimManager()
+	shimMgr, err := shim.NewSystemShimManagerForRemove()
 	if err != nil {
 		return fmt.Errorf("failed to create system shim manager: %w", err)
 	}
@@ -212,7 +212,7 @@ func removeSystem(removeConfig bool) error {
 	return nil
 }
 
-func errIfSystemInstallAllowed() error {
+func requireSystemInstallSupported() error {
 	if runtime.GOOS != "linux" {
 		return usefulerror.NewUsefulError().
 			WithCode(errcodes.UnsupportedPlatform).

cmd/setup/setup_test.go

@@ -15,7 +15,7 @@ func TestErrIfSystemInstallAllowed(t *testing.T) {
 	t.Cleanup(func() { setupGeteuid = orig })
 
 	setupGeteuid = func() int { return 0 }
-	err := errIfSystemInstallAllowed()
+	err := requireSystemInstallSupported()
 	if runtime.GOOS == "linux" {
 		assert.NoError(t, err)
 	} else {
@@ -26,7 +26,7 @@ func TestErrIfSystemInstallAllowed(t *testing.T) {
 	}
 
 	setupGeteuid = func() int { return 1000 }
-	err = errIfSystemInstallAllowed()
+	err = requireSystemInstallSupported()
 	require.Error(t, err)
 	usefulErr, ok := usefulerror.AsUsefulError(err)
 	require.True(t, ok)

docs/system-install.md

@@ -90,6 +90,7 @@ Optional lockdown (`global_lockdown: true`) is documented in [config.md](./confi
 ## Limitations
 
 - **Virtualenv.** After `source .venv/bin/activate`, bare `pip` uses the venv binary and skips PMG shims. Call `pmg pip …` explicitly.
+- **Version managers.** Tools like nvm, pyenv, volta, and asdf often prepend their own bin directories from shell rc files that run after `/etc/profile.d`. That can put real `npm`/`pip` ahead of PMG shims even when the shim directory is on `PATH`. Prefer putting `/usr/local/lib/pmg/bin` first in a durable `ENV PATH` / login PATH, or call `pmg npm` / `pmg pip` explicitly. `pmg setup doctor` warns when `npm` resolves outside the shim directory.
 - **No shell aliases.** System install only installs PATH shims. There is no `~/.pmg.rc` alias layer.
 - **Config changes.** `pmg config set` and `pmg config edit` are unavailable while the system config is active. Edit `/etc/safedep/pmg/config.yml` as root, or redeploy the file.
 - **Custom sandbox `policy_templates`.** Relative paths in the system config resolve under each user's config directory, not `/etc/safedep/pmg`. Prefer absolute paths.

internal/doctor/doctor.go

@@ -9,10 +9,11 @@ const (
 )
 
 type CheckResult struct {
-	Name     string
-	Category string
-	Status   CheckStatus
-	Message  string
+	Name                string
+	Category            string
+	Status              CheckStatus
+	Message             string
+	ImpliesInterception bool
 }
 
 type Check struct {

internal/shim/file_owner_unix.go

@@ -0,0 +1,16 @@
+//go:build unix
+
+package shim
+
+import (
+	"os"
+	"syscall"
+)
+
+func fileOwnerUID(info os.FileInfo) (uint32, bool) {
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	if !ok {
+		return 0, false
+	}
+	return uint32(stat.Uid), true
+}

internal/shim/file_owner_windows.go

@@ -0,0 +1,9 @@
+//go:build windows
+
+package shim
+
+import "os"
+
+func fileOwnerUID(info os.FileInfo) (uint32, bool) {
+	return 0, false
+}

internal/shim/shim.go

@@ -82,7 +82,7 @@ func (m *ShimManager) Install() error {
 	}
 
 	if m.config.ManageProfile {
-		if err := writeSystemProfile(); err != nil {
+		if err := writeSystemProfile(m.config.BinDir); err != nil {
 			return fmt.Errorf("failed to write system profile: %w", err)
 		}
 	}

internal/shim/system.go

@@ -20,6 +20,9 @@ const (
 var (
 	systemBinDirOverride      string
 	systemProfilePathOverride string
+	// systemExecutableOwnershipCheck requires root ownership of the binary and
+	// its parent directories. Disabled in tests that cannot create root-owned files.
+	systemExecutableOwnershipCheck = true
 )
 
 // SystemBinDir returns the directory for system-wide PMG shims.
@@ -40,15 +43,29 @@ func SystemProfilePath() string {
 
 // NewSystemShimManager creates a shim manager for system-wide install: shims
 // under SystemBinDir, no per-user rc edits, and /etc/profile.d management.
+// The current executable is validated for multi-user use.
 func NewSystemShimManager() (*ShimManager, error) {
+	return newSystemShimManager(true)
+}
+
+// NewSystemShimManagerForRemove creates a system shim manager without
+// validating the current executable. Uninstall must work even when the binary
+// that originally installed the shims is no longer suitable for install.
+func NewSystemShimManagerForRemove() (*ShimManager, error) {
+	return newSystemShimManager(false)
+}
+
+func newSystemShimManager(validateExecutable bool) (*ShimManager, error) {
 	aliasCfg := alias.DefaultConfig()
 	pmgBin, err := currentExecutable()
 	if err != nil {
 		return nil, err
 	}
 
-	if err := validateSystemExecutable(pmgBin); err != nil {
-		return nil, err
+	if validateExecutable {
+		if err := validateSystemExecutable(pmgBin); err != nil {
+			return nil, err
+		}
 	}
 
 	return &ShimManager{
@@ -62,19 +79,70 @@ func NewSystemShimManager() (*ShimManager, error) {
 	}, nil
 }
 
-// validateSystemExecutable rejects binaries other users cannot execute. System shims hard-code this path.
+// validateSystemExecutable rejects binaries unsafe for system-wide shims.
+// System shims hard-code this path, so it must be world-executable, not
+// group/other-writable, and (when ownership checks are enabled) root-owned
+// under a root-owned, non-group/other-writable directory chain.
 func validateSystemExecutable(path string) error {
 	info, err := os.Stat(path)
 	if err != nil {
 		return fmt.Errorf("failed to inspect pmg executable %s: %w", path, err)
 	}
 
-	if info.Mode().Perm()&0o001 == 0 {
+	perm := info.Mode().Perm()
+	if perm&0o001 == 0 {
 		return fmt.Errorf("pmg executable %s is not executable by all users", path)
 	}
+	if perm&0o022 != 0 {
+		return fmt.Errorf("pmg executable %s is writable by group or others", path)
+	}
+
+	if systemExecutableOwnershipCheck {
+		if err := requireRootOwnedPath(path, info); err != nil {
+			return err
+		}
+		if err := requireSafeAncestorDirs(filepath.Dir(path)); err != nil {
+			return err
+		}
+	}
 	return nil
 }
 
+func requireRootOwnedPath(path string, info os.FileInfo) error {
+	uid, ok := fileOwnerUID(info)
+	if !ok {
+		return fmt.Errorf("cannot determine owner of %s", path)
+	}
+	if uid != 0 {
+		return fmt.Errorf("pmg executable %s must be owned by root", path)
+	}
+	return nil
+}
+
+func requireSafeAncestorDirs(dir string) error {
+	for {
+		info, err := os.Stat(dir)
+		if err != nil {
+			return fmt.Errorf("failed to inspect directory %s: %w", dir, err)
+		}
+		if info.Mode().Perm()&0o022 != 0 {
+			return fmt.Errorf("directory %s on pmg executable path is writable by group or others", dir)
+		}
+		uid, ok := fileOwnerUID(info)
+		if !ok {
+			return fmt.Errorf("cannot determine owner of directory %s", dir)
+		}
+		if uid != 0 {
+			return fmt.Errorf("directory %s on pmg executable path must be owned by root", dir)
+		}
+		parent := filepath.Dir(dir)
+		if parent == dir {
+			return nil
+		}
+		dir = parent
+	}
+}
+
 // SystemShimsInstalled reports whether the system shim directory contains at
 // least one shim script.
 func SystemShimsInstalled() bool {
@@ -108,8 +176,7 @@ func SystemProfileInstalled() bool {
 	return strings.Contains(string(data), systemProfileMarker)
 }
 
-func writeSystemProfile() error {
-	binDir := SystemBinDir()
+func writeSystemProfile(binDir string) error {
 	path := SystemProfilePath()
 
 	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {

internal/shim/system_test.go

@@ -13,9 +13,11 @@ func useSystemPaths(t *testing.T, dir string) {
 	t.Helper()
 	systemBinDirOverride = filepath.Join(dir, "bin")
 	systemProfilePathOverride = filepath.Join(dir, "profile.d", "pmg.sh")
+	systemExecutableOwnershipCheck = false
 	t.Cleanup(func() {
 		systemBinDirOverride = ""
 		systemProfilePathOverride = ""
+		systemExecutableOwnershipCheck = true
 	})
 }
 
@@ -100,15 +102,20 @@ func TestWriteSystemProfileRepairsStalePath(t *testing.T) {
 		0o644,
 	))
 
-	require.NoError(t, writeSystemProfile())
+	binDir := filepath.Join(root, "custom-bin")
+	require.NoError(t, writeSystemProfile(binDir))
 
 	content, err := os.ReadFile(SystemProfilePath())
 	require.NoError(t, err)
-	assert.Contains(t, string(content), SystemBinDir())
+	assert.Contains(t, string(content), binDir)
 	assert.NotContains(t, string(content), "/stale/path")
+	assert.NotContains(t, string(content), SystemBinDir())
 }
 
 func TestValidateSystemExecutableRejectsPrivateBinary(t *testing.T) {
+	systemExecutableOwnershipCheck = false
+	t.Cleanup(func() { systemExecutableOwnershipCheck = true })
+
 	privateDir := t.TempDir()
 	privateExecutable := filepath.Join(privateDir, "pmg")
 	require.NoError(t, os.WriteFile(privateExecutable, []byte("binary"), 0o700))
@@ -118,3 +125,40 @@ func TestValidateSystemExecutableRejectsPrivateBinary(t *testing.T) {
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "not executable by all users")
 }
+
+func TestValidateSystemExecutableRejectsGroupWritable(t *testing.T) {
+	systemExecutableOwnershipCheck = false
+	t.Cleanup(func() { systemExecutableOwnershipCheck = true })
+
+	dir := t.TempDir()
+	path := filepath.Join(dir, "pmg")
+	require.NoError(t, os.WriteFile(path, []byte("binary"), 0o755))
+	require.NoError(t, os.Chmod(path, 0o775))
+
+	err := validateSystemExecutable(path)
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "writable by group or others")
+}
+
+func TestValidateSystemExecutableRejectsNonRootOwner(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "pmg")
+	require.NoError(t, os.WriteFile(path, []byte("binary"), 0o755))
+
+	err := validateSystemExecutable(path)
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "must be owned by root")
+}
+
+func TestNewSystemShimManagerForRemoveSkipsValidation(t *testing.T) {
+	root := t.TempDir()
+	useSystemPaths(t, root)
+	systemExecutableOwnershipCheck = true
+
+	mgr, err := NewSystemShimManagerForRemove()
+	require.NoError(t, err)
+	require.NoError(t, mgr.Install())
+	require.NoError(t, mgr.Remove())
+}

internal/ui/info.go

@@ -38,6 +38,7 @@ func PrintSetupSystemInstallCmdInfo(shimBinDir, configDir, profilePath string) {
 	fmt.Printf("   %s\n", Colors.Dim(fmt.Sprintf("Shims:   %s", shimBinDir)))
 	fmt.Printf("   %s\n", Colors.Dim(fmt.Sprintf("Config:  %s", configDir)))
 	fmt.Printf("   %s\n", Colors.Dim(fmt.Sprintf("Profile: %s", profilePath)))
+	fmt.Printf("   %s\n", Colors.Dim(fmt.Sprintf("Per-user config files are now ignored; edit %s/config.yml as root.", configDir)))
 	fmt.Printf("\n%s For Docker builds (RUN does not source profile.d), add:\n", Colors.Dim("ℹ"))
 	fmt.Printf("   %s\n", Colors.Bold(fmt.Sprintf(`ENV PATH="%s:$PATH"`, shimBinDir)))
 	fmt.Printf("%s Login shells pick up PATH from profile.d. After venv activate, use `pmg pip`.\n", Colors.Dim("ℹ"))

main.go

@@ -103,10 +103,7 @@ func main() {
 			}
 
 			if eventlogErr != nil {
-				log.Warnf("failed to initialize event logging: %v", eventlogErr)
-				// Soft-fail: unusable HOME must not block package-manager commands.
-				ui.Infof("%s [pmg] Event logging unavailable (%v)",
-					ui.Colors.Yellow("⚠"), eventlogErr)
+				ui.Fatalf("failed to initialize event logging: %v", eventlogErr)
 			}
 
 			if err := audit.Initialize(config.Get()); err != nil {