fix(security): shell-quote sandboxName in exec and DNS proxy commands#1392
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughApplied shell quoting to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/shellquote-sandbox.test.js (1)
22-35: Strengthen the scanner to handle multilinerun/runCapturecalls.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
📒 Files selected for processing (2)
bin/lib/onboard.jstest/shellquote-sandbox.test.js
wscurran
added
security
Builds on NVIDIA#1392 by validating sandboxName overrides at the createSandbox boundary, moving the dashboard readiness check onto the structured OpenShell helper path, and running the DNS proxy setup through argv-style execution instead of bash -c interpolation. Adds regression coverage for the new runner helper, invalid sandboxNameOverride rejection, and the createSandbox command paths. Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Signed-off-by: 13ernkastel <LennonCMJ@live.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Clean fix — the two-line change is exactly right, and the regression test catching future unquoted usages is a nice touch.
One ask before merge: please rebase onto current main so we can run E2E cleanly. Once that's done this is ready for an approving review.
latenighthackathon
force-pushed
the
fix/shellquote-sandbox-name
branch
from
01d86b9 to
dc69c6a
Compare
|
Rebased onto current main - ready for E2E. Thanks for the review @ericksoa! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
2105-2107: Consider quotingGATEWAY_NAMEhere for consistency.For consistency with the defense-in-depth approach applied to the DNS proxy command (line 2513), you may want to also apply
shellQuote(GATEWAY_NAME)here and at line 2167. WhileGATEWAY_NAMEis currently a safe constant, consistent quoting reduces cognitive load and future-proofs against changes.♻️ Suggested change
- run(`bash "${path.join(SCRIPTS, "fix-coredns.sh")}" ${GATEWAY_NAME} 2>&1 || true`, { + run(`bash "${path.join(SCRIPTS, "fix-coredns.sh")}" ${shellQuote(GATEWAY_NAME)} 2>&1 || true`, { ignoreError: true, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 2105 - 2107, Wrap the GATEWAY_NAME argument to the run call in a shell-safe quoted form to match the defense-in-depth used elsewhere: update the run invocation that calls the fix-coredns.sh script (the call using run(`bash "${path.join(SCRIPTS, "fix-coredns.sh")}" ${GATEWAY_NAME} 2>&1 || true`, { ignoreError: true })) to pass shellQuote(GATEWAY_NAME) instead of raw GATEWAY_NAME; apply the same change at the other occurrence noted (the similar run call around line 2167) so both uses of GATEWAY_NAME consistently use shellQuote before interpolating into the command string.
🤖 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 2105-2107: Wrap the GATEWAY_NAME argument to the run call in a
shell-safe quoted form to match the defense-in-depth used elsewhere: update the
run invocation that calls the fix-coredns.sh script (the call using run(`bash
"${path.join(SCRIPTS, "fix-coredns.sh")}" ${GATEWAY_NAME} 2>&1 || true`, {
ignoreError: true })) to pass shellQuote(GATEWAY_NAME) instead of raw
GATEWAY_NAME; apply the same change at the other occurrence noted (the similar
run call around line 2167) so both uses of GATEWAY_NAME consistently use
shellQuote before interpolating into the command string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4efba74e-6c27-424e-8324-9d9cbc824823
📒 Files selected for processing (1)
bin/lib/onboard.js
|
@latenighthackathon It looks like some of the onboarding tests are timing out. Can you merge latest main and try running locally? If you have any questions please let us know! |
sandboxName and GATEWAY_NAME are interpolated into shell command strings passed to run()/runCapture() without shellQuote(), which is inconsistent with other user-controlled values in the same file (lines 460, 1438, 3005). Wrap both values with shellQuote() to prevent shell metacharacter interpretation. Add regression test that scans onboard.js for any unquoted sandboxName in run/runCapture calls. Closes NVIDIA#1391 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Line-by-line scanning misses multiline run()/runCapture() calls. Switch to a regex that matches the full call including the template literal, so violations spanning multiple lines are caught. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
latenighthackathon
force-pushed
the
fix/shellquote-sandbox-name
branch
from
1f62f31 to
7f24f7b
Compare
|
@brandonpelfrey Thanks for the heads up! Rebased onto current main. CI should pick up a fresh run. Let me know if the timeout persists and I'll dig into it locally. |
| @@ -0,0 +1,40 @@ | |||
| // Verify shellQuote is applied to sandboxName in shell commands | |||
There was a problem hiding this comment.
Sorry for missing this before. SPDX header is missing. Should be automatically applied by the pre-commit hook.
|
@latenighthackathon looks good, just one last request re: SPDX header in the new file. Thank you again |
latenighthackathon
force-pushed
the
fix/shellquote-sandbox-name
branch
from
57501bc to
361bc58
Compare
|
@brandonpelfrey Thanks! Added the SPDX header — should be good to go now. |
#1566) ## Summary - Update 6 `runCapture` mock matchers in `test/onboard.test.js` to match the shell-quoted sandbox name format introduced by 8f8b4e7 - The security fix wrapped `sandboxName` with `shellQuote()` in exec and DNS proxy commands, but the test mocks still matched the old unquoted format (`sandbox exec my-assistant curl` vs `sandbox exec 'my-assistant' curl`) - This caused 4 CI failures: 3 timeouts (dashboard readiness loop never matched) and 1 assertion error (`null !== 0`) ## Test plan - [x] `npx vitest run` — all 1126 tests pass (67 files, 0 failures) - [x] All pre-commit and pre-push hooks pass Fixes the CI break introduced by #1392. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated test matchers to correctly detect sandbox invocations that use a quoted sandbox name in command strings. * Simplified test logic for scanning run/runCapture templates into an equivalent single-line expression, preserving existing violation detection and assertions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
4 participants
Summary
sandboxNamewithshellQuote()in the dashboard readiness check (openshell sandbox exec)GATEWAY_NAMEandsandboxNamewithshellQuote()in the DNS proxy setup commandonboard.jsfor any unquotedsandboxNameinrun()/runCapture()callsProblem
sandboxNameis interpolated into shell command strings passed torun()/runCapture()(which execute viabash -c) withoutshellQuote(). The same file imports and usesshellQuote()for other user-controlled values (lines 460, 1438, 3005) but omits it here.Without quoting:
With quoting:
Test plan
shellQuote()output in Docker (node:22-slim) for semicolon, subshell, and pipe injection payloadsCloses #1391
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests