Skip to content

fix(chat): in-chat approval card can show a different command than Allow runs - #1499

Open
SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f051-approval-card-sanitize
Open

SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f051-approval-card-sanitize

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes the in-chat approval card showing a different command from the one Allow runs when gateway text contains hidden or reordered characters. A display that conceals or omits command content must never offer Allow.

User Impact

The card renders sanitized command and message text. When the command or message is oversized, truncated, or hides command syntax, only Deny is offered. A direct response call also cannot send Allow for that pending card. Reviewable commands retain Allow once, Always allow, and Deny.

Why This Change Was Made

The approval dialog already refuses commands that cannot be reviewed in full. The contributor patch sanitized the in-chat text but dropped the sanitizer's safety flags, so the card could still offer Allow on a suppressed or incomplete display. This follow-up uses SanitizeWithStatus when mapping the card and checks the matching pending card's actions before calling exec.approval.resolve. The original contributor commit is preserved on this PR branch.

Evidence

Final-head focused mapper/provider tests cover an oversized or truncated command, oversized or truncated message, secret redaction, zero-width Unicode concealment, and review-safe displays. A deny-only card rejects both direct Allow once and Always allow responses without sending a gateway RPC; Deny still sends the RPC.

Change Type

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway approval response
  • Permissions, privacy, or security
  • Tests, CI, or docs

Required proof pools

  • windows-winui-interactive: current-head in-chat approval-card display and Deny-only action need isolated visible proof.
  • windows-wsl-mxc: exec approval safety boundary needs strict, non-skipping containment proof.
  • windows-wsl-gateway-e2e: real gateway approval event and resolution path not yet exercised.

Validation

  • ./build.ps1: passed on Windows ARM64 at de0164bfef42479bc98ae8fc02a3df1a40b233db.
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore: 4,104 passed, 35 skipped, 0 failed.
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore: 3,083 passed, 0 skipped, 0 failed.
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --filter 'FullyQualifiedName~ChatEventMapperTests|FullyQualifiedName~RespondToPermissionAsync' --no-restore: 17 passed, 0 failed.
  • ./scripts/validate-mxc-e2e.ps1 without -AllowSkip: blocked, exits 1. Eleven peripheral tests passed, but six required MXC proofs skipped because this host reports appcontainer-dacl rather than MXC BaseContainer. This is not MXC merge proof; run on an MXC-capable Windows host.
  • Rubber-duck review of the final behavior found no blocking source issue. After merging main, test builds emitted ONNX Runtime version-conflict warnings introduced upstream by the dependency bump; both required suites passed.

Real behavior proof

  • Current head: de0164bfef42479bc98ae8fc02a3df1a40b233db on the original contributor branch, after integrating current main 273b0182745a3093c0e09f306ca8a1fff6ef3c5a.
  • Observed in executable tests: mapped approval events escape hidden Unicode; sanitizer suppression/truncation/concealment yields Actions = [deny], while review-safe displays retain the original action set. The provider refuses direct Allow once/Always allow RPCs for a deny-only pending approval, retains the pending card, and lets Deny resolve it.
  • Screenshot or artifact links: none; no active changed-state UI screenshot exists.
  • Not verified / blocked: no live gateway approval card or isolated tray screenshot. Computer Use cannot pass the data-directory and branch arguments required to launch run-app-local.ps1 -Isolated -AllowNonMain; starting the EXE directly would use real user state, so it was not attempted. A gateway-to-node MXC invocation was not exercised on this host. These proof pools remain open.

Security Impact

  • New permissions or capabilities? No.
  • Secrets or tokens handling changed? Only the existing display sanitizer's status is now respected by the card; runtime secret handling is unchanged.
  • New or changed network calls? No new calls. An unsafe Allow request is blocked before the existing approval RPC.
  • Command or tool execution surface changed? Approval eligibility changed at the chat UI/provider boundary, not the MXC executor or gateway policy.
  • Data access scope changed? No.

Compatibility and Migration

  • Backward compatible? Reviewable approval requests retain their existing actions and resolution flow.
  • Config or environment changes? No.
  • Migration needed? No.

Review Conversations

  • Contributor feedback about the fail-open card was incorporated on the original PR branch.
  • Live UI, gateway, and strict MXC proof remain open before merge.

