fix: prevent exec approval revocation races#103515
Conversation
openclaw-barnacle
Bot
added
docs
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Codex review: needs real behavior proof before merge. Reviewed July 10, 2026, 4:32 PM ET / 20:32 UTC. Summary Reproducibility: yes. The exact-head source gives deterministic paths for policy tightening or file deletion between evaluation and final commit/CAS, plus a direct inherited-entry deletion no-op; focused live execution was not repeated during this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Use a mandatory persisted-policy snapshot contract for every delayed explicit and automatic authorization, make every set/CAS path non-mutating until freshness is accepted, preserve allowlist entry ownership in resolved UI rows, and then prove tightening, deletion, inherited-edit, and concurrent-add cases on one exact repaired head. Do we have a high-confidence way to reproduce the issue? Yes. The exact-head source gives deterministic paths for policy tightening or file deletion between evaluation and final commit/CAS, plus a direct inherited-entry deletion no-op; focused live execution was not repeated during this read-only review. Is this the best way to solve the issue? No, not on this head. A shared locked transaction is the right owner boundary, but optional snapshots, partial automatic-policy checks, pre-CAS mutation, and lost entry ownership leave the intended revocation invariant incomplete. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 99e1a6081bca. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
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
Review history (6 earlier review cycles)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2be0b5d7d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expectedPolicy && | ||
| !execApprovalPolicySnapshotIsCurrent( |
There was a problem hiding this comment.
When source is explicit-approval but no policySnapshot is supplied, this branch skips the policy comparison and returns. The new node/gateway callers only attach policySnapshot when an allow-always grant is being persisted, so an allow-once prompt created under full/ask=always can still execute after the operator tightens the approvals file to allowlist/ask=off or removes the matching rule, as long as the new security is not deny. Require a snapshot for explicit approvals or have the callers always pass the snapshot captured before prompting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f242a146ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case let .explicitOnce(security): | ||
| guard ExecSecurity.narrower(security, current.agent.security) != .deny else { | ||
| throw self.executionAuthorizationChangedError() | ||
| } | ||
| return |
There was a problem hiding this comment.
Fresh evidence beyond the existing TypeScript comment: the new macOS commit path added here has the same gap for .explicitOnce. If a prompt was approved with allow-once, then before commitExecution takes the lock an operator changes ask to .always, changes security to .allowlist, or removes/downgrades the matching allowlist rule, this branch only rejects a new .deny policy and still executes the stale approval. The .explicitAlways branch immediately below compares the captured policySnapshot; .explicitOnce needs equivalent snapshot validation so one-shot human approvals cannot survive policy tightening or revocation.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26cc13b1b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (params.authorization.source === "auto-review") { | ||
| if (ask === "always") { | ||
| throw new Error("Exec approval changed before execution"); | ||
| } | ||
| return; |
There was a problem hiding this comment.
With source: "auto-review", this branch only rejects deny and ask=always. If an auto-reviewed command is waiting to commit and the approvals file tightens from full to allowlist (or from ask=off to on-miss) before the lock is taken, the changed security/ask is ignored and the stale automatic approval still executes, while the current-policy path below rejects the same drift. Please require the evaluated security/ask to remain current, or re-derive a current allowlist basis, for auto-review commits.
Useful? React with 👍 / 👎.
| case let .autoReview(security): | ||
| guard ExecSecurity.narrower(security, current.agent.security) != .deny, | ||
| current.agent.ask != .always |
There was a problem hiding this comment.
For forwarded .autoReview, this guard only fails for deny or ask == .always. If the upstream auto-review was made under a broader policy and the store is tightened to .allowlist before this locked commit, the branch returns without checking for a current allowlist basis, so the stale automatic approval can still execute. Mirror the current-policy validation here by rejecting evaluated security/ask drift or requiring a current basis.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
github-actions
Bot
added
the
dependencies-changed
openclaw-barnacle
Bot
added
channel: nextcloud-talk
a632790 to
b363012
Compare
|
Land-ready review completed on exact head The policy-owner transaction now linearizes current-policy validation, optional durable grants, and execution usage before spawn across CLI, Gateway, Node, and macOS. Revocation, tightening, source downgrade, malformed state, and persistence failure remain fail closed; independent grants and the exact nil-to- Proof:
No remaining code finding or proof gap. Proceeding through the native guarded landing workflow. Update: the native guard correctly required the root changelog entry to remain release-owned. The two inherited baseline failures then landed fixed in #103925. Exact head |
clawsweeper
Bot
removed
rating: 🧂 unranked krab
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cc44c2a5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (params.authority === "auto-review") { | ||
| if (current.hostAsk === "always") { | ||
| throw new Error("exec denied: host=node ask=always requires human approval"); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Fresh node-host evidence beyond the existing auto-review comments: this auto-review branch only rejects security=deny or ask=always before forwarding the approved node.invoke. If the command was auto-reviewed under a broader host policy and the exec approvals file tightens to allowlist/ask=off or removes the relevant rule before dispatch, this still returns and the node only enforces its own local policy, so the Gateway-owned host revocation no longer applies. Re-derive the current host allowlist basis (or require the evaluated security/ask/allowlist snapshot to remain valid) before returning here.
Useful? React with 👍 / 👎.
| switch ExecHostRequestEvaluator.validateCommand( | ||
| command: params.command, | ||
| rawCommand: params.rawCommand) |
There was a problem hiding this comment.
This new validation path rejects normal Gateway-dispatched Mac node commands such as command: ["/bin/sh", "-lc", "echo hi"] with rawCommand: "echo hi": ExecSystemRunCommandValidator only accepts the shell preview when hasTrailingPositionalArgvAfterInlineCommand is false, but that helper currently counts the inline command token itself as trailing positional argv. Mac nodes do not advertise system.run.prepare, so buildLocalPreparedNodeRun intentionally sends transportRawCommand as the preview for these legacy/native nodes; adding this validator makes ordinary host=node exec calls fail with INVALID_REQUEST: rawCommand does not match command.
Useful? React with 👍 / 👎.
|
Merged via squash.
|
1 participant
What Problem This Solves
Exec-approval mutations could save stale state after an administrator revoked or tightened policy. That could restore a revoked allowlist entry, execute after a late policy change, or lose one of two concurrent
allow-alwaysgrants. The macOS approval socket also used a Foundation read API that can wait for a full buffer instead of returning a short JSONL frame, deadlocking both ends on valid requests.This is the reviewed maintainer replacement for #78226 because the contributor fork could not be updated from the secured execution environment. It preserves Coy Geek's authorship credit in the repaired commit history.
Closes #78225.
Closes #44749.
Why This Change Was Made
User Impact
Revoked approvals remain revoked under concurrent runtime and settings updates. Late authorization changes prevent execution instead of being overwritten. Independent simultaneous
allow-alwaysapprovals are both retained. macOS approval requests now return short JSONL responses promptly and idle peers are bounded by kernel socket timeouts.Change Type
Scope
Root Cause
Execution-time helpers mutated approval data derived from an earlier evaluation snapshot and wrote it outside the complete cross-process serialization boundary. Authorization and persistence were separate operations, so a policy change could occur between them. macOS settings also replaced whole allowlist arrays from UI snapshots.
Implementation
src/infra/exec-approvals.tsand the matching macOS store.argPatternbytes, to remain present.recvcalls and receive/send timeouts instead of Foundation full-count reads.Evidence
7cc44c2a5fad0ebd44e6e32957d79eb524a269c3.git diff --check, strict SwiftFormat, targeted SwiftLint, Swift frontend parsing, and standalone Swift typecheck: passed; the final Swift socket/fixture bundle also passes strict formatting and frontend parsing..alias accepted; nested unsafe alias rejected; mutating group/everyone ACL rejected; current-user ACL accepted.run_783fefc61664: security-equivalent parent verified, instance profile empty, Tailscale absent, native i18n unchanged at 2,923 entries, and 404 focused tests passed across six Vitest shards.ExecApprovalsPromptServerTests,ExecApprovalsSocketPathGuardTests, andExecApprovalsStoreRefactorTestspassed twice; an independent prompt-server rerun passed 5/5 in 0.125 seconds.git merge-tree --write-tree 8cc75e285b 99e1a6081bproduced the exact7cc44c2a5ftree; all 11 incorporated-main blobs match current main byte-for-byte, and the residual binary patch hash is unchanged at78629da716139bacb60af4138cf02ddcc33ddc35efd278f7b774a07c2f6bf38f.29121165105: 54 successful and 4 skipped source jobs with workflow success. Full PR rollup settled at 86 successful, 20 skipped, 1 neutral, 1 routine auto-response cancellation, 0 failures, and 0 pending checks.Security Impact
recvplus socket receive/send timeouts; no remote endpoint or permission changed.Compatibility / Migration
Risks and Mitigations
argPattern; removal or source change fails validation.Human Verification
macos-swift, Android, security, Gateway, Node, iOS, type, and focused quality lanes all passed.Review Conversations