Skip to content

fix(mcp): require owner for Claude permission replies#98256

Merged
eleqtrizit merged 2 commits into
openclaw:mainfrom
eleqtrizit:801
Jun 30, 2026
Merged

fix(mcp): require owner for Claude permission replies#98256
eleqtrizit merged 2 commits into
openclaw:mainfrom
eleqtrizit:801

Conversation

@eleqtrizit

@eleqtrizit eleqtrizit commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

What Problem This Solves

Claude channel permission replies were matched only by the short pending request ID. A non-owner channel participant could therefore resolve a privileged Claude Code permission prompt by sending a matching yes <id> or no <id> message.

Changes

Why This Change Was Made

  • Carry the authenticated senderIsOwner decision from channel and Gateway ingress through user-turn transcript persistence and session.message broadcasts.
  • Require an explicit owner user message before converting a reply into notifications/claude/channel/permission.
  • Fail closed when ownership metadata is missing while preserving the message as an ordinary channel event.
  • Document that Claude permission replies come from the command owner.
  • Extend the packaged MCP-channel Docker scenario with a non-admin reply followed by an owner reply.

User Impact

Command owners retain the existing text-reply approval flow. Non-owners, assistant messages, and messages without authenticated ownership metadata can no longer approve or deny Claude Code permission requests.

Validation

Evidence

  • Targeted packaged Docker MCP/channel run — passed on the current PR head. The functional image launched a real Gateway and stdio MCP bridge; an authenticated non-admin reply was forwarded as an ordinary channel message without resolving the permission, then an authenticated admin-owner reply resolved the same pending permission. Redacted artifact markers: nonOwnerReplyForwarded: true, nonOwnerPermissionBlocked: true, ownerPermissionAllowed: true.
  • node scripts/run-vitest.mjs src/mcp/channel-bridge.test.ts src/mcp/channel-server.test.ts src/gateway/server-session-events.test.ts src/sessions/user-turn-transcript.test.ts src/auto-reply/reply/get-reply-run.media-only.test.ts src/gateway/server-methods/chat.directive-tags.test.ts — 8 files, 408 tests passed across 3 shards.
  • node scripts/run-vitest.mjs test/scripts/docker-e2e-observability.test.ts test/scripts/test-projects.test.ts — 2 files, 188 tests passed.
  • node_modules/.bin/oxfmt --check --threads=1 <changed files> — passed.
  • node scripts/check-docs-mdx.mjs docs/cli/mcp.md — passed.
  • git diff --check upstream/main...HEAD — passed.
  • Structured Codex autoreview — clean after fixing the initial negative-proof ordering finding; no accepted/actionable findings remain.
  • Current upstream CI includes passing lint, production/test types, docs, architecture/boundary guards, QA smoke, and affected test shards.

Notes

  • AI-assisted change; the implementation and test results were reviewed locally.
  • No config, CLI, or migration changes.
  • Missing trusted ownership metadata intentionally fails closed for permission decisions while preserving ordinary message delivery. This compatibility choice still requires explicit maintainer acceptance.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui gateway Gateway runtime size: S maintainer Maintainer-authored PR labels Jun 30, 2026
@clawsweeper

clawsweeper Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 4:34 PM ET / 20:34 UTC.

Summary
The branch propagates authenticated sender ownership metadata through Gateway chat, transcript persistence, and session.message events, gates Claude MCP permission replies on owner user messages, updates MCP docs, and adds unit plus Docker MCP-channel proof.

PR surface: Source +37, Tests +145, Docs 0. Total +182 across 16 files.

Reproducibility: yes. from source inspection: current main resolves a pending Claude permission from any matching yes/no short-id session message before checking role or owner metadata. I did not run a failing current-main live repro, but the PR's Docker artifact proves the after-fix owner and non-owner paths on the PR head.

Review metrics: 2 noteworthy metrics.

  • Approval conversion gate: 1 tightened. The bridge now requires an authenticated owner user message before converting text into a privileged Claude permission notification.
  • Owner metadata propagation: 1 optional session.message field added. The approval gate depends on senderIsOwner surviving persistence and broadcast, so maintainers should notice the compatibility surface.

Stored data model
Persistent data-model change detected: serialized state: src/sessions/user-turn-transcript.test.ts, serialized state: src/sessions/user-turn-transcript.ts, serialized state: src/sessions/user-turn-transcript.types.ts, vector/embedding metadata: src/mcp/channel-bridge.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #98256
Summary: This PR is the current open canonical candidate for the Claude MCP permission reply spoofing path; earlier session-binding attempts are closed unmerged, while the merged pending-map cleanup only partially overlaps the same bridge state.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] Merging can intentionally stop Claude text-reply approvals from any live session.message source that has not been upgraded to carry trusted senderIsOwner metadata, even though ordinary message delivery is preserved.

Maintainer options:

  1. Accept the fail-closed owner gate (recommended)
    Maintainers can approve the compatibility change because the PR preserves ordinary channel messages while blocking untrusted permission decisions when ownership metadata is absent.
  2. Add a compatibility path first
    If missing ownership metadata should not fail closed by default, add a narrow tested compatibility or explicit strict-mode path before merge.
  3. Pause for broader approval policy
    Pause or close this PR if Claude channel text approvals should be redesigned together with a wider owner/admin authorization policy.

Next step before merge

  • [P2] Protected-label and compatibility-sensitive authorization policy acceptance remains; there is no narrow automated repair to queue.

Security
Cleared: The diff tightens a privileged Claude permission approval boundary and does not add third-party code, dependency, workflow, secret, package-resolution, or supply-chain changes.

Review details

Best possible solution:

Merge this owner-gated approval fix after maintainers explicitly accept the fail-closed missing-metadata behavior and the exact head remains green with the Docker MCP-channel proof.

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

Yes, from source inspection: current main resolves a pending Claude permission from any matching yes/no short-id session message before checking role or owner metadata. I did not run a failing current-main live repro, but the PR's Docker artifact proves the after-fix owner and non-owner paths on the PR head.

Is this the best way to solve the issue?

Yes for the code shape: carrying the authenticated owner bit and gating at the MCP bridge is narrower than adding a required permission-request protocol field and also blocks same-session non-owner replies. The unresolved question is maintainer acceptance of the fail-closed compatibility behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9f07b21e6231.

Label changes

Label justifications:

  • P1: The PR addresses a security-relevant Claude MCP permission approval bypass in a live channel workflow.
  • merge-risk: 🚨 compatibility: Failing closed on missing ownership metadata can break existing text-reply approval flows from sources that do not yet carry the trusted owner bit.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): Sufficient logs: the PR body and downloaded Docker artifact show a packaged Gateway plus stdio MCP bridge run where a non-owner reply stayed ordinary and an owner reply resolved the permission on the PR head.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient logs: the PR body and downloaded Docker artifact show a packaged Gateway plus stdio MCP bridge run where a non-owner reply stayed ordinary and an owner reply resolved the permission on the PR head.
Evidence reviewed

PR surface:

Source +37, Tests +145, Docs 0. Total +182 across 16 files.

View PR surface stats
Area Files Added Removed Net
Source 7 46 9 +37
Tests 8 147 2 +145
Docs 1 1 1 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 16 194 12 +182

What I checked:

  • Current-main permission bypass source: Current main converts any matching Claude yes/no short-id text into a permission notification before checking message role or authenticated owner metadata. (src/mcp/channel-bridge.ts:587, 9f07b21e6231)
  • PR owner gate: At PR head, the bridge requires role === "user" and payload.senderIsOwner === true before resolving a pending Claude permission; missing, non-owner, or assistant messages fall through as ordinary events. (src/mcp/channel-bridge.ts:589, fd00c587bcd2)
  • Owner metadata persistence: The PR stores senderIsOwner under __openclaw on persisted user turns and preserves that bit through before-message hooks. (src/sessions/user-turn-transcript.ts:246, fd00c587bcd2)
  • Session broadcast propagation: The PR reads persisted __openclaw.senderIsOwner metadata and emits it as top-level session.message metadata for MCP bridge consumers. (src/gateway/server-session-events.ts:197, fd00c587bcd2)
  • Gateway owner source: The PR marks Gateway admin chat.send turns as owner-authenticated; non-admin Gateway clients omit the owner bit, which makes permission conversion fail closed while preserving ordinary messages. (src/gateway/server-methods/chat.ts:3887, fd00c587bcd2)
  • Unit coverage for the gate: The PR adds bridge tests proving non-owner, missing-metadata, and assistant replies do not resolve a pending permission, while owner user replies do resolve it. (src/mcp/channel-bridge.test.ts:55, fd00c587bcd2)

