Skip to content

fix: validate openshell binary to prevent npm package shadowing #970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SaiSharan2005 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: validate openshell binary to prevent npm package shadowing #970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bin/lib/onboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,6 +1111,14 @@ function installOpenshell() {
if (output) {
console.error(output);
}
console.error("");
console.error(" OpenShell CLI binary failed to install.");
console.error(" You can install it manually:");
console.error("");
console.error(' curl -fsSL "https://github.com/NVIDIA/OpenShell/releases/latest/download/openshell-$(uname -m)-unknown-linux-musl.tar.gz" -o /tmp/openshell.tar.gz');
console.error(" tar xzf /tmp/openshell.tar.gz -C /tmp");
console.error(" install -m 755 /tmp/openshell /usr/local/bin/openshell");
console.error("");
return { installed: false, localBin: null, futureShellPathHint: null };
}
const localBin = process.env.XDG_BIN_HOME || path.join(process.env.HOME || "", ".local", "bin");
Expand Down
32 changes: 29 additions & 3 deletions bin/lib/resolve-openshell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,52 @@
const { execSync } = require("child_process");
const fs = require("fs");

/**
* Verify that the binary at `binPath` is the OpenShell CLI and not another
* package that happens to share the same name (e.g. the npm `openshell`
* gateway package installed as a transitive dependency of `openclaw`).
*
* The OpenShell CLI prints a version string starting with "openshell "
* when invoked with `--version`.
*/
function isOpenshellCLI(binPath) {
try {
const out = execSync(`"${binPath}" --version`, {
encoding: "utf-8",
timeout: 5000,
stdio: ["ignore", "pipe", "ignore"],
}).trim();
return /^openshell\s+\d+/.test(out);
Comment on lines +17 to +22
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -C3 'execSync\(`"\$\{binPath\}" --version`' bin/lib/resolve-openshell.js

Repository: NVIDIA/NemoClaw

Length of output: 268

🏁 Script executed:

cat -n bin/lib/resolve-openshell.js | head -80

Repository: 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.

} catch {
return false;
}
}

/**
* Resolve the openshell binary path.
*
* Checks `command -v` first (must return an absolute path to prevent alias
* injection), then falls back to common installation directories.
*
* Every candidate is verified with `isOpenshellCLI()` to ensure the resolved
* binary is the real OpenShell CLI and not a same-named npm package.
*
* @param {object} [opts] DI overrides for testing
* @param {string|null} [opts.commandVResult] Mock result (undefined = run real command)
* @param {function} [opts.checkExecutable] (path) => boolean
* @param {function} [opts.checkCLI] (path) => boolean — override for `isOpenshellCLI`
* @param {string} [opts.home] HOME override
* @returns {string|null} Absolute path to openshell, or null if not found
*/
function resolveOpenshell(opts = {}) {
const home = opts.home ?? process.env.HOME;
const checkCLI = opts.checkCLI || isOpenshellCLI;

// Step 1: command -v
if (opts.commandVResult === undefined) {
try {
const found = execSync("command -v openshell", { encoding: "utf-8" }).trim();
if (found.startsWith("/")) return found;
if (found.startsWith("/") && checkCLI(found)) return found;
} catch { /* ignored */ }
} else if (opts.commandVResult && opts.commandVResult.startsWith("/")) {
return opts.commandVResult;
Expand All @@ -40,10 +66,10 @@ function resolveOpenshell(opts = {}) {
"/usr/bin/openshell",
];
for (const p of candidates) {
if (checkExecutable(p)) return p;
if (checkExecutable(p) && checkCLI(p)) return p;
}

return null;
}

module.exports = { resolveOpenshell };
module.exports = { resolveOpenshell, isOpenshellCLI };