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 2 commits into
base: main
Choose a base branch
from
+37 −3
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
2 commits
Select commit Hold shift + click to select a range
9785e4f
fix: validate openshell binary to prevent npm package shadowing
Mar 26, 2026
ccc87e2
Merge branch 'main' into fix/openshell-binary-detection
cv Mar 30, 2026
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 @@ -1383,6 +1383,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("");
Comment on lines +1386 to +1393
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Changed fallback block in bin/lib/onboard.js =="
nl -ba bin/lib/onboard.js | sed -n '1378,1405p'

echo
echo "== Locate install-openshell.sh =="
SCRIPT_PATH="$(fd -HI 'install-openshell\.sh$' | head -n1)"
echo "$SCRIPT_PATH"

echo
echo "== Installer script (first 260 lines) =="
nl -ba "$SCRIPT_PATH" | sed -n '1,260p'

echo
echo "== Platform/arch handling tokens in installer =="
rg -n 'darwin|linux|musl|uname -m|uname -s|aarch64|arm64|x86_64|amd64|release' "$SCRIPT_PATH"

Repository: NVIDIA/NemoClaw

Length of output: 169

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Changed fallback block in bin/lib/onboard.js (lines 1378-1405) =="
cat -n bin/lib/onboard.js | sed -n '1378,1405p'

echo ""
echo "== Locate install-openshell.sh =="
SCRIPT_PATH="$(fd -HI 'install-openshell\.sh$' | head -n1)"
echo "Found: $SCRIPT_PATH"

echo ""
echo "== Installer script (first 300 lines) =="
cat -n "$SCRIPT_PATH" | sed -n '1,300p'

echo ""
echo "== Platform/arch handling tokens in installer =="
rg -n 'darwin|linux|musl|uname|aarch64|arm64|x86_64|amd64|release' "$SCRIPT_PATH" | head -30

Repository: NVIDIA/NemoClaw

Length of output: 6127

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for installOpenshell invocation points =="
rg -n 'installOpenshell' bin/lib/onboard.js | head -20

echo ""
echo "== Check if onboard.js is platform-specific or universal =="
rg -n -A5 'function installOpenshell\|const installOpenshell' bin/lib/onboard.js | head -30

echo ""
echo "== Check for any macOS exclusions in onboarding logic =="
rg -n 'Darwin|uname -s|process.platform.*darwin|process.platform.*win32' bin/lib/onboard.js | head -20

Repository: NVIDIA/NemoClaw

Length of output: 254

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Context around installOpenshell call (line 1532) =="
cat -n bin/lib/onboard.js | sed -n '1520,1545p'

echo ""
echo "== Function signature and early lines of installOpenshell =="
cat -n bin/lib/onboard.js | sed -n '1373,1395p'

echo ""
echo "== Check broader context for platform filtering =="
cat -n bin/lib/onboard.js | sed -n '1500,1550p'

echo ""
echo "== Check if process.platform is used to gate onboarding steps =="
rg -n 'process\.platform|process\.env\.OSTYPE' bin/lib/onboard.js | head -20

Repository: NVIDIA/NemoClaw

Length of output: 5528

Fallback install instructions are platform-incompatible and may fail due to incorrect asset naming and missing permission checks.

The hardcoded manual install commands (lines 1390–1392) assume Linux-musl and will fail on macOS where NemoClaw onboarding is supported. The asset name uses $(uname -m) directly, but uname -m returns amd64 or arm64 on some systems, whereas OpenShell releases use x86_64 and aarch64. Additionally, the fallback always attempts installation to /usr/local/bin without checking write permissions; the installer script properly handles this by falling back to ~/.local/bin if needed.

Align the fallback logic with scripts/install-openshell.sh by:

  1. Detecting OS (Darwin vs. Linux) and mapping to correct asset naming
  2. Mapping architecture labels (uname -m → release-compatible names)
  3. Checking /usr/local/bin writability; fall back to ~/.local/bin if needed
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 1386 - 1393, The fallback OpenShell
manual-install message and commands are platform- and arch-incorrect and ignore
install permissions; update the fallback logic in the OpenShell install block
(where the manual curl/tar/install lines are emitted) to mirror
scripts/install-openshell.sh: detect OS (Darwin vs Linux) and choose the correct
release asset prefix, map uname -m to release arch names (e.g., amd64→x86_64,
arm64→aarch64), and check writability of /usr/local/bin (fall back to
~/.local/bin if not writable) before printing the manual install commands so the
curl URL and target path are correct for the user’s platform and permissions.

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 };
Loading