Likely related people:

  • steipete: Introduced the current split of the MCP channel bridge/server/shared surfaces that own this permission path. (role: feature introducer and refactor owner; confidence: high; commits: ba02905c4f11; files: src/mcp/channel-bridge.ts, src/mcp/channel-server.ts, src/mcp/channel-shared.ts)
  • fuller-stack-dev: Current blame on handleSessionMessageEvent and the permission conversion block points to the recent bridge rewrite in 455f813. (role: recent current-main carrier; confidence: medium; commits: 455f813d6ee6; files: src/mcp/channel-bridge.ts, src/mcp/channel-server.ts, src/gateway/server-session-events.ts)
  • eleqtrizit: Besides this PR, recent current-main history includes owner/admin authorization hardening with senderIsOwner-adjacent behavior in the auto-reply path. (role: recent adjacent owner-auth contributor; confidence: high; commits: 72f837a4a478, 50ed5b35c038, fd00c587bcd2; files: src/auto-reply/reply/dispatch-from-config.ts, src/mcp/channel-bridge.ts, src/gateway/server-session-events.ts)
  • vincentkoc: Recent history touched MCP bridge and Gateway approval/client seams adjacent to the pending permission and event-subscription path. (role: recent MCP runtime seam contributor; confidence: medium; commits: 0f7d9c957093; files: src/mcp/channel-bridge.ts, src/gateway/operator-approvals-client.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 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.

@eleqtrizit eleqtrizit self-assigned this Jun 30, 2026
@clawsweeper clawsweeper Bot added 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 Jun 30, 2026
@clawsweeper clawsweeper Bot added mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 30, 2026
@eleqtrizit

eleqtrizit commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Behavioral proof is now available on the current PR head.

Real setup: the targeted workflow packed the candidate, installed it into the functional Docker image, launched a real Gateway, connected the real stdio MCP server, and exercised the permission flow through authenticated Gateway clients. No provider credentials or private channel identifiers were used.

Observed sequence:

  1. A permission request with one pending short ID was sent through the MCP notification handler.
  2. An authenticated non-admin client (operator.read + operator.write, no admin scope) sent the matching approval text.
  3. The MCP client received that text as an ordinary Claude channel message. After the processing/quiet barrier, it had emitted no permission decision.
  4. An authenticated admin-owner client then sent the same matching text.
  5. The MCP client emitted the allow decision only after that owner action.

Redacted artifact output:

{
  "ok": true,
  "nonOwnerReplyForwarded": true,
  "nonOwnerPermissionBlocked": true,
  "ownerPermissionAllowed": true
}

The mcp-channels lane passed in 14 seconds inside the targeted workflow: https://github.com/eleqtrizit/openclaw/actions/runs/28472137502

The durable scenario is checked in under test/e2e/qa-lab/runtime/mcp-channels-docker-client.ts; it records a raw-message boundary before the owner action so a late decision cannot be misattributed to the owner path.

Compatibility decision for maintainer review: missing trusted ownership metadata intentionally fails closed for permission decisions while preserving the reply as an ordinary message. This prevents an unauthenticated ownership claim from becoming an approval; transports that want text approvals must carry the ownership decision established at authenticated ingress.

Additional fresh validation:

  • Focused behavior suite: 8 files, 408 tests passed across 3 Vitest shards.
  • Docker harness tooling tests: 2 files, 188 tests passed.
  • Upstream CI: lint, production/test types, docs, QA smoke, affected test shards, and relevant boundary checks passed.
  • Structured Codex autoreview: one ordering finding accepted and fixed; final rerun clean with no actionable findings.

@eleqtrizit

eleqtrizit commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 30, 2026
@eleqtrizit
eleqtrizit merged commit b885c81 into openclaw:main Jun 30, 2026
159 of 172 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 1, 2026
@eleqtrizit
fix(mcp): require owner for Claude permission replies (openclaw#98256)
64d17b8 
* fix(mcp): require owner for Claude permission replies

* fix(mcp): prove owner-gated permission replies
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
@eleqtrizit @QiuYuang
fix(mcp): require owner for Claude permission replies (openclaw#98256)
ae346ca 
* fix(mcp): require owner for Claude permission replies

* fix(mcp): prove owner-gated permission replies
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 1, 2026
@eleqtrizit chenyangjun-xy
fix(mcp): require owner for Claude permission replies (openclaw#98256)
455ef61 
* fix(mcp): require owner for Claude permission replies

* fix(mcp): prove owner-gated permission replies
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
@eleqtrizit @chenyangjun-xy
fix(mcp): require owner for Claude permission replies (openclaw#98256)
91fa590 
* fix(mcp): require owner for Claude permission replies

* fix(mcp): prove owner-gated permission replies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@eleqtrizit eleqtrizit

Labels

app: web-ui App: web-ui docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@eleqtrizit