Skip to content

fix: validate sandbox names to prevent shell injection#52

Closed
futhgar wants to merge 2 commits intoNVIDIA:mainfrom
futhgar:fix/validate-sandbox-names
Closed

fix: validate sandbox names to prevent shell injection#52
futhgar wants to merge 2 commits intoNVIDIA:mainfrom
futhgar:fix/validate-sandbox-names

Conversation

@futhgar
Copy link
Copy Markdown

@futhgar futhgar commented Mar 16, 2026

Summary

Sandbox and instance names from user input are interpolated directly into shell commands (openshell sandbox connect ${name}, docker run --name ${name}, etc.) without validation. A name containing shell metacharacters (;, $(), backticks, pipes) could execute arbitrary commands on the host.

Note: This addresses the same issue as #45 but differs in error handling approach and includes unit tests. Key differences:

  • validateName() throws an Error instead of calling process.exit(1), allowing callers to handle validation failures contextually
  • CLI dispatch catches the error silently and falls through to the "Unknown command" handler, preserving existing UX for typos like nemoclaw boguscmd
  • Includes 7 unit tests (test/runner.test.js)

Fix

Add validateName() to runner.js enforcing ^[a-zA-Z0-9][a-zA-Z0-9._-]{0,62}$ and call it at the three user-input entry points:

  • CLI sandbox dispatchnemoclaw <name> <action> (nemoclaw.js)
  • deploy commandnemoclaw deploy <instance-name> (nemoclaw.js)
  • onboard wizard — sandbox name prompt (onboard.js)

Changes

File Change
bin/lib/runner.js Add validateName() with regex enforcement and export it
bin/nemoclaw.js Import validateName, call it in deploy() and sandbox dispatch
bin/lib/onboard.js Import validateName, call it after sandbox name prompt
test/runner.test.js 7 unit tests covering valid names, length limits, empty input, metacharacter rejection

Test plan

  • npm test — 45/45 pass (38 existing + 7 new)
  • nemoclaw my-assistant connect — valid name, passes validation
  • nemoclaw "test; echo pwned" connect — rejected with clear error
  • nemoclaw deploy '$(whoami)' — rejected
  • nemoclaw onboard with empty name → defaults to my-assistant (valid)
  • nemoclaw boguscmd — still shows "Unknown command" (not "Invalid name")

Fixes #45

futhgar added 2 commits March 16, 2026 17:09
The pypi and npm presets used protocol: rest with tls: terminate, which
intercepts TLS connections and restricts traffic to explicit HTTP method
rules. This breaks pip and npm because both tools establish HTTPS
connections via CONNECT tunneling, which the TLS-terminating proxy
rejects with 403 errors.

Switch both presets to access: full, matching the pattern already used
by the github and npm_registry policies in the base sandbox
configuration (openclaw-sandbox.yaml). This allows package managers to
establish direct TLS connections to their registries.

Fixes NVIDIA#19

Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
Sandbox and instance names from user input are interpolated directly
into shell commands (openshell sandbox connect, docker run --name, etc.)
without validation. A name containing shell metacharacters could execute
arbitrary commands on the host.

Add validateName() to runner.js enforcing ^[a-zA-Z0-9][a-zA-Z0-9._-]{0,62}$
and call it at the three user-input entry points:

- CLI sandbox dispatch (nemoclaw <name> <action>)
- deploy command (nemoclaw deploy <instance-name>)
- onboard wizard (sandbox name prompt)

Includes 7 unit tests covering valid names, length limits, empty input,
shell metacharacter rejection, and custom error labels.

Fixes NVIDIA#45

Signed-off-by: futhgar <jmaldonado.rosa@gmail.com>
@futhgar
Copy link
Copy Markdown
Author

futhgar commented Mar 19, 2026

Closing — this overlaps with #170 which was merged and addresses the same sandbox name validation issue. Thanks!

@futhgar futhgar closed this Mar 19, 2026
mafueee pushed a commit to mafueee/NemoClaw that referenced this pull request Mar 28, 2026
Closes NVIDIA#48, NVIDIA#52

## Summary
- Replace the envoy-gateway-based TLS setup with inline PKI generation during cluster bootstrap, generating CA, server, and client certificates directly in the `navigator-bootstrap` crate
- Remove all envoy gateway Helm templates (`gateway.yaml`, `gatewayclass.yaml`, `grpcroute.yaml`, PKI job, traffic policies) and the `Dockerfile.pki-job`
- Add native mTLS support to the navigator server with `tokio-rustls`, mounting client TLS certs as volumes into sandbox pods
- Update cluster entrypoint, healthcheck, and deploy scripts to work with the new direct-TLS architecture
- Add TLS security e2e test and fix formatting/clippy warnings

## Test Plan
- All unit tests pass (`cargo test --workspace`)
- Clippy clean (`cargo clippy --workspace --all-targets`)
- Format clean (`cargo fmt --all -- --check`)
- Python tests pass (`uv run pytest python/`)
- Full `mise run pre-commit` passes
mafueee pushed a commit to mafueee/NemoClaw that referenced this pull request Mar 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant