fix(security): harden process execution — shell injection + credential leak via ps aux

[dumko2001]

Summary

Closes #325 — API key exposed in ps aux when creating inference provider.

This PR is a comprehensive hardening across all 3 execution layers. It covers the credential leak (also targeted by #330 and #382) plus the underlying shell injection vector that those PRs do not address.

---

Relationship to other open PRs

PR	What it fixes	Approach	Gaps
#330	Credential leak in 4 files	process.env[key] = val (global mutation)	No shell injection fix
#382	Credential leak in 6 files	per-call options?.env (cleaner)	No shell injection fix
#335	Sandbox name injection in onboard.js	shellEscape() — still passes through bash -c	Other files untouched; bypass risk if run() ever changes
#381	validate_name() in a bash script	Shell-level guard only	JS layer untouched
This PR	All of the above + more	argv arrays (no shell interpreter) + per-call env	—

If #330 or #382 merges first, this PR rebases cleanly — same per-call env pattern as #382.

Why argv arrays beat shellEscape(): passing [prog, ...args] to spawnSync with shell: false gives a hard guarantee — no shell interpreter at all. shellEscape() silently stops working if the surrounding run() call ever stops using bash -c.

---

Commits

Commit 1 — fix(runner): safe argv primitives + opts.env overwrite fix (bin/lib/runner.js)

• runArgv(prog, args, opts?) — spawnSync with shell: false; metacharacter-proof
• runCaptureArgv(prog, args, opts?) — execFileSync with shell: false; returns stdout
• assertSafeName(name) — validates [a-zA-Z0-9][a-zA-Z0-9_-]{0,62}, exits 1 on rejection
• mergeEnv(opts) fixed: old code { ...opts.env } silently dropped PATH/HOME/DOCKER_HOST when caller passed partial env

Commit 2 — fix(cli): shell-string → argv arrays across all CJS files

• bin/lib/onboard.js, bin/lib/nim.js, bin/lib/policies.js, bin/nemoclaw.js
• assertSafeName called before every command taking user-controlled name
• setupSpark(): removes NVIDIA_API_KEY=VALUE from sudo argv (sudo -E inherits env)
• Temp policy file written with { mode: 0o600 } instead of world-readable default

Commit 3 — fix(credentials): env-lookup form for --credential (all 3 layers)

• nemoclaw/src/commands/onboard.ts: per-call { env: { [credentialEnv]: apiKey } } — no global process.env mutation (same pattern as security: pass provider credentials via environment instead of CLI arguments #382)
• nemoclaw-blueprint/orchestrator/runner.py: run_cmd gets extra_env param; type-based target_cred_env fallback
• nemoclaw-blueprint/blueprint.yaml: credential_env: NVIDIA_API_KEY added to default profile (absence caused silent auth failures)
• nemoclaw/src/onboard/config.ts: { mode: 0o600 } on config write
• nemoclaw/dist/: compiled TS output updated

---

Test evidence

node --test test/*.test.js  ->  84 pass, 0 fail
npx vitest run              ->  22 pass, 0 fail
npx tsc --noEmit            ->  clean

New test files:

• test/runner.test.js (22 tests): assertSafeName rejects ;, $(, |, ../; runCaptureArgv metachar-proof; mergeEnv preserves PATH
• test/credential-exposure.test.js (6 tests): static scan + runtime PoC confirms no KEY=value in argv

Note: #330 also adds test/credential-exposure.test.js. Our version includes those static assertions plus 3 runtime injection PoC tests. If #330 merges first we carry our extras forward.

---

Before / after: ps aux

Before: openshell provider create --name nvidia-nim --credential "NVIDIA_API_KEY=nvapi-abc123..."

After: openshell provider create --name nvidia-nim --credential NVIDIA_API_KEY

Value lives only in the child process environment, never in argv.

---

Supersedes: #148, #191, #225, #330, #335. Complements: #382.

Summary by CodeRabbit

Release Notes

• New Features

  • Local inference provider auto-detection: vllm and ollama providers are now automatically detected and selected when available locally.

• Bug Fixes

  • Enhanced security for command execution to prevent shell injection vulnerabilities.
  • Improved credential handling to prevent accidental exposure in process listings.
  • Stricter input validation for sandbox and instance names.

• Tests

  • Added regression test suite for credential exposure detection.
  • Extended runner test coverage for safety and reliability.

[github-actions]

This repository limits contributors to 10 open pull requests. Please close or merge existing PRs before opening new ones.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e64e99a8-afe3-4f65-9607-f195c1a2a1e3

📥 Commits

Reviewing files that changed from the base of the PR and between 3ba517d and 892bc25.

⛔ Files ignored due to path filters (5)

• nemoclaw/dist/commands/onboard.d.ts.map is excluded by !**/dist/**, !**/*.map
• nemoclaw/dist/commands/onboard.js is excluded by !**/dist/**
• nemoclaw/dist/commands/onboard.js.map is excluded by !**/dist/**, !**/*.map
• nemoclaw/dist/onboard/config.js is excluded by !**/dist/**
• nemoclaw/dist/onboard/config.js.map is excluded by !**/dist/**, !**/*.map

📒 Files selected for processing (11)

• bin/lib/nim.js
• bin/lib/onboard.js
• bin/lib/policies.js
• bin/lib/runner.js
• bin/nemoclaw.js
• nemoclaw-blueprint/blueprint.yaml
• nemoclaw-blueprint/orchestrator/runner.py
• nemoclaw/src/commands/onboard.ts
• nemoclaw/src/onboard/config.ts
• test/credential-exposure.test.js
• test/runner.test.js

---

📝 Walkthrough

Walkthrough

Command execution throughout the codebase migrates from shell-string execution to argv-based operations to prevent unintended shell interpretation. Input validation via assertSafeName is introduced to prevent unsafe sandbox/instance names. Credential handling is refactored to pass secrets via environment variables rather than embedding them in CLI arguments. New test coverage validates safety properties.

Changes

Cohort / File(s)	Summary
Core Runner Utilities bin/lib/runner.js	Added assertSafeName input validator, runArgv and runCaptureArgv for shell-safe execution, mergeEnv helper, and updated stdout-capture with try/catch and ignoreError handling. Extended module exports with new safety and argv functions.
CLI Command Handlers bin/lib/nim.js, bin/lib/onboard.js, bin/lib/policies.js, bin/nemoclaw.js	Migrated Docker, curl, OpenShell, and script invocations from shell-string run/runCapture to argv-based runArgv/runCaptureArgv. Added assertSafeName validation for sandbox/instance names. Replaced inline shell pipelines and file operations with Node.js fs methods and explicit argv construction.
Configuration & Orchestration nemoclaw-blueprint/blueprint.yaml, nemoclaw-blueprint/orchestrator/runner.py, nemoclaw/src/commands/onboard.ts, nemoclaw/src/onboard/config.ts	Updated credential handling: credential_env now specifies which environment variable holds secrets; credentials passed via process env rather than --credential KEY=VALUE arguments. Added execOpenShell env parameter merging. Set file permissions to 0o600 for sensitive config files.
Test Coverage test/credential-exposure.test.js, test/runner.test.js	New comprehensive test suite for assertSafeName, argv-based execution, environment variable handling, and regression assertions ensuring credentials never leak via literal CLI arguments or shell interpolation patterns.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~28 minutes

Poem

🐰 From shell strings to argv arrays so clean,
No secrets exposed where ps can be seen,
Names validated, credentials concealed,
The safety migration is sealed!
Shell-special chars now literals stay—
A hardened approach, hip-hop-hooray! 🔒

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

Tip

CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.

Add a .trivyignore file to your project to customize which findings Trivy reports.

[dumko2001]

@erickosa — flagging this for your review when you get a chance.

Context: This PR fixes issue #325 (API key visible in ps aux via --credential KEY=VALUE). It's rebased on today's upstream main (commit 3ba517d) and all 176 tests pass.

What makes this PR additive over #382: PR #382 fixes the credential leak. This PR does that plus:

• runArgv/runCaptureArgv in runner.js — argv-array execution that is structurally shell-injection-proof (no bash -c, no metacharacter interpretation ever)
• assertSafeName guard on every user-controlled name entering a command
• mergeEnv fix — the old { ...process.env, ...opts.env, ...opts } spread silently dropped PATH/HOME/DOCKER_HOST when a caller passed opts.env
• blueprint.yaml default profile gets credential_env: NVIDIA_API_KEY (its absence caused silent auth failures)
• config.ts writes with { mode: 0o600 }
• 34 regression tests (injection PoC, mergeEnv preservation, stdin isolation)

Happy to split into smaller PRs or rebase on #382 if that's preferred. No rush — just wanted to make sure the shell injection layer gets visibility.