- Run command and message through ExecApprovalCommandDisplaySanitizer before the card renders them
- Keep the approval request id unchanged
- Tray test covers bidi, zero-width, and line-separator payloads

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 24, 2026
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 24, 2026, 3:26 PM ET / 19:26 UTC (Revision 5).

ClawSweeper review

What this changes

The branch sanitizes command and message text on Windows chat approval cards, limits unreviewable requests to Deny, and checks the pending card before sending an approval response to the Gateway.

Merge readiness

⛔ Blocked before merge - 5 items remain

Current main still offers Allow on in-chat approvals without checking whether the command display is reviewable. This PR addresses that gap, and the latest commit resolves the reported missing-command explanation. The approval boundary still needs current-head, real-path proof before merge.

Priority: P1
Reviewed head: c8a3785a31f73842d0e947c2c8b4a1873567599a

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused patch and tests are useful, but real approval-path proof and the stale-authority check remain merge gates.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: Authority-chain proof required: Current-head mapper and mocked-bridge tests cover Deny-only filtering, but no isolated WinUI card or real Gateway trace shows reviewable Allow succeeding and an unreviewable or superseded Allow stopping before RPC I/O. The earlier strict MXC run exited 1 after required proofs skipped. Redact private addresses, endpoints, and credentials in shared evidence; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review if it does not. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: Authority-chain proof required: Current-head mapper and mocked-bridge tests cover Deny-only filtering, but no isolated WinUI card or real Gateway trace shows reviewable Allow succeeding and an unreviewable or superseded Allow stopping before RPC I/O. The earlier strict MXC run exited 1 after required proofs skipped. Redact private addresses, endpoints, and credentials in shared evidence; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review if it does not. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Current main behavior: The current-main mapper places gateway command and message text in the card and always supplies the default approval actions.
Introduced reviewability decision: The mapper uses sanitizer status for both fields and offers only Deny if command text is missing, truncated, oversized, or conceals syntax, or if message text has those unsafe conditions.
Approval authority boundary: The provider checks the matching pending card's actions before sending the decision, but the state check and Gateway RPC are separate operations. A superseded in-flight approval has no demonstrated final-effect rejection.
Findings None None.
Security Needs attention Unproven stale approval rejection at Gateway I/O: The new pending-card check releases its state lock before the provider sends the approval RPC. A replacement or revocation during that interval needs final-effect evidence; the available tests exercise a mocked bridge without that transition.

How this fits together

The Windows tray receives execution approval events from the Gateway and turns them into chat cards. A user's card action returns through the chat provider to the Gateway approval RPC, which can permit command execution.

flowchart LR
  GatewayEvent[Gateway approval event] --> Sanitizer[Command display sanitizer]
  Sanitizer --> CardDecision[Reviewable display?]
  CardDecision --> ChatCard[Chat approval card]
  ChatCard --> PendingCheck[Pending action check]
  PendingCheck --> GatewayRPC[Gateway approval response]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: Authority-chain proof required: Current-head mapper and mocked-bridge tests cover Deny-only filtering, but no isolated WinUI card or real Gateway trace shows reviewable Allow succeeding and an unreviewable or superseded Allow stopping before RPC I/O. The earlier strict MXC run exited 1 after required proofs skipped. Redact private addresses, endpoints, and credentials in shared evidence; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review if it does not. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve security concern: Unproven stale approval rejection at Gateway I/O - The new pending-card check releases its state lock before the provider sends the approval RPC. A replacement or revocation during that interval needs final-effect evidence; the available tests exercise a mocked bridge without that transition.
  • Resolve merge risk (P1) - The authority check is separate from the Gateway send. A pending approval that is superseded while a response is in flight has not been shown to reject the stale Allow before Gateway I/O.
  • Resolve merge risk (P1) - The changed Deny-only card and an allowed reviewable request have no current-head isolated WinUI and real Gateway after-fix trace. The reported strict MXC run skipped its required proofs and exited 1.
  • Complete next step (P2) - Provide current-head isolated WinUI and real Gateway final-effect proof for reviewable, unreviewable, and superseded approvals, and complete strict MXC validation without skipped required proofs.

Findings

  • [medium] Unproven stale approval rejection at Gateway I/O — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:941
Agent review details

Security

Needs attention: The patch narrows Allow eligibility, but current evidence does not establish rejection of a superseded approval before the final Gateway send.

Review metrics

Metric Value Why it matters
Production and test lines production +36/-6, tests +114 The focused runtime change is accompanied by substantial mapper and mocked-provider regression coverage.

Merge-risk options

Maintainer options:

  1. Prove the approval boundary (recommended)
    Capture current-head isolated card behavior and a real Gateway trace showing reviewable Allow and unreviewable or superseded Allow rejection before the RPC, then complete strict MXC validation on a capable host.

Technical review

Best possible solution:

Keep Allow available only for fully reviewable, current approvals, with a visible Deny-only explanation and final-effect evidence for allowed, forbidden, and superseded responses before landing.

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

Yes, from source: current main puts unsanitized Gateway command text on the card while offering Allow, and the new mapper tests exercise concealed and incomplete display cases. A live current-main card was not run in this review.

Is this the best way to solve the issue?

Yes, the sanitizer-status decision and provider guard are focused on the established approval flow. Real-path evidence is still needed to establish that the guard holds at the Gateway effect.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 273b0182745a.

Labels

Label changes:

No label changes.

Label justifications:

  • P1: The PR addresses a command approval display mismatch in an active user authorization workflow.
  • merge-risk: 🚨 security-boundary: Merging changes when the chat UI permits a command execution approval, and final-effect rejection for stale authority remains unproven.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: Current-head mapper and mocked-bridge tests cover Deny-only filtering, but no isolated WinUI card or real Gateway trace shows reviewable Allow succeeding and an unreviewable or superseded Allow stopping before RPC I/O. The earlier strict MXC run exited 1 after required proofs skipped. Redact private addresses, endpoints, and credentials in shared evidence; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review if it does not. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [medium] Unproven stale approval rejection at Gateway I/O — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:941
    The new pending-card check releases its state lock before the provider sends the approval RPC. A replacement or revocation during that interval needs final-effect evidence; the available tests exercise a mocked bridge without that transition.
    Confidence: 0.79

What I checked:

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • shanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add final-effect proof for the nearest unauthorized principal and prove supersession invalidates a pending Allow before Gateway I/O.
  • Capture current-head isolated WinUI card evidence and complete strict, non-skipping MXC validation.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (4 earlier review cycles)
  • reviewed 2026-09-24T01:56:09.273Z sha 8737bb1 :: needs real behavior proof before merge. :: [P1] Disable Allow when sanitization conceals the command
  • reviewed 2026-09-24T05:30:24.802Z sha 8737bb1 :: needs real behavior proof before merge. :: [P1] Disable Allow when sanitization conceals the command
  • reviewed 2026-09-24T11:17:47.144Z sha 8737bb1 :: needs real behavior proof before merge. :: [P1] Disable Allow when sanitization conceals the command
  • reviewed 2026-09-24T11:26:20.433Z sha de0164b :: needs real behavior proof before merge. :: none

@shanselman shanselman added status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. and removed status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. labels Sep 24, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d9a0183f-460a-4854-b545-89033a081f21
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d9a0183f-460a-4854-b545-89033a081f21
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d9a0183f-460a-4854-b545-89033a081f21
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 24, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Sep 24, 2026
@karkarl

karkarl commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Global triage: NEEDS_HUMAN_TEST. Take confidence 65%; recommendation confidence 92%; effort small-to-medium; risk medium.

Reviewed exact head de0164bfef42. The mapper and RPC choke-point guards consistently fail closed, and focused tests pass. Merge proof is incomplete: the reported validate-mxc-e2e.ps1 run exits 1 because all six MXC proofs skip, which is not valid exec-approval proof. Empty or missing command payloads also become Deny-only without an on-card explanation, and the provider guard logs then returns without user feedback.

Owner: maintainer proof scheduler. Run strict windows-wsl-mxc, capture isolated windows-winui-interactive evidence for truncated, oversized, concealed, and empty-command cards, and prove a real windows-wsl-gateway-e2e approval trace. Do not treat -AllowSkip as passing.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

c8a3785 keeps Deny as the only action when the command text is missing, and the card now says that no command was included so only Deny is available.

Local filter Map_ApprovalRequestOffersOnlyDeny: 8 passed. Strict windows-wsl-mxc without -AllowSkip, WinUI cards for truncated, oversized, concealed, and empty commands, and a live gateway approval trace were not run on this pass.

This branch has not been deployed

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

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants