Policy: add sandbox posture conformance checks#85572
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
|
Codex review: needs maintainer review before merge. Reviewed May 28, 2026, 11:58 PM ET / 03:58 UTC. Summary PR surface: Source +1222, Tests +1077, Docs -74. Total +2225 across 6 files. Reproducibility: not applicable. this is a feature PR rather than a bug report. The changed behavior is supported by redacted policy-check output and focused Policy doctor/CLI tests. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land after normal CI once maintainers accept the explicit config-only sandbox posture contract and keep the PR body or squash message clear about the new Policy surface. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a feature PR rather than a bug report. The changed behavior is supported by redacted policy-check output and focused Policy doctor/CLI tests. Is this the best way to solve the issue? Yes with maintainer acceptance; the Policy plugin is the right owner, and the docs/body clearly frame the feature as config conformance rather than runtime enforcement. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 3c8ad8cbaa1b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +1222, Tests +1077, Docs -74. Total +2225 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Pearl Review Wisp Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
74ff672 to
96ddb7a
Compare
6a58d74 to
dc74127
Compare
|
@clawsweeper re-review Updated this draft PR on top of the refreshed #85817 stack and addressed the contributor-facing review items. What changed:
Validation run locally in WSL from
No merge performed; PR remains draft. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
omarshahine
left a comment
There was a problem hiding this comment.
Reviewed against 5a33378f9c (the rebase base). Strong implementation — the unobservable-rule design (policy/sandbox-container-posture-unobservable) is the standout: refusing to silently pass when a container rule applies to a backend that can't expose the evidence is exactly the right safety bias for a security-boundary feature. Verbose-but-auditable one-function-per-rule pattern is the correct tradeoff here. Tests, docs, evidence/check separation, and the scoped-overlay extension all look great.
Two items before merge:
1. Missing CHANGELOG entry. Needs ## Unreleased → ### Changes. Suggested wording:
Policy: add sandbox posture conformance checks for
sandbox.requireMode,sandbox.allowBackends, container host network / namespace-join / read-only mounts / runtime-socket-mounts / unconfined-profiles, and sandbox browserrequireCdpSourceRange, plus scoped overlay support viascopes.<scopeName>.sandbox. Container rules on backends that cannot observe them surfacepolicy/sandbox-container-posture-unobservableinstead of silently passing. (#85572)
2. Windows Docker bind parser false-positives on drive-letter destinations. splitPolicyBindSpec / policyBindSeparatorIndex only skip the host-side drive-letter prefix. For a real-world Windows bind like C:\Users\foo:C:\container:ro:
policyBindSeparatorIndexcorrectly finds the:betweenfooand the destinationC:\container:ro, sohost = "C:\Users\foo"is right.- But
rest = "C:\container:ro", thenrest.indexOf(":")finds the:after the destination'sC(index 1), sooptions = "\container:ro"instead of"ro", andmode = "rw"instead of"ro".
Result: a false-positive policy/sandbox-container-mount-mode-required finding for legitimately read-only Windows binds with drive-letter destinations. Fix options: (a) re-skip a drive-letter prefix in rest before indexOf(":"), (b) walk all : positions while recognizing the Windows drive pattern on both sides, or (c) document a constraint that Windows binds must use POSIX paths (/c/Users/foo:/c/container:ro). There are no Windows-bind tests today; adding one either way would lock the behavior.
Optional polish (non-blocking)
bindHostLooksLikeDockerSocketis Docker-only by name and by implementation. Worth a one-line comment naming the scope, since Podman (/var/run/podman/podman.sock) or containerd (/run/containerd/containerd.sock) would silently passpolicy/sandbox-container-runtime-socket-mountif those backends are ever supported.scopedSandboxDefaultDisabledForAgentcorrectly treats agents not inagents.list[]as "default applies" (matches the documented inheritance behavior), but the code path is identical to "agent in list with no sandbox override". A one-line comment naming both cases would help future readers.pushSandboxPostureEvidenceemits amodeentry with value"off"even for unconfigured sandboxes, sorequireMode: ["all"]firespolicy/sandbox-mode-unapprovedagainst a fresh install. Tested as intentional via "reports sandbox posture denied by policy", which is the right safety bias — but worth saying explicitly indocs/cli/policy.mdthat policy treats missingsandbox.modeas the implicit default"off".
206 tests pass on the focused vitest run. Mergeable. Once F1 and F2 are addressed this is ready to land — proof: sufficient + ready for maintainer look labels are appropriate.
|
@clawsweeper re-review Addressed Omar's review on #85572:
I did not leave a Proof after the fix: |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
galiniliev
left a comment
There was a problem hiding this comment.
Reviewed current head 4669398b14011115ec5804b660415c820feb648c.
No blocking findings from my pass. The solution looks like the right owner-boundary fix for the stated goal: the Policy plugin observes authored OpenClaw config, reports config drift, and stays out of live container/runtime enforcement. The policy/sandbox-container-posture-unobservable path is also the right safety behavior for security-adjacent policy: a backend that cannot expose container evidence should fail the authored posture claim instead of silently passing it.
Code-quality verdict: I would not collapse the per-rule finding functions into a fully generic evaluator right now. The explicit functions in extensions/policy/src/doctor/register.ts make each security-sensitive rule auditable, and the tests cover the important backend, scope, browser, bind, and Windows-bind cases. That said, the sandbox rule names now repeat across POLICY_RULE_METADATA, shape validation, policyHasSandboxPostureRules, the unobservable-rule list, and the individual finding functions. A small follow-up refactor could define one typed sandbox rule descriptor table for the rule key, policy path, check id, strictness, and observed evidence kind, then keep the current explicit rule functions where they add behavior. That would reduce drift risk without hiding the policy decisions behind too much abstraction.
I could not run a fresh local test pass in this checkout because node_modules is missing and pnpm is not installed here. I reviewed the source, docs, scoped guides, existing review thread, and current CI/PR proof instead.
|
@galiniliev thanks, agreed on keeping the explicit finding functions while reducing the repeated sandbox rule key lists. Pushed
Validation after the change:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
can we expand to support non-docker sockets? |
|
@kevinslin agreed. I broadened this instead of narrowing the public policy wording. Pushed signed commit
I also added focused coverage so read-only containerd/Podman socket binds still fail Proof on current PR head: |
|
@clawsweeper re-review Updated signed head: Change since last review:
Proof:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Maintainer approval: I explicitly accept the config-only sandbox posture contract for this PR. The new @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
(cherry picked from commit 89510d5)
Follow-up for reviewer feedback: centralizes the sandbox rule descriptors used by strictness metadata, shape validation, rule detection, and unobservable container-posture evidence so the sandbox policy keys have one shared source.
Adds config-level Policy conformance checks for sandbox posture:
{ "sandbox": { "requireMode": ["all", "non-main"], "allowBackends": ["docker", "openshell"], "containers": { "denyHostNetwork": true, "denyContainerNamespaceJoin": true, "requireReadOnlyMounts": true, "denyContainerRuntimeSocketMounts": true, "denyUnconfinedProfiles": true }, "browser": { "requireCdpSourceRange": true } } }This stays in the config-conformance lane: it checks authored OpenClaw config and reports drift through policy/doctor findings. It does not inspect live containers, host filesystem permissions, process state, or network state, and it does not add runtime enforcement.
The policy contract is conceptual container posture. The evidence OpenClaw can observe today comes from Docker-backed sandbox config (
agents.*.sandbox.docker.*) plus sandbox browser container config (agents.*.sandbox.browser.*). That is why findings may point at Docker config paths even though policy authors writesandbox.containers.*. If asandbox.containers.*rule is enabled for a sandbox backend that cannot observe that field, policy emitspolicy/sandbox-container-posture-unobservableagainst the backend setting instead of silently passing. This finding means the workspace is not conformant to the authored policy: the policy requires a container posture field that the selected backend cannot evidence.The PR also extends the named
scopes.<scopeName>overlay model from #85817 so sandbox posture can be scoped to explicit runtimeagentIds. Scope names are descriptive buckets; matching uses the selector values inside the scope.agentIdssupportstools,agents.workspace, andsandbox. To express different sandbox posture for Docker, OpenShell, or future backends, define separate agent groups/scopes and leave unsupported container rules unset or false where the backend cannot evidence them:{ "scopes": { "release-workspace": { "agentIds": ["release-agent", "review-agent"], "agents": { "workspace": { "allowedAccess": ["none", "ro"] } } }, "release-lockdown": { "agentIds": ["release-agent"], "tools": { "exec": { "allowHosts": ["sandbox"], "allowSecurity": ["deny", "allowlist"], "requireAsk": ["always"] }, "denyTools": ["exec", "process", "write", "edit", "apply_patch"] }, "sandbox": { "requireMode": ["all"], "allowBackends": ["docker"], "containers": { "denyHostNetwork": true, "requireReadOnlyMounts": true } } }, "shell-sandbox": { "agentIds": ["shell-agent"], "sandbox": { "allowBackends": ["openshell"], "containers": { "requireReadOnlyMounts": false } } } } }Verification
pnpm exec oxfmt --check --threads=1 docs/cli/policy.md docs/plugins/reference/policy.md extensions/policy/src/doctor/register.ts extensions/policy/src/doctor/register.test.ts extensions/policy/src/policy-state.tspnpm tsgo:extensionsnode scripts/run-bundled-extension-oxlint.mjspnpm docs:check-mdx docs/cli/policy.md docs/plugins/reference/policy.mdsandboxBackendsreferences in the touched policy docs/source/testsOPENCLAW_VITEST_FS_MODULE_CACHE_PATH=/tmp/openclaw-85572-sandbox-cut node scripts/run-vitest.mjs extensions/policy/src/doctor/register.test.ts extensions/policy/src/cli.test.ts -- --reporter=dot --testTimeout=30000-> 2 files, 207 tests passedgit diff --check~/.codex/skills/codex-review/scripts/codex-review --mode branch-> no accepted/actionable findingsReal behavior proof
Behavior addressed: Policy can now express sandbox posture requirements for mode, backend, host container networking, container namespace joins, read-only container mounts, container runtime socket mounts, unconfined container profiles, sandbox browser CDP source range, agent-scoped sandbox overlays, and unobservable container-posture findings for backends that cannot evidence enabled container rules.
Real environment tested: WSL Ubuntu 24.04 checkout at
/root/src/openclaw-policy-sandbox-posture, using focused policy doctor and CLI tests.Exact steps or command run after this patch: see Verification commands above.
Evidence after fix: focused tests cover failing sandbox config, passing sandbox config, inherited defaults, per-agent sandbox posture, scoped
agentIdsoverlays, invalid-scope isolation, browser container network posture, browser mount inheritance, sandbox policy typo rejection, unobservable container posture on non-Docker backends, inactive container/browser scoped fallback behavior, mixed-case backend normalization, and strictness metadata for scoped sandbox rules. Docs now cover scoped overlay syntax indocs/cli/policy.mdand remove the separatedocs/plan/policy-agent-scoped-overlays.mdplan page.Observed result after fix: the focused policy test suite passes with the new
scopes.<scopeName>.sandbox.*andagentIdsbehavior, and the touched code and CLI/plugin docs use the current top-level named scope syntax.What was not tested: live Docker/browser containers were not launched because this PR intentionally checks config conformance only, not runtime sandbox behavior.