Fix #166: Quote sandbox names in shell commands#335
Fix #166: Quote sandbox names in shell commands#335hkc5 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe command constructions in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
wscurran
added
the
NemoClaw CLI
Fixes NVIDIA#166 - Add shellEscape() function to properly escape sandbox names in shell commands by wrapping in single quotes and handling embedded single quotes correctly - Apply shell escaping to all shell commands that use sandboxName: - openshell sandbox delete - openshell sandbox create --name - openshell forward start This replaces the previous double-quoted interpolation ("${sandboxName}") with shellEscape(), which is more robust and prevents shell injection when sandbox names contain special characters.
hkc5
force-pushed
the
fix-sandbox-name-quoting
branch
from
bf84ea6 to
fd3b4a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/onboard.js (2)
60-71: Duplicate shell-escaping helper exists.A similar function
shellQuote()already exists at lines 101-103 using the'\''escape pattern. Both patterns are valid POSIX techniques, but having two functions for the same purpose violates DRY. Consider consolidating into a single helper.♻️ Suggested consolidation
Either rename and reuse the existing
shellQuote()for all shell-escaping needs, or replace it with this newshellEscape():-function shellQuote(value) { - return `'${String(value).replace(/'/g, `'\\''`)}'`; -} +// Reuse shellEscape for all shell quoting needs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 60 - 71, There are two duplicate helpers (shellEscape and shellQuote); remove the duplication by consolidating to a single helper and updating call sites: choose one name (prefer keep shellQuote or rename shellEscape) and delete the other function, then replace all references to the removed function with the retained function (ensure the retained implementation uses a POSIX-safe pattern such as replacing ' with '\'' or equivalent used in shellQuote/shellEscape). Update any module exports or imports in this file to reference only the chosen function (shellEscape or shellQuote) so all usages call the single consolidated helper.
454-454: Shell escaping applied correctly to these call sites.The use of
shellEscape(sandboxName)at sandbox delete (line 454), create--name(line 473), and forward start (line 541) is consistent with the PR objectives.However, other
sandboxNameusages in this file still use double-quoted interpolation without escaping:
- Line 237:
openshell sandbox get "${sandboxName}"- Line 523:
openshell sandbox delete "${sandboxName}"- Line 816:
openshell sandbox connect "${sandboxName}"While the sandbox name validation at line 429 restricts input to
[a-z0-9-](making injection impossible), the inconsistent escaping pattern is a code smell. Consider applyingshellEscape()uniformly for maintainability and defense-in-depth.♻️ Apply shellEscape consistently
- const exists = runCapture(`openshell sandbox get "${sandboxName}" 2>/dev/null`, { ignoreError: true }); + const exists = runCapture(`openshell sandbox get ${shellEscape(sandboxName)} 2>/dev/null`, { ignoreError: true });- const delResult = run(`openshell sandbox delete "${sandboxName}" 2>/dev/null || true`, { ignoreError: true }); + const delResult = run(`openshell sandbox delete ${shellEscape(sandboxName)} 2>/dev/null || true`, { ignoreError: true });- run(`cat <<'EOF_NEMOCLAW_SYNC' | openshell sandbox connect "${sandboxName}" + run(`cat <<'EOF_NEMOCLAW_SYNC' | openshell sandbox connect ${shellEscape(sandboxName)}Also applies to: 473-473, 541-541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` at line 454, Replace the remaining double-quoted interpolations of sandboxName with shellEscape(sandboxName) to make escaping consistent and defense-in-depth; specifically update any calls that build shell commands with sandboxName (e.g., run("openshell sandbox get \"${sandboxName}\""), run("openshell sandbox delete \"${sandboxName}\""), and run("openshell sandbox connect \"${sandboxName}\"")) to use shellEscape(sandboxName) instead, keeping the same command text (openshell sandbox get/delete/connect and the --name option) and reusing the existing shellEscape helper and run function so all run(...) invocations embed the escaped sandboxName variable.
🤖 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/onboard.js`:
- Around line 60-71: There are two duplicate helpers (shellEscape and
shellQuote); remove the duplication by consolidating to a single helper and
updating call sites: choose one name (prefer keep shellQuote or rename
shellEscape) and delete the other function, then replace all references to the
removed function with the retained function (ensure the retained implementation
uses a POSIX-safe pattern such as replacing ' with '\'' or equivalent used in
shellQuote/shellEscape). Update any module exports or imports in this file to
reference only the chosen function (shellEscape or shellQuote) so all usages
call the single consolidated helper.
- Line 454: Replace the remaining double-quoted interpolations of sandboxName
with shellEscape(sandboxName) to make escaping consistent and defense-in-depth;
specifically update any calls that build shell commands with sandboxName (e.g.,
run("openshell sandbox get \"${sandboxName}\""), run("openshell sandbox delete
\"${sandboxName}\""), and run("openshell sandbox connect \"${sandboxName}\""))
to use shellEscape(sandboxName) instead, keeping the same command text
(openshell sandbox get/delete/connect and the --name option) and reusing the
existing shellEscape helper and run function so all run(...) invocations embed
the escaped sandboxName variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9453f3e3-9d85-4b90-bccc-de3a8f1279b2
📒 Files selected for processing (1)
bin/lib/onboard.js
Removes the newly added shellEscape() function which duplicated the pre-existing shellQuote() helper. All call sites updated to use shellQuote().
2 participants
Summary
shellEscape()helper function that wraps strings in single quotes and safely handles embedded single quotes (replacing the previous"${sandboxName}"double-quote interpolation)shellEscape()to all three shell commands that usesandboxName:openshell sandbox deleteopenshell sandbox create --nameopenshell forward startThis is a re-submission of the quoting fix originally in PR #211, which was accidentally opened with
mainas both the head and base branch. Only the quoting/escaping changes are included here — the lowercase normalization changes are tracked separately in PR #212.Test plan
my-assistant) — should work as beforeshellEscape()correctly handles names containing single quotes (e.g.it's) — should produce'it'"'"'s'sandbox delete,sandbox create --name,forward start) all use the escaped nameSummary by CodeRabbit