fix(deploy): use execFileSync to avoid command injection (Fixes #55) - #110
Closed
deepujain wants to merge 0 commit into
Closed
fix(deploy): use execFileSync to avoid command injection (Fixes #55)#110deepujain wants to merge 0 commit into
deepujain wants to merge 0 commit into
Conversation
5 tasks
ericksoa
added a commit
that referenced
this pull request
Mar 21, 2026
Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes #55, #110, #475, #540, #48, #171.
Collaborator
|
@deepujain — the execFileSync change to prevent command injection is exactly the kind of hardening we want. Quick ask: this repo has moved forward quite a bit (new CI, features, refactors), so could you rebase this branch on the latest main? That'll let us review it against the current codebase. You've been a consistent contributor and we really appreciate it! |
kjw3
pushed a commit
that referenced
this pull request
Mar 21, 2026
* security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes #55, #110, #475, #540, #48, #171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
deepujain
force-pushed
the
fix/55-deploy-execFileSync
branch
from
March 21, 2026 22:32
5491424 to
1dbf82f
Compare
Contributor
Author
Ryuketsukami
pushed a commit
to Ryuketsukami/NemoClaw
that referenced
this pull request
Mar 24, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
jessesanford
pushed a commit
to jessesanford/NemoClaw
that referenced
this pull request
Mar 24, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
mafueee
pushed a commit
to mafueee/NemoClaw
that referenced
this pull request
Mar 28, 2026
6 tasks
wscurran
added
feature
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
3 participants
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
Replaces
execSyncwithexecFileSync/spawnSyncin the deploy path so user-controlled input (instance name) is not passed through a shell. This mitigates command injection if the instance name were ever sourced from untrusted input.Fixes #55.
Changes
which brev:execSync("which brev", ...)→execFileSync("which", ["brev"], ...)so no shell is spawned.brev ls:execSync("brev ls 2>&1", ...)→spawnSync("brev", ["ls"], { encoding: "utf-8", shell: false })and combine stdout + stderr programmatically to detect existing instances.execSync(\ssh ... ${name} 'echo ok'`, ...)→execFileSync("ssh", ["-o", "ConnectTimeout=5", "-o", "StrictHostKeyChecking=no", name, "echo ok"], ...)` so the instance name is a single argument and not interpreted by a shell.Testing
npm test— all tests pass. No new unit tests (deploy path is integration-only; security hardening).