fix(shields,state): keep gateway readable and runtime sessions writable under shields-up#4155
Open
laitingsheng wants to merge 19 commits into
Open
fix(shields,state): keep gateway readable and runtime sessions writable under shields-up#4155laitingsheng wants to merge 19 commits into
laitingsheng wants to merge 19 commits into
Conversation
…te root-owned subdirs in audit NemoClaw #4065: shields-up locked HIGH_RISK_STATE_DIRS as root:root, which stripped sandbox-group ownership from descendants and broke OpenClaw plugin discovery (`extensions/<plugin>/` became unreachable to the gateway, which is only granted sandbox-group access). It also left `agents/main/` non-writable to the sandbox user, so the OpenClaw TUI's lazy mkdir of `agents/main/sessions/` failed with EACCES on first launch under lockdown. Switch the state-dir lock to `root:sandbox` (top-level configDir is still `root:root`) so the gateway keeps `r-x` via the sandbox group on descendants stripped to 2750 by `chmod -R go-w`, and restore `agents/*/sessions` to `sandbox:sandbox 2770` after the main lock loop so the agent keeps writing session metadata under lockdown. NemoClaw #4059: pre-backup audit joined per-dir `find` invocations with `&&`, so a single permission-denied subdir made the whole chain exit 1 and the rebuild treated every state dir as failed. Join with `;` and wrap each `find` with `|| true` — the audit's real signal is its stdout (symlink / hardlink / special-file rows); exit codes from perm-denied root-owned subdirs are noise. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
fix
Sandbox
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughExtract state-dir locking into a shared module with a PrivilegedExec adapter, add symlink preflight checks, implement applyStateDirLockMode (lock/unlock, verify, restore writable runtime subpaths), wire into shields up/down, and add tests and docs validating atomicity and symlink safety. ChangesShields Lockdown Refactoring and Runtime Permission Hardening
Sequence DiagramsequenceDiagram
participant CLI as CLI (shields command)
participant Index as shields/index
participant StateLock as state-dir-lock module
participant PrivExec as PrivilegedExec adapter
participant Sandbox as docker/exec (container)
participant FS as Host filesystem / fixtures
CLI->>Index: run lockAgentConfig
Index->>PrivExec: create stateDirLockExec (exec + capture)
Index->>StateLock: preflightStateDirLock(privExec, configDir)
StateLock->>PrivExec: capture sh -c preflight (workspace-* & roots)
PrivExec->>Sandbox: docker exec -- sh -c preflight
Sandbox->>PrivExec: stdout markers (symlinked roots or OK)
StateLock->>Index: return issues[] or []
alt issues found
Index->>CLI: throw "Config not locked: ..."
else no issues
Index->>StateLock: applyStateDirLockMode(privExec, configDir, owner, isLocking=true)
StateLock->>PrivExec: capture lock scripts (high-risk, workspace-*, confidentiality)
PrivExec->>Sandbox: docker exec -- sh -c lock scripts
Sandbox->>PrivExec: stdout markers (mutation results)
StateLock->>PrivExec: capture verify + restore scripts
PrivExec->>Sandbox: docker exec -- sh -c restore agents/*/sessions
Sandbox->>FS: sessions dirs created with sandbox:sandbox 2770
StateLock->>Index: return [] (no issues) or issues
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 4 needs attention, 7 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/state/sandbox.ts (1)
1108-1122: Run the state lifecycle E2E jobs for this audit-semantic change before merge.This changes backup audit behavior under permission-denied traversal, so it’s worth validating with
state-backup-restore-e2e,snapshot-commands-e2e, andrebuild-openclaw-e2e.As per coding guidelines: "
src/lib/state/sandbox.ts: This file manages sandbox state ... E2E test recommendation: state-backup-restore-e2e, snapshot-commands-e2e, rebuild-openclaw-e2e."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/state/sandbox.ts` around lines 1108 - 1122, This change alters the backup audit behavior in src/lib/state/sandbox.ts (see the auditCmd construction and the "Pre-backup audit" log) so run the full state lifecycle E2E suites before merging: execute state-backup-restore-e2e, snapshot-commands-e2e, and rebuild-openclaw-e2e against a fresh sandbox to validate permission-denied traversal is tolerated and legitimate rebuilds still succeed; if any test fails, adjust the auditCmd logic or error handling around the shell-quoted dir mapping (the code building auditCmd and the surrounding pre-backup audit flow) until the E2E suites pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/snapshot.test.ts`:
- Around line 1056-1171: Update the fake ssh stub used in the two tests ("treats
audit-find exit 1 with empty stdout as a successful audit (NemoClaw `#4059`)" and
"still rejects violations from readable dirs even if a sibling find exits
non-zero") so that when the command contains "find " the script exits non-zero
(e.g., process.exit(1)) unless the command string also contains the tolerant
shape "|| true"; in other words, change the cmd.includes("find ") branch in both
writeExecutable calls to check for cmd.includes("|| true") and only return
non-zero when that tolerant token is absent, preserving existing stdout behavior
for other branches.
---
Nitpick comments:
In `@src/lib/state/sandbox.ts`:
- Around line 1108-1122: This change alters the backup audit behavior in
src/lib/state/sandbox.ts (see the auditCmd construction and the "Pre-backup
audit" log) so run the full state lifecycle E2E suites before merging: execute
state-backup-restore-e2e, snapshot-commands-e2e, and rebuild-openclaw-e2e
against a fresh sandbox to validate permission-denied traversal is tolerated and
legitimate rebuilds still succeed; if any test fails, adjust the auditCmd logic
or error handling around the shell-quoted dir mapping (the code building
auditCmd and the surrounding pre-backup audit flow) until the E2E suites pass.
🪄 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: Enterprise
Run ID: b97cf8fe-d359-487b-8af3-17cb329e2c93
📒 Files selected for processing (5)
src/lib/shields/index.tssrc/lib/state/sandbox.tstest/repro-2681-group-writable.test.tstest/repro-4065-shields-up-runtime-perms.test.tstest/snapshot.test.ts
…file Address review: scrub `#4065` / `#4059` mentions from production code comments and test docstrings, and rename the new shields-up regression test from `repro-4065-…` to `shields-up-runtime-perms.test.ts` so the filename describes behaviour rather than an issue number. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26355812622
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26356219674
|
…d test Address review: 1. `restoreWritableRuntimeSubpaths` expanded the full pattern `agents/*/sessions` as a single glob. On a fresh sandbox where `sessions` does not exist yet, the glob has no matches and the shell leaves the literal pattern, which the `*"*"*` guard then drops — so `sessions/` was never created and the post-lockdown TUI mkdir still failed with EACCES. Split each pattern into a parent glob (expanded against the existing tree) plus a leaf to create, so the helper always mkdir's the missing leaf inside every existing parent. 2. The two pre-backup audit tests stubbed the SSH fake as always-exit-0 on `find`, so the `|| true` tolerance wrapper was not actually exercised. Make the fake exit non-zero with a Permission-denied stderr unless the audit cmd includes `|| true`, so the tests fail loudly if the wrapper is dropped. 3. New behavioural test runs the actual restore-helper script body against a real filesystem fixture and asserts that `agents/main/sessions` is created when only `agents/main` exists beforehand. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/shields/index.ts`:
- Around line 490-499: The loop in restoreWritableRuntimeSubpaths
(WRITABLE_RUNTIME_SUBPATHS handling) currently treats symlinked directories as
regular dirs and thus creates/mutates sessions/ on the symlink target; update
the parent existence guard to skip symlinks (e.g., require directory AND not a
symlink) or explicitly check [ -L "$parent" ] and continue when true so
mkdir/chown/chmod are not applied to symlink targets; make this change around
the loop that iterates parents (the for parent in "$@"; do ... [ -d "$parent" ]
|| continue portion) to ensure symlinked runtime parents are ignored.
In `@test/shields-up-runtime-perms.test.ts`:
- Around line 129-135: The test extracts patterns with restoreShell.slice(4)
which includes configDir, causing configDir to be passed twice into spawnSync
and altering the argv layout; change the extraction to restoreShell.slice(5) so
that patterns contains only the glob patterns, leaving the explicit "configDir"
argument in the spawnSync call (referencing restoreShell, script, patterns, and
the spawnSync invocation).
🪄 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: Enterprise
Run ID: 1eb0107a-ac61-4d44-ab10-07c74795f156
📒 Files selected for processing (3)
src/lib/shields/index.tstest/shields-up-runtime-perms.test.tstest/snapshot.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26356782985
|
… test argv slice Address review: 1. The privileged restore helper called `[ -d "$parent" ]` before `mkdir`/`chown`/`chmod`, but `[ -d ]` follows symlinks. A pre-lockdown agent that swapped `agents/<id>` for a symlink to an arbitrary host path could redirect the post-lock `mkdir -p ".../sessions"` and `chown -R sandbox:sandbox` through that link and rewrite ownership on any directory the privileged exec context can reach. Drop the parent (and the target leaf) when either is a symlink, before any mutation. 2. The behavioural test extracted patterns with `slice(4)`, which kept the captured `configDir` in the argv passed to bash — so the helper ran with `configDir` listed twice and the test argv diverged from the real `privilegedSandboxExec` call shape. Use `slice(5)` so only the patterns are forwarded. 3. New behavioural test asserts the symlink guard: when `agents/<id>` is a symlink to a sibling host directory, the helper must not create `sessions/` under either the link target or the link itself. Also reword one comment to avoid contested terminology. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/shields-up-runtime-perms.test.ts (1)
135-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
sh(notbash) when replaying the restore helperBoth restore-shell captures match
sh -c, but both behavioral replays runspawnSync("bash", ["-c", ...]), which can mask/bin/shcompatibility regressions. Switch the replay shell toshin both replay blocks (around lines 135-139 and 173-177).♻️ Proposed fix
- const result = spawnSync( - "bash", - ["-c", `${script}\n`, "sh", configDir, ...patterns], - { encoding: "utf-8", timeout: 5000 }, - ); + const result = spawnSync( + "sh", + ["-c", `${script}\n`, "sh", configDir, ...patterns], + { encoding: "utf-8", timeout: 5000 }, + );Apply the same change to the second replay block as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shields-up-runtime-perms.test.ts` around lines 135 - 139, In test/shields-up-runtime-perms.test.ts the replay uses spawnSync("bash", ["-c", ...]) which hides /bin/sh compatibility issues; change both replay invocations to use spawnSync("sh", ["-c", ...]) instead (update the call sites where spawnSync is invoked with "bash" in the two replay blocks around the current spawnSync(...) usages so they call "sh" and keep the same args and options).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/shields-up-runtime-perms.test.ts`:
- Around line 135-139: In test/shields-up-runtime-perms.test.ts the replay uses
spawnSync("bash", ["-c", ...]) which hides /bin/sh compatibility issues; change
both replay invocations to use spawnSync("sh", ["-c", ...]) instead (update the
call sites where spawnSync is invoked with "bash" in the two replay blocks
around the current spawnSync(...) usages so they call "sh" and keep the same
args and options).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea69a11f-9462-4b87-aa13-0ece1af0192b
📒 Files selected for processing (3)
src/lib/shields/index.tstest/shields-up-runtime-perms.test.tstest/snapshot.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26357693833
|
…ed comment
Address review:
1. The behavioural test for the runtime-subpath restore was running the
captured script body under `bash` rather than `sh`, while the
production helper invokes it via `sh -c`. A bash-only construct
slipping into the helper would pass the test but break the real call
site. Switch the replay to `spawnSync("sh", …)` in both fixtures so
the test covers exactly what the privileged exec runs.
2. Drop the NC-2227-05 tag from the updated `lockAgentConfig` comment.
The state-directory ownership story is now self-contained in
`HIGH_RISK_STATE_DIRS` doc + the helper itself; the issue tag does
not add information and conflicts with the no-issue-refs-in-comments
guidance applied across the rest of this PR.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/shields/index.ts (1)
399-458:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject symlinked state-dir roots before recursive
chmod -R/chown -Rlock stepGNU coreutils defaults make
chmod -Rfollow symlink command-line arguments, so if aconfigDir/<stateDir>orconfigDir/workspace-*entry is a symlink to a directory, the recursivechmod -Rcan apply outsideconfigDirduring the shields-up lock path. (chown -Rdefaults to non-dereference for symlink command-line args, but the recursivechmod -Ris enough for this to be a security/correctness blocker.)Suggested hardening
for (const dirName of HIGH_RISK_STATE_DIRS) { const dirPath = `${configDir}/${dirName}`; + try { + privilegedSandboxExec(sandboxName, [ + "sh", + "-c", + '[ -d "$1" ] && [ ! -L "$1" ]', + "sh", + dirPath, + ]); + } catch { + continue; + } try { privilegedSandboxExec(sandboxName, ["chown", "-R", owner, dirPath]); } catch { // Directory may not exist for this agent — silently skip } @@ for dir in "$config_dir"/workspace-*; do + [ -L "$dir" ] && continue [ -d "$dir" ] || continue chown -R "$owner" "$dir" 2>/dev/null || true chmod "$dir_mode" "$dir" 2>/dev/null || true [ "$clear_setgid" = "1" ] && chmod g-s "$dir" 2>/dev/null || true chmod -R "$recursive_mode" "$dir" 2>/dev/null || true doneAlso applies to: 756-760
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/index.ts` around lines 399 - 458, The recursive chown/chmod operations in the HIGH_RISK_STATE_DIRS loop and the workspace-* shell block can follow symlinked roots and affect paths outside configDir; before calling privilegedSandboxExec for any dirPath or iterating workspace-* entries (symbols: HIGH_RISK_STATE_DIRS, privilegedSandboxExec, configDir, dirPath, workspace-* pattern, recursiveMode, dirMode, owner, clearSetgid, isLocking), reject/skips any entry that is a symlink (use a non-following lstat/test -L check) so recursive -R operations are only applied to real directories; update both the TypeScript loop (skip when lstat indicates symlink) and the embedded shell script (skip entries where [ -L "$dir" ] ) to avoid following symlink roots.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/shields/index.ts`:
- Around line 399-458: The recursive chown/chmod operations in the
HIGH_RISK_STATE_DIRS loop and the workspace-* shell block can follow symlinked
roots and affect paths outside configDir; before calling privilegedSandboxExec
for any dirPath or iterating workspace-* entries (symbols: HIGH_RISK_STATE_DIRS,
privilegedSandboxExec, configDir, dirPath, workspace-* pattern, recursiveMode,
dirMode, owner, clearSetgid, isLocking), reject/skips any entry that is a
symlink (use a non-following lstat/test -L check) so recursive -R operations are
only applied to real directories; update both the TypeScript loop (skip when
lstat indicates symlink) and the embedded shell script (skip entries where [ -L
"$dir" ] ) to avoid following symlink roots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf8d1c18-3301-4936-baba-55a23d2cf2e7
📒 Files selected for processing (2)
src/lib/shields/index.tstest/shields-up-runtime-perms.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26358552843
|
…p lock
Address review:
1. The shields-up lock loop ran `chown -R`/`chmod -R`/`chmod g-s` on
each `${configDir}/${dirName}` (and on every `workspace-*` glob hit)
without rejecting symlinked roots. A pre-lockdown agent that swapped
e.g. `extensions/` or `workspace-main/` for a symlink to a host path
could redirect those recursive ownership and mode mutations at an
attacker-controlled directory. Consolidate the per-state-dir loop
into a single privileged shell exec that skips symlinks (`[ -L "$path" ]
&& continue`) before any mutation, and add the same guard to the
existing `workspace-*` shell loop.
2. Drop the `NC-2227-05` issue tag from the state-directory header
comment for consistency with the rest of this PR.
Updates the regression tests:
- `repro-2681`: assert the unlock fan-out via the new `sh -c` script
shape (workspace included as an arg, plus the workspace-* glob path
still present).
- `shields-up-runtime-perms`: assert the state-dir lock and workspace-*
scripts both contain the `[ -L … ] && continue` guard, and add a
behavioural fixture that proves a symlinked `extensions/` root is
skipped (its host target keeps its original mode and file contents).
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26359318121
|
Address review: Previously, when shields-up encountered a symlinked high-risk state-dir root (or a symlinked `workspace-*` dir), the privileged lock script silently skipped it via `[ -L "$path" ] && continue`. That refused to follow the link — good — but left the dir as-is and reported the lock as successful. The sandbox would then sit in "shields up (lockdown active)" status while a state-dir root still pointed at a writable host path, exactly the security regression the symlink guard was meant to prevent. Have the two consolidated lock shell scripts always exit 0 but emit `symlinked-root\t<path>` on stdout for every symlinked root they refuse to touch. `applyStateDirLockMode` parses those lines and returns them as lock failures when invoked under `isLocking=true`. `lockAgentConfig` now throws "Config not locked: state dir root is a symlink: …" before any further verification, refusing to acknowledge shields-up. Unlock is unchanged: skipping symlinked roots is the correct best-effort behaviour there. New regression test exercises the end-to-end path: when the captured shell script reports a symlinked root via the mocked exec, the `lockAgentConfig` call throws with the expected diagnostic. Tests for the static script shape are updated to match the new `if [ -L … ]; then printf …; fi` form instead of the previous `[ -L … ] && continue`. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26360739914
|
… + doc update Address review: 1. The inline `[ -L … ] && printf 'symlinked-root\t…'` guard in the mutation script ran per-iteration. A symlinked root later in the list could still leave earlier (non-symlinked) state dirs already reowned to `root:sandbox` by the time the lock helper bailed out. shields-up would then report "config not locked" while the tree was partially mutated. Add a dedicated preflight pass that runs before any `chown`/`chmod`, scans every high-risk state-dir root *and* every `workspace-*` dir for symlinks, and returns the full list. When `isLocking=true` and the preflight finds any symlinked root, `applyStateDirLockMode` short-circuits without touching the mutation pass or the sessions-restore helper, and `lockAgentConfig` throws `Config not locked: state dir root is a symlink: …`. The inline symlink guards in the mutation scripts stay for defence-in-depth in case the preflight and the mutation observe different fs state. 2. New regression test mocks the preflight script to report a symlinked root and asserts (a) `lockAgentConfig` throws with the expected diagnostic and (b) no mutation calls (state-dir lock, workspace-* lock, or sessions-restore) were ever issued. 3. Add a second regression test that asserts a dedicated preflight script (no `chown`/`chmod`, just the `[ -L … ] && printf` checks) is present in the recorded call sequence. 4. Update `docs/security/best-practices.mdx` to document the new `root:sandbox` state-dir ownership, the `agents/<id>/sessions` runtime carve-out, and the hard fail on symlinked state-dir roots. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4155.docs.buildwithfern.com/nemoclaw |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26361443080
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26381246289
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26458071131
|
cv
previously approved these changes
May 27, 2026
cv
approved these changes
May 30, 2026
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26691453416
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26691795412
|
…leged-exec in tests Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…ip group read on credentials Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/security/best-practices.mdx (2)
204-204: ⚡ Quick winRewrite in active voice.
Line 204 contains multiple passive constructions that should be rewritten in active voice. As per coding guidelines, active voice is required in NemoClaw documentation.
Examples from this line:
- "are owned by
root:sandbox" → "The lock helper owns these directories asroot:sandbox"- "the sandbox user is denied write" → "the lock denies write access to the sandbox user"
- "dirs that aren't present... are silently skipped" → "the lock helper silently skips dirs that aren't present"
As per coding guidelines: "Active voice required. Flag passive constructions. Bad: 'A gateway is created by the CLI.' Good: 'The CLI creates a gateway.'"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/security/best-practices.mdx` at line 204, Rewrite the sentence in active voice throughout: have the lock helper own the high-risk state directories as root:sandbox (instead of "are owned by root:sandbox"), state that the lock helper lets the OpenClaw gateway (a member of the sandbox group) keep read access while the lock denies write access to the sandbox user via chmod -R go-w, and say the lock helper applies root:root 700 with chmod -R go-rwX to secret directories to prevent gateway and sandbox reads; explicitly state that the lock helper restores sandbox:sandbox 2770 on shields-down, that the list is the union of state directories declared by every shipped agent manifest and that the lock helper silently skips dirs not present in an agent's config tree, and finally mark runtime-mutable subtrees (sessions/, memories/, logs/, cache/, plans/, openclaw-weixin/) as intentionally exempt so agents can continue operating.
204-206: ⚡ Quick winSplit into one sentence per line.
Lines 204-206 contain multiple sentences on single lines, which makes diffs harder to review. As per coding guidelines, documentation source should use one sentence per line.
Proposed rewrite with one sentence per line
Line 204 should be split into approximately:
Under lockdown, the high-risk state directories (`skills`, `hooks`, `cron`, `agents`, `extensions`, `plugins`, `workspace`, `memory`, `devices`, `canvas`, `telegram`, `wechat`, `whatsapp`, `platforms`, `weixin`, `profiles`, `skins`) are owned by `root:sandbox` rather than `root:root`, so the OpenClaw gateway (a member of the `sandbox` group) keeps read access to plugin and agent code while the sandbox user is denied write through `chmod -R go-w`. Secret-bearing directories (`credentials`, `identity`, `pairing`) get a stricter posture: `root:root 700` with `chmod -R go-rwX`, so neither the sandbox user nor the gateway can read them while shields are up. The mutable-default posture (`sandbox:sandbox 2770`) is restored for both groups on shields-down. The list is the union of state directories declared by every shipped agent manifest; dirs that aren't present in a given agent's config tree are silently skipped. Runtime-mutable subtrees (`sessions/`, `memories/`, `logs/`, `cache/`, `plans/`, and `openclaw-weixin/` which is regenerated at image-build time) are intentionally exempt so the agent can keep operating.Line 205 should be split:
A narrow set of runtime-data subpaths is exempted from the lock so the agent can keep operating — currently `agents/<agent-id>/sessions/`, which the OpenClaw TUI creates and writes session metadata into; the lock helper restores those subpaths to `sandbox:sandbox 2770` after the surrounding tree is locked.Line 206 appears to already be a single sentence.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/security/best-practices.mdx` around lines 204 - 206, Split the multi-sentence paragraphs into one sentence per line in docs/security/best-practices.mdx: break the paragraph that begins "Under lockdown, the high-risk state directories..." into separate lines so each sentence (the root:sandbox ownership sentence; the secret-bearing directories sentence; the mutable-default posture sentence; the union-of-state-directories sentence; the runtime-mutable subtrees sentence) is on its own line, and likewise split the paragraph that begins "A narrow set of runtime-data subpaths..." so that the single long sentence about agents/<agent-id>/sessions/ and the lock helper restoring sandbox:sandbox 2770 is on its own line; preserve exact directory and permission strings (`skills`, `hooks`, `agents`, `credentials`, `identity`, `pairing`, `sandbox:sandbox 2770`, `root:root 700`, etc.) and do not change wording other than inserting line breaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/security/best-practices.mdx`:
- Line 204: Rewrite the sentence in active voice throughout: have the lock
helper own the high-risk state directories as root:sandbox (instead of "are
owned by root:sandbox"), state that the lock helper lets the OpenClaw gateway (a
member of the sandbox group) keep read access while the lock denies write access
to the sandbox user via chmod -R go-w, and say the lock helper applies root:root
700 with chmod -R go-rwX to secret directories to prevent gateway and sandbox
reads; explicitly state that the lock helper restores sandbox:sandbox 2770 on
shields-down, that the list is the union of state directories declared by every
shipped agent manifest and that the lock helper silently skips dirs not present
in an agent's config tree, and finally mark runtime-mutable subtrees (sessions/,
memories/, logs/, cache/, plans/, openclaw-weixin/) as intentionally exempt so
agents can continue operating.
- Around line 204-206: Split the multi-sentence paragraphs into one sentence per
line in docs/security/best-practices.mdx: break the paragraph that begins "Under
lockdown, the high-risk state directories..." into separate lines so each
sentence (the root:sandbox ownership sentence; the secret-bearing directories
sentence; the mutable-default posture sentence; the union-of-state-directories
sentence; the runtime-mutable subtrees sentence) is on its own line, and
likewise split the paragraph that begins "A narrow set of runtime-data
subpaths..." so that the single long sentence about agents/<agent-id>/sessions/
and the lock helper restoring sandbox:sandbox 2770 is on its own line; preserve
exact directory and permission strings (`skills`, `hooks`, `agents`,
`credentials`, `identity`, `pairing`, `sandbox:sandbox 2770`, `root:root 700`,
etc.) and do not change wording other than inserting line breaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a7293e3-fb88-4655-80b7-2f66b6550f45
📒 Files selected for processing (1)
docs/security/best-practices.mdx
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26707810999
|
…restore failures Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/security/best-practices.mdx`:
- Around line 210-211: The docs claim that "Shields-up exempts runtime-mutable
subtrees (`sessions/`, `memories/`, `logs/`, `cache/`, `plans/`, plus
`openclaw-weixin/`)", which contradicts the actual shields behavior and the lock
helper that only preserves `agents/<agent-id>/sessions/` (restored to
`sandbox:sandbox 2770`); update the text to remove the outdated multi-subtree
exemption list and replace it with the accurate statement that the only runtime
carve-out is `agents/<agent-id>/sessions/`, and ensure the sentence references
the lock helper behavior (restores those subpaths to `sandbox:sandbox 2770`) so
operators aren’t misled by the old list.
🪄 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: Enterprise
Run ID: f8fe9182-7699-41f7-98c2-a3a5ffdd6293
📒 Files selected for processing (4)
docs/security/best-practices.mdxsrc/lib/shields/index.tssrc/lib/shields/state-dir-lock.tstest/shields-up-runtime-perms.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/shields-up-runtime-perms.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26708316674
|
…rding Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26708769299
|
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
4 participants
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
Under shields-up the OpenClaw gateway lost sandbox-group access to plugin/agent state dirs, so the TUI reported "plugin not found" for
/nemoclawandopenclaw-weixinand crashed withEACCES: mkdir agents/main/sessionson first launch. Lock the high-risk state dirs asroot:sandbox(top-level config dir staysroot:root), lock secret-bearing dirs (credentials,identity,pairing) to a stricterroot:root 700withchmod -R go-rwX, restoreagents/<id>/sessionstosandbox:sandbox 2770after the lock, and refuse to lock when the state-dir symlink preflight fails or any state-dir root is itself a symlink.Related Issue
Fixes #4065
Changes
src/lib/shields/state-dir-lock.ts(new) — lock pipeline extracted from the shields monolith: state-dir inventory (HIGH_RISK_STATE_DIRS,CONFIDENTIALITY_STATE_DIRS,WRITABLE_RUNTIME_SUBPATHS), symlink preflight, mutation scripts, post-lock stat verification, and the runtime-subpath restore helper. Preflight, mutation, verification, and restore each emit tab-prefixed marker lines so callers can surface every failure (exec errors, mutation failures, mid-lock symlink swaps, mode/owner drift, restore-time mkdir/chown/chmod failures, restore-time stat mismatches) as lock issues. All embedded scripts end withexit 0so a trailing[ … ]test cannot leak a non-zero status to the privileged exec layer.src/lib/shields/index.ts—lockAgentConfighoistspreflightStateDirLockbefore any chmod/chown so file mutations cannot leak out if a state-dir root is a symlink. Refers to the newstate-dir-lockmodule via a smallstateDirLockExecadapter; the shields entrypoint stays focused on shields state transitions.test/shields-up-runtime-perms.test.ts(new) — regression coverage for state-dir lock ownership, theagents/*/sessionsrestore loop, the preflight symlink guard, and atomicity within the state-dir fan-out when a symlinked root is detected (file-level mutations are now guarded by the hoisted preflight inlockAgentConfig).test/repro-2681-group-writable.test.ts— updates the existinglockAgentConfigassertion to expectroot:sandboxin the state-dir-lock shell command shape.docs/security/best-practices.mdx— documents theroot:sandboxposture for high-risk dirs, the stricterroot:root 700posture for secret-bearing dirs, the runtime-mutable exemptions, and the symlink preflight contract.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests