fix: validate sandbox and instance names to prevent shell injection

[areporeporepo]

summary

this validates sandbox and instance names before they are interpolated into shell commands.

changes

• add validateName() in bin/lib/runner.js
• validate names in cli dispatch, deploy, and onboard sandbox creation
• reject invalid names early while preserving the existing valid-name path

testing

• npm test
• npm --prefix nemoclaw run build

checklist

• follows conventional commits
• commits are signed off

Summary by CodeRabbit

Release Notes

• New Features
  • Added validation for sandbox and instance names to ensure they meet naming requirements. Invalid names are now rejected upfront with informative error messages, preventing downstream issues.

[wscurran]

Thanks for tackling shell injection vulnerabilities by validating sandbox and instance names.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new name validation function was added to ensure sandbox and instance names comply with strict requirements (1–63 characters, alphanumeric start, alphanumeric plus ., _, - characters). The validator is called at two entry points in the CLI to reject invalid names early before proceeding with deployment or dispatch operations.

Changes

Cohort / File(s)	Summary
Name Validation Helper bin/lib/runner.js	Added exported validateName(name) function that validates names against regex pattern, logs error message, and exits on failure.
Validation Integration bin/nemoclaw.js	Imported validateName and integrated it at two control-flow entry points: deploy(instanceName) before credential checks and sandbox-scoped dispatch before registry lookup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops forth with a name and a plan,
But checks it twice—always the safest span!
No numbers-first chaos, no wild characters roam,
Just alphanumerics that know their way home. 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% 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 describes the main change: adding validation for sandbox and instance names to prevent shell injection, which aligns with the changeset's primary objective.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

Tip

You can customize the tone of the review comments and chat replies.

Configure the tone_instructions setting to customize the tone of the review comments and chat replies. For example, you can set the tone to Act like a strict teacher, Act like a pirate and more.

[coderabbitai]

🧹 Nitpick comments (1)

bin/nemoclaw.js (1)

83-86: Consider consolidating regex patterns for consistency.

setup() uses a different validation pattern (/^[a-z0-9][a-z0-9-]*[a-z0-9]$/) than the new validateName(). While the value comes from the registry (so should be pre-validated), this inconsistency could lead to confusion or subtle mismatches. Consider reusing validateName() here.

Similarly, start() at lines 183-184 uses /^[a-zA-Z0-9._-]+$/ which allows names starting with ., _, or -, unlike validateName().

🤖 Prompt for AI Agents

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

In `@bin/nemoclaw.js` around lines 83 - 86, The sandbox name validation is
inconsistent: replace the inline regex in setup() that builds safeName
(currently using /^[a-z0-9][a-z0-9-]*[a-z0-9]$/) with a call to the existing
validateName() helper to ensure a single canonical rule (use
registry.listSandboxes().defaultSandbox and pass it to validateName() before
assigning safeName and calling run(...)); likewise update start() to validate
the sandbox/name using validateName() instead of /^[a-zA-Z0-9._-]+$/ so names
starting with . _ - are rejected consistently; adjust safeName to an empty
string when validateName() returns false and keep the run(...) call as-is.

🤖 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/nemoclaw.js`:
- Around line 83-86: The sandbox name validation is inconsistent: replace the
inline regex in setup() that builds safeName (currently using
/^[a-z0-9][a-z0-9-]*[a-z0-9]$/) with a call to the existing validateName()
helper to ensure a single canonical rule (use
registry.listSandboxes().defaultSandbox and pass it to validateName() before
assigning safeName and calling run(...)); likewise update start() to validate
the sandbox/name using validateName() instead of /^[a-zA-Z0-9._-]+$/ so names
starting with . _ - are rejected consistently; adjust safeName to an empty
string when validateName() returns false and keep the run(...) call as-is.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7094695c-8a7d-46f5-b471-a412b9096374

📥 Commits

Reviewing files that changed from the base of the PR and between 20d0c95 and 21c74f6.

📒 Files selected for processing (2)

• bin/lib/runner.js
• bin/nemoclaw.js

[ericksoa]

Superseded by #584 which added validateName() and shellQuote() across all CLI entry points, covering sandbox names, instance names, model names, API keys, container names, and path traversal.