feat: Linux system-wide setup (pmg setup install --system)#377
Merged
Conversation
Install shared shims, managed configuration, and login-shell PATH integration so golden images and multi-user hosts can protect package installs for every user. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Tighten shim detection, profile repair, and install ordering while trimming over-specific doctor/info hints from the system-install path. Co-authored-by: Cursor <cursoragent@cursor.com>
Use UserBinDir for PATH checks and pass aliases as not required under system install without treating that as active interception. Co-authored-by: Cursor <cursoragent@cursor.com>
Prefix the warning with [pmg] and drop the redundant continuing clause. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover root system setup, PATH/profile.d, managed config, non-root interception, and remove. Pin pnpm 11.10.0 on the package-manager e2e job after an integrity crash on pnpm add. Co-authored-by: Cursor <cursoragent@cursor.com>
Align package.json with the pnpm version we want in CI so action-setup stops erroring on a version mismatch after the e2e integrity flake. Co-authored-by: Cursor <cursoragent@cursor.com>
pnpm 11.x `pnpm init` still writes onFail:download; `pnpm add` then fails after PMG analysis even on 11.10.0. Seed the temp package with npm init instead. Co-authored-by: Cursor <cursoragent@cursor.com>
The e2e integrity crash is avoided by npm init; the 11.10.0 bump is no longer needed. Co-authored-by: Cursor <cursoragent@cursor.com>
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>
Doctor now verifies every installed package manager against the shim directory, and system-install validation only requires a safe parent directory. Cloud sync records username/uid on invocation context for multi-user hosts sharing one endpoint. Co-authored-by: Cursor <cursoragent@cursor.com>
- shim: make system executable resolution injectable so tests pass under umask 002; skip the root-owner test when running as root - doctor: treat resolution into either the system or per-user shim dir as intercepted, and collapse the shim-in-PATH check to a single call site - setup: make remove (both --system and per-user) best-effort with errors.Join so one failed step no longer strands the other artifact - shim: allow a group-writable install parent dir (Debian/Ubuntu ship /usr/local/bin as root:staff 2775) while still rejecting world-writable and non-root-owned parents - audit: attribute cloud events to SUDO_USER when running under sudo - docs: drop the soft-fail event-logging claim (hard-fail is retained)
The GitHub ubuntu-latest runner ships /usr/local/bin world-writable so tooling can install without sudo. System install correctly refuses a world-writable dir for the shared binary (any local user could replace it and hijack every user's npm/pip). No FHS-compliant distro or Docker image ships it world-writable — it is always root:root 0755 or root:staff 2775 — so this normalizes only the anomalous CI runner back to standard perms and still exercises the real /usr/local/bin path.
A pmg run as root with a preserved HOME (GitHub runners, sudo -E, su without -) creates the invoking user's ~/.config/safedep as root-owned, and event-log init then fail-closes every later non-root command. Make that state self-solvable: - event-log init permission errors exit with a usefulerror naming the likely cause and the chown fix instead of a bare fatal - pmg setup doctor probes event-log dir writability and reports the same fix via a new per-result Fix override - document the mechanism and remedy in system-install.md, along with the binary ownership requirements for --system - consolidate this branch's doctor tests into doctor_test.go
Path resolution trusted HOME (and XDG_*), which sudo and su can preserve from the invoking user (GitHub runners, sudo -E, su without -). Any pmg run as root then created root-owned ~/.config/safedep inside that user's home, and event-log init fail-closed every later non-root pmg/npm/pip run for them. System install made sudo pmg the documented flow, turning this latent bug into the happy path. When euid is 0, configDir and cacheDir now resolve from root's passwd home instead of the environment, so root state lands under /root and user homes are never touched. PMG_CONFIG_DIR/PMG_CACHE_DIR still win, non-root resolution is unchanged, and Windows is unaffected (no euid). Event-log init stays fatal on failure; sudo-run package events are attributed via SUDO_USER and synced by the exit auto-sync as usual. E2E: GitHub runners preserve HOME under sudo, so assert that no sudo pmg run leaks state into the runner's home, and that the managed-config refusal fails for the documented reason rather than a permission brick.
The chown hint is only correct when another account created files inside the current user's own home. When a leaked HOME or XDG_CONFIG_HOME points at another user's home (e.g. sudo -u on GitHub runners), following it would chown that user's directory and brick their pmg instead. Classify the failure against the passwd home, which the leaked environment cannot influence, and prescribe: - dir inside own home: restore ownership with chown - dir outside own home: fix the leaked environment, never chown - explicit PMG_CONFIG_DIR: make it writable Used by both the fatal event-log error and the doctor check, and the docs troubleshooting now carries the same two-case triage.
GitHub runners export XDG_CONFIG_HOME=/home/runner/.config and it leaks through sudo -u, so the pmgtest pmg resolved the runner user's config dir and fail-closed on its runner-owned log file (run 29289868727 shows the triaged error catching exactly this). Set it inside the login shell so it wins regardless of how the leak is delivered. The remedy now returns a full-help and doctor-table pair from a single triage, and drops the do-not-chown tail from the leak message.
Belongs with the previous commit; it was left unstaged and e0580b1 does not compile without it.
Running as uid 0 without a resolvable root passwd entry (scratch containers, minimal chroots) panicked at startup on every command, because the euid-based path resolution had no fallback. Fall back to env-derived resolution there: without a passwd database there is no user switching, so the cross-user poisoning that branch prevents cannot occur. Also restore the underlying cause in the generic event-log init error (minimal output hid it after the usefulerror change), and document that root's per-user data lives under /root regardless of a preserved HOME.
…check Address review findings on the system-install PR: - cloud_sink: honor SUDO_USER for audit attribution only when euid==0. Without the gate any user could set SUDO_USER and spoof cloud-audit attribution to another account. Matches the guard in cmd/setup/cert.go. - config: divert per-user paths to root's passwd home only on an actual sudo elevation (euid==0 && SUDO_USER set), not for every root euid. The blanket root diversion ignored HOME/XDG_CONFIG_HOME and silently stopped reading genuine root users' config (golden Docker images), regressing two tests that only fail when the suite runs as root. Genuine root honors the environment as before; su without - leaves no marker and stays a documented, loud-failing residual. - doctor: add a system-only check re-validating that the binary the installed shims exec is still root-owned and non-writable, catching permission/ownership drift after install. - shim: fold the duplicated shim-scan loop into firstShimContent.
Address remaining review comments: - shim: force root:root 0755 on the managed system dirs (shim tree and profile.d) after MkdirAll. A pre-created dir with weaker ownership, possible under Debian's group-writable /usr/local/lib, would let a non-root user replace the shims every account executes. - audit: when SUDO_USER has no passwd entry (minimal containers), attribute cloud events from sudo's recorded SUDO_USER/SUDO_UID env instead of falling back to root. Still gated on euid 0. - setup: reword the root-without---system warning; alias/shim install follows HOME, so claiming it configures only root's home was wrong. - shim: skip the non-root-owner validation test on Windows, where file ownership is not resolvable.
The system-install validation checked the binary's own permissions and the parent's tamper-safety but never reachability: a 0755 root-owned binary under a 0700 directory (e.g. /root/pmg) passed every check while every non-root user's shim failed with exit 127. Walk the directory chain to / and require the search bit for others; doctor's system binary check inherits this. E2E gains a reject case for a binary under a non-searchable directory. setup info: render alias/user-shim/system-shim rows through one installed-state formatter (location when installed, "not installed" otherwise) instead of a mix of booleans, paths, and prose.
writeSystemProfile chowned/chmod'd /etc/profile.d itself, a shared system directory pmg does not own, silently overriding any perms a sysadmin set on it. Secure only the file pmg writes (pmg.sh) via secureSystemFile, which also forces 0644 explicitly so a restrictive root umask cannot leave the snippet non-world-readable (which would drop the shim dir from other users' login-shell PATH). Docs: add Limitations entries for the group-writable install dir bypass (validation is defeatable on non-sticky group-writable dirs like Debian's /usr/local/bin) and the elevation-only scope (su without - can still poison the caller's home; sudo -u cannot poison another account). Trim the requireSafeParentDir comment to a pointer.
Address the open review threads on #376: - rename realUserHomeDir to currentUserHomeDir and fail when the passwd entry has no home directory - split UnwritableConfigDirRemedy into classifyUnwritableDir (cause diagnosis) and pure message rendering so each function has one job - replace cmd/setup's duplicate pathIsUnderDir with the shared config.PathWithinDir, now guarding empty inputs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rWmR6NS47FpedJsrTWk8A
SafeDep Report Summary
Package Details
This report is generated by SafeDep Github App |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
==========================================
+ Coverage 55.78% 55.81% +0.02%
==========================================
Files 217 220 +3
Lines 14781 15262 +481
==========================================
+ Hits 8246 8518 +272
- Misses 5790 5969 +179
- Partials 745 775 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vet Summary ReportThis report is generated by vet Policy Checks
Malicious Package AnalysisMalicious package analysis was performed using SafeDep Cloud API Malicious Package Analysis Report
Changed PackagesChanged Packages
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a Linux “system-wide” installation plane for PMG (pmg setup install --system) intended for shared machines/images so a single root install protects all users, including updated setup doctor/info output, hardened shim/profile handling, expanded docs, and a new CI E2E job.
Changes:
- Introduces system-wide shim/profile installation under
/usr/local/lib/pmg/binand/etc/profile.d/pmg.sh, plus install/remove CLI support via--system. - Extends config/runtime path resolution and remedy messaging for sudo/root scenarios; adds system-managed config write/remove helpers.
- Enhances
pmg setup doctor/pmg setup info, adds audit attribution hardening (SUDO_USERonly trusted when euid==0), and adds docs + CI E2E coverage for system install.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Links system-wide install docs from the main README. |
| main.go | Routes eventlog init failures through a useful-error classifier with remedies. |
| internal/ui/info.go | Adds completion output for system-wide install (shims/config/profile guidance). |
| internal/shim/system.go | Implements system install paths, profile.d management, and binary safety validation. |
| internal/shim/system_test.go | Unit tests for system shim/profile install/remove and binary validation helpers. |
| internal/shim/shim.go | Adds system-install knobs (skip rc edits, manage profile), best-effort removal, shared markers. |
| internal/shim/shim_test.go | Adds a regression test ensuring remove reports directory-removal errors. |
| internal/shim/path.go | Updates PATH filtering to strip the system shim dir to avoid recursion. |
| internal/shim/path_test.go | Adds test coverage for stripping the system shim dir without env markers. |
| internal/shim/file_owner_windows.go | Windows stub for file owner UID resolution. |
| internal/shim/file_owner_unix.go | Unix implementation for file owner UID resolution via syscall.Stat_t. |
| internal/doctor/doctor.go | Extends check results to carry interception implication + per-result fix overrides. |
| internal/audit/cloud_sink.go | Adds username/uid attribution; hardens SUDO_USER trust to euid==0 only. |
| internal/audit/cloud_sink_test.go | Tests for new invoking-user attribution behavior and spoofing resistance. |
| go.work.sum | Updates module sums (dependency metadata). |
| go.sum | Updates module sums for buf-generated protocolbuffers dependency. |
| go.mod | Bumps buf-generated protocolbuffers dependency version. |
| docs/system-install.md | New documentation for Linux system-wide install, artifacts, Docker PATH, limitations, troubleshooting. |
| docs/persistent-proxy.md | Updates docs to point readers to the new system-install documentation. |
| docs/development.md | Adds system install doc to the development docs index. |
| docs/config.md | Adds a pointer to system-install docs from the config docs. |
| config/rootdir_test.go | Tests for sudo/root home/config/cache dir resolution behavior. |
| config/remedy_test.go | Tests for unwritable-dir remedy classification and messaging. |
| config/managed_config_test.go | Adds tests for writing/removing the system template config. |
| config/config.go | Adds root-dir resolution helpers, unwritable-dir triage, system config helpers, and shared path helper. |
| cmd/setup/setup.go | Adds --system to setup install/remove; best-effort cleanup via errors.Join; platform/root gating. |
| cmd/setup/setup_test.go | Tests platform/root gating for --system. |
| cmd/setup/info.go | Extends setup info to report user vs system shims and alias state consistently. |
| cmd/setup/doctor.go | Expands doctor to detect system installs, validate system binary drift, and verify “resolves to shim” behavior. |
| cmd/setup/doctor_test.go | Unit tests for new doctor helpers (resolution classification, interception implication, event-log probe). |
| .github/workflows/pmg-e2e.yml | Adds a new Linux E2E workflow job to validate system install, artifacts, refusals, and non-root interception. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fe3f77615
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
os.WriteFile and os.MkdirAll honor the process umask, so a hardened root umask (e.g. 077) produced 0700 shims other users cannot execute and a 0600/0700 managed config non-root pmg runs cannot read - silently disabling the system-wide policy. Chmod/chown the artifacts explicitly after writing, with unix regression tests running under umask 077. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rWmR6NS47FpedJsrTWk8A
abhisek
commented
Jul 14, 2026
- move PathWithinDir and the root-owned mkdir/chmod helpers into a new internal/fsutil package; config no longer exports a generic fs util - create managed-config directories with MkdirAllRootOwned, which only sets ownership and mode on directories it creates - drops the hardcoded safedep parent-dir heuristic and never touches pre-existing directories - collapse NewSystemShimManagerForRemove into NewSystemShimManager and validate the executable in Install, where the action needs it; Remove works regardless of binary state - rename ShimConfig.ManageProfile to SystemProfile and document it - name the Linux-only system paths linuxSystemBinDir/ProfilePath and document the Unix-only validation semantics - share the PMG_BIN shim variable name between writeShimScript and parseShimPMGBin via the shimPMGBinVar constant - cloud attribution falls back to the effective uid when SUDO_UID is absent, so sudo-invoked commands are not misattributed to root Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rWmR6NS47FpedJsrTWk8A
Sahilb315
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #376
Summary
Adds a root-only Linux system install so one machine or image protects every user account (shared VMs, golden Docker images, CI runners):
pmg setup install --system/pmg setup remove --system: shared shims under/usr/local/lib/pmg/bin, managed config at/etc/safedep/pmg/config.yml, PATH via/etc/profile.d/pmg.sh.pmg setup doctorandpmg setup infounderstand both install planes; doctor verifies package managers actually resolve to a shim (catches nvm/pyenv-style PATH shadowing) and re-validates the system binary's ownership/permissions to detect post-install drift.docs/system-install.mdcovering artifacts, DockerENV PATHusage, limitations (virtualenv, version managers, managed config), and permission remedies.Supersedes #376; includes all review feedback from that thread.
Security posture
root:root 0755even when pre-created.remove --systemskips binary validation so a broken install can always be uninstalled; removal is best-effort (errors.Join) so one failed step doesn't strand the rest.PMG_CONFIG_DIR, leakedHOME/XDG_*from another account, or a root-created directory in the user's own home — the wrong remedy (chown-ing another account's directory) is never suggested./rootonly for sudo-elevated runs (SUDO_USERset), sosudo pmgcan't poison the invoking user's home; genuine root keeps honoringHOME/XDG_*(Docker images that set them on purpose).SUDO_USER-based cloud attribution is honored only when euid is 0 to prevent spoofing.Changes since #376's last review
Resolves the three open threads on #376:
realUserHomeDirrenamed tocurrentUserHomeDir; it now fails when the passwd entry has no home directory.UnwritableConfigDirRemedysplit intoclassifyUnwritableDir(cause diagnosis, documented enum) and pure message rendering — one job per function.cmd/setup's duplicate path helper in favor of a sharedconfig.PathWithinDir.Test plan
go test ./...(full suite; also verified green running as root, which exercises the sudo/root path-resolution branches —guardandpackagemanagernetwork-dependent tests excluded in sandboxed envs)e2e-system-install: root install, artifact checks, user-owned/private binary rejection, managed-config refusal, non-root user malware block via system shims, clean removal🤖 Generated with Claude Code
https://claude.ai/code/session_011rWmR6NS47FpedJsrTWk8A
Generated by Claude Code