fix: bind Claude permission replies to session by lifelikezzh · Pull Request #56420 · openclaw/openclaw

lifelikezzh · 2026-03-28T13:55:45Z

Motivation

Fix a security issue where Claude permission replies (yes/no <id>) could be spoofed from any session because pending requests were keyed only by a short request_id.
Ensure permission replies are only accepted from the same session that originated the permission request to prevent cross-session or untrusted-channel approvals.
Preserve existing behavior for legitimate requests while closing the approval spoofing vector.

Description

Require session_key in the notifications/claude/channel/permission_request schema and propagate it through the MCP server; changed src/mcp/channel-shared.ts and src/mcp/channel-server.ts.
Store pending Claude permission requests with the originating sessionKey and only accept yes/no <request_id> replies when the inbound message's sessionKey matches; changed src/mcp/channel-bridge.ts.
Add and update tests to cover session-bound permission handling and regression scenarios, including rejecting attacker-session replies and accepting trusted-session replies; changed src/mcp/channel-server.test.ts.

Testing

Ran the targeted unit tests with pnpm test -- src/mcp/channel-server.test.ts, which passed (1 passed, 6 tests).
Ran the repository verification gate with pnpm check, which completed successfully (lint/type checks passed).

greptile-apps · 2026-03-28T13:59:30Z

Greptile Summary

This PR closes a security hole where any session could spoof a Claude permission reply (yes/no <id>) by simply knowing the short request_id. It binds each pending permission to the originating sessionKey and rejects replies from any other session, preserving the existing flow for the legitimate session.

ClaudePermissionRequest type and ClaudePermissionRequestSchema gain a required session_key / sessionKey field — this is a breaking schema change for existing senders of notifications/claude/channel/permission_request.
handleClaudePermissionRequest stores the sessionKey alongside each pending permission entry.
handleSessionMessageEvent replaces the old has() check with a strict sessionKey equality guard; unmatched replies fall through to normal message enqueueing (no silent drops).
Tests cover attacker-session rejection, trusted-session acceptance, and a full integration scenario — good coverage of the new security path.
Two minor defensive gaps noted: session_key in the schema permits empty strings (which create permanently irresolvable pending permissions), and the stored sessionKey is not trimmed before being compared against the toText-trimmed inbound value.

Confidence Score: 5/5

Safe to merge — the security fix is correctly implemented and well-tested; remaining findings are minor defensive suggestions that do not affect normal operation.

All open findings are P2 style/defensive suggestions (empty-string schema validation, missing .trim() normalization). Neither constitutes a present defect for any realistic caller. The core session-binding logic is sound, the early-return path prevents double-processing, and the test suite covers both the attack vector and the happy path.

No files require special attention beyond the two minor suggestions in src/mcp/channel-shared.ts and src/mcp/channel-server.ts.

Important Files Changed

Filename	Overview
src/mcp/channel-bridge.ts	Stores sessionKey with each pending permission and validates it on reply. The core session-binding logic is correct: attacker sessions that match the reply pattern fall through to normal message enqueueing without triggering the permission callback.
src/mcp/channel-shared.ts	Added sessionKey to ClaudePermissionRequest type and session_key to ClaudePermissionRequestSchema. Minor defensive gap: session_key uses z.string() without .min(1), permitting empty strings that create irresolvable pending permissions.
src/mcp/channel-server.ts	Correctly propagates session_key from the notification schema through to handleClaudePermissionRequest. The raw (untrimmed) string is passed as sessionKey, which could mismatch trimmed values at comparison time.
src/mcp/channel-server.test.ts	New unit test exercises attacker-session rejection and trusted-session acceptance. Integration test updated to include session_key in the permission_request notification and adds an attacker-session negative assertion. Coverage is thorough for the new security path.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/mcp/channel-shared.ts
Line: 122

Comment:
**`session_key` accepts empty strings**

`z.string()` allows an empty `""` to pass schema validation. If a caller sends an empty `session_key`, the pending permission is stored with an empty string as `sessionKey`. Because `toText(payload.sessionKey)` in `handleSessionMessageEvent` returns `undefined` for empty/blank strings, the comparison `pendingPermission?.sessionKey === sessionKey` would evaluate to false for such a request. The permission can never be resolved and leaks in `pendingClaudePermissions` indefinitely.

Consider tightening the schema with `.min(1)` to reject empty strings at the boundary.

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/mcp/channel-server.ts
Line: 43

Comment:
**`sessionKey` not normalized before storage**

`params.session_key` is stored as-is (raw, not trimmed). In contrast, `handleSessionMessageEvent` extracts the inbound session key via `toText(payload.sessionKey)`, which trims leading/trailing whitespace. If a notification ever carries a `session_key` with surrounding whitespace, the stored value will never equal the trimmed inbound key, making the bound permission permanently irresolvable without any error.

