Skip to content

Commit b885c81

Browse files
eleqtriziteleqtrizit
authored
fix(mcp): require owner for Claude permission replies (#98256)
* fix(mcp): require owner for Claude permission replies * fix(mcp): prove owner-gated permission replies
1 parent 9f07b21 commit b885c81

16 files changed

Lines changed: 194 additions & 12 deletions

‎docs/cli/mcp.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ Current bridge behavior:
253253

254254
- inbound `user` transcript messages are forwarded as `notifications/claude/channel`
255255
- Claude permission requests received over MCP are tracked in-memory
256-
- if the linked conversation later sends `yes abcde` or `no abcde`, the bridge converts that to `notifications/claude/channel/permission`
256+
- if the command owner in the linked conversation later sends `yes abcde` or `no abcde`, the bridge converts that to `notifications/claude/channel/permission`
257257
- these notifications are live-session only; if the MCP client disconnects, there is no push target
258258

259259
This is intentionally client-specific. Generic MCP clients should rely on the standard polling tools.

‎src/auto-reply/reply/get-reply-run.media-only.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2833,6 +2833,9 @@ describe("runPreparedReply media-only handling", () => {
28332833

28342834
const call = requireRunReplyAgentCall();
28352835
expect(call?.followupRun.run.senderIsOwner).toBe(true);
2836+
expect(call?.followupRun.userTurnTranscriptRecorder?.message).toMatchObject({
2837+
__openclaw: { senderIsOwner: true },
2838+
});
28362839
});
28372840

28382841
it("keeps sender ownership when drained system events are present", async () => {

‎src/auto-reply/reply/get-reply-run.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1247,6 +1247,7 @@ export async function runPreparedReply(
12471247
userTurnTranscriptText !== undefined || userTurnMediaForPersistence.length > 0
12481248
? {
12491249
text: userTurnTranscriptText,
1250+
senderIsOwner: command.senderIsOwner,
12501251
...(inputProvenance ? { provenance: inputProvenance } : {}),
12511252
...(userTurnMediaForPersistence.length > 0
12521253
? {

‎src/gateway/server-methods/chat.directive-tags.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4401,6 +4401,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
44014401
content: "bench update",
44024402
timestamp: expect.any(Number),
44034403
idempotencyKey: "idem-system-provenance-acp:user",
4404+
__openclaw: { senderIsOwner: true },
44044405
provenance: {
44054406
...provenance,
44064407
sourceSessionKey: undefined,

‎src/gateway/server-methods/chat.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1320,7 +1320,7 @@ function isAcpBridgeClient(client: GatewayRequestHandlerOptions["client"]): bool
13201320
);
13211321
}
13221322

1323-
function canInjectSystemProvenance(client: GatewayRequestHandlerOptions["client"]): boolean {
1323+
function hasGatewayAdminScope(client: GatewayRequestHandlerOptions["client"]): boolean {
13241324
const scopes = Array.isArray(client?.connect?.scopes) ? client.connect.scopes : [];
13251325
return scopes.includes(ADMIN_SCOPE);
13261326
}
@@ -3437,7 +3437,7 @@ export const chatHandlers: GatewayRequestHandlers = {
34373437
p.systemProvenanceReceipt ||
34383438
suppressCommandInterpretation ||
34393439
explicitOriginResult.value) &&
3440-
!canInjectSystemProvenance(client)
3440+
!hasGatewayAdminScope(client)
34413441
) {
34423442
respond(
34433443
false,
@@ -3884,6 +3884,7 @@ export const chatHandlers: GatewayRequestHandlers = {
38843884
text: rawMessage,
38853885
timestamp: now,
38863886
idempotencyKey: `${clientRunId}:user`,
3887+
...(hasGatewayAdminScope(client) ? { senderIsOwner: true } : {}),
38873888
...(systemInputProvenance ? { provenance: systemInputProvenance } : {}),
38883889
};
38893890
const userTurnInputPromise: Promise<UserTurnInput> = userTurnMediaPromise.then((media) => ({

‎src/gateway/server-session-events.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,4 +100,16 @@ describe("createTranscriptUpdateBroadcastHandler", () => {
100100
},
101101
});
102102
});
103+
104+
it("broadcasts the authenticated sender ownership decision", async () => {
105+
await expect(
106+
emitAssistantTranscriptUpdate(false, {
107+
role: "user",
108+
content: [{ type: "text", text: "Owner turn" }],
109+
__openclaw: { senderIsOwner: true },
110+
}),
111+
).resolves.toMatchObject({
112+
senderIsOwner: true,
113+
});
114+
});
103115
});

‎src/gateway/server-session-events.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,18 @@ function readMessageIdempotencyKey(message: unknown): string | undefined {
3838
return typeof value === "string" && value.trim() ? value : undefined;
3939
}
4040

41+
function readMessageSenderIsOwner(message: unknown): boolean | undefined {
42+
if (!message || typeof message !== "object" || Array.isArray(message)) {
43+
return undefined;
44+
}
45+
const openclaw = (message as Record<string, unknown>)["__openclaw"];
46+
if (!openclaw || typeof openclaw !== "object" || Array.isArray(openclaw)) {
47+
return undefined;
48+
}
49+
const value = (openclaw as Record<string, unknown>).senderIsOwner;
50+
return typeof value === "boolean" ? value : undefined;
51+
}
52+
4153
function resolveSessionMessageBroadcastKeys(sessionKey: string, agentId?: string): string[] {
4254
// Global sessions can be subscribed through either the raw global key or the
4355
// default-agent scoped key; non-default agent global sessions stay scoped.
@@ -182,6 +194,7 @@ async function handleTranscriptUpdateBroadcast(
182194
hasActiveRun,
183195
});
184196
const idempotencyKey = readMessageIdempotencyKey(update.message);
197+
const senderIsOwner = readMessageSenderIsOwner(update.message);
185198
const rawMessage = attachOpenClawTranscriptMeta(update.message, {
186199
...(typeof update.messageId === "string" ? { id: update.messageId } : {}),
187200
...(idempotencyKey ? { idempotencyKey } : {}),
@@ -193,6 +206,7 @@ async function handleTranscriptUpdateBroadcast(
193206
"session.message",
194207
{
195208
sessionKey,
209+
...(senderIsOwner === undefined ? {} : { senderIsOwner }),
196210
...(visibleAgentId ? { agentId: visibleAgentId } : {}),
197211
message,
198212
...(typeof update.messageId === "string" ? { messageId: update.messageId } : {}),

‎src/mcp/channel-bridge.test.ts‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ type BridgeInternals = {
1616
pendingClaudePermissions: Map<string, unknown>;
1717
pendingApprovals: Map<string, unknown>;
1818
pendingSweepInterval: NodeJS.Timeout | null;
19-
pollEvents: (filter: WaitFilter, limit?: number) => {
19+
pollEvents: (
20+
filter: WaitFilter,
21+
limit?: number,
22+
) => {
2023
events: QueueEvent[];
2124
nextCursor: number;
2225
};
@@ -31,6 +34,11 @@ type BridgeInternals = {
3134
event: string;
3235
payload?: Record<string, unknown>;
3336
}) => Promise<void>;
37+
handleSessionMessageEvent: (payload: {
38+
sessionKey: string;
39+
senderIsOwner?: boolean;
40+
message: { role: string; content: unknown };
41+
}) => Promise<void>;
3442
listPendingApprovals: () => unknown[];
3543
close: () => Promise<void>;
3644
server: { server: { notification: (n: unknown) => Promise<void> } } | null;
@@ -44,6 +52,72 @@ function makeBridge(verbose = false): BridgeInternals {
4452
}) as unknown as BridgeInternals;
4553
}
4654

55+
describe("OpenClawChannelBridge — Claude permission authorization", () => {
56+
test.each([
57+
{ name: "non-owner", senderIsOwner: false, role: "user" },
58+
{ name: "missing owner metadata", senderIsOwner: undefined, role: "user" },
59+
{ name: "assistant message", senderIsOwner: true, role: "assistant" },
60+
])("does not resolve a pending permission from a $name reply", async (reply) => {
61+
const bridge = makeBridge();
62+
const notification = vi.fn(async () => undefined);
63+
bridge.server = { server: { notification } };
64+
try {
65+
await bridge.handleClaudePermissionRequest({
66+
requestId: "abcde",
67+
toolName: "Bash",
68+
description: "run npm test",
69+
inputPreview: "{}",
70+
});
71+
72+
await bridge.handleSessionMessageEvent({
73+
sessionKey: "agent:main:telegram:group:-100123",
74+
senderIsOwner: reply.senderIsOwner,
75+
message: {
76+
role: reply.role,
77+
content: [{ type: "text", text: "yes abcde" }],
78+
},
79+
});
80+
81+
expect(notification).not.toHaveBeenCalled();
82+
expect(bridge.pendingClaudePermissions.has("abcde")).toBe(true);
83+
expect(bridge.queue.at(-1)).toMatchObject({ type: "message", text: "yes abcde" });
84+
} finally {
85+
await bridge.close();
86+
}
87+
});
88+
89+
test("resolves a pending permission from an owner user reply", async () => {
90+
const bridge = makeBridge();
91+
const notification = vi.fn(async () => undefined);
92+
bridge.server = { server: { notification } };
93+
try {
94+
await bridge.handleClaudePermissionRequest({
95+
requestId: "abcde",
96+
toolName: "Bash",
97+
description: "run npm test",
98+
inputPreview: "{}",
99+
});
100+
101+
await bridge.handleSessionMessageEvent({
102+
sessionKey: "agent:main:telegram:group:-100123",
103+
senderIsOwner: true,
104+
message: {
105+
role: "user",
106+
content: [{ type: "text", text: "yes abcde" }],
107+
},
108+
});
109+
110+
expect(notification).toHaveBeenCalledWith({
111+
method: "notifications/claude/channel/permission",
112+
params: { request_id: "abcde", behavior: "allow" },
113+
});
114+
expect(bridge.pendingClaudePermissions.has("abcde")).toBe(false);
115+
} finally {
116+
await bridge.close();
117+
}
118+
});
119+
});
120+
47121
describe("OpenClawChannelBridge — pendingClaudePermissions / pendingApprovals memory bounds", () => {
48122
beforeEach(() => {
49123
vi.useFakeTimers();

‎src/mcp/channel-bridge.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,9 @@ export class OpenClawChannelBridge {
584584
const role = toText(payload.message?.role);
585585
const text = extractFirstTextBlock(payload.message);
586586
const permissionMatch = text ? CLAUDE_PERMISSION_REPLY_RE.exec(text) : null;
587-
if (permissionMatch) {
587+
// Ownership is decided at authenticated channel ingress and carried on the
588+
// live transcript event. Missing metadata fails closed for approvals.
589+
if (role === "user" && payload.senderIsOwner === true && permissionMatch) {
588590
const requestId = normalizeOptionalLowercaseString(permissionMatch[2]);
589591
if (requestId && this.pendingClaudePermissions.has(requestId)) {
590592
this.pendingClaudePermissions.delete(requestId);

‎src/mcp/channel-server.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,7 @@ describe("openclaw channel mcp server", () => {
324324
}
325325
).handleSessionMessageEvent({
326326
sessionKey,
327+
senderIsOwner: true,
327328
lastChannel: "imessage",
328329
lastTo: "+15551234567",
329330
messageId: "msg-user-1",
@@ -358,6 +359,7 @@ describe("openclaw channel mcp server", () => {
358359
}
359360
).handleSessionMessageEvent({
360361
sessionKey,
362+
senderIsOwner: true,
361363
lastChannel: "imessage",
362364
lastTo: "+15551234567",
363365
messageId: "msg-user-2",

0 commit comments

Comments
 (0)