fix(security): command injection across all CLI entry points

[ericksoa]

Summary

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().

Supersedes #55, #110, #475, #540, #48, #171.

Changes

File	Change
bin/lib/runner.js	Add shellQuote() and validateName() as shared utilities
bin/nemoclaw.js	Replace execSync with execFileSync in deploy; apply shellQuote() to all user input in shell commands; add validateName() at CLI dispatch
bin/lib/onboard.js	Quote model names and API key in setupInference(); remove duplicate shellQuote()
bin/lib/nim.js	Quote container names, images, ports in all docker commands
bin/lib/policies.js	shellQuote() in policy commands; path traversal fix in loadPreset()
bin/lib/local-inference.js	Remove duplicate shellQuote(); import from runner.js
test/runner.test.js	9 new tests for shellQuote and validateName
test/policies.test.js	Updated for shell quoting; path traversal test added

Vulnerability classes fixed

1. Shell injection via instance name — nemoclaw deploy "test; rm -rf /" could execute arbitrary commands through 8+ unquoted SSH/rsync/scp/brev interpolations
2. Shell injection via sandbox name — sandbox names from CLI args or registry flowed into openshell commands unquoted
3. Shell injection via model name — openshell inference set --model ${model} was completely unquoted
4. Shell injection via API key — NVIDIA_API_KEY interpolated into shell strings with double quotes (expandable)
5. Shell injection via container name — docker commands used unquoted names derived from sandbox names
6. Path traversal in preset loader — loadPreset("../../etc/passwd") could read arbitrary files
7. Predictable temp file — Date.now() replaced with mkdtempSync()

Test results

32/32 unit tests pass. 14/14 integration tests pass including real bash round-trip verification with canary files proving no injection side effects.

Summary by CodeRabbit

• Security

  • Hardened shell-injection protections by consistently quoting user/environment-derived values across CLI, deployment, and service commands.
  • Added strict name validation to reject invalid or malicious instance/sandbox identifiers.
  • Improved temporary file handling using unpredictable temp directories and stricter file permissions with best-effort cleanup.

• Tests

  • Expanded coverage for quoting behavior, name validation, path traversal protections, and added regression guards to prevent regressions.

[coderabbitai]

📝 Walkthrough

Walkthrough

Centralized shell escaping and name validation into bin/lib/runner.js (shellQuote, validateName), replaced local quoting with these helpers across multiple modules, switched several execSync calls to execFileSync, added preset path-traversal protection, and expanded tests for quoting and name validation.

Changes

Cohort / File(s)	Summary
Core Utilities bin/lib/runner.js	Added and exported shellQuote(value) (single-quote escaping) and validateName(name, label) (RFC‑1123 style, max 63 chars). Exports updated.
Local Inference bin/lib/local-inference.js	Removed local shellQuote; now imports and uses shellQuote from runner in Ollama command builders.
NIM / Docker bin/lib/nim.js	Imported shellQuote; quoted container/image/name arguments in start/stop/status flows; coerced port to Number for health checks.
Onboarding bin/lib/onboard.js	Switched to runner shellQuote; quoted CHAT_UI_URL, NVIDIA_API_KEY, model and credential arguments in sandbox/provider command construction.
Policies & Presets bin/lib/policies.js	Applied shellQuote to --policy/--wait/--full args. Hardened loadPreset(name) with path.resolve confinement and replaced temp-file handling with a secure temp directory and restrictive file perms; added best-effort cleanup.
CLI / Sandbox & Scripts bin/nemoclaw.js, scripts/telegram-bridge.js	Replaced execSync with execFileSync (argv arrays). Imported shellQuote and validateName; validate names; quote user-derived values (instance, sandbox, API key, GPU); use secure temp dirs/files and improved cleanup; tightened remote-command assembly.
Tests test/runner.test.js, test/policies.test.js, test/onboard-readiness.test.js	Added tests for shellQuote and validateName, regression guards (no execSync in key files, single shellQuote location, CLI safety for malicious input, scripts validating SANDBOX_NAME). Updated expectations to single-quoted arguments and added preset path-traversal tests.

Sequence Diagram(s)

sequenceDiagram
    participant CLI
    participant Runner
    participant OS
    participant Remote

    CLI->>Runner: validateName(name)
    Runner-->>CLI: validated name / throw
    CLI->>Runner: shellQuote(value)
    Runner-->>CLI: quotedValue
    CLI->>OS: execFileSync(cmd, [args...quotedValue...])
    OS->>Remote: SSH / Docker / curl (argv-safe)
    Remote-->>OS: response
    OS-->>CLI: stdout / exit

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐇 I hop through code with whiskers bright,
I wrap each arg to keep things tight.
Quotes stitched snug, names checked just right,
Temp dirs tidy, commands polite.
A bunny patch that tucks the night. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the main change: comprehensive command injection vulnerability fixes across CLI entry points.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/comprehensive-shell-injection

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

bin/lib/onboard.js (1)

453-473: ⚠️ Potential issue | 🔴 Critical

Quote the env assignments before passing them to run().

Lines 463-466 still splice raw CHAT_UI_URL and NVIDIA_API_KEY values into a bash -c string at Line 473. A value like CHAT_UI_URL=http://x; touch /tmp/pwned # will execute on the host before openshell sandbox create even runs.

🐛 Proposed fix

   const basePolicyPath = path.join(ROOT, "nemoclaw-blueprint", "policies", "openclaw-sandbox.yaml");
   const createArgs = [
-    `--from "${buildCtx}/Dockerfile"`,
-    `--name "${sandboxName}"`,
-    `--policy "${basePolicyPath}"`,
+    `--from ${shellQuote(path.join(buildCtx, "Dockerfile"))}`,
+    `--name ${shellQuote(sandboxName)}`,
+    `--policy ${shellQuote(basePolicyPath)}`,
   ];
   // --gpu is intentionally omitted. See comment in startGateway().

   console.log(`  Creating sandbox '${sandboxName}' (this takes a few minutes on first run)...`);
   const chatUiUrl = process.env.CHAT_UI_URL || 'http://127.0.0.1:18789';
