Resolve spawned system executables by absolute path (#509) - #596
Resolve spawned system executables by absolute path (#509)#596rahul188 wants to merge 3 commits into
Conversation
Several spawn/exec calls used bare executable names and relied on PATH to resolve them at runtime, so a foreign entry prepended to PATH (shell rc files, direnv, tool shims) could cause a different binary to run. - remote-channel.ts: spawn the blocking update script with process.execPath — the exact Node binary already running the server — instead of the bare name 'node'. The path was already being logged one line above, so there is no ambiguity about which runtime to use. - setup-claude-server.js: resolve killall/pkill/open to their standard absolute locations via a small resolveSystemBinary() helper that prefers an on-disk absolute path and falls back to the bare name when the tool lives elsewhere, so distros that install it in a different directory keep working. Fixes wonderwhy-er#509
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExecutable invocations now avoid PATH-based resolution. Restart and uninstall commands resolve system utilities from candidate absolute paths, while remote-device offline updates use the currently running Node binary. ChangesExecutable path hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
setup-claude-server.js (1)
536-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the same binary resolution in
uninstall-claude-server.js(lines 493-519).killall,pkill, andopenstill run by bare name here, so PATH shadowing protection is inconsistent with the setup flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup-claude-server.js` around lines 536 - 548, Apply the setup flow’s resolveSystemBinary behavior to uninstall-claude-server.js, adding or reusing the corresponding resolver and resolving killall, pkill, and open before execution. Ensure each command uses the resolved absolute-path candidate with the existing bare-name fallback, matching the protection already implemented in resolveSystemBinary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@setup-claude-server.js`:
- Around line 536-548: Apply the setup flow’s resolveSystemBinary behavior to
uninstall-claude-server.js, adding or reusing the corresponding resolver and
resolving killall, pkill, and open before execution. Ensure each command uses
the resolved absolute-path candidate with the existing bare-name fallback,
matching the protection already implemented in resolveSystemBinary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fad91fc-408c-4b21-bb87-71cadee384bb
📒 Files selected for processing (2)
setup-claude-server.jssrc/remote-device/remote-channel.ts
The uninstall flow still spawned these by bare name, so the PATH shadowing protection only covered setup.
|
Good catch — applied. One small correction to the finding: Both scripts pass |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
uninstall-claude-server.js (1)
531-533: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winResolve
openusing absolute paths to prevent PATH shadowing.As per the PR objectives and comments summary, the requirement is to use absolute paths for macOS/Linux
open. However, it was omitted here under the incorrect assumption that the uninstall script does not restart Claude. TherestartClaudefunction clearly attempts to restart the application, soopenmust be protected against PATH shadowing just likekillallandpkill.🔒️ Proposed fix
} else if (platform === "darwin") { - await execAsync(`open -a "Claude"`); + const openBin = resolveSystemBinary(['/usr/bin/open'], 'open'); + await execAsync(`"${openBin}" -a "Claude"`); updateUninstallStep(startStep, 'completed');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uninstall-claude-server.js` around lines 531 - 533, Update the macOS branch of restartClaude to invoke open via its required absolute path instead of resolving it through PATH, while preserving the existing Claude application launch and uninstall-step completion behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@uninstall-claude-server.js`:
- Around line 531-533: Update the macOS branch of restartClaude to invoke open
via its required absolute path instead of resolving it through PATH, while
preserving the existing Claude application launch and uninstall-step completion
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16bfb041-abf3-408d-8d07-e9454e270c88
📒 Files selected for processing (1)
uninstall-claude-server.js
|
Right — restartClaude in the uninstall script does relaunch the app, so the macOS open call needed the same treatment. Applied resolveSystemBinary(['/usr/bin/open'], 'open') there, matching the setup script. |
|
Hi 👋 Gentle nudge on this PR — it's mergeable and the required checks are passing (one non-required/optional check shows as unstable, which shouldn't block merge). Would appreciate a review whenever you have a moment, and I'm happy to address anything needed. Thanks for your time and for maintaining this project! 🙏 |
1 participant
Summary
Several
spawn/execcalls resolved their executable throughPATH, which is inherited from the parent process and can be reordered by shell startup files,direnv, or any tool that prepends a shims directory. This hardens the two spots called out in #509 to use fixed, unambiguous binaries.Changes
src/remote-device/remote-channel.ts— the blocking offline-update script was launched withspawnSync('node', …). The code already logsprocess.execPathon the line above ("Using node executable:"), so this switches the spawn toprocess.execPath. That's the exact Node binary already running the server, so the child runs on the same runtime and can't be shadowed by a differentnodeonPATH.setup-claude-server.js— the macOS/Linux restart path shelled out to barekillall,pkill, andopen. These now resolve through a smallresolveSystemBinary(candidates, fallback)helper that returns the first absolute path that exists on disk (/usr/bin/killall,/usr/bin/pkillor/bin/pkill,/usr/bin/open) and falls back to the bare name if the tool is installed elsewhere, so uncommon distro layouts keep working. Paths are quoted to tolerate spaces.Left as-is intentionally:
taskkilland thecmd.exewrapper — resolved from the protectedSystem32directory; out of scope for this issue.claudelaunch — that's the user's own CLI onPATH, not a fixed system utility, so it has no stable absolute location to prefer.Testing
npm run build(tsc) passes; the compileddist/remote-device/remote-channel.jsshowsspawnSync(process.execPath, …).node --check setup-claude-server.jspasses.resolveSystemBinaryreturns the absolute path when present and falls back to the bare name otherwise.Fixes #509
Summary by CodeRabbit
killall/pkilland macOSopen) instead of relying onPATH.