Applying `.trim()` here would make this consistent with the downstream comparison.

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

_{Reviews (1): Last reviewed commit: "MCP: bind Claude permission replies to s..." | Re-trigger Greptile}

greptile-apps · 2026-03-28T13:59:34Z

  method: z.literal("notifications/claude/channel/permission_request"),
  params: z.object({
    request_id: z.string(),
+    session_key: z.string(),


session_key accepts empty strings

z.string() allows an empty "" to pass schema validation. If a caller sends an empty session_key, the pending permission is stored with an empty string as sessionKey. Because toText(payload.sessionKey) in handleSessionMessageEvent returns undefined for empty/blank strings, the comparison pendingPermission?.sessionKey === sessionKey would evaluate to false for such a request. The permission can never be resolved and leaks in pendingClaudePermissions indefinitely.

Consider tightening the schema with .min(1) to reject empty strings at the boundary.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/mcp/channel-shared.ts Line: 122 Comment: **`session_key` accepts empty strings** `z.string()` allows an empty `""` to pass schema validation. If a caller sends an empty `session_key`, the pending permission is stored with an empty string as `sessionKey`. Because `toText(payload.sessionKey)` in `handleSessionMessageEvent` returns `undefined` for empty/blank strings, the comparison `pendingPermission?.sessionKey === sessionKey` would evaluate to false for such a request. The permission can never be resolved and leaks in `pendingClaudePermissions` indefinitely. Consider tightening the schema with `.min(1)` to reject empty strings at the boundary. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-28T13:59:35Z

@@ -42,6 +42,7 @@ export async function createOpenClawChannelMcpServer(opts: OpenClawMcpServeOptio
  server.server.setNotificationHandler(ClaudePermissionRequestSchema, async ({ params }) => {
    await bridge.handleClaudePermissionRequest({


sessionKey not normalized before storage

params.session_key is stored as-is (raw, not trimmed). In contrast, handleSessionMessageEvent extracts the inbound session key via toText(payload.sessionKey), which trims leading/trailing whitespace. If a notification ever carries a session_key with surrounding whitespace, the stored value will never equal the trimmed inbound key, making the bound permission permanently irresolvable without any error.

Applying .trim() here would make this consistent with the downstream comparison.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/mcp/channel-server.ts Line: 43 Comment: **`sessionKey` not normalized before storage** `params.session_key` is stored as-is (raw, not trimmed). In contrast, `handleSessionMessageEvent` extracts the inbound session key via `toText(payload.sessionKey)`, which trims leading/trailing whitespace. If a notification ever carries a `session_key` with surrounding whitespace, the stored value will never equal the trimmed inbound key, making the bound permission permanently irresolvable without any error. Applying `.trim()` here would make this consistent with the downstream comparison. How can I resolve this? If you propose a fix, please make it concise.

openclaw-barnacle · 2026-04-30T04:38:02Z

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

clawsweeper · 2026-04-30T05:49:47Z

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep this PR open because current main still has the cross-session Claude permission approval bug, but the submitted branch is not merge-ready: it conflicts with the merged pending-map TTL cleanup, adds a required MCP notification field without upgrade approval, and lacks real behavior proof.

Root-cause cluster
Relationship: canonical
Canonical: #56420
Summary: This PR is the active same-repository item for binding Claude MCP permission replies to the originating session; the merged pending-map cleanup is adjacent and does not fix cross-session spoofing.

Members:

superseded: MCP: bind Claude permission replies to originating session #56396 - It proposed the same session-binding change but is closed unmerged, while this PR is the active successor.
partial_overlap: mcp/channel-bridge: pendingClaudePermissions / pendingApprovals leak — no TTL, no close-clear, no cap #71646 - It covered unbounded pending-map retention in the same class, not cross-session permission spoofing.
partial_overlap: fix(mcp): bound pendingClaudePermissions / pendingApprovals via TTL sweeper + close clear #71648 - It merged TTL and close-clear cleanup for the same maps, creating a rebase requirement but not adding session binding.

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

Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review.

So I’m closing this here because the remaining work is already tracked in the canonical issue.

Review details

Best possible solution:

Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review.

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

Yes from source inspection: current main stores pending Claude permissions by request id timestamp and accepts yes/no <id> from any session that supplies that id. I did not run a live MCP/Gateway repro in this read-only review.

Is this the best way to solve the issue?

No as submitted. Session binding is the right invariant, but this branch needs a current-main rebase, a maintainer-approved protocol upgrade path, non-empty normalized session keys, and real behavior proof.

Security review:

Security review needs attention: The patch improves an approval boundary but needs attention for the required protocol change, normalization gap, and missing real behavior proof.

[medium] Required session key changes approval protocol — src/mcp/channel-shared.ts:119
Making session_key mandatory can fail closed for existing Claude permission-request senders; maintainers need to approve and prove that upgrade behavior before merge.
Confidence: 0.88
[low] Unnormalized session keys can strand approvals — src/mcp/channel-server.ts:43
The new session key is stored raw but compared with a normalized inbound session key, so malformed whitespace values can leave permission requests unresolvable until cleanup.
Confidence: 0.83

AGENTS.md: found and applied where relevant.

What I checked:

stale F-rated PR: PR was opened 2026-03-28T13:55:45Z, is older than 60 days, and the latest review rated it F.
proof blocker: real behavior proof is mock_only and proof tier is F, so this branch is not merge-ready without contributor follow-up.
no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

steipete: GitHub path history shows repeated introduction, refactor, docs, and test work around the channel MCP bridge and server. (role: feature introducer and adjacent owner; confidence: high; commits: 71f37a59cacf, ba02905c4f11, d8326f13c330; files: src/mcp/channel-bridge.ts, src/mcp/channel-server.ts, src/mcp/channel-server.test.ts)
Feelw00: Authored the merged TTL and close-clear cleanup for the same pendingClaudePermissions storage path that this PR now needs to rebase over. (role: recent adjacent contributor; confidence: high; commits: 1931bb8de172, c6b1fede5a2b; files: src/mcp/channel-bridge.ts, src/mcp/channel-bridge.test.ts, src/mcp/channel-server.test.ts)
vincentkoc: Recent history touches MCP server/shared surfaces and runtime seam refactors adjacent to the channel bridge behavior. (role: recent area contributor; confidence: medium; commits: 7758f5e22418, 20ef410d6432, 0f7d9c957093; files: src/mcp/channel-server.ts, src/mcp/channel-shared.ts, src/mcp/channel-bridge.ts)

Codex review notes: model internal, reasoning high; reviewed against 12c34fc3a951.

…clear OpenClawChannelBridge held two instance-bound Maps (pendingClaudePermissions L50, pendingApprovals L51) that lacked TTL/sweeper, close-clear, and cap — while the sibling collections in the same class (queue cap=1000, pendingWaiters close-clear + per-waiter setTimeout) were all bounded. Long-running `openclaw mcp serve` processes accumulated entries on missed Claude permission replies and on gateway WebSocket drops that silenced *.approval.resolved frames. Fix scope (one-thing-per-PR): - Wrap entries in PendingClaudePermissionEntry / PendingApprovalEntry with internal createdAtMs / trackedAtMs (instance bookkeeping, decoupled from payload-supplied fields so sweeper fallback is stable). - Lazy-start a 5-min sweepPendingExpired() interval on first set, .unref()'d so it never keeps the process alive. - TTLs: 1h for pendingClaudePermissions (createdAtMs-anchored), respects payload.expiresAtMs for pendingApprovals (falls back to trackedAtMs+30min when the server omits it). - close() now clears both Maps and clearInterval()s the sweeper. - handleClaudePermissionRequest / trackApproval early-return when this.closed to prevent post-close ghost writes. Cap/FIFO is intentionally split to a follow-up PR (one-thing-per-PR; sweeper covers the slow leak; cap addresses adversarial bursts). Tests: - New src/mcp/channel-bridge.test.ts (7 it blocks): TTL sweep for both Maps, default-TTL fallback, both-payload-fields-undefined case, close clears + vi.getTimerCount()===0, no-op-after-close, lazy-init guard. - channel-server.test.ts:366 fixture synced to wrapper shape. AI-assisted (openclaw-audit pipeline). Closes openclaw#71646. cross-review: - 5-agent post-harness primary_decision=proceed (real=4 + fix-insufficient=1 scope-down → applied: cap deferred). - 3-agent pre-pr round 1: critical-devil flagged 4 issues → fixed in this commit. - 3-agent pre-pr round 2: 3/3 real-problem-real-fix → proceed_to_pr. Upstream-dup check clean: 6w channel-bridge.ts commits all refactor/seam axis; PR openclaw#56420 (sessionKey binding) orthogonal but touches the same set() lines — rebase order may matter.

clawsweeper · 2026-05-20T05:09:06Z

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?

The egg game starts only after the PR passes the real-behavior proof check.
Before that, no creature or rarity is rolled. The treat waits for real proof.
This is still just collectible flavor: proof affects review readiness, not creature quality.

…clear OpenClawChannelBridge held two instance-bound Maps (pendingClaudePermissions L50, pendingApprovals L51) that lacked TTL/sweeper, close-clear, and cap — while the sibling collections in the same class (queue cap=1000, pendingWaiters close-clear + per-waiter setTimeout) were all bounded. Long-running `openclaw mcp serve` processes accumulated entries on missed Claude permission replies and on gateway WebSocket drops that silenced *.approval.resolved frames. Fix scope (one-thing-per-PR): - Wrap entries in PendingClaudePermissionEntry / PendingApprovalEntry with internal createdAtMs / trackedAtMs (instance bookkeeping, decoupled from payload-supplied fields so sweeper fallback is stable). - Lazy-start a 5-min sweepPendingExpired() interval on first set, .unref()'d so it never keeps the process alive. - TTLs: 1h for pendingClaudePermissions (createdAtMs-anchored), respects payload.expiresAtMs for pendingApprovals (falls back to trackedAtMs+30min when the server omits it). - close() now clears both Maps and clearInterval()s the sweeper. - handleClaudePermissionRequest / trackApproval early-return when this.closed to prevent post-close ghost writes. Cap/FIFO is intentionally split to a follow-up PR (one-thing-per-PR; sweeper covers the slow leak; cap addresses adversarial bursts). Tests: - New src/mcp/channel-bridge.test.ts (7 it blocks): TTL sweep for both Maps, default-TTL fallback, both-payload-fields-undefined case, close clears + vi.getTimerCount()===0, no-op-after-close, lazy-init guard. - channel-server.test.ts:366 fixture synced to wrapper shape. AI-assisted (openclaw-audit pipeline). Closes openclaw#71646. cross-review: - 5-agent post-harness primary_decision=proceed (real=4 + fix-insufficient=1 scope-down → applied: cap deferred). - 3-agent pre-pr round 1: critical-devil flagged 4 issues → fixed in this commit. - 3-agent pre-pr round 2: 3/3 real-problem-real-fix → proceed_to_pr. Upstream-dup check clean: 6w channel-bridge.ts commits all refactor/seam axis; PR openclaw#56420 (sessionKey binding) orthogonal but touches the same set() lines — rebase order may matter.

openclaw-barnacle · 2026-06-10T04:58:02Z

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

clawsweeper · 2026-06-21T13:31:59Z

ClawSweeper applied the proposed close for this PR.

Action: closed this PR.
Close reason: duplicate or superseded.
Evidence: durable ClawSweeper review.

MCP: bind Claude permission replies to session

face4c8

openclaw-barnacle Bot added the size: S label Mar 28, 2026

greptile-apps Bot reviewed Mar 28, 2026

View reviewed changes

This was referenced Apr 25, 2026

mcp/channel-bridge: pendingClaudePermissions / pendingApprovals leak — no TTL, no close-clear, no cap #71646

Closed

fix(mcp): bound pendingClaudePermissions / pendingApprovals via TTL sweeper + close clear #71648

Merged

openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Apr 30, 2026

openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label May 1, 2026

openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 19, 2026

openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 10, 2026

openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jun 10, 2026

clawsweeper Bot removed the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 14, 2026

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 14, 2026

clawsweeper Bot closed this Jun 21, 2026

clawsweeper Bot mentioned this pull request Jun 30, 2026

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: bind Claude permission replies to session#56420

fix: bind Claude permission replies to session#56420
lifelikezzh wants to merge 1 commit into
openclaw:mainfrom
lifelikezzh:codex/propose-fix-for-mcp-bridge-vulnerability

lifelikezzh commented Mar 28, 2026

Uh oh!

greptile-apps Bot commented Mar 28, 2026

Important Files Changed

Uh oh!

greptile-apps Bot Mar 28, 2026

Uh oh!

greptile-apps Bot Mar 28, 2026

Uh oh!

openclaw-barnacle Bot commented Apr 30, 2026

Uh oh!

clawsweeper Bot commented Apr 30, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 20, 2026

Uh oh!

openclaw-barnacle Bot commented Jun 10, 2026

Uh oh!

clawsweeper Bot commented Jun 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		@@ -42,6 +42,7 @@ export async function createOpenClawChannelMcpServer(opts: OpenClawMcpServeOptio
		server.server.setNotificationHandler(ClaudePermissionRequestSchema, async ({ params }) => {
		await bridge.handleClaudePermissionRequest({

Uh oh!

Uh oh!

Conversation

lifelikezzh commented Mar 28, 2026

Motivation

Description

Testing

Uh oh!

greptile-apps Bot commented Mar 28, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

greptile-apps Bot Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

openclaw-barnacle Bot commented Apr 30, 2026

Uh oh!

clawsweeper Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clawsweeper Bot commented May 20, 2026

Uh oh!

openclaw-barnacle Bot commented Jun 10, 2026

Uh oh!

clawsweeper Bot commented Jun 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented Apr 30, 2026 •

edited

Loading