-  const envArgs = [`CHAT_UI_URL=${chatUiUrl}`];
+  const envArgs = [shellQuote(`CHAT_UI_URL=${chatUiUrl}`)];
   if (process.env.NVIDIA_API_KEY) {
-    envArgs.push(`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`);
+    envArgs.push(shellQuote(`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`));
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 453 - 473, The env assignments are
interpolated raw into the shell string passed to run(), allowing malicious
values to inject shell code; update the creation of envArgs (used when building
the command for run()) so each assignment is safely shell-quoted/escaped before
joining (e.g., wrap each CHAT_UI_URL=... and NVIDIA_API_KEY=... assignment in
proper single-quote/escaped form or use a shell-escaping helper) so the final
command passed to run() cannot execute injected content; ensure the change is
applied where envArgs is constructed and where run(...) is called (refer to
envArgs, run(), and createResult/sandboxName).

bin/lib/policies.js (1)

172-181: ⚠️ Potential issue | 🟠 Major

Replace the Date.now() temp path with a private temp directory.

Line 173 still creates a predictable filename in a shared temp location. Another local process can pre-create or swap that path before writeFileSync(), so the temp-file race this PR is trying to remove is still reachable here.

🐛 Proposed fix

   // Write temp file and apply
-  const tmpFile = path.join(os.tmpdir(), `nemoclaw-policy-${Date.now()}.yaml`);
-  fs.writeFileSync(tmpFile, merged, "utf-8");
+  const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-"));
+  const tmpFile = path.join(tmpDir, "policy.yaml");
+  fs.writeFileSync(tmpFile, merged, { encoding: "utf-8", mode: 0o600, flag: "wx" });

   try {
     run(buildPolicySetCommand(tmpFile, sandboxName));

     console.log(`  Applied preset: ${presetName}`);
   } finally {
-    fs.unlinkSync(tmpFile);
+    fs.rmSync(tmpDir, { recursive: true, force: true });
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/policies.js` around lines 172 - 181, The current code creates a
predictable temp file via path.join(os.tmpdir(),
`nemoclaw-policy-${Date.now()}.yaml`) which is vulnerable to TOCTOU; instead
create a private temp directory with fs.mkdtempSync(path.join(os.tmpdir(),
'nemoclaw-policy-')) and write the YAML into a file inside that dir (e.g.
path.join(tmpDir, 'policy.yaml')), then call run(buildPolicySetCommand(tmpFile,
sandboxName)) as before, and in the finally block unlink the file and remove the
temp directory (fs.rmdirSync or fs.rmSync) to ensure full cleanup; reference
tmpFile/tmpDir, fs.mkdtempSync, writeFileSync(tmpFile, merged,...),
buildPolicySetCommand, run, merged, presetName, sandboxName and adjust cleanup
accordingly.

🤖 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/nemoclaw.js`:
- Around line 161-165: The temporary secret file creation and cleanup around
envTmp and the scp upload are not exception-safe: wrap the upload call (the run
invocation that performs scp) in a try/finally so that fs.unlinkSync(envTmp) and
fs.rmdirSync(path.dirname(envTmp)) always run even if run(...) throws; locate
the block that constructs envTmp, writes it (fs.writeFileSync), calls run(`scp
... ${qname}:/home/ubuntu/nemoclaw/.env`) and move the unlinkSync/rmdirSync
calls into a finally block to guarantee removal of secrets on all failure paths.
- Around line 156-160: The code writes unescaped secret values into .env via the
envLines array (see envLines, getCredential) and later sources it, which allows
token content with newlines or shell metacharacters to be executed; fix by
escaping each value before adding to envLines (e.g., wrap values in single
quotes and escape any internal single quotes/newlines, or alternatively
serialize values using a safe format like base64 and decode when needed or stop
using shell `source` and use a dotenv parser), specifically update the logic
that builds envLines (including the NVIDIA_API_KEY line, GITHUB_TOKEN and
TELEGRAM_BOT_TOKEN entries) to perform proper shell-escaping/quoting of the
token strings before writing the file.

In `@test/runner.test.js`:
- Around line 154-168: The test "CLI rejects malicious sandbox names before
shell commands (e2e)" currently only asserts non-zero exit code which can mask a
regression where the injected "whoami" runs; update the test (in
test/runner.test.js) to also assert that the spawned process output does NOT
contain the current user name or typical whoami output by capturing
os.userInfo().username and checking that neither result.stdout nor result.stderr
includes that username (use spawnSync, result, stdout, stderr symbols) — this
proves the payload never executed in the child process.

---

Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 453-473: The env assignments are interpolated raw into the shell
string passed to run(), allowing malicious values to inject shell code; update
the creation of envArgs (used when building the command for run()) so each
assignment is safely shell-quoted/escaped before joining (e.g., wrap each
CHAT_UI_URL=... and NVIDIA_API_KEY=... assignment in proper single-quote/escaped
form or use a shell-escaping helper) so the final command passed to run() cannot
execute injected content; ensure the change is applied where envArgs is
constructed and where run(...) is called (refer to envArgs, run(), and
createResult/sandboxName).

In `@bin/lib/policies.js`:
- Around line 172-181: The current code creates a predictable temp file via
path.join(os.tmpdir(), `nemoclaw-policy-${Date.now()}.yaml`) which is vulnerable
to TOCTOU; instead create a private temp directory with
fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-policy-')) and write the YAML
into a file inside that dir (e.g. path.join(tmpDir, 'policy.yaml')), then call
run(buildPolicySetCommand(tmpFile, sandboxName)) as before, and in the finally
block unlink the file and remove the temp directory (fs.rmdirSync or fs.rmSync)
to ensure full cleanup; reference tmpFile/tmpDir, fs.mkdtempSync,
writeFileSync(tmpFile, merged,...), buildPolicySetCommand, run, merged,
presetName, sandboxName and adjust cleanup accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 794ed133-2aa7-444e-98bb-376a65730668

📥 Commits

Reviewing files that changed from the base of the PR and between a7102a4 and 3c31672.

📒 Files selected for processing (9)

• bin/lib/local-inference.js
• bin/lib/nim.js
• bin/lib/onboard.js
• bin/lib/policies.js
• bin/lib/runner.js
• bin/nemoclaw.js
• scripts/telegram-bridge.js
• test/policies.test.js
• test/runner.test.js

[ericksoa]

All five findings addressed in 3832ec3:

1. Secrets in .env unquoted (Critical) — Fixed. All three token values now wrapped with shellQuote() before writing to the env file.
2. Temp secret cleanup on scp failure (Major) — Fixed. Upload wrapped in try/finally with individual catch blocks for cleanup.
3. e2e test weak assertion (Minor) — Fixed. Test now uses a canary file to prove the injected payload never executes, not just that exit code is non-zero.
4. Raw env args in onboard run() (Critical, outside diff) — Fixed. CHAT_UI_URL and NVIDIA_API_KEY values shell-quoted before interpolation.
5. Predictable temp path in policies (Major, outside diff) — Fixed. Replaced Date.now() with mkdtempSync + mode: 0o600.

Also fixed 4 test expectation mismatches from the main merge (double-quote → single-quote for shellQuote()). 186/186 tests pass.

[coderabbitai]

🧹 Nitpick comments (1)

bin/lib/policies.js (1)

31-36: Path traversal protection is effective, but has redundant condition.

The path.resolve + startsWith check correctly prevents path traversal attacks. However, file !== PRESETS_DIR is redundant since the filename always ends with .yaml suffix (from `${name}.yaml`), making it impossible for file to equal PRESETS_DIR.

   const file = path.resolve(PRESETS_DIR, `${name}.yaml`);
-  if (!file.startsWith(PRESETS_DIR + path.sep) && file !== PRESETS_DIR) {
+  if (!file.startsWith(PRESETS_DIR + path.sep)) {
     console.error(`  Invalid preset name: ${name}`);
     return null;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/policies.js` around lines 31 - 36, In loadPreset, the path traversal
guard redundantly checks "file !== PRESETS_DIR" even though file is constructed
as `${name}.yaml`; remove that redundant comparison and keep the effective check
using path.resolve and startsWith(PRESETS_DIR + path.sep) to validate the
resolved file path (reference: function loadPreset and PRESETS_DIR).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@bin/lib/policies.js`:
- Around line 31-36: In loadPreset, the path traversal guard redundantly checks
"file !== PRESETS_DIR" even though file is constructed as `${name}.yaml`; remove
that redundant comparison and keep the effective check using path.resolve and
startsWith(PRESETS_DIR + path.sep) to validate the resolved file path
(reference: function loadPreset and PRESETS_DIR).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8a31217c-a4e9-4be7-aa8f-7465e89a1d79

📥 Commits

Reviewing files that changed from the base of the PR and between d749336 and 3832ec3.

📒 Files selected for processing (5)

• bin/lib/onboard.js
• bin/lib/policies.js
• bin/nemoclaw.js
• test/onboard-readiness.test.js
• test/runner.test.js

✅ Files skipped from review due to trivial changes (1)

• test/onboard-readiness.test.js

🚧 Files skipped from review as they are similar to previous changes (2)

• bin/lib/onboard.js
• bin/nemoclaw.js

[brianwtaylor]

▎ Reviewed — this is solid. Centralizing shellQuote + validateName in
runner.js is the right call, and the regression guards (canary e2e,
duplicate-definition ban, execSync scanner) are a nice touch.

▎ I closed #540 in favor of this — yours covers
more ground and goes further with the execFileSync migration. Better overall solution.

-brian

[kjw3]

Reviewed the latest rebase onto main and this looks ready.

What I checked:

• The security hardening is real and appropriately scoped: shared shell quoting, name validation, safer temp-file handling, and replacement of the highest-risk execSync shell paths with safer process invocation where it matters.
• The PR meaningfully consolidates behavior instead of scattering one-off fixes. Centralizing shellQuote() / validateName() in runner.js and removing duplicate implementations is the right direction.
• The follow-up issues raised during review were addressed in the final patch, including quoting secrets before sourcing .env, cleaning up temp secret files reliably, and covering the regression path with stronger tests.

Validation status:

• CI is green.
• Install flow validated after the rebase to current main.

Net: this is a real security improvement, not just churn, and it improves the codebase overall.

Let it rip 🤙