fix(macos): fix node-pty posix_spawnp error with postinstall script #347
Conversation
WalkthroughThis PR adds a postinstall script to fix execute permissions for the node-pty spawn-helper binary on macOS, addressing spawn failures that occur when the binary lacks proper executable permissions after installation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/index.js`:
- Around line 491-521: The ALLOWED_PATHS check can be bypassed by symlinks and
doesn't expand "~"; update the logic that computes targetPath and the permission
check in the handler to (1) expand '~' for dirPath and for each entry in
ALLOWED_PATHS before resolving, (2) call path.resolve on those expanded values,
then use fs.realpathSync (or async realpath) to get the real filesystem path for
both targetPath and each allowed path, handling realpath errors with a try/catch
and treating failures as disallowed, and (3) compare realTarget === realAllowed
or realTarget.startsWith(realAllowed + path.sep) to enforce containment;
reference targetPath, ALLOWED_PATHS, and the isAllowed check when making these
changes.
- Around line 1121-1123: The shell selection currently sets shell to '/bin/bash'
for non-Windows but doesn't handle systems without bash; update the logic around
the shell and shellArgs (the variables named shell and shellArgs used where
pty.spawn() is called) to check for the existence of '/bin/bash' (e.g.,
fs.existsSync or fs.access) and if it's missing fall back to '/bin/sh', and emit
a warning/log when falling back; retain the Windows branch using
'powershell.exe' and ensure the arguments remain '-c' for Unix or ['-Command',
shellCommand] for Windows so pty.spawn() always receives an absolute shell path.
In `@src/components/ChatInterface.jsx`:
- Around line 1937-1941: The saved provider fallback leaves localStorage stale;
when initializing provider in the useState initializer (the provider state using
setProvider, isProviderEnabled, getDefaultProvider and the 'selected-provider'
localStorage key), detect when saved is disabled and update localStorage to the
chosen default (or call setProvider with the default) so storage and state stay
in sync; ensure the same update occurs only when fallback happens to avoid
overwriting a valid saved value.
server/index.js
Outdated
|
|
||
| // Default to first allowed path or home directory | ||
| const homeDir = os.homedir(); | ||
| let targetPath = dirPath ? dirPath.replace('~', homeDir) : homeDir; | ||
|
|
||
| let targetPath; | ||
|
|
||
| if (dirPath) { | ||
| targetPath = dirPath.replace('~', homeDir); | ||
| } else if (ALLOWED_PATHS && ALLOWED_PATHS.length > 0) { | ||
| // If ALLOWED_PATHS is set, default to first allowed path | ||
| targetPath = ALLOWED_PATHS[0]; | ||
| } else { | ||
| targetPath = homeDir; | ||
| } | ||
|
|
||
| // Resolve and normalize the path | ||
| targetPath = path.resolve(targetPath); | ||
|
|
||
|
|
||
| // Security check - if ALLOWED_PATHS is set, ensure path is within allowed paths | ||
| if (ALLOWED_PATHS && ALLOWED_PATHS.length > 0) { | ||
| const isAllowed = ALLOWED_PATHS.some(allowedPath => { | ||
| const resolved = path.resolve(allowedPath); | ||
| return targetPath === resolved || targetPath.startsWith(resolved + path.sep); | ||
| }); | ||
|
|
||
| if (!isAllowed) { | ||
| return res.status(403).json({ | ||
| error: `Access restricted to: ${ALLOWED_PATHS.join(', ')}`, | ||
| allowedPaths: ALLOWED_PATHS | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent symlink escapes when enforcing ALLOWED_PATHS.
path.resolve + startsWith can be bypassed via symlinks (e.g., an allowed folder containing a symlink to /etc). Also, ALLOWED_PATHS containing ~ will currently resolve to a non-existent path and produce false 404s. Consider resolving real paths for comparison and expanding ~ up front.
🔒 Suggested fix
- // Default to first allowed path or home directory
- const homeDir = os.homedir();
- let targetPath;
+ // Default to first allowed path or home directory
+ const homeDir = os.homedir();
+ const expandHome = (p) => p.replace(/^~(?=$|[\\/])/, homeDir);
+ const realpathSafe = async (p) => {
+ const resolved = path.resolve(expandHome(p));
+ try {
+ return await fsPromises.realpath(resolved);
+ } catch {
+ return resolved;
+ }
+ };
+ let targetPath;
if (dirPath) {
- targetPath = dirPath.replace('~', homeDir);
+ targetPath = expandHome(dirPath);
} else if (ALLOWED_PATHS && ALLOWED_PATHS.length > 0) {
// If ALLOWED_PATHS is set, default to first allowed path
- targetPath = ALLOWED_PATHS[0];
+ targetPath = expandHome(ALLOWED_PATHS[0]);
} else {
targetPath = homeDir;
}
// Resolve and normalize the path
targetPath = path.resolve(targetPath);
// Security check - if ALLOWED_PATHS is set, ensure path is within allowed paths
if (ALLOWED_PATHS && ALLOWED_PATHS.length > 0) {
- const isAllowed = ALLOWED_PATHS.some(allowedPath => {
- const resolved = path.resolve(allowedPath);
- return targetPath === resolved || targetPath.startsWith(resolved + path.sep);
- });
+ const realTargetPath = await realpathSafe(targetPath);
+ const resolvedAllowedPaths = await Promise.all(
+ ALLOWED_PATHS.map(p => realpathSafe(p))
+ );
+ const isAllowed = resolvedAllowedPaths.some(resolved =>
+ realTargetPath === resolved || realTargetPath.startsWith(resolved + path.sep)
+ );🤖 Prompt for AI Agents
In `@server/index.js` around lines 491 - 521, The ALLOWED_PATHS check can be
bypassed by symlinks and doesn't expand "~"; update the logic that computes
targetPath and the permission check in the handler to (1) expand '~' for dirPath
and for each entry in ALLOWED_PATHS before resolving, (2) call path.resolve on
those expanded values, then use fs.realpathSync (or async realpath) to get the
real filesystem path for both targetPath and each allowed path, handling
realpath errors with a try/catch and treating failures as disallowed, and (3)
compare realTarget === realAllowed or realTarget.startsWith(realAllowed +
path.sep) to enforce containment; reference targetPath, ALLOWED_PATHS, and the
isAllowed check when making these changes.
There was a problem hiding this comment.
This comment is out of scope for this PR. This PR only contains the node-pty spawn-helper fix (scripts/fix-node-pty.js and package.json postinstall hook).
The ALLOWED_PATHS feature is addressed in PR #350, which includes symlink protection via fs.realpath().
There was a problem hiding this comment.
@feraudet, thank you for the clarification! You're absolutely right—this security concern about ALLOWED_PATHS and symlink escapes is out of scope for PR #347, which focuses solely on fixing the node-pty spawn-helper permissions issue.
I appreciate you noting that the ALLOWED_PATHS feature and its symlink protection via fs.realpath() are being addressed in PR #350. I'll consider this comment resolved for this PR.
server/index.js
Outdated
| // Use appropriate shell based on platform (use full path for macOS) | ||
| const shell = os.platform() === 'win32' ? 'powershell.exe' : '/bin/bash'; | ||
| const shellArgs = os.platform() === 'win32' ? ['-Command', shellCommand] : ['-c', shellCommand]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -1130 server/index.js | tail -20Repository: siteboon/claudecodeui
Length of output: 1175
🏁 Script executed:
wc -l server/index.jsRepository: siteboon/claudecodeui
Length of output: 85
🏁 Script executed:
# Check for README and documentation
fd -e md . | head -20Repository: siteboon/claudecodeui
Length of output: 145
🏁 Script executed:
# Check package.json for deployment/platform info
cat package.json | head -50Repository: siteboon/claudecodeui
Length of output: 1407
🏁 Script executed:
# Search for references to bash, shell, platform, or deployment in the codebase
rg -i "platform|deployment|docker|container|nix" --type md | head -30Repository: siteboon/claudecodeui
Length of output: 266
🏁 Script executed:
# Read README to understand platform support and requirements
head -100 README.mdRepository: siteboon/claudecodeui
Length of output: 3333
🏁 Script executed:
# Check if there are any Dockerfile or container-related files
fd -e Dockerfile -e dockerfileRepository: siteboon/claudecodeui
Length of output: 47
🏁 Script executed:
# Search for any existing shell/bash-related issues or comments in the code
rg -i "bash|/bin/bash" --type js -B 2 -A 2Repository: siteboon/claudecodeui
Length of output: 10168
🏁 Script executed:
# Check if node-pty is in dependencies
grep -i "pty\|node-pty" package.jsonRepository: siteboon/claudecodeui
Length of output: 147
🏁 Script executed:
# Look for other shell spawning patterns in the codebase
rg "pty\.spawn|execFile|spawn" --type js -B 2 -A 2 | head -50Repository: siteboon/claudecodeui
Length of output: 3227
🏁 Script executed:
# Check what version of node-pty and understand its requirements
cat package.json | grep -A 30 "dependencies"Repository: siteboon/claudecodeui
Length of output: 1143
🏁 Script executed:
# Look at complete context around the shell spawning in server/index.js
sed -n '1100,1150p' server/index.jsRepository: siteboon/claudecodeui
Length of output: 2833
🏁 Script executed:
# Check if there are any other instances of pty.spawn in the codebase
rg "pty\.spawn" --type js -B 5 -A 5Repository: siteboon/claudecodeui
Length of output: 802
🏁 Script executed:
# Check node-pty documentation or comments about shell requirements
rg "shell.*pty|pty.*shell" --type js -B 2 -A 2Repository: siteboon/claudecodeui
Length of output: 973
🏁 Script executed:
# Search for any platform-specific shell handling or fallback patterns
rg "process\.env\.SHELL|SHELL.*fallback|sh.*-c" --type js -B 2 -A 2Repository: siteboon/claudecodeui
Length of output: 17158
🏁 Script executed:
# Check if there are any platform detection patterns or warnings in comments
rg "darwin|macOS|platform|shell" server/index.js | head -30Repository: siteboon/claudecodeui
Length of output: 2645
🏁 Script executed:
# Check node-pty documentation or examples in node_modules if available
find node_modules -name "*.md" -path "*node-pty*" 2>/dev/null | head -5Repository: siteboon/claudecodeui
Length of output: 47
🏁 Script executed:
# Search for any other shell spawning patterns that might show best practices
rg "spawn.*shell|exec.*sh" server/ --type js -B 1 -A 1 | head -40Repository: siteboon/claudecodeui
Length of output: 2450
🏁 Script executed:
# Check if there's any documentation about system requirements or dependencies
cat README.md | grep -i "requirement\|depend\|bash\|shell" | head -20Repository: siteboon/claudecodeui
Length of output: 741
🏁 Script executed:
# Look for any installation or setup scripts that might indicate bash requirement
fd -type f -name "*.sh" -o -name "install*"Repository: siteboon/claudecodeui
Length of output: 235
🏁 Script executed:
# Check if there are any comments in the actual shell spawning code that provide context
sed -n '1070,1140p' server/index.jsRepository: siteboon/claudecodeui
Length of output: 4037
🏁 Script executed:
# Check the node-pty documentation online or in any comments about shell requirements
rg "node-pty|spawn.*shell" --type json -B 2 -A 2Repository: siteboon/claudecodeui
Length of output: 1450
🏁 Script executed:
# Look at the actual usage of pty.spawn to understand what shell paths are needed
ast-grep --pattern 'pty.spawn($_, $_, $_)'Repository: siteboon/claudecodeui
Length of output: 1065
Consider fallback shell handling for broader Unix compatibility.
While /bin/bash is standard on most Linux distributions and macOS, it may be unavailable on certain systems (Alpine Linux, NixOS, minimal containers). However, since pty.spawn() requires an absolute path to the shell binary, the suggested fallback to process.env.SHELL || 'bash' won't work—pty.spawn() cannot resolve relative shell names through PATH.
A more practical approach:
- Keep
/bin/bashas primary for common systems - Fall back to
/bin/shif bash is unavailable (more universally present) - Or detect shell availability at startup and warn if bash is missing
♻️ Alternative approach
const platform = os.platform();
let shell;
if (platform === 'win32') {
shell = 'powershell.exe';
} else {
// Check for bash, fall back to sh
try {
require('fs').accessSync('/bin/bash', require('fs').constants.X_OK);
shell = '/bin/bash';
} catch {
shell = '/bin/sh'; // Fallback to POSIX sh
}
}
const shellArgs = platform === 'win32' ? ['-Command', shellCommand] : ['-c', shellCommand];🤖 Prompt for AI Agents
In `@server/index.js` around lines 1121 - 1123, The shell selection currently sets
shell to '/bin/bash' for non-Windows but doesn't handle systems without bash;
update the logic around the shell and shellArgs (the variables named shell and
shellArgs used where pty.spawn() is called) to check for the existence of
'/bin/bash' (e.g., fs.existsSync or fs.access) and if it's missing fall back to
'/bin/sh', and emit a warning/log when falling back; retain the Windows branch
using 'powershell.exe' and ensure the arguments remain '-c' for Unix or
['-Command', shellCommand] for Windows so pty.spawn() always receives an
absolute shell path.
There was a problem hiding this comment.
This comment is out of scope for this PR. This PR only contains the node-pty spawn-helper fix (scripts/fix-node-pty.js and package.json postinstall hook).
The ALLOWED_PATHS feature is addressed in PR #350, which includes symlink protection via fs.realpath().
There was a problem hiding this comment.
@feraudet, you're absolutely right—my apologies for the noise. The shell path change from 'bash' to '/bin/bash' is incidental to this PR's core objective of fixing the node-pty spawn-helper permissions issue. I should have focused my review on the actual changes introduced: scripts/fix-node-pty.js and the package.json postinstall hook.
Thank you for keeping the PR scope focused and for the clarification about PR #350. 👍
Add postinstall script that fixes spawn-helper permissions on macOS. The node-pty package ships spawn-helper without execute permissions (644), causing "posix_spawnp failed" errors when spawning terminal processes. Closes siteboon#284 Co-Authored-By: Claude Opus 4.5 <[email protected]>
feraudet
force-pushed
the
fix/node-pty-spawn-helper
branch
from
1de9c4c to
9fab7a8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Fixes the "Error: posix_spawnp failed" error on macOS when using the terminal/shell features (e.g., Claude CLI Login).
Root cause: node-pty ships the
spawn-helperbinary without execute permissions (644 instead of 755).Solution: Add a postinstall script that automatically sets the correct permissions after
npm install.Changes
scripts/fix-node-pty.js- Script that fixes spawn-helper permissions on macOSpackage.json- Add postinstall hookRelated Issues
Test plan
npm installon macOS🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.