From 0e7a992d3f3155199c1acc2dd9a53c5b3a4d3ada Mon Sep 17 00:00:00 2001 From: Devin Robison Date: Fri, 17 Apr 2026 14:45:12 -0600 Subject: [PATCH] fix(agents): filter bundled tools through final policy (#68195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agents): filter bundled tools through final policy * changelog: filter bundled tools through final policy (#68195) * forward agentId into compaction tool-policy filter Pass effectiveSkillAgentId to applyFinalEffectiveToolPolicy in the compaction path so per-agent tool policies apply to bundled tools during compaction the same way they do during normal runs. * scope final tool-policy filter to bundled tools only Running the full tool-policy pipeline on the merged core + bundled tool list re-filters core tools whose plugin WeakMap metadata no longer survives the normalize/hook wrappers applied by createOpenClawCodingTools(). Narrow the helper to only the newly-appended bundled MCP/LSP tools so plugin-provided core tools keep matching group:plugins and plugin-id allowlist entries. * harden authorization signals on final tool policy - message.action gateway handler now server-derives senderIsOwner from the authenticated gateway client scopes (ADMIN_SCOPE on client.connect.scopes) and ignores any senderIsOwner value on the wire, so a non-admin scoped caller cannot spoof owner status to unlock owner-only channel actions or owner-only tool policy. Schema keeps the field optional for wire compat but documents that it is ignored. - applyFinalEffectiveToolPolicy now cross-checks caller-provided groupId against the session-derived group context resolved from sessionKey (and spawnedBy). When they disagree, the caller groupId plus its adjacent groupChannel/groupSpace are dropped and a warn is emitted, so a caller that fabricates a different group id cannot reach a more permissive group-scoped tool policy during the final bundled-tool filter. Added a JSDoc trust invariant on the helper input describing the required server-verified identity contract. * align compact agentId resolution with core tools Drop the explicit agentId on applyFinalEffectiveToolPolicy during compaction. The core tool set produced just above via createOpenClawCodingTools(...) also omits agentId, so resolveEffectiveToolPolicy falls back to resolveAgentIdFromSessionKey(sessionKey) in both places. Passing effectiveSkillAgentId only to the final filter made the two policy lookups diverge on legacy/non-agent session keys where the sessionKey path resolves to main but effectiveSkillAgentId follows the configured default-agent path, which could deny or allow bundled tools under a different per-agent policy than the already-created core tools. * tighten trusted propagation for owner and group signals - message.action gateway handler: full-operator callers (shared-secret bearer or operator.admin scope) now propagate the request-provided senderIsOwner through to channel action handlers instead of having it hard-coded off. Previously the hardened path force-derived ownership from ADMIN_SCOPE alone, which broke owner-gated actions when the trusted runtime forwards them via the least-privilege gateway path (callGatewayLeastPrivilege requests only the method scope, so even legitimate owner senders were downgraded to senderIsOwner=false). Narrowly-scoped callers (e.g. operator.write-only) still have the wire value forced to false so a non-admin caller cannot assert ownership. - applyFinalEffectiveToolPolicy: fail-closed when the session key and spawnedBy encode no group context. Previously the helper only dropped a caller-provided groupId that conflicted with a non-empty set of session-derived group ids, which left an accept-caller fallback open when the session had no group context at all (direct/cron/subagent session keys). An attacker who could run without a group-bound session could then supply an arbitrary groupId and reach a more permissive group-scoped tool policy. Now: no session-derived group context plus any caller-provided groupId drops the caller value and warns. * suppress unavailable-core-tool warnings in bundled-only pass applyToolPolicyPipeline infers its coreToolNames reference set from the tools array it is filtering. The bundled-only second pass only sees the MCP/LSP subset, so normal core allowlist entries (for example tools.allow: ['read', 'exec']) would look "unknown" during this pass and emit misleading warnings even when the config is valid for the full effective tool set — polluting logs and potentially evicting real diagnostics from the shared warning cache. Set suppressUnavailableCoreToolWarning on every step of this pass so known core-tool allowlist entries stay silent; genuinely unknown entries still surface through the otherEntries warning path. --- CHANGELOG.md | 1 + src/agents/pi-embedded-runner/compact.ts | 33 +++- .../effective-tool-policy.test.ts | 120 ++++++++++++ .../effective-tool-policy.ts | 175 ++++++++++++++++++ src/agents/pi-embedded-runner/run/attempt.ts | 28 ++- src/agents/pi-tools.policy.ts | 2 +- src/gateway/protocol/schema/agent.ts | 4 + src/gateway/server-methods/send.test.ts | 98 +++++++++- src/gateway/server-methods/send.ts | 22 ++- 9 files changed, 468 insertions(+), 15 deletions(-) create mode 100644 src/agents/pi-embedded-runner/effective-tool-policy.test.ts create mode 100644 src/agents/pi-embedded-runner/effective-tool-policy.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e07b8656f83..c5a590096e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ Docs: https://docs.openclaw.ai - Cron/isolated-agent: preserve `trusted: false` on isolated cron awareness events mirrored into the main session, and forward the optional `trusted` flag through the gateway cron wrapper so explicit trust downgrades survive session-key scoping. (#68210) - Agents/fallback: recognize bare leading ZenMux `402 ...` quota-refresh errors without misclassifying plain numeric `402 ...` text, and keep the embedded fallback regression coverage stable. (#47579) Thanks @bwjoke. - Failover/google: only treat `INTERNAL` status payloads as retryable timeouts when they also carry a `500` code, so malformed non-500 payloads do not enter the retry path. (#68238) Thanks @altaywtf and @Openbling. +- Agents/tools: filter bundled MCP/LSP tools through the final owner-only and tool-policy pipeline after merging them into the effective tool list, so existing allowlists, deny rules, sandbox policy, subagent policy, and owner-only restrictions apply to bundled tools the same way they apply to core tools. (#68195) ## 2026.4.15 diff --git a/src/agents/pi-embedded-runner/compact.ts b/src/agents/pi-embedded-runner/compact.ts index ccf980e0d7b..a7b10aced41 100644 --- a/src/agents/pi-embedded-runner/compact.ts +++ b/src/agents/pi-embedded-runner/compact.ts @@ -106,6 +106,7 @@ import { compactWithSafetyTimeout, resolveCompactionTimeoutMs, } from "./compaction-safety-timeout.js"; +import { applyFinalEffectiveToolPolicy } from "./effective-tool-policy.js"; import { buildEmbeddedExtensionFactories } from "./extensions.js"; import { applyExtraParamsToAgent } from "./extra-params.js"; import { getDmHistoryLimitFromSessionKey, limitHistoryTurns } from "./history.js"; @@ -554,11 +555,33 @@ export async function compactEmbeddedPiSessionDirect( ], }) : undefined; - const effectiveTools = [ - ...tools, - ...(bundleMcpRuntime?.tools ?? []), - ...(bundleLspRuntime?.tools ?? []), - ]; + const filteredBundledTools = applyFinalEffectiveToolPolicy({ + bundledTools: [...(bundleMcpRuntime?.tools ?? []), ...(bundleLspRuntime?.tools ?? [])], + config: params.config, + sandboxToolPolicy: sandbox?.tools, + sessionKey: sandboxSessionKey, + // Intentionally omit explicit agentId: the core tools just built with + // createOpenClawCodingTools(...) also omit it, so both paths resolve + // agentId the same way via resolveAgentIdFromSessionKey(sessionKey). + // Passing effectiveSkillAgentId here would diverge from the core-tool + // policy for legacy/non-agent session keys where the two sources fall + // back to different ids. + modelProvider: model.provider, + modelId, + messageProvider: resolvedMessageProvider, + agentAccountId: params.agentAccountId, + groupId: params.groupId, + groupChannel: params.groupChannel, + groupSpace: params.groupSpace, + spawnedBy: params.spawnedBy, + senderId: params.senderId, + senderName: params.senderName, + senderUsername: params.senderUsername, + senderE164: params.senderE164, + senderIsOwner: params.senderIsOwner, + warn: (message) => log.warn(message), + }); + const effectiveTools = [...tools, ...filteredBundledTools]; const allowedToolNames = collectAllowedToolNames({ tools: effectiveTools }); logProviderToolSchemaDiagnostics({ tools: effectiveTools, diff --git a/src/agents/pi-embedded-runner/effective-tool-policy.test.ts b/src/agents/pi-embedded-runner/effective-tool-policy.test.ts new file mode 100644 index 00000000000..2e210d067f2 --- /dev/null +++ b/src/agents/pi-embedded-runner/effective-tool-policy.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, it } from "vitest"; +import type { AnyAgentTool } from "../tools/common.js"; +import { applyFinalEffectiveToolPolicy } from "./effective-tool-policy.js"; + +function makeTool(name: string, ownerOnly = false): AnyAgentTool { + return { + name, + label: name, + description: name, + parameters: { type: "object", properties: {} }, + ownerOnly, + execute: async () => ({ content: [{ type: "text", text: "ok" }], details: {} }), + }; +} + +describe("applyFinalEffectiveToolPolicy", () => { + it("filters bundled tools through the configured allowlist", () => { + const filtered = applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__fs_delete"), makeTool("mcp__bundle__fs_read")], + config: { tools: { allow: ["mcp__bundle__fs_read"] } }, + warn: () => {}, + }); + + expect(filtered.map((tool) => tool.name)).toEqual(["mcp__bundle__fs_read"]); + }); + + it("applies owner-only filtering to bundled tools", () => { + const filtered = applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__read"), makeTool("mcp__bundle__admin", true)], + senderIsOwner: false, + warn: () => {}, + }); + + expect(filtered.map((tool) => tool.name)).toEqual(["mcp__bundle__read"]); + }); + + it("returns the empty array unchanged when there are no bundled tools", () => { + const filtered = applyFinalEffectiveToolPolicy({ + bundledTools: [], + config: { tools: { allow: ["message"] } }, + warn: () => {}, + }); + + expect(filtered).toEqual([]); + }); + + it("drops caller-provided groupId when it disagrees with session-derived group context", () => { + const warnings: string[] = []; + applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__read")], + // Session key encodes a concrete group (discord room 111); caller tries + // to override with a different group id so a more permissive group + // policy for group 222 could be consulted. + sessionKey: "agent:alice:discord:group:111", + groupId: "222", + groupChannel: "#different", + warn: (message) => warnings.push(message), + }); + + expect(warnings).toContain( + "effective tool policy: dropping caller-provided groupId that does not match session-derived group context", + ); + }); + + it("drops caller-provided groupId when session encodes no group context (fail-closed)", () => { + const warnings: string[] = []; + applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__read")], + // Direct/non-group session key: no session-derived group ids. A caller + // supplying a groupId here has no server-verified ground truth; it + // must be dropped so a spoofed group cannot reach a permissive policy. + sessionKey: "agent:alice:main", + groupId: "admin-group", + groupChannel: "#admin", + warn: (message) => warnings.push(message), + }); + + expect(warnings).toContain( + "effective tool policy: dropping caller-provided groupId that does not match session-derived group context", + ); + }); + + it("leaves groupId untouched when caller did not supply one", () => { + const warnings: string[] = []; + applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__read")], + sessionKey: "agent:alice:main", + warn: (message) => warnings.push(message), + }); + + expect(warnings).not.toContain( + "effective tool policy: dropping caller-provided groupId that does not match session-derived group context", + ); + }); + + it("does not emit unknown-entry warnings for core tool allowlists in the bundled pass", () => { + const warnings: string[] = []; + applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__read")], + // Core tool names like `read` and `exec` are not in the bundled-only + // input here, but they are valid core tools resolved by the first + // pass. The bundled pass must not warn about them as "unknown". + config: { tools: { allow: ["read", "exec", "mcp__bundle__read"] } }, + warn: (message) => warnings.push(message), + }); + + expect(warnings.some((w) => w.includes("unknown entries"))).toBe(false); + }); + + it("still warns on genuinely unknown entries in the bundled pass", () => { + const warnings: string[] = []; + applyFinalEffectiveToolPolicy({ + bundledTools: [makeTool("mcp__bundle__read")], + config: { tools: { allow: ["mcp__bundle__read", "totally-made-up-tool"] } }, + warn: (message) => warnings.push(message), + }); + + expect(warnings.some((w) => w.includes("totally-made-up-tool"))).toBe(true); + }); +}); diff --git a/src/agents/pi-embedded-runner/effective-tool-policy.ts b/src/agents/pi-embedded-runner/effective-tool-policy.ts new file mode 100644 index 00000000000..7d1cf085f22 --- /dev/null +++ b/src/agents/pi-embedded-runner/effective-tool-policy.ts @@ -0,0 +1,175 @@ +import type { OpenClawConfig } from "../../config/types.openclaw.js"; +import { getPluginToolMeta } from "../../plugins/tools.js"; +import { isSubagentSessionKey } from "../../routing/session-key.js"; +import { + resolveEffectiveToolPolicy, + resolveGroupContextFromSessionKey, + resolveGroupToolPolicy, + resolveSubagentToolPolicyForSession, +} from "../pi-tools.policy.js"; +import { + applyToolPolicyPipeline, + buildDefaultToolPolicyPipelineSteps, + type ToolPolicyPipelineStep, +} from "../tool-policy-pipeline.js"; +import { + applyOwnerOnlyToolPolicy, + mergeAlsoAllowPolicy, + resolveToolProfilePolicy, +} from "../tool-policy.js"; +import type { AnyAgentTool } from "../tools/common.js"; + +/** + * Identity inputs used by `resolveGroupToolPolicy` to look up channel/group + * tool policy. These fields are an authorization signal (they can widen + * bundled-tool availability via a group-scoped allowlist), so callers MUST + * pass values derived from server-verified session metadata (session key, + * inbound transport event), not from tool-call or model-controlled input. + * The helper cross-checks caller-provided `groupId` against session-derived + * group ids and drops the caller value when they disagree, but it cannot + * detect drift on fields that have no session-bound counterpart. + */ +type FinalEffectiveToolPolicyParams = { + // Tools appended to the core tool set after `createOpenClawCodingTools()` + // has already applied owner-only and tool-policy filtering (e.g. bundled + // MCP/LSP tools). Only these are filtered here; re-running the pipeline over + // the already-filtered core tools would drop plugin tools whose WeakMap + // metadata no longer survives core-tool wrapping/normalization. + bundledTools: AnyAgentTool[]; + config?: OpenClawConfig; + sandboxToolPolicy?: { allow?: string[]; deny?: string[] }; + sessionKey?: string; + agentId?: string; + modelProvider?: string; + modelId?: string; + messageProvider?: string; + agentAccountId?: string | null; + groupId?: string | null; + groupChannel?: string | null; + groupSpace?: string | null; + spawnedBy?: string | null; + senderId?: string | null; + senderName?: string | null; + senderUsername?: string | null; + senderE164?: string | null; + senderIsOwner?: boolean; + warn: (message: string) => void; +}; + +function resolveTrustedGroupId(params: FinalEffectiveToolPolicyParams): { + groupId: string | null | undefined; + dropped: boolean; +} { + const callerGroupId = (params.groupId ?? "").trim(); + if (!callerGroupId) { + return { groupId: params.groupId, dropped: false }; + } + const sessionGroupIds = resolveGroupContextFromSessionKey(params.sessionKey).groupIds ?? []; + const spawnedGroupIds = resolveGroupContextFromSessionKey(params.spawnedBy).groupIds ?? []; + const trusted = [...sessionGroupIds, ...spawnedGroupIds]; + // Fail-closed: if the session/spawnedBy keys do not encode a group context, + // we have no server-verified ground truth to compare the caller value + // against. A non-group session (direct, subagent, cron) should not consult + // a group-scoped tool policy at all, and accepting the caller's groupId + // here would let an attacker widen bundled-tool availability by sending + // an arbitrary group id. + if (trusted.length === 0) { + return { groupId: null, dropped: true }; + } + if (trusted.includes(callerGroupId)) { + return { groupId: params.groupId, dropped: false }; + } + return { groupId: null, dropped: true }; +} + +export function applyFinalEffectiveToolPolicy( + params: FinalEffectiveToolPolicyParams, +): AnyAgentTool[] { + if (params.bundledTools.length === 0) { + return params.bundledTools; + } + const trustedGroup = resolveTrustedGroupId(params); + if (trustedGroup.dropped) { + params.warn( + "effective tool policy: dropping caller-provided groupId that does not match session-derived group context", + ); + } + const { + agentId, + globalPolicy, + globalProviderPolicy, + agentPolicy, + agentProviderPolicy, + profile, + providerProfile, + profileAlsoAllow, + providerProfileAlsoAllow, + } = resolveEffectiveToolPolicy({ + config: params.config, + sessionKey: params.sessionKey, + agentId: params.agentId, + modelProvider: params.modelProvider, + modelId: params.modelId, + }); + + const groupPolicy = resolveGroupToolPolicy({ + config: params.config, + sessionKey: params.sessionKey, + spawnedBy: params.spawnedBy, + messageProvider: params.messageProvider, + groupId: trustedGroup.groupId, + groupChannel: trustedGroup.dropped ? null : params.groupChannel, + groupSpace: trustedGroup.dropped ? null : params.groupSpace, + accountId: params.agentAccountId, + senderId: params.senderId, + senderName: params.senderName, + senderUsername: params.senderUsername, + senderE164: params.senderE164, + }); + const profilePolicy = resolveToolProfilePolicy(profile); + const providerProfilePolicy = resolveToolProfilePolicy(providerProfile); + const profilePolicyWithAlsoAllow = mergeAlsoAllowPolicy(profilePolicy, profileAlsoAllow); + const providerProfilePolicyWithAlsoAllow = mergeAlsoAllowPolicy( + providerProfilePolicy, + providerProfileAlsoAllow, + ); + const subagentPolicy = + isSubagentSessionKey(params.sessionKey) && params.sessionKey + ? resolveSubagentToolPolicyForSession(params.config, params.sessionKey) + : undefined; + const ownerFiltered = applyOwnerOnlyToolPolicy(params.bundledTools, params.senderIsOwner === true); + // Suppress unavailable-core-tool warnings on every step of this pass. + // `applyToolPolicyPipeline` infers `coreToolNames` from the `tools` array + // it's filtering, and this pass only sees the bundled MCP/LSP subset. + // Normal core allowlist entries (e.g. `tools.allow: ["read", "exec"]`) + // would look "unknown" relative to that reduced set even though they are + // valid core names already resolved by `createOpenClawCodingTools()` in + // the first pass — keeping those warnings on would pollute logs and evict + // real diagnostics from the shared warning cache. Genuinely unknown + // entries (typos) still surface through the `otherEntries` path in + // `applyToolPolicyPipeline`. + const pipelineSteps: ToolPolicyPipelineStep[] = [ + ...buildDefaultToolPolicyPipelineSteps({ + profilePolicy: profilePolicyWithAlsoAllow, + profile, + profileUnavailableCoreWarningAllowlist: profilePolicy?.allow, + providerProfilePolicy: providerProfilePolicyWithAlsoAllow, + providerProfile, + providerProfileUnavailableCoreWarningAllowlist: providerProfilePolicy?.allow, + globalPolicy, + globalProviderPolicy, + agentPolicy, + agentProviderPolicy, + groupPolicy, + agentId, + }), + { policy: params.sandboxToolPolicy, label: "sandbox tools.allow" }, + { policy: subagentPolicy, label: "subagent tools.allow" }, + ].map((step) => ({ ...step, suppressUnavailableCoreToolWarning: true })); + return applyToolPolicyPipeline({ + tools: ownerFiltered, + toolMeta: (tool) => getPluginToolMeta(tool), + warn: params.warn, + steps: pipelineSteps, + }); +} diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index df08822a05e..76d3e9d1407 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -125,6 +125,7 @@ import { isRunnerAbortError } from "../abort.js"; import { isCacheTtlEligibleProvider, readLastCacheTtlTimestamp } from "../cache-ttl.js"; import { resolveCompactionTimeoutMs } from "../compaction-safety-timeout.js"; import { runContextEngineMaintenance } from "../context-engine-maintenance.js"; +import { applyFinalEffectiveToolPolicy } from "../effective-tool-policy.js"; import { buildEmbeddedExtensionFactories } from "../extensions.js"; import { applyExtraParamsToAgent, resolveAgentTransportOverride } from "../extra-params.js"; import { prepareGooglePromptCacheStreamFn } from "../google-prompt-cache.js"; @@ -678,11 +679,28 @@ export async function runEmbeddedAttempt( ], }) : undefined; - const effectiveTools = [ - ...tools, - ...(bundleMcpRuntime?.tools ?? []), - ...(bundleLspRuntime?.tools ?? []), - ]; + const filteredBundledTools = applyFinalEffectiveToolPolicy({ + bundledTools: [...(bundleMcpRuntime?.tools ?? []), ...(bundleLspRuntime?.tools ?? [])], + config: params.config, + sandboxToolPolicy: sandbox?.tools, + sessionKey: sandboxSessionKey, + agentId: sessionAgentId, + modelProvider: params.provider, + modelId: params.modelId, + messageProvider: params.messageChannel ?? params.messageProvider, + agentAccountId: params.agentAccountId, + groupId: params.groupId, + groupChannel: params.groupChannel, + groupSpace: params.groupSpace, + spawnedBy: params.spawnedBy, + senderId: params.senderId, + senderName: params.senderName, + senderUsername: params.senderUsername, + senderE164: params.senderE164, + senderIsOwner: params.senderIsOwner, + warn: (message) => log.warn(message), + }); + const effectiveTools = [...tools, ...filteredBundledTools]; const allowedToolNames = collectAllowedToolNames({ tools: effectiveTools, clientTools, diff --git a/src/agents/pi-tools.policy.ts b/src/agents/pi-tools.policy.ts index 048194b71c1..ac65d8bf924 100644 --- a/src/agents/pi-tools.policy.ts +++ b/src/agents/pi-tools.policy.ts @@ -174,7 +174,7 @@ function buildScopedGroupIdCandidates(groupId?: string | null): string[] { return [raw]; } -function resolveGroupContextFromSessionKey(sessionKey?: string | null): { +export function resolveGroupContextFromSessionKey(sessionKey?: string | null): { channel?: string; groupIds?: string[]; } { diff --git a/src/gateway/protocol/schema/agent.ts b/src/gateway/protocol/schema/agent.ts index 173aab37571..fbfe2be37b5 100644 --- a/src/gateway/protocol/schema/agent.ts +++ b/src/gateway/protocol/schema/agent.ts @@ -70,6 +70,10 @@ export const MessageActionParamsSchema = Type.Object( params: Type.Record(Type.String(), Type.Unknown()), accountId: Type.Optional(Type.String()), requesterSenderId: Type.Optional(Type.String()), + // Honored only when the RPC caller has the full operator scope set + // (shared-secret bearer or `operator.admin`). For narrowly-scoped + // callers (e.g. `operator.write`-only) the gateway forces this to + // `false` regardless of the value sent here. senderIsOwner: Type.Optional(Type.Boolean()), sessionKey: Type.Optional(Type.String()), sessionId: Type.Optional(Type.String()), diff --git a/src/gateway/server-methods/send.test.ts b/src/gateway/server-methods/send.test.ts index 3b1b7f141b1..b797cc65bc7 100644 --- a/src/gateway/server-methods/send.test.ts +++ b/src/gateway/server-methods/send.test.ts @@ -156,14 +156,17 @@ async function runPollWithClient( return { respond }; } -async function runMessageActionRequest(params: Record) { +async function runMessageActionRequest( + params: Record, + client?: { connect?: { scopes?: string[] } } | null, +) { const respond = vi.fn(); await sendHandlers["message.action"]({ params: params as never, respond, context: makeContext(), req: { type: "req", id: "1", method: "message.action" }, - client: null as never, + client: (client ?? null) as never, isWebchatConnect: () => false, }); return { respond }; @@ -954,4 +957,95 @@ describe("gateway send mirroring", () => { { channel: "whatsapp" }, ); }); + + it("forces senderIsOwner=false for narrowly-scoped callers but honors it for full operators", async () => { + const capture = { senderIsOwner: undefined as boolean | undefined }; + const reactPlugin: ChannelPlugin = { + id: "whatsapp", + meta: { + id: "whatsapp", + label: "WhatsApp", + selectionLabel: "WhatsApp", + docsPath: "/channels/whatsapp", + blurb: "WhatsApp owner-derivation test plugin.", + }, + capabilities: { chatTypes: ["direct"], reactions: true }, + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({ enabled: true }), + isConfigured: () => true, + }, + actions: { + describeMessageTool: () => ({ actions: ["react"] }), + supportsAction: ({ action }) => action === "react", + handleAction: async ({ senderIsOwner }) => { + capture.senderIsOwner = senderIsOwner; + return jsonResult({ ok: true }); + }, + }, + }; + mocks.getChannelPlugin.mockReturnValue(reactPlugin); + + // Narrowly-scoped caller (e.g. gateway-forwarding least-privilege path + // that only requests operator.write): wire senderIsOwner=true must be + // forced to false so a non-admin scoped caller cannot unlock owner-only + // channel actions. + setActivePluginRegistry( + createTestRegistry([ + { pluginId: "whatsapp", source: "test", plugin: reactPlugin }, + ]), + "send-test-owner-derive-non-admin", + ); + await runMessageActionRequest( + { + channel: "whatsapp", + action: "react", + params: { chatJid: "+15551234567", messageId: "wamid.x", emoji: "✅" }, + senderIsOwner: true, + idempotencyKey: "idem-owner-derive-non-admin", + }, + { connect: { scopes: ["operator.write"] } }, + ); + expect(capture.senderIsOwner).toBe(false); + + // Full operator (admin-scoped): the trusted runtime is allowed to + // forward the real channel-sender ownership bit. Wire true → true. + setActivePluginRegistry( + createTestRegistry([ + { pluginId: "whatsapp", source: "test", plugin: reactPlugin }, + ]), + "send-test-owner-derive-admin-true", + ); + await runMessageActionRequest( + { + channel: "whatsapp", + action: "react", + params: { chatJid: "+15551234567", messageId: "wamid.y", emoji: "✅" }, + senderIsOwner: true, + idempotencyKey: "idem-owner-derive-admin-true", + }, + { connect: { scopes: ["operator.admin"] } }, + ); + expect(capture.senderIsOwner).toBe(true); + + // Full operator forwarding a non-owner sender: wire false → false + // (admin scope does not inflate ownership on its own). + setActivePluginRegistry( + createTestRegistry([ + { pluginId: "whatsapp", source: "test", plugin: reactPlugin }, + ]), + "send-test-owner-derive-admin-false", + ); + await runMessageActionRequest( + { + channel: "whatsapp", + action: "react", + params: { chatJid: "+15551234567", messageId: "wamid.z", emoji: "✅" }, + senderIsOwner: false, + idempotencyKey: "idem-owner-derive-admin-false", + }, + { connect: { scopes: ["operator.admin"] } }, + ); + expect(capture.senderIsOwner).toBe(false); + }); }); diff --git a/src/gateway/server-methods/send.ts b/src/gateway/server-methods/send.ts index 7626ffea36d..426880e5a93 100644 --- a/src/gateway/server-methods/send.ts +++ b/src/gateway/server-methods/send.ts @@ -34,6 +34,7 @@ import { validatePollParams, validateSendParams, } from "../protocol/index.js"; +import { ADMIN_SCOPE } from "../method-scopes.js"; import { formatForLog } from "../ws-log.js"; import type { GatewayRequestContext, GatewayRequestHandlers } from "./types.js"; @@ -185,7 +186,7 @@ function cacheGatewayDedupeFailure(params: { } export const sendHandlers: GatewayRequestHandlers = { - "message.action": async ({ params, respond, context }) => { + "message.action": async ({ params, respond, context, client }) => { const p = params; if (!validateMessageActionParams(p)) { respond( @@ -216,6 +217,23 @@ export const sendHandlers: GatewayRequestHandlers = { }; idempotencyKey: string; }; + // Owner status is an authorization signal used to unlock owner-only + // channel actions and owner-only tool policy. The legitimate propagation + // path is the trusted runtime forwarding a real channel-sender ownership + // bit through the gateway RPC — but that wire value must not be honored + // for callers who are not already full operators. Per SECURITY.md, + // shared-secret bearer and admin-scoped callers get the full default + // operator scope set (including `operator.admin`); those callers are + // trusted to forward `senderIsOwner`. Narrowly-scoped callers + // (e.g. `operator.write`-only, including the gateway-forwarding + // least-privilege path) are not trusted to assert ownership, so their + // wire value is forced to `false` to prevent a non-admin scoped caller + // from unlocking owner-only channel actions by setting + // `senderIsOwner: true` on the request. + const callerScopes = client?.connect?.scopes ?? []; + const callerIsFullOperator = + Array.isArray(callerScopes) && callerScopes.includes(ADMIN_SCOPE); + const senderIsOwner = callerIsFullOperator && request.senderIsOwner === true; const idem = request.idempotencyKey; const dedupeKey = `message.action:${idem}`; const cached = context.dedupe.get(dedupeKey); @@ -265,7 +283,7 @@ export const sendHandlers: GatewayRequestHandlers = { params: request.params, accountId: normalizeOptionalString(request.accountId) ?? undefined, requesterSenderId: normalizeOptionalString(request.requesterSenderId) ?? undefined, - senderIsOwner: request.senderIsOwner, + senderIsOwner, sessionKey: normalizeOptionalString(request.sessionKey) ?? undefined, sessionId: normalizeOptionalString(request.sessionId) ?? undefined, agentId: normalizeOptionalString(request.agentId) ?? undefined,