fix sudo handling and pkgx-under-/root reachability#84
Open
tannevaled wants to merge 8 commits into
Open
Conversation
Restore the privilege drop when running as root via sudo (so pkgx caches stay user-owned), but only when the resolved pkgx binary is actually reachable from $SUDO_USER. Falls back to running pkgx as root rather than crashing the inner sudo with "Permission denied" when pkgx lives under /root/.pkgx/ (pkgxdev#68). - Restore drop-privilege behaviour for `sudo pkgm` (fixes the regression flagged by jhheider on pkgxdev#83). - Resolve an alternative pkgx under $SUDO_USER's home / /usr/local when the current path is unreachable to $SUDO_USER. - Override HOME so pkgx caches under the invoking user's tree. - Stop mutating `args` so the args[0] lookup at line 341 keeps working. - Avoid the `Deno.env.get("USER")!` non-null assertion crash. - Call install_prefix() once (it has filesystem side effects). - Keep the visible log surface unchanged: only the pre-existing "installing as root" warning fires by default; the new "pkgx unreachable" diagnostic is gated behind PKGM_DEBUG. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates pkgm’s sudo/root execution flow when installing into the system prefix (/usr/local) to restore dropping privileges back to the invoking user (so caches remain user-owned), while avoiding failures when the resolved pkgx binary isn’t actually reachable by $SUDO_USER (eg when pkgx lives under root-owned private directories).
Changes:
- Reworks
query_pkgx()command selection sosudo -u $SUDO_USERis only used when running as root viasudoandpkgxis deemed reachable by that user. - Overrides
HOME(when dropping privileges) to keeppkgxcaches under the invoking user’s home. - Adds helper functions to discover a reachable
pkgxpath and to look up a user’s home directory.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+830
to
+835
| // Conservative heuristic: private home dirs are typically mode 700, so a | ||
| // path under another user's home is unreachable. System paths and the | ||
| // user's own home are assumed reachable. | ||
| if (p.startsWith("/root/")) return user === "root"; | ||
| const m = p.match(/^\/home\/([^/]+)\//); | ||
| if (m) return m[1] === user; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+809
to
+820
| if (existsSync(root)) { | ||
| let best: { v: SemVer; path: string } | undefined; | ||
| for (const entry of Deno.readDirSync(root)) { | ||
| if (!entry.isDirectory || !entry.name.startsWith("v")) continue; | ||
| try { | ||
| const v = new SemVer(entry.name.slice(1)); | ||
| const path = join(root, entry.name, "bin/pkgx"); | ||
| if (!existsSync(path)) continue; | ||
| if (!best || v.gt(best.v)) best = { v, path }; | ||
| } catch { /* skip malformed version dir */ } | ||
| } | ||
| if (best) return best.path; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Author
|
@copilot apply changes based on the comments in this thread |
Comment on lines
+811
to
+818
| for (const entry of Deno.readDirSync(root)) { | ||
| if (!entry.isDirectory || !entry.name.startsWith("v")) continue; | ||
| try { | ||
| const v = new SemVer(entry.name.slice(1)); | ||
| const path = join(root, entry.name, "bin/pkgx"); | ||
| if (!existsSync(path)) continue; | ||
| if (!best || v.gt(best.v)) best = { v, path }; | ||
| } catch { /* skip malformed version dir */ } |
Author
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Author
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+832
to
+836
| const local = join(home, ".local/bin/pkgx"); | ||
| if (existsSync(local)) return local; | ||
| } | ||
| if (existsSync("/usr/local/bin/pkgx")) return "/usr/local/bin/pkgx"; | ||
| return undefined; |
Comment on lines
+839
to
+846
| function reachable_as(p: string, user: string): boolean { | ||
| // Conservative heuristic: private home dirs are typically mode 700, so a | ||
| // path under another user's home is unreachable. System paths and the | ||
| // user's own home are assumed reachable. | ||
| if (p.startsWith("/root/")) return user === "root"; | ||
| const m = p.match(/^\/home\/([^/]+)\//); | ||
| if (m) return m[1] === user; | ||
| return true; |
Comment on lines
+843
to
+844
| if (p.startsWith("/root/")) return user === "root"; | ||
| const m = p.match(/^\/home\/([^/]+)\//); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+313
to
+320
| const reachable = pkgx_reachable_as(pkgx, sudoUser); | ||
| if (reachable) { | ||
| cmd = "/usr/bin/sudo"; | ||
| cmd_args = ["-u", sudoUser, "--", reachable, ...args]; | ||
| // Override HOME, or pkgx will cache back under /root/ where sudoUser | ||
| // can't reach it on the next invocation. | ||
| const home = user_home(sudoUser); | ||
| if (home) env.HOME = home; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Author
|
@jhheider can you take a look please? |
Contributor
|
it looks good, @tannevaled ; codex review flagged two areas for concern:
(1) is likely rare-but-not-impossible. i know we have one user trying to do that kind of shared-install system, but i don't know if this would trigger his issues. @magnusviri : any thoughts here? even better would be if we could add a CI test-suite to run through your code paths and check that everything is working and permissions are what's intended (i.e. no catastrophic changes to uid:0). |
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.
Restore the privilege drop when running as root via sudo (so pkgx caches stay user-owned), but only when the resolved pkgx binary is actually reachable from $SUDO_USER. Falls back to running pkgx as root rather than crashing the inner sudo with "Permission denied" when pkgx lives under /root/.pkgx/ (#68).
sudo pkgm(fixes the regression flagged by jhheider on Refactor sudo handling for package installation #83).argsso the args[0] lookup at line 341 keeps working.Deno.env.get("USER")!non-null assertion crash.