fix(sandbox): guard unsupported openclaw config writes#2416
Open
nvshaxie wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(sandbox): guard unsupported openclaw config writes#2416nvshaxie wants to merge 1 commit intoNVIDIA:mainfrom
nvshaxie wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Use a stable openclaw wrapper in the sandbox image so unsupported config-mutating commands fail with actionable guidance instead of surfacing low-level permission errors. Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughA wrapper script for the Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/Container
participant Wrapper as openclaw-wrapper.sh
participant RealCLI as openclaw-real
User->>Wrapper: openclaw configure [args]
Wrapper->>Wrapper: Detect 'configure' command
Wrapper-->>User: Error: Config is read-only<br/>Exit sandbox & run nemoclaw onboard --resume
User->>Wrapper: openclaw agents add [args]
Wrapper->>Wrapper: Detect 'agents add' command
Wrapper-->>User: Error: Adding agents unsupported<br/>in NemoClaw sandbox
User->>Wrapper: openclaw agent [--local] [args]
Wrapper->>Wrapper: Detect --local flag
Wrapper-->>User: [SECURITY] Warning: Local mode<br/>bypasses gateway protections
User->>Wrapper: openclaw tui [args]
Wrapper->>Wrapper: No blocking rules match
Wrapper->>RealCLI: exec openclaw-real tui [args]
RealCLI-->>User: Execute command normally
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Dockerfile (1)
258-265: Add a build-time passthrough smoke check after swapping binaries.After the move/install, a quick
openclaw --versioncheck would fail fast if wrapper delegation breaks.♻️ Suggested patch
RUN OPENCLAW_BIN="$(command -v openclaw)" \ && if [ "$OPENCLAW_BIN" != "/usr/local/bin/openclaw" ]; then \ echo "Error: expected openclaw at /usr/local/bin/openclaw, got $OPENCLAW_BIN"; \ exit 1; \ fi \ && mv "$OPENCLAW_BIN" /usr/local/bin/openclaw-real \ && install -m 755 /usr/local/lib/nemoclaw/openclaw-wrapper.sh /usr/local/bin/openclaw \ - && chmod 755 /usr/local/bin/openclaw-real + && chmod 755 /usr/local/bin/openclaw-real \ + && /usr/local/bin/openclaw --version >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 258 - 265, Add a smoke-check that runs openclaw --version after moving the real binary and installing the wrapper: after mv "$OPENCLAW_BIN" /usr/local/bin/openclaw-real and install -m 755 .../openclaw-wrapper.sh /usr/local/bin/openclaw, invoke the wrapper with a quick execution of openclaw --version (or command -v/openclaw-real if needed) and fail the build if it returns non-zero; ensure the check references OPENCLAW_BIN, /usr/local/bin/openclaw-real, and openclaw-wrapper.sh so it runs immediately after the swap and exits with a non-zero status on failure.scripts/openclaw-wrapper.sh (1)
45-45: Guard missing delegate binary with a clearer error beforeexec.If
/usr/local/bin/openclaw-realis absent, users get a raw shell error. An explicit check preserves the “actionable UX” goal even under wiring drift.♻️ Suggested patch
+if [ ! -x "$REAL_OPENCLAW" ]; then + echo "Error: OpenClaw runtime binary not found at $REAL_OPENCLAW." >&2 + echo "Rebuild the sandbox from the host with: nemoclaw onboard --resume" >&2 + exit 1 +fi + exec "$REAL_OPENCLAW" "$@"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/openclaw-wrapper.sh` at line 45, Add a guard before the final exec to check that the delegate binary referenced by REAL_OPENCLAW exists and is executable; if REAL_OPENCLAW is unset or not executable, print a clear error message to stderr mentioning the missing path and how to fix it (e.g., install the real binary or set REAL_OPENCLAW) and exit with a non-zero status instead of falling through to exec; update the block around the existing exec "$REAL_OPENCLAW" "$@" invocation to perform this check and early exit.test/openclaw-wrapper.test.ts (1)
13-21: Cover theopenclaw agent --localwarning branch too.The wrapper includes a security-warning path for
--local; adding assertions here will keep that behavior regression-proof.♻️ Suggested patch
it("blocks config-mutating commands with actionable guidance", () => { const src = fs.readFileSync(WRAPPER, "utf-8"); expect(src).toContain("openclaw configure"); expect(src).toContain("nemoclaw onboard --resume"); expect(src).toContain("openclaw agents add"); expect(src).toContain("not currently supported"); + expect(src).toContain("[SECURITY] Warning: 'openclaw agent --local' bypasses the NemoClaw gateway."); + expect(src).toContain("NOT enforced in local mode."); expect(src).toContain('exec "$REAL_OPENCLAW" "$@"'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/openclaw-wrapper.test.ts` around lines 13 - 21, Update the test "blocks config-mutating commands with actionable guidance" to also assert the wrapper contains the warning branch for the agent --local path: read WRAPPER as before and add expect assertions that src contains the `openclaw agent --local` (or `agent --local`) token and the specific security-warning text emitted for the --local branch, plus the existing `exec "$REAL_OPENCLAW" "$@"` assertion so the --local warning branch is covered and cannot regress; look for these strings in the test file `openclaw-wrapper.test.ts` around the existing expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 258-265: Add a smoke-check that runs openclaw --version after
moving the real binary and installing the wrapper: after mv "$OPENCLAW_BIN"
/usr/local/bin/openclaw-real and install -m 755 .../openclaw-wrapper.sh
/usr/local/bin/openclaw, invoke the wrapper with a quick execution of openclaw
--version (or command -v/openclaw-real if needed) and fail the build if it
returns non-zero; ensure the check references OPENCLAW_BIN,
/usr/local/bin/openclaw-real, and openclaw-wrapper.sh so it runs immediately
after the swap and exits with a non-zero status on failure.
In `@scripts/openclaw-wrapper.sh`:
- Line 45: Add a guard before the final exec to check that the delegate binary
referenced by REAL_OPENCLAW exists and is executable; if REAL_OPENCLAW is unset
or not executable, print a clear error message to stderr mentioning the missing
path and how to fix it (e.g., install the real binary or set REAL_OPENCLAW) and
exit with a non-zero status instead of falling through to exec; update the block
around the existing exec "$REAL_OPENCLAW" "$@" invocation to perform this check
and early exit.
In `@test/openclaw-wrapper.test.ts`:
- Around line 13-21: Update the test "blocks config-mutating commands with
actionable guidance" to also assert the wrapper contains the warning branch for
the agent --local path: read WRAPPER as before and add expect assertions that
src contains the `openclaw agent --local` (or `agent --local`) token and the
specific security-warning text emitted for the --local branch, plus the existing
`exec "$REAL_OPENCLAW" "$@"` assertion so the --local warning branch is covered
and cannot regress; look for these strings in the test file
`openclaw-wrapper.test.ts` around the existing expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c22a511c-badc-4d7d-93af-560f0d6de836
📒 Files selected for processing (6)
Dockerfilescripts/openclaw-wrapper.shsrc/lib/sandbox-build-context.tstest/onboard.test.tstest/openclaw-wrapper.test.tstest/sandbox-build-context.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openclawwrapper in the sandbox image soopenclaw configureandopenclaw agents addfail with actionable guidance instead of low-level permission errorsnemoclaw onboardcan copy it into rebuilt sandbox imagesTest plan
openclaw agents add ...sandbox failure on Brev before the fixnemoclaw, then recreated the sandbox on Brev/usr/local/bin/openclawresolves to the wrapper and/usr/local/bin/openclaw-realresolves to the real binary inside the sandboxopenclaw agents add my_agent --workspace ~/.openclaw/workspace-my-agentnow returns the new UX guard messageopenclaw configurenow returns the new UX guard messageopenclaw --versionstill passes through to the real binaryvitestrun in this workspace is currently blocked by the existing Node/vitestERR_REQUIRE_ESMenvironment issueFixes #2145
Made with Cursor
Summary by CodeRabbit
New Features
Tests