Skip to content

feat(cli): add trust windows for time-bounded exec approval#46956

Open
keylimesoda wants to merge 11 commits intoopenclaw:mainfrom
keylimesoda:trust-windows-v2
Open

feat(cli): add trust windows for time-bounded exec approval#46956
keylimesoda wants to merge 11 commits intoopenclaw:mainfrom
keylimesoda:trust-windows-v2

Conversation

@keylimesoda
Copy link
Copy Markdown

@keylimesoda keylimesoda commented Mar 15, 2026

AI Disclosure

Built with Claude and GPT-5.3 Codex. Ran AI review passes as a sanity check before submitting. Fully tested — 64 new test cases covering the main flows and auth edge cases.


Summary

  • Problem: Right now it's all-or-nothing for exec approvals: either approve every command individually, or turn off approvals entirely. When you're doing a long build or batch refactor you already trust, the per-command prompts get old fast.
  • Why it matters: There's no safe middle ground. People who get tired of prompts just disable approvals. Good security is usable security. Using a time-bound system is a pattern common to many other types of escalation systems (sudo, access tokens, etc.)
  • What changed: Three new CLI commands (trust, untrust, trust-status) that let an operator open a time-limited window where commands run without prompts. In-memory trust state, one enforcement point in resolveExecApprovalsFromFile, JSONL audit trail of commands that ran during the window, caller auth checks.
  • What did NOT change: The existing exec approval pipeline is untouched — the trust override is additive, bolted on at the end of resolveExecApprovalsFromFile. No changes to auth, tokens, network calls, or wire protocol. Agents can't initiate trust windows; only interactive CLI sessions can. This PR doesn't expose this feature through common usage points like TUI, Discord, Whatsapp, etc. It does lay a foundation for those features to be added next.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

User-visible / Behavior Changes

  • New commands:
    • openclaw approvals trust --minutes N — opens a trust window (default cap: 60m, up to 480m with --force)
    • openclaw approvals untrust — revokes the window and prints what commands ran
    • openclaw approvals trust-status — shows whether a window is active and how much time is left
  • While a trust window is active, commands for that agent run without approval prompts (security=full, ask=off).
  • Trust state is in-memory only. Restarting the gateway clears everything.
  • A JSONL audit log is written to ~/.openclaw/trust-audit-<agentId>.jsonl during the window (mode 0o600). untrust deletes it by default; --keep-audit preserves it.
  • All three commands require an interactive CLI terminal. Agent sessions and non-CLI clients are rejected.

Security Impact (required)

  • New permissions/capabilities? Yes
  • Secrets/tokens handling changed? No
  • New/changed network calls? No — new RPC methods go over the existing WebSocket connection
  • Command/tool execution surface changed? Yes — during a trust window, commands skip the approval prompt
  • Data access scope changed? No

Risk + mitigation:

This is a privilege escalation feature, so it needs to be locked down:

  • Who can grant: Only interactive CLI sessions with device identity (or loopback + token/password auth). Agents can't grant trust to themselves — requireNonAgentSession checks both OPENCLAW_SESSION_KEY and OPENCLAW_AGENT_ID.
  • How long: 60m default cap. You need --force to go higher, and 480m is a hard ceiling.
  • How many: You can't stack windows. One per agent, period. Revoke first if you want a new one. Importantly, this prevents re-entry or extension by an agent.
  • What's recorded: JSONL audit of every command that ran, plus gateway structured logs on grant/revoke. The audit is best-effort (won't fail exec on I/O errors), but the gateway logs are always there.
  • What persists: Nothing. In-memory only. Process restart = clean slate.

Repro + Verification

Environment

  • OS: macOS / Linux / Windows
  • Runtime/container: Node.js 22+
  • Model/provider: N/A (infrastructure change)
  • Integration/channel: CLI → Gateway WebSocket
  • Relevant config: ~/.openclaw/exec-approvals.json (unchanged format)

Steps

  1. Start the gateway: openclaw gateway start
  2. Grant a trust window: openclaw approvals trust --minutes 10 --yes
  3. Run commands — they execute without approval prompts
  4. Check status: openclaw approvals trust-status
  5. Revoke: openclaw approvals untrust --yes

Expected

  • Step 2: Prints trust window active with duration and expiry time
  • Step 3: Commands run without prompts
  • Step 4: Shows active with remaining minutes
  • Step 5: Prints revocation confirmation with audit summary

Actual

  • Matches expected.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

64 tests across 8 files (1,100+ lines of test code). Coverage includes:

  • Grant/revoke lifecycle, duration caps, anti-stacking, expired windows
  • Caller auth: device identity, loopback auth, non-CLI rejection, agent-session blocking (both env vars)
  • Audit: append, load, summarize, cleanup, malformed JSONL resilience, empty commands, boundary formatting
  • CLI: --force, --keep-audit, --minutes flag propagation, TTY checks
  • Exec integration: audit entries on gateway/node success and error paths

Happy-path coverage is solid. Edge cases around real device-identity auth are tested via mocked GatewayClient objects, not live auth flows.

Human Verification (required)

  • Verified scenarios: Trust grant → command execution without prompts → status check → revoke with audit summary. Rejection from non-TTY, agent sessions, and non-CLI callers. Anti-stacking. Duration cap enforcement.
  • Edge cases checked: Malformed JSONL lines (skipped), audit write failure (logged, doesn't break exec), --force with durations above 60m, idempotent untrust when no window exists, revoke after natural expiry.
  • What I did not verify: Load testing with many concurrent trust windows. Real device-identity auth (mocked in tests). Swift protocol model compilation on macOS.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? Yes — all new code. Older clients work identically.
  • Config/env changes? No
  • Migration needed? No

Failure Recovery (if this breaks)

  • How to disable/revert: Revert this PR. Trust state is in-memory — restarting the gateway clears everything. No persisted state to clean up.
  • Files/config to restore: None. Audit files in ~/.openclaw/ can be safely deleted.
  • Known bad symptoms: If the trust override in resolveExecApprovalsFromFile misbehaves, commands could bypass approval prompts when they shouldn't. Watch for trustWindowActive: true in resolved approvals when no window was granted.

Risks and Mitigations

  • Risk: User grants a long trust window and walks away — agent has elevated permissions for up to 8 hours.

    • Mitigation: Default cap is 60 minutes. --force required for longer. 480m hard max. Audit trail + gateway logs record everything.
  • Risk: Audit trail is best-effort — I/O errors don't fail exec, so there could be gaps.

    • Mitigation: Gateway structured logs record grant/revoke events regardless. The audit file is a nice-to-have; the gateway logs are the real safety net.

Ric Lewis and others added 7 commits March 14, 2026 22:03
- Remove dead expiredNotified/grantNotified fields from TrustWindow
- Add gateway exec error audit entry in .catch path
- Add 6 new tests: agent-session guard, trust.status shape,
  untrust cleanup, invalid agentId, summary boundaries
- Improve CLI help text and error messages
- Add MAX_COMMAND_CHARS rationale comment
- Regenerate protocol schema

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Security fixes:
- Add structured gateway logs for trust grant/revoke events (sec2-1)
- Use resolved approvals in host context instead of hardcoded override (sec2-3)
- Clear stale audit files at grant time (sr2-2)
- Re-check trust active before post-flight audit append (sr2-3)

CLI and readability:
- Import ABSOLUTE_MAX_TRUST_MINUTES constant in CLI (sr2-1)
- Add requireNonAgentSession guard to trust-status (sr2-4)
- Only chmod audit file on creation (sr2-5)
- Improve trust confirmation prompt with security context (pm2-6)
- Add JSDoc and inline comments for constants and auth paths

Tests (+12 new):
- Grant rejects zero/negative/non-integer minutes (qa2-1)
- Revoke rejects invalid agentId (qa2-7)
- Duplicate grant returns INVALID_REQUEST (qa2-2)
- Expired window returns null in trust.status (qa2-3)
- Empty command returns null from audit (qa2-5)
- Malformed JSONL lines skipped on load (qa2-6)
- Cleanup no-op when file absent (qa2-9)
- Duration formatting hours branch (qa2-12)
- Force and keep-audit flag propagation (qa2-10, qa2-11)
- Fix mock to include logGateway.info

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round 3 fix: cover the OPENCLAW_AGENT_ID branch of requireNonAgentSession
in addition to the existing OPENCLAW_SESSION_KEY test (qa3-1).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
These notification-tracking fields were added to TrustWindow during early
development but are never read. Their removal was missed in the Round 1
fix commit.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@openclaw-barnacle openclaw-barnacle Bot added app: macos App: macos app: web-ui App: web-ui gateway Gateway runtime cli CLI command changes agents Agent runtime and tooling size: XL labels Mar 15, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 15, 2026

Greptile Summary

This PR introduces time-bounded exec approval trust windows — a well-motivated feature that fills the usability gap between per-command prompts and fully-disabled approvals. The implementation is additive, keeping the existing approval pipeline intact and bolting the override onto the end of resolveExecApprovalsFromFile. Auth enforcement (CLI-only, TTY check, agent-session block on both OPENCLAW_SESSION_KEY and OPENCLAW_AGENT_ID) and the RPC layer are solid, and the 64-test suite covers the core scenarios well.

Key issues found:

  • Audit race condition (logic, medium): In bash-tools.exec.ts and bash-tools.exec-host-node.ts, the success and error audit paths double-check isTrustWindowActive after the command has already completed. If the operator runs untrust in the narrow window between command completion and the audit write, entries are silently dropped and won't appear in the revocation summary. trustWindowActiveAtStart alone is the right predicate — see inline comments on lines 570 and 607 of bash-tools.exec.ts, and lines 368/388 of bash-tools.exec-host-node.ts.
  • Trust window overrides security: "deny" (logic, medium): normalizeSecurity("full", resolvedAgent.security) returns "full" unconditionally, meaning an active trust window silently promotes deny-configured agents to full execution. If security: "deny" is ever intended as an irrevocable block, this is an undocumented privilege escalation path. See inline comment on exec-approvals.ts line 624.
  • Unbounded cache growth (style): Expired trustWindowCache entries are never evicted. Lazy eviction in getTrustWindow would bound the map size with minimal code. See inline comment on line 171.
  • Summarize preview cliff (style): summarizeTrustAudit shows all commands up to 10 but truncates to 5 when > 10, creating a jarring jump at 11 entries. See inline comment on trust-audit.ts line 192.

Confidence Score: 3/5

  • Merging is safe from a feature perspective, but the audit race condition means the audit trail can silently lose entries at revocation time — undermining the primary accountability mechanism of the feature.
  • The two logic issues (audit race and deny-policy override) are real gaps worth addressing before this ships. The audit race in particular is a correctness problem for the feature's security story: the audit is explicitly cited as a key mitigation for the elevated-trust risk, and a gap at revocation time is exactly the scenario an operator cares about. The deny-policy override is undocumented and could surprise operators who use security: "deny" as a hard ceiling. Neither issue causes data loss or crashes, but both undermine stated security guarantees.
  • src/agents/bash-tools.exec.ts (audit race on success and error paths), src/agents/bash-tools.exec-host-node.ts (same race), and src/infra/exec-approvals.ts (deny-policy override and cache growth).
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 570-578

Comment:
**Race window silently drops audit entries on revoke**

The second `isTrustWindowActive(getTrustWindow(agentId))` call (lines 572–573) introduces a race: if the operator runs `untrust` in the moment between command completion and the audit write, the entry is silently dropped. `revokeTrustWindow` calls `summarizeTrustAudit` synchronously, so commands completing concurrently with a revocation will never appear in the final summary.

`trustWindowActiveAtStart` is the correct and sufficient predicate — the command started inside the window and should be recorded regardless of what happened afterward. The same pattern recurs in the `.catch` path below (lines 608–611) and in `bash-tools.exec-host-node.ts` (lines 368 and 388).

```suggestion
            if (host === "gateway" && trustWindowActiveAtStart) {
              tryAppendTrustAuditEntry({
                agentId,
                command: params.command,
                exitCode: outcome.exitCode,
                durationMs: outcome.durationMs,
                logLabel: "gateway",
              });
            }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 607-614

Comment:
**Same race window in the error path**

Mirrors the issue on the success path above — the second `isTrustWindowActive` check can silently suppress the error-path audit entry when the trust window is concurrently revoked. Drop the re-check here too.

```suggestion
            if (host === "gateway" && trustWindowActiveAtStart) {
              tryAppendTrustAuditEntry({
                agentId,
                command: params.command,
                exitCode: null,
                logLabel: "gateway error",
              });
            }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 624-630

Comment:
**Trust window silently promotes `security: "deny"` to `"full"`**

`normalizeSecurity("full", resolvedAgent.security)` returns `"full"` unconditionally because `"full"` is a valid literal — the fallback is never reached. This means an active trust window overrides an agent's `security: "deny"` config to `"full"`, bypassing what may be intended as an absolute execution block (e.g., for sandboxed or read-only agents).

If `security: "deny"` is ever used as a hard ceiling rather than a soft default, this is a privilege escalation path that isn't called out in the PR description or enforced at grant time. Consider either:

1. Rejecting `grantTrustWindow` when the resolved agent security is `"deny"`, or
2. Explicitly documenting that trust windows are intentionally stronger than deny policies, so operators know a deny policy alone won't prevent trust grants.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/infra/trust-audit.ts
Line: 192-202

Comment:
**Preview threshold and limit are inconsistent**

When `commands.length <= 10`, all commands are shown. When `> 10`, only `previewLimit = 5` are shown with "and N more". This creates a cliff: 10 commands displays all 10, but 11 commands displays only 5. The threshold and limit should match to avoid the jarring jump.

```suggestion
    const previewLimit = 10;
    if (commands.length <= previewLimit) {
      for (const command of commands) {
        lines.push(`- ${command}`);
      }
    } else {
      for (const command of commands.slice(0, previewLimit)) {
        lines.push(`- ${command}`);
      }
      lines.push(`- …and ${commands.length - previewLimit} more`);
    }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 171

Comment:
**Expired trust windows accumulate in cache indefinitely**

`trustWindowCache` is a module-level `Map` that is never pruned of expired entries. Only explicit `revokeTrustWindow` or `initTrustWindowCache` calls remove entries. In a long-running gateway that processes many transient agent IDs, stale expired entries will accumulate until the process restarts.

Consider adding lazy eviction: in `getTrustWindow`, after a `Map.get`, check whether the returned window has already expired and call `trustWindowCache.delete(key)` before returning `undefined`. This keeps the cache bounded without requiring a background timer.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: 933a09b

Comment on lines 604 to +616
});
})
.catch((err) => {
if (
host === "gateway" &&
trustWindowActiveAtStart &&
isTrustWindowActive(getTrustWindow(agentId))
) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
exitCode: null,
logLabel: "gateway error",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

durationMs missing from error-path audit entry

In the success path (a few lines above), the audit entry includes durationMs: outcome.durationMs. In the .catch() error path, durationMs is omitted entirely. The startedAt timestamp is captured just before the async gateway call, so the elapsed duration is available even on error — the same Date.now() - startedAt pattern used in bash-tools.exec-host-node.ts (error path, line 327) could apply here.

Omitting it means error entries in the JSONL audit will always have durationMs: null, reducing the usefulness of the audit summary for debugging failed commands. Consider capturing the start time in scope and passing it, consistent with the node-host path.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 604-616

Comment:
**`durationMs` missing from error-path audit entry**

In the success path (a few lines above), the audit entry includes `durationMs: outcome.durationMs`. In the `.catch()` error path, `durationMs` is omitted entirely. The `startedAt` timestamp is captured just before the async gateway call, so the elapsed duration is available even on error — the same `Date.now() - startedAt` pattern used in `bash-tools.exec-host-node.ts` (error path, line 327) could apply here.

Omitting it means error entries in the JSONL audit will always have `durationMs: null`, reducing the usefulness of the audit summary for debugging failed commands. Consider capturing the start time in scope and passing it, consistent with the node-host path.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 250c92ac3f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/bash-tools.exec.ts
keylimesoda and others added 2 commits March 14, 2026 23:22
The Swift ExecApprovalsTrustParams had a grantedBy field and
ExecApprovalsUntrustParams had a revokedBy field, but both were
removed from the TypeScript schema (additionalProperties: false).
Any macOS client setting these would get a validation error.

Regenerated via pnpm protocol:gen:swift.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@keylimesoda
Copy link
Copy Markdown
Author

keylimesoda commented Mar 15, 2026

Addressed the Swift model mismatch: regenerated both GatewayModels.swift files via pnpm protocol:gen:swift.

The grantedBy field in ExecApprovalsTrustParams and evokedBy in ExecApprovalsUntrustParams are now removed, matching the TypeScript schema (AdditionalProperties: false).

The two style notes (normalize helpers vs direct assignment, and durationMs in error-path audit) are non-blocking per the review.

@keylimesoda
Copy link
Copy Markdown
Author

@greptile review

Comment on lines +570 to +578
if (
host === "gateway" &&
trustWindowActiveAtStart &&
isTrustWindowActive(getTrustWindow(agentId))
) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
exitCode: outcome.exitCode,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race window silently drops audit entries on revoke

The second isTrustWindowActive(getTrustWindow(agentId)) call (lines 572–573) introduces a race: if the operator runs untrust in the moment between command completion and the audit write, the entry is silently dropped. revokeTrustWindow calls summarizeTrustAudit synchronously, so commands completing concurrently with a revocation will never appear in the final summary.

trustWindowActiveAtStart is the correct and sufficient predicate — the command started inside the window and should be recorded regardless of what happened afterward. The same pattern recurs in the .catch path below (lines 608–611) and in bash-tools.exec-host-node.ts (lines 368 and 388).

Suggested change
if (
host === "gateway" &&
trustWindowActiveAtStart &&
isTrustWindowActive(getTrustWindow(agentId))
) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
exitCode: outcome.exitCode,
if (host === "gateway" && trustWindowActiveAtStart) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
exitCode: outcome.exitCode,
durationMs: outcome.durationMs,
logLabel: "gateway",
});
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 570-578

Comment:
**Race window silently drops audit entries on revoke**

The second `isTrustWindowActive(getTrustWindow(agentId))` call (lines 572–573) introduces a race: if the operator runs `untrust` in the moment between command completion and the audit write, the entry is silently dropped. `revokeTrustWindow` calls `summarizeTrustAudit` synchronously, so commands completing concurrently with a revocation will never appear in the final summary.

`trustWindowActiveAtStart` is the correct and sufficient predicate — the command started inside the window and should be recorded regardless of what happened afterward. The same pattern recurs in the `.catch` path below (lines 608–611) and in `bash-tools.exec-host-node.ts` (lines 368 and 388).

```suggestion
            if (host === "gateway" && trustWindowActiveAtStart) {
              tryAppendTrustAuditEntry({
                agentId,
                command: params.command,
                exitCode: outcome.exitCode,
                durationMs: outcome.durationMs,
                logLabel: "gateway",
              });
            }
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +607 to +614
if (
host === "gateway" &&
trustWindowActiveAtStart &&
isTrustWindowActive(getTrustWindow(agentId))
) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same race window in the error path

Mirrors the issue on the success path above — the second isTrustWindowActive check can silently suppress the error-path audit entry when the trust window is concurrently revoked. Drop the re-check here too.

Suggested change
if (
host === "gateway" &&
trustWindowActiveAtStart &&
isTrustWindowActive(getTrustWindow(agentId))
) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
if (host === "gateway" && trustWindowActiveAtStart) {
tryAppendTrustAuditEntry({
agentId,
command: params.command,
exitCode: null,
logLabel: "gateway error",
});
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 607-614

Comment:
**Same race window in the error path**

Mirrors the issue on the success path above — the second `isTrustWindowActive` check can silently suppress the error-path audit entry when the trust window is concurrently revoked. Drop the re-check here too.

```suggestion
            if (host === "gateway" && trustWindowActiveAtStart) {
              tryAppendTrustAuditEntry({
                agentId,
                command: params.command,
                exitCode: null,
                logLabel: "gateway error",
              });
            }
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +624 to +630
const finalAgent: Required<ExecApprovalsDefaults> = trustWindowActive
? {
...resolvedAgent,
security: normalizeSecurity(trustWindow.security, resolvedAgent.security),
ask: normalizeAsk(trustWindow.ask, resolvedAgent.ask),
}
: resolvedAgent;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trust window silently promotes security: "deny" to "full"

normalizeSecurity("full", resolvedAgent.security) returns "full" unconditionally because "full" is a valid literal — the fallback is never reached. This means an active trust window overrides an agent's security: "deny" config to "full", bypassing what may be intended as an absolute execution block (e.g., for sandboxed or read-only agents).

If security: "deny" is ever used as a hard ceiling rather than a soft default, this is a privilege escalation path that isn't called out in the PR description or enforced at grant time. Consider either:

  1. Rejecting grantTrustWindow when the resolved agent security is "deny", or
  2. Explicitly documenting that trust windows are intentionally stronger than deny policies, so operators know a deny policy alone won't prevent trust grants.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 624-630

Comment:
**Trust window silently promotes `security: "deny"` to `"full"`**

`normalizeSecurity("full", resolvedAgent.security)` returns `"full"` unconditionally because `"full"` is a valid literal — the fallback is never reached. This means an active trust window overrides an agent's `security: "deny"` config to `"full"`, bypassing what may be intended as an absolute execution block (e.g., for sandboxed or read-only agents).

If `security: "deny"` is ever used as a hard ceiling rather than a soft default, this is a privilege escalation path that isn't called out in the PR description or enforced at grant time. Consider either:

1. Rejecting `grantTrustWindow` when the resolved agent security is `"deny"`, or
2. Explicitly documenting that trust windows are intentionally stronger than deny policies, so operators know a deny policy alone won't prevent trust grants.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread src/infra/trust-audit.ts
Comment on lines +192 to +202
const previewLimit = 5;
if (commands.length <= 10) {
for (const command of commands) {
lines.push(`- ${command}`);
}
} else {
for (const command of commands.slice(0, previewLimit)) {
lines.push(`- ${command}`);
}
lines.push(`- …and ${commands.length - previewLimit} more`);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preview threshold and limit are inconsistent

When commands.length <= 10, all commands are shown. When > 10, only previewLimit = 5 are shown with "and N more". This creates a cliff: 10 commands displays all 10, but 11 commands displays only 5. The threshold and limit should match to avoid the jarring jump.

Suggested change
const previewLimit = 5;
if (commands.length <= 10) {
for (const command of commands) {
lines.push(`- ${command}`);
}
} else {
for (const command of commands.slice(0, previewLimit)) {
lines.push(`- ${command}`);
}
lines.push(`- …and ${commands.length - previewLimit} more`);
}
const previewLimit = 10;
if (commands.length <= previewLimit) {
for (const command of commands) {
lines.push(`- ${command}`);
}
} else {
for (const command of commands.slice(0, previewLimit)) {
lines.push(`- ${command}`);
}
lines.push(`- …and ${commands.length - previewLimit} more`);
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/trust-audit.ts
Line: 192-202

Comment:
**Preview threshold and limit are inconsistent**

When `commands.length <= 10`, all commands are shown. When `> 10`, only `previewLimit = 5` are shown with "and N more". This creates a cliff: 10 commands displays all 10, but 11 commands displays only 5. The threshold and limit should match to avoid the jarring jump.

```suggestion
    const previewLimit = 10;
    if (commands.length <= previewLimit) {
      for (const command of commands) {
        lines.push(`- ${command}`);
      }
    } else {
      for (const command of commands.slice(0, previewLimit)) {
        lines.push(`- ${command}`);
      }
      lines.push(`- …and ${commands.length - previewLimit} more`);
    }
```

How can I resolve this? If you propose a fix, please make it concise.

export const DEFAULT_MAX_TRUST_MINUTES = 60; // 1h — conservative default
export const ABSOLUTE_MAX_TRUST_MINUTES = 480; // 8h — absolute max with --force

const trustWindowCache = new Map<string, TrustWindow>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expired trust windows accumulate in cache indefinitely

trustWindowCache is a module-level Map that is never pruned of expired entries. Only explicit revokeTrustWindow or initTrustWindowCache calls remove entries. In a long-running gateway that processes many transient agent IDs, stale expired entries will accumulate until the process restarts.

Consider adding lazy eviction: in getTrustWindow, after a Map.get, check whether the returned window has already expired and call trustWindowCache.delete(key) before returning undefined. This keeps the cache bounded without requiring a background timer.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 171

Comment:
**Expired trust windows accumulate in cache indefinitely**

`trustWindowCache` is a module-level `Map` that is never pruned of expired entries. Only explicit `revokeTrustWindow` or `initTrustWindowCache` calls remove entries. In a long-running gateway that processes many transient agent IDs, stale expired entries will accumulate until the process restarts.

Consider adding lazy eviction: in `getTrustWindow`, after a `Map.get`, check whether the returned window has already expired and call `trustWindowCache.delete(key)` before returning `undefined`. This keeps the cache bounded without requiring a background timer.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 933a09b5ce

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

// so allow that authenticated path for trusted CLI usage.
const sharedAuthMethod = client?.authMethod;
const usesSharedAuth = sharedAuthMethod === "token" || sharedAuthMethod === "password";
return usesSharedAuth && isLoopbackAddress(client?.clientIp);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Permit trusted localhost auth path for trust operations

isTrustGrantCallerAllowed gates shared-auth callers on isLoopbackAddress(client?.clientIp), but the WebSocket handshake intentionally clears clientIp for local/loopback requests (src/gateway/server/ws-connection/message-handler.ts sets reportedClientIp to undefined for local clients), so valid CLI sessions using localhost token/password auth (the no-device-identity path this feature explicitly allows) are rejected as "non-CLI". This makes exec.approvals.trust, ...untrust, and ...trust.status unusable in that supported auth mode.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 505e878960

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

});

function requireNonAgentSession(action: string): void {
if (process.env.OPENCLAW_SESSION_KEY || process.env.OPENCLAW_AGENT_ID) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Enforce agent-session blocking on gateway, not env vars

This check is ineffective because OPENCLAW_SESSION_KEY/OPENCLAW_AGENT_ID are not populated by the codebase outside this file/tests (repo-wide search), so agent-spawned CLI processes can pass it and still call exec.approvals.trust (for example from exec with pty=true) to open a trust window. Since this is a security boundary, the non-agent restriction needs to be enforced server-side in the trust RPC handlers using authenticated session metadata rather than client environment variables.

Useful? React with 👍 / 👎.

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 28, 2026

Codex review: found issues before merge.

Summary
The PR adds CLI trust, untrust, and trust-status commands plus Gateway RPC/protocol, Swift models, in-memory trust-window enforcement, JSONL audit logging, and related tests for time-bounded exec approval bypasses.

Reproducibility: yes. for review purposes. The blocking issues are source-reproducible from current main plus the provided PR diff by inspecting the trust RPC gate, local WebSocket client IP handling, hard-deny policy behavior, and audit append predicates.

Next step before merge
This is an XL security-sensitive feature with competing open proposals and unresolved authorization/policy semantics, so it needs maintainer/secops design consolidation rather than an automated repair lane.

Security
Needs attention: The patch introduces a security-sensitive exec approval bypass with concrete authorization, hard-deny, and audit-accounting concerns.

Review findings

  • [P1] Enforce non-agent trust grants on the Gateway — src/gateway/server-methods/exec-approvals.ts:127-130
  • [P1] Keep localhost shared auth usable for trust — src/gateway/server-methods/exec-approvals.ts:136
  • [P1] Preserve deny policies as a hard ceiling — src/infra/exec-approvals.ts:624-628
Review details

Best possible solution:

Land a consolidated trust-window design that enforces grant authority at the Gateway, preserves or deliberately redefines deny-policy semantics, records trusted-command audit entries from start-time trust state, and updates CLI, protocol, Swift clients, docs, and regression tests together.

Do we have a high-confidence way to reproduce the issue?

Yes for review purposes. The blocking issues are source-reproducible from current main plus the provided PR diff by inspecting the trust RPC gate, local WebSocket client IP handling, hard-deny policy behavior, and audit append predicates.

Is this the best way to solve the issue?

No. The feature direction is useful, but this PR is not the best mergeable solution until the Gateway owns the authorization boundary and the deny/audit semantics are settled explicitly.

Full review comments:

  • [P1] Enforce non-agent trust grants on the Gateway — src/gateway/server-methods/exec-approvals.ts:127-130
    The trust RPC accepts a CLI client with device identity before the Gateway can prove the request did not originate from an agent-run CLI. The non-agent guard is client-side env checking, so this privilege-escalation boundary needs server-verifiable origin or session metadata.
    Confidence: 0.84
  • [P1] Keep localhost shared auth usable for trust — src/gateway/server-methods/exec-approvals.ts:136
    The PR says loopback token/password auth is supported, but this requires isLoopbackAddress(client?.clientIp). Current WebSocket handling clears local reported client IPs to undefined, so local shared-auth CLI calls without device identity are rejected.
    Confidence: 0.88
  • [P1] Preserve deny policies as a hard ceiling — src/infra/exec-approvals.ts:624-628
    When a trust window is active, the resolver promotes the agent policy to the trust window's full/off values. Current host-context behavior treats security=deny as a hard exec block, so this silently bypasses an explicit deny policy.
    Confidence: 0.86
  • [P2] Record commands that start under trust — src/agents/bash-tools.exec.ts:570-574
    The audit append is gated on trust being active at both command start and completion. A command that starts during the window but finishes after expiry or revoke is omitted from the audit even though it ran under bypassed approval policy.
    Confidence: 0.9
  • [P3] Evict expired trust windows from the cache — src/infra/exec-approvals.ts:200-201
    Expired entries remain in the module-level trust window cache unless explicitly revoked or the gateway restarts. A long-running gateway with many transient agent IDs can accumulate stale entries for the process lifetime.
    Confidence: 0.72

Overall correctness: patch is incorrect
Overall confidence: 0.87

Security concerns:

  • [high] Agent-spawned CLI can self-grant trust — src/gateway/server-methods/exec-approvals.ts:127
    The server-side trust gate accepts CLI device identity while the non-agent restriction is enforced only by client-side environment variables, which is not a reliable boundary for a feature that disables approval prompts.
    Confidence: 0.84
  • [high] Trust windows override explicit exec deny policy — src/infra/exec-approvals.ts:624
    The resolver promotes a trusted agent to full execution, contradicting current hard-deny behavior and allowing a trust grant to bypass a configured host-exec block.
    Confidence: 0.86
  • [medium] Audit can miss trusted commands after expiry or revoke — src/agents/bash-tools.exec.ts:570
    Commands that start while trusted but finish after the window is no longer active are dropped from the JSONL audit trail, weakening the feature's accountability mitigation.
    Confidence: 0.9

What I checked:

  • Current main has no trust-window implementation: A targeted search for trustWindow, TrustWindow, trust-audit, trust-status, exec.approvals.trust, grantTrustWindow, revokeTrustWindow, and approvedUntil returned no matches in current main. (dfadf03e1f50)
  • Current Gateway method list lacks trust RPCs: Current main exposes exec.approvals.get/set and node get/set, but no trust, untrust, or trust.status methods. (src/gateway/server-methods-list.ts:37, dfadf03e1f50)
  • Current resolver has no trust-window state: ExecApprovalsResolved contains path, socketPath, token, defaults, agent, agentSources, allowlist, and file, with no trustWindowActive field or override. (src/infra/exec-approvals.ts:180, dfadf03e1f50)
  • Current host policy keeps deny as a hard block: resolveExecHostApprovalContext intersects requested and host approval security with minSecurity and throws when the effective host security is deny. (src/agents/bash-tools.exec-host-shared.ts:203, dfadf03e1f50)
  • Current WebSocket client IP handling clears local IPs: Local clients and untrusted proxy-header cases store reportedClientIp as undefined, so a new trust gate that requires isLoopbackAddress(client?.clientIp) will reject that shared-auth local path. (src/gateway/server/ws-connection/message-handler.ts:293, dfadf03e1f50)
  • Discussion already identified unresolved blockers: The existing ClawSweeper review comment and Greptile comments both call out audit race/accounting and deny-policy override concerns; the later ClawSweeper review also identifies Gateway-side authorization and localhost shared-auth blockers. (505e8789606e)

Likely related people:

  • steipete: The available shallow history attributes the central exec approval files to Peter Steinberger in the current tree snapshot, and the PR timeline shows steipete subscribed to this security-sensitive area. (role: recent maintainer / routing candidate; confidence: low; commits: a94897d99c38; files: src/infra/exec-approvals.ts, src/cli/exec-approvals-cli.ts, src/agents/bash-tools.exec-host-shared.ts)
  • vincentkoc: Recent local history includes approval-flow test maintenance, and the changelog credits vincentkoc on exec approval security and native approval routing work adjacent to this PR's authorization surface. (role: recent approval/security maintainer; confidence: medium; commits: 52dbc4d6807b, e782f47ecabf; files: CHANGELOG.md, src/gateway/server/ws-connection/message-handler.ts, src/gateway/server-methods/exec-approvals.ts)
  • ngutman: The changelog credits ngutman on multiple macOS/iOS exec approval changes, and this PR changes the generated Swift protocol model surface used by macOS clients. (role: adjacent macOS approval owner; confidence: low; files: apps/macos/Sources/OpenClawProtocol/GatewayModels.swift, apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift, CHANGELOG.md)

Remaining risk / open question:

  • The PR introduces a security-sensitive mode where commands can run without prompts during an active window.
  • The non-agent restriction relies partly on client-side environment checks rather than a server-verifiable Gateway boundary.
  • The proposed trust override changes explicit security=deny semantics that current code and docs treat as a hard stop.
  • Commands that start while trusted can be omitted from the JSONL audit if the window expires or is revoked before the append path runs.

Codex review notes: model gpt-5.5, reasoning high; reviewed against dfadf03e1f50.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling app: macos App: macos app: web-ui App: web-ui cli CLI command changes gateway Gateway runtime size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant