fix: validate openshell binary to prevent npm package shadowing#970
Open
SaiSharan2005 wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix: validate openshell binary to prevent npm package shadowing#970SaiSharan2005 wants to merge 1 commit intoNVIDIA:mainfrom
SaiSharan2005 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
- Add isOpenshellCLI() check to verify the resolved binary is the real OpenShell CLI (via --version output), not the npm gateway package - Surface clear error message with manual install steps when install-openshell.sh fails Fixes NVIDIA#967
Contributor
📝 WalkthroughWalkthroughEnhanced OpenShell CLI installation failure messaging with manual recovery instructions and added CLI version verification to resolve-openshell to ensure the correct Rust binary is detected, preventing npm package shadowing on PATH. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller Code
participant Resolve as resolveOpenshell()
participant CheckExec as checkExecutable()
participant CheckCLI as isOpenshellCLI()
participant Binary as openshell Binary
Caller->>Resolve: resolveOpenshell(opts)
loop for each candidate path
Resolve->>CheckExec: checkExecutable(path)
CheckExec-->>Resolve: executable?
alt if executable
Resolve->>CheckCLI: opts.checkCLI || isOpenshellCLI(path)
CheckCLI->>Binary: path --version
Binary-->>CheckCLI: version output
CheckCLI-->>CheckCLI: match /^openshell\s+\d+/?
CheckCLI-->>Resolve: is valid CLI?
alt if valid CLI
Resolve-->>Caller: return verified path
else if invalid CLI
Resolve->>Resolve: continue to next candidate
end
else not executable
Resolve->>Resolve: continue to next candidate
end
end
alt no valid candidate found
Resolve-->>Caller: return null
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/resolve-openshell.js (1)
54-56:⚠️ Potential issue | 🟠 Major
commandVResultoverride path skips CLI validation.Lines [54]-[56] return
opts.commandVResultwithoutcheckCLI(...), which bypasses the new binary-type guard in DI/test mode.Suggested fix
- } else if (opts.commandVResult && opts.commandVResult.startsWith("/")) { - return opts.commandVResult; + } else if (opts.commandVResult && opts.commandVResult.startsWith("/")) { + if (checkCLI(opts.commandVResult)) return opts.commandVResult; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/resolve-openshell.js` around lines 54 - 56, The branch that returns opts.commandVResult when it starts with "/" bypasses the CLI validation; update this path to pass opts.commandVResult through checkCLI(...) before returning so the new binary-type guard runs in DI/test mode—specifically, locate the branch handling opts.commandVResult (the code that currently returns opts.commandVResult) and replace the bare return with a return of checkCLI(opts.commandVResult) (or otherwise validate via the existing checkCLI helper) so checkCLI is applied consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/resolve-openshell.js`:
- Around line 17-22: Replace the unsafe shell interpolation of binPath
(execSync(`"${binPath}" --version`)) with a direct process execution using
execFileSync and argv (e.g., execFileSync(binPath, ['--version'], ...)) to
eliminate command injection risks and preserve encoding/timeout/stdio options;
also ensure the DI-injected path returned as commandVResult is validated by
calling the existing checkCLI function (the same validation used for the
`command -v` branch) before returning it to avoid false-positive resolutions.
---
Outside diff comments:
In `@bin/lib/resolve-openshell.js`:
- Around line 54-56: The branch that returns opts.commandVResult when it starts
with "/" bypasses the CLI validation; update this path to pass
opts.commandVResult through checkCLI(...) before returning so the new
binary-type guard runs in DI/test mode—specifically, locate the branch handling
opts.commandVResult (the code that currently returns opts.commandVResult) and
replace the bare return with a return of checkCLI(opts.commandVResult) (or
otherwise validate via the existing checkCLI helper) so checkCLI is applied
consistently.
🪄 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
Run ID: f307f723-e422-4169-be3b-13d4b5685e2d
📒 Files selected for processing (2)
bin/lib/onboard.jsbin/lib/resolve-openshell.js
Comment on lines
+17
to
+22
| const out = execSync(`"${binPath}" --version`, { | ||
| encoding: "utf-8", | ||
| timeout: 5000, | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }).trim(); | ||
| return /^openshell\s+\d+/.test(out); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'execSync\(`"\$\{binPath\}" --version`' bin/lib/resolve-openshell.jsRepository: NVIDIA/NemoClaw
Length of output: 268
🏁 Script executed:
cat -n bin/lib/resolve-openshell.js | head -80Repository: NVIDIA/NemoClaw
Length of output: 3189
Fix command injection vulnerability and add CLI validation to DI override path.
Line 17 interpolates binPath into a shell command string, which is vulnerable if the path contains shell metacharacters. Execute the binary directly with argv using execFileSync. Additionally, lines 54–56 return the DI-injected commandVResult without validating it with checkCLI, unlike the real command -v path at line 52, which creates an inconsistency that could produce false-positive resolution in tests.
Suggested fixes
-const { execSync } = require("child_process");
+const { execSync, execFileSync } = require("child_process");At line 17:
- const out = execSync(`"${binPath}" --version`, {
+ const out = execFileSync(binPath, ["--version"], {
encoding: "utf-8",
timeout: 5000,
stdio: ["ignore", "pipe", "ignore"],
}).trim();At lines 54–56:
} else if (opts.commandVResult && opts.commandVResult.startsWith("/")) {
- return opts.commandVResult;
+ if (checkCLI(opts.commandVResult)) return opts.commandVResult;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/resolve-openshell.js` around lines 17 - 22, Replace the unsafe shell
interpolation of binPath (execSync(`"${binPath}" --version`)) with a direct
process execution using execFileSync and argv (e.g., execFileSync(binPath,
['--version'], ...)) to eliminate command injection risks and preserve
encoding/timeout/stdio options; also ensure the DI-injected path returned as
commandVResult is validated by calling the existing checkCLI function (the same
validation used for the `command -v` branch) before returning it to avoid
false-positive resolutions.
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
1 participant
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.
Summary
Add
isOpenshellCLI()validation to prevent the npmopenshellgateway package from shadowing the real OpenShell CLI binary during install. Also surface a clear error with manual install steps wheninstall-openshell.shfails.Related Issue
Fixes #967
Changes
isOpenshellCLI()inbin/lib/resolve-openshell.js— runsopenshell --versionand verifies output matchesopenshell <version>command -vlookup and fallback candidate paths now verified withisOpenshellCLI()opts.checkCLIDI override for testabilityinstallOpenshell()inbin/lib/onboard.jsnow prints manual install commands on failureType of Change
Testing
isOpenshellCLI()correctly identifies the OpenShell CLI binary (openshell --version→openshell x.y.z)isOpenshellCLI()rejects the npm gateway packageinstallOpenshell()prints manual install steps on failureChecklist
General
Code Changes
Summary by CodeRabbit