fix(sandbox): add missing exec-approvals.json symlink#1236
fix(sandbox): add missing exec-approvals.json symlink#1236TonyLuo-NV 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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a writable sandbox state file and symlink under Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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/sandbox-provisioning.test.js (1)
68-83: Consider extractingfs.readFileSyncto reduce redundant I/O.The Dockerfile content is read separately in each parameterized test iteration. While this works correctly, reading once at describe-block scope would be cleaner and slightly faster.
♻️ Optional: Extract file read to describe scope
describe("sandbox provisioning: all expected symlinks present in Dockerfile.base", () => { const EXPECTED_DIR_SYMLINKS = [ "agents", "extensions", "workspace", "skills", "hooks", "identity", "devices", "canvas", "cron", "memory", ]; const EXPECTED_FILE_SYMLINKS = ["update-check.json", "exec-approvals.json"]; + const dockerfileBaseSrc = fs.readFileSync(DOCKERFILE_BASE, "utf-8"); it.each(EXPECTED_DIR_SYMLINKS)("symlink for directory '%s' is created", (name) => { - const src = fs.readFileSync(DOCKERFILE_BASE, "utf-8"); - expect(src).toContain(`ln -s /sandbox/.openclaw-data/${name} /sandbox/.openclaw/${name}`); + expect(dockerfileBaseSrc).toContain(`ln -s /sandbox/.openclaw-data/${name} /sandbox/.openclaw/${name}`); }); it.each(EXPECTED_FILE_SYMLINKS)("symlink for file '%s' is created", (name) => { - const src = fs.readFileSync(DOCKERFILE_BASE, "utf-8"); - expect(src).toContain(`ln -s /sandbox/.openclaw-data/${name} /sandbox/.openclaw/${name}`); + expect(dockerfileBaseSrc).toContain(`ln -s /sandbox/.openclaw-data/${name} /sandbox/.openclaw/${name}`); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sandbox-provisioning.test.js` around lines 68 - 83, Move the fs.readFileSync(DOCKERFILE_BASE, "utf-8") call out of each it.each and into the surrounding describe scope so the Dockerfile contents are read once; create a local const (e.g. dockerfileSrc) in the describe block and replace the per-test reads in the it.each blocks that reference EXPECTED_DIR_SYMLINKS and EXPECTED_FILE_SYMLINKS to use dockerfileSrc instead.
🤖 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/sandbox-provisioning.test.js`:
- Around line 68-83: Move the fs.readFileSync(DOCKERFILE_BASE, "utf-8") call out
of each it.each and into the surrounding describe scope so the Dockerfile
contents are read once; create a local const (e.g. dockerfileSrc) in the
describe block and replace the per-test reads in the it.each blocks that
reference EXPECTED_DIR_SYMLINKS and EXPECTED_FILE_SYMLINKS to use dockerfileSrc
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7b6ef09-ce6d-4b06-a7e3-0ab5405c0f28
📒 Files selected for processing (4)
DockerfileDockerfile.basetest/e2e-gateway-isolation.shtest/sandbox-provisioning.test.js
TonyLuo-NV
force-pushed
the
fix/1027-exec-approvals-symlink
branch
from
e4adb21 to
8edce8e
Compare
CI failure analysis:
|
The OpenClaw gateway failed with EACCES when writing exec-approvals.json because no symlink existed from .openclaw/ to the writable .openclaw-data/ directory. Add the symlink and backing file in Dockerfile.base so the sandbox user can read/write exec approvals through the two-layer design. Keep openclaw.json and .config-hash as root:root 444 to preserve the NVIDIA#514 security mitigation (prevent agent from tampering with auth/CORS). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-gateway-isolation.sh (1)
193-206: Consider unifying the assertion style with Tests 2 and 5.Test 9d splits the stat output with awk for separate owner/permissions checks, while Tests 2 and 5 use exact string matching. Both approaches are correct, but unifying them would improve maintainability.
♻️ Optional: unified approach matching Tests 2/5
info "9d. /sandbox/.openclaw/ directory is owned by root:root with permissions 755" OUT=$(run_as_root "stat -c '%U:%G %a' /sandbox/.openclaw") -OWNER=$(echo "$OUT" | awk '{print $1}') -DIR_PERMS=$(echo "$OUT" | awk '{print $2}') -if [ "$OWNER" = "root:root" ]; then - pass ".openclaw directory owned by root:root" +if [ "$OUT" = "root:root 755" ]; then + pass ".openclaw directory is root:root 755" else - fail ".openclaw directory owner wrong: $OWNER (expected root:root)" -fi -if [ "$DIR_PERMS" = "755" ]; then - pass ".openclaw directory permissions are 755" -else - fail ".openclaw directory permissions wrong: $DIR_PERMS (expected 755)" + fail ".openclaw directory wrong: $OUT (expected root:root 755)" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-gateway-isolation.sh` around lines 193 - 206, Unify Test 9d to use an exact string match like Tests 2 and 5 by comparing the full stat output stored in OUT (from run_as_root "stat -c '%U:%G %a' /sandbox/.openclaw") against an expected string "root:root 755"; replace the separate OWNER/DIR_PERMS awk parsing and two checks with a single if [ "$OUT" = "root:root 755" ] then pass ".openclaw owned and perms OK" else fail ".openclaw owner/perms wrong: $OUT (expected root:root 755)" fi, updating the pass/fail messages and keeping references to OUT, run_as_root, pass, and fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 80-82: Update the misleading Dockerfile comment: clarify that
openclaw.json is immutable at runtime (root:root 444) and cannot be modified by
the gateway process, and state that runtime exec approvals are stored in
exec-approvals.json (symlinked) rather than openclaw.json; keep the note about
Landlock/root-owned directory as the primary security boundary but remove the
claim that the gateway updates openclaw.json for exec approvals.
---
Nitpick comments:
In `@test/e2e-gateway-isolation.sh`:
- Around line 193-206: Unify Test 9d to use an exact string match like Tests 2
and 5 by comparing the full stat output stored in OUT (from run_as_root "stat -c
'%U:%G %a' /sandbox/.openclaw") against an expected string "root:root 755";
replace the separate OWNER/DIR_PERMS awk parsing and two checks with a single if
[ "$OUT" = "root:root 755" ] then pass ".openclaw owned and perms OK" else fail
".openclaw owner/perms wrong: $OUT (expected root:root 755)" fi, updating the
pass/fail messages and keeping references to OUT, run_as_root, pass, and fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bc6fb3d-9e3b-49e1-8856-70280dc9c8fc
📒 Files selected for processing (4)
DockerfileDockerfile.basetest/e2e-gateway-isolation.shtest/sandbox-provisioning.test.js
✅ Files skipped from review due to trivial changes (1)
- Dockerfile.base
🚧 Files skipped from review as they are similar to previous changes (1)
- test/sandbox-provisioning.test.js
TonyLuo-NV
force-pushed
the
fix/1027-exec-approvals-symlink
branch
from
648be6c to
a1d8a94
Compare
- Correct misleading Dockerfile comment: openclaw.json is immutable at runtime (root:root 444), not updated by the gateway. Exec approvals live in exec-approvals.json via symlink. - Extract fs.readFileSync to describe-block scope in sandbox-provisioning tests to eliminate redundant I/O per test iteration. - Unify assertion style in e2e-gateway-isolation.sh Test 9d to use exact string matching consistent with Tests 2 and 5. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the exec-approvals symlink fix. The codebase has changed significantly since April 1 — including a full TypeScript migration — so this will need a rebase on |
wscurran
added
the
status: rebase
2 participants
Summary
Fixes #1027 —
GatewayRequestError: EACCES permission denied, open '/sandbox/.openclaw/exec-approvals.json'exec-approvals.jsondata file and symlink inDockerfile.baseso the sandbox user can read/write exec approvals through the existing two-layer design (.openclaw/→.openclaw-data/)openclaw.jsonand.config-hashasroot:root 444to preserve the Agent can self-modify openclaw.json to bypass auth and CORS controls #514 security mitigation (prevent agent from tampering with auth/CORS)Dockerfilereferencing Agent can self-modify openclaw.json to bypass auth and CORS controls #514 and OpenClaw GatewayRequestError — EACCES permission denied #1027refresh_config_hash()fromnemoclaw-start.shexec-approvals.json,memory,update-check.jsonto e2e symlink verificationsandbox-provisioning.test.jsunit tests verifying Dockerfile provisioning correctnessTest plan
npm test— 696 tests passed (cli + plugin projects)exec-approvals.jsonmissing and EACCES in dashboard → rebuilt with fix → confirmed symlink present and read/write worksopenclaw.jsonstaysroot:root 444(no Agent can self-modify openclaw.json to bypass auth and CORS controls #514 regression).config-hashstaysroot:root 444(integrity check preserved).openclaw/directory staysroot:root 755(agent cannot create files or replace symlinks)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
Tests