fix(security): shell-quote sandboxName in exec and DNS proxy commands

[latenighthackathon]

Summary

• Wrap sandboxName with shellQuote() in the dashboard readiness check (openshell sandbox exec)
• Wrap both GATEWAY_NAME and sandboxName with shellQuote() in the DNS proxy setup command
• Add regression test that scans onboard.js for any unquoted sandboxName in run()/runCapture() calls

Problem

sandboxName is interpolated into shell command strings passed to run()/runCapture() (which execute via bash -c) without shellQuote(). The same file imports and uses shellQuote() for other user-controlled values (lines 460, 1438, 3005) but omits it here.

Without quoting:

openshell sandbox exec test; rm -rf / curl ...

With quoting:

openshell sandbox exec 'test; rm -rf /' curl ...

Test plan

• 3 regression tests pass (vitest): quoted exec, quoted DNS proxy, no-unquoted-sandboxName scan
• Verified shellQuote() output in Docker (node:22-slim) for semicolon, subshell, and pipe injection payloads
• All 46 policy + security tests pass

Closes #1391

Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com

Summary by CodeRabbit

• Bug Fixes

  • Onboarding commands now properly quote sandbox and gateway identifiers, preventing failures when names contain spaces or special characters.

• Tests

  • Added tests that validate generated onboarding shell commands include the required quoting for sandbox and gateway identifiers to avoid injection or parsing issues.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: de01cd7a-b49c-4b4a-9329-3ee20a31e98e

📥 Commits

Reviewing files that changed from the base of the PR and between a4849ae and 01d86b9.

📒 Files selected for processing (1)

• bin/lib/onboard.js

---

📝 Walkthrough

Walkthrough

Applied shell quoting to sandboxName and GATEWAY_NAME in shell command strings inside bin/lib/onboard.js; added test/shellquote-sandbox.test.js to verify those values are quoted and to scan for any unquoted ${sandboxName} occurrences.

Changes

Cohort / File(s)	Summary
Security Fix bin/lib/onboard.js	Replaced unquoted ${sandboxName}/${GATEWAY_NAME} in shell command templates with shellQuote(sandboxName) and shellQuote(GATEWAY_NAME) for the openshell sandbox exec ... curl readiness check and the setup-dns-proxy.sh invocation.
Test Coverage test/shellquote-sandbox.test.js	Added Vitest that asserts shellQuote(sandboxName) is used in the openshell readiness command, asserts both shellQuote(GATEWAY_NAME) and shellQuote(sandboxName) appear in the DNS proxy command, and scans run/runCapture template usages to ensure no unquoted ${sandboxName} remain.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nudged the shells and tucked each name tight,

Quotes around values to sleep through the night,
A test to patrol every slippery spot,
I thumped my foot—no surprises, all caught,
🥕 under moonlight the sandbox is right.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 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 security fix: shell-quoting sandboxName in exec and DNS proxy commands, matching the main objectives of the changeset.
Linked Issues check	✅ Passed	The PR fully addresses all objectives from #1391: both sandboxName and GATEWAY_NAME are wrapped with shellQuote() in the exec and DNS proxy commands, and regression tests are added.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the security issue: shell-quoting in specified commands and adding regression tests. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ 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.}

[coderabbitai]

🧹 Nitpick comments (1)

test/shellquote-sandbox.test.js (1)

22-35: Strengthen the scanner to handle multiline run/runCapture calls.

The current per-line check can miss unquoted ${sandboxName} when the call and template literal span different lines. A block-level scan would make this regression test reliable.

Proposed refactor

   it("does not have unquoted sandboxName in runCapture or run calls", () => {
-    const lines = src.split("\n");
     const violations = [];
-    for (let i = 0; i < lines.length; i++) {
-      const line = lines[i];
-      if (
-        (line.includes("run(") || line.includes("runCapture(")) &&
-        line.includes("${sandboxName}") &&
-        !line.includes("shellQuote(sandboxName)")
-      ) {
-        violations.push(`Line ${i + 1}: ${line.trim()}`);
-      }
-    }
+    const callRegex = /\b(run|runCapture)\(\s*`([\s\S]*?)`\s*(?:,|\))/g;
+    let match;
+    while ((match = callRegex.exec(src)) !== null) {
+      const command = match[2];
+      if (command.includes("${sandboxName}") && !command.includes("${shellQuote(sandboxName)}")) {
+        const line = src.slice(0, match.index).split("\n").length;
+        violations.push(`Line ${line}: ${match[1]}(...) contains unquoted \${sandboxName}`);
+      }
+    }
     expect(violations).toEqual([]);
   });

🤖 Prompt for AI Agents

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

In `@test/shellquote-sandbox.test.js` around lines 22 - 35, The test currently
scans line-by-line so it misses multiline run/runCapture calls; instead treat
the file as a single string and search block-wise: iterate matches of
/\\b(run|runCapture)\\s*\\((?:[\\s\\S]*?)`([\\s\\S]*?)`[\\s\\S]*?\\)/g (or
equivalent) against src and for each match check if the captured template
contains "${sandboxName}" and does not contain "shellQuote(sandboxName)"; push a
violation with context if so and assert violations is empty. Update references
in the test to use src (the full file string), the run/runCapture regex matcher,
and the violations array logic accordingly.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@test/shellquote-sandbox.test.js`:
- Around line 22-35: The test currently scans line-by-line so it misses
multiline run/runCapture calls; instead treat the file as a single string and
search block-wise: iterate matches of
/\\b(run|runCapture)\\s*\\((?:[\\s\\S]*?)`([\\s\\S]*?)`[\\s\\S]*?\\)/g (or
equivalent) against src and for each match check if the captured template
contains "${sandboxName}" and does not contain "shellQuote(sandboxName)"; push a
violation with context if so and assert violations is empty. Update references
in the test to use src (the full file string), the run/runCapture regex matcher,
and the violations array logic accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 378a2e84-f191-436d-b392-67ab422ed1ca

📥 Commits

Reviewing files that changed from the base of the PR and between 6386bf8 and cf4ce32.

📒 Files selected for processing (2)

• bin/lib/onboard.js
• test/shellquote-sandbox.test.js