Replaced execSync with execFileSync in bin/nemoclaw.js to avoid spawning a shell for command execution#55
Closed
hypervenom1426 wants to merge 3 commits intoNVIDIA:mainfrom
Closed
Replaced execSync with execFileSync in bin/nemoclaw.js to avoid spawning a shell for command execution#55hypervenom1426 wants to merge 3 commits intoNVIDIA:mainfrom
hypervenom1426 wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
…th execFileSync in bin/nemoclaw.js to avoid spawning a shell for command execution.
fix: prevent command injection in deploy command
Replaced execSync with execFileSync in bin/nemoclaw.js to avoid spawning a shell for command execution. This structural change mitigates command injection vulnerabilities where user-provided input, such as the instance name, could be evaluated by the shell to execute arbitrary commands.
Specifically:
- Switched 'which brev' and 'brev ls' checks to use execFileSync.
- Handled 'brev ls' shell redirects programmatically by checking both err.stdout and err.stderr.
- Converted the ssh reachability check to use execFileSync with an array of arguments, replacing the redirect with { stdio: 'ignore' }."
…th execFileSync in bin/nemoclaw.js to avoid spawning a shell for command execution.
Replaced execSync with execFileSync in bin/nemoclaw.js to avoid spawning a shell for command execution. This structural change mitigates command injection vulnerabilities where user-provided input, such as the instance name, could be evaluated by the shell to execute arbitrary commands.
Specifically:
- Switched 'which brev' and 'brev ls' checks to use execFileSync.
- Handled 'brev ls' shell redirects programmatically by checking both err.stdout and err.stderr.
- Converted the ssh reachability check to use execFileSync with an array of arguments, replacing the redirect with { stdio: 'ignore' }."
deepujain
added a commit
to deepujain/NemoClaw
that referenced
this pull request
Mar 17, 2026
…A#55) Signed-off-by: Deepak Jain <deepujain@gmail.com>
5 tasks
Contributor
|
Thanks for mitigating the command injection vulnerability by replacing execSync with execFileSync, this improves the security of our command execution and protects against potential attacks. |
wscurran
added
the
priority: high
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.
Contributor
|
Hi @hypervenom1426! Nice catch switching to execFileSync — that's a good security improvement. Heads up that the repo has been evolving fast since this was opened, with new CI checks and features landing regularly. A rebase onto the latest main would be super helpful so we can review this in context. Thanks for the contribution! |
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
jnun
pushed a commit
to jnun/EasyClaw
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 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
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
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.
This structural change mitigates command injection vulnerabilities where user-provided input, such as the instance name, could be evaluated by the shell to execute arbitrary commands.
Specifically: