test(installer): make npm resolution permission test root-safe#2253
test(installer): make npm resolution permission test root-safe#2253
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Approach is correct — dropping to nobody is the right way to make permission tests meaningful on root-backed runners. The guard in nonRootSpawnOptions() is well-scoped and the fixture permissions are properly set up.
However, the WSL-E2E job is still failing on this exact test:
FAIL test/install-npm-resolution.test.ts > reports npm link targets as unwritable
when npm_prefix/lib exists but cannot create node_modules
AssertionError: expected null to be +0
result.status is null (signal kill, not clean exit), which suggests nobody can't access something outside the test fixture — likely the repo checkout directory used as cwd (the default when cwd is undefined). On the WSL runner, the repo root may not be world-traversable.
A couple of options:
- Pass
cwd: tmpand ensure the installer payload is accessible from there - Explicitly
chmod a+rxthe ancestor directoriesnobodyneeds to traverse - Skip the uid-drop path on WSL if the runner layout makes it impractical
The fix works on Linux (checks job) and macOS — just the WSL path needs attention before this can merge.
When dropping to nobody uid/gid on root-backed WSL runners, the repo checkout directory is not traversable. Copy the installer payload into the test's temp dir and use that as cwd so the spawned process can access both. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/install-npm-resolution.test.ts`:
- Around line 62-70: The nonRootSpawnOptions function currently calls
execFileSync("id", ["-u","nobody"]) / ["-g","nobody"] and parses results which
can throw or produce invalid output on minimal images; change
nonRootSpawnOptions to guard these calls with a try/catch and validate the
parsed numbers (Number.isFinite and not NaN) before returning uid/gid, and if
any step fails (execFileSync throws, trim/parseInt yields NaN, or values are out
of range) return {} as a safe fallback so tests don't error on environment
differences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f4dcb06-3414-4d52-8822-565d3075f38c
📒 Files selected for processing (1)
test/install-npm-resolution.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/install-npm-resolution.test.ts (1)
67-70:⚠️ Potential issue | 🟠 MajorHarden
nobodyUID/GID discovery to avoid environment-driven test failures.Line 68 and Line 69 can throw (or parse invalid values) on minimal/nonstandard Linux images, which turns this into setup fragility instead of behavior validation.
Suggested hardening
function nonRootSpawnOptions(): { uid?: number; gid?: number } { if (typeof process.getuid !== "function" || process.getuid() !== 0 || process.platform !== "linux") { return {}; } - return { - uid: Number.parseInt(execFileSync("id", ["-u", "nobody"], { encoding: "utf-8" }).trim(), 10), - gid: Number.parseInt(execFileSync("id", ["-g", "nobody"], { encoding: "utf-8" }).trim(), 10), - }; + try { + const uid = Number.parseInt(execFileSync("id", ["-u", "nobody"], { encoding: "utf-8" }).trim(), 10); + const gid = Number.parseInt(execFileSync("id", ["-g", "nobody"], { encoding: "utf-8" }).trim(), 10); + if (!Number.isInteger(uid) || uid < 0 || !Number.isInteger(gid) || gid < 0) { + return {}; + } + return { uid, gid }; + } catch { + return {}; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-npm-resolution.test.ts` around lines 67 - 70, The current helper that returns uid/gid by calling execFileSync("id", ["-u","nobody"]) and execFileSync("id", ["-g","nobody"]) is brittle on minimal images; wrap each execFileSync call in a try/catch, validate the trimmed output with a /^\d+$/ check and Number.parseInt before using it, and if validation fails (or exec throws) fall back to a safe constant UID/GID (e.g. 65534) so tests don't depend on environment; update the return object that sets uid/gid accordingly and optionally include a debug/log message when falling back.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/install-npm-resolution.test.ts`:
- Around line 67-70: The current helper that returns uid/gid by calling
execFileSync("id", ["-u","nobody"]) and execFileSync("id", ["-g","nobody"]) is
brittle on minimal images; wrap each execFileSync call in a try/catch, validate
the trimmed output with a /^\d+$/ check and Number.parseInt before using it, and
if validation fails (or exec throws) fall back to a safe constant UID/GID (e.g.
65534) so tests don't depend on environment; update the return object that sets
uid/gid accordingly and optionally include a debug/log message when falling
back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b02e3025-f9b1-499e-8614-e0d4bb1a7cb5
📒 Files selected for processing (1)
test/install-npm-resolution.test.ts
WSL does not support setuid via Node's spawnSync uid/gid options (EACCES even when running as root). Switch to `runuser -u nobody` inside the bash command to drop privileges for the permission test. Also copy the installer payload into the temp dir so nobody can read it, and use tmp as cwd so nobody can traverse to it. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
CI is green (pending WSL confirmation on latest commit). The final diff against main is clean.
Summary
The permission test (npm_link_targets_writable) was failing on root-backed CI runners because test -w always returns true for root. The fix drops to nobody before running the permission check so the assertion is meaningful.
What changed
isLinuxRoot()helper replacesnonRootSpawnOptions()— simpler, noexecFileSyncdependency.rawSnippetmode onrunInstallerFunction— lets callers control sourcing when they need to wrap the command (e.g., insu).- Root path uses
su -s /bin/bash nobody -c '...'— drops privileges at the bash level. Node'suid/gidspawn options don't work on WSL (EACCES), andrunuserisn't available on WSL's minimal image.suis universally available. - Installer payload copied to
/tmpandcwdset to the temp dir sonobodycan access both. - Non-root path is unchanged — same behavior as before.
Note
Commit history has debug commits from diagnosing the WSL issue — will clean up on squash merge.
2 participants
Summary
This PR fixes the post-merge main-CI regression introduced after #485 by making the installer npm-resolution permission test match non-root installer semantics on root-backed runners. The production installer logic is unchanged; only the test harness is adjusted so the assertion no longer depends on root-only writability behavior.
Changes
test/install-npm-resolution.test.tsso the permission-sensitivenpm_link_targets_writable()assertion runs asnobodyon Linux root runnersType of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit