From 91a4635bdca98d9b0664fd609af0fb8411fb5f40 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Thu, 28 May 2026 16:00:01 +0530 Subject: [PATCH] Tighten phone-control mutation authorization [AI] (#87150) * fix: require admin authorization for phone control mutations * addressing codex review * addressing codex review * addressing ci * addressing ci * test: restore provider registry mock isolation * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + extensions/phone-control/index.test.ts | 51 ++++++++- extensions/phone-control/index.ts | 26 +++-- src/gateway/server-methods/config.ts | 9 +- src/plugins/command-registration.ts | 16 ++- src/plugins/command-registry-state.ts | 8 ++ src/plugins/commands.test.ts | 103 ++++++++++++++++++ src/plugins/commands.ts | 3 +- .../contracts/host-hooks.contract.test.ts | 8 ++ src/plugins/registry.ts | 1 + src/plugins/types.ts | 2 + 11 files changed, 209 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90f4bd735db..45309899b9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Tighten phone-control mutation authorization [AI]. (#87150) Thanks @pgondhi987. - Clarify directive persistence authorization policy [AI]. (#86369) Thanks @pgondhi987. - Agents/Codex: keep spawned agent cwd/workspace state separated, keep hook context prompt-local, release session locks on timeout abort, avoid session event queue self-wait, preserve shared app-server state across startup or helper failures, keep native hook relay alive across restarts, route workspace memory through tools, resolve Codex runtime models first, report quarantined dynamic tools, format `skills` command output, and bound compaction/steering retries. (#87218, #86875, #86123, #87399, #87375, #87383, #87400) Thanks @mbelinky, @Alix-007, @luoyanglang, @yetval, and @sjf. - Channels: thread canonical session keys into outbound hooks, preserve Matrix room-id case, keep fallback tool warnings mention-inert, retain delivered Slack final replies during late cleanup, continue iMessage polling after denied reactions, suppress duplicate native exec approvals, preserve Telegram SecretRef prompt config, suppress Discord recovered tool warnings, and block untrusted Teams service URLs. (#73706, #75670, #87366, #87451, #87334) Thanks @zeroaltitude, @lukeboyett, @xiaotian, and @eleqtrizit. diff --git a/extensions/phone-control/index.test.ts b/extensions/phone-control/index.test.ts index 334faf44317..a53b484f400 100644 --- a/extensions/phone-control/index.test.ts +++ b/extensions/phone-control/index.test.ts @@ -129,6 +129,8 @@ describe("phone-control plugin", () => { it("arms sms.send as part of the writes group", async () => { await withRegisteredPhoneControl(async ({ command, writeConfigFile, getConfig }) => { expect(command.name).toBe("phone"); + expect(command.requiredScopes).toBeUndefined(); + expect(command.exposeSenderIsOwner).toBe(true); const res = await command.handler({ ...createCommandContext("arm writes 30s"), @@ -163,23 +165,38 @@ describe("phone-control plugin", () => { }); }); - it("allows external channel callers without operator.admin to mutate phone control", async () => { + it("blocks external non-owner callers without operator.admin from mutating phone control", async () => { await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { const res = await command.handler({ ...createCommandContext("arm writes 30s"), channel: "telegram", + senderIsOwner: false, }); - expect(res?.text ?? "").toContain("Phone control: armed"); - expect(writeConfigFile).toHaveBeenCalledTimes(1); + expect(res?.text ?? "").toContain("requires operator.admin"); + expect(writeConfigFile).not.toHaveBeenCalled(); }); }); - it("allows external channel callers without operator.admin to disarm phone control", async () => { + it("blocks external non-owner callers without operator.admin from disarming phone control", async () => { await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { const res = await command.handler({ ...createCommandContext("disarm"), channel: "telegram", + senderIsOwner: false, + }); + + expect(res?.text ?? "").toContain("requires operator.admin"); + expect(writeConfigFile).not.toHaveBeenCalled(); + }); + }); + + it("allows external non-owner callers without operator.admin to read phone control status", async () => { + await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { + const res = await command.handler({ + ...createCommandContext("status"), + channel: "telegram", + senderIsOwner: false, }); expect(res?.text ?? "").toContain("Phone control: disarmed."); @@ -187,6 +204,19 @@ describe("phone-control plugin", () => { }); }); + it("allows external non-owner callers without operator.admin to read phone control help", async () => { + await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { + const res = await command.handler({ + ...createCommandContext("help"), + channel: "telegram", + senderIsOwner: false, + }); + + expect(res?.text ?? "").toContain("/phone status"); + expect(writeConfigFile).not.toHaveBeenCalled(); + }); + }); + it("regression: blocks non-webchat gateway callers with operator.write from arm/disarm", async () => { await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { const armRes = await command.handler({ @@ -220,6 +250,19 @@ describe("phone-control plugin", () => { }); }); + it("allows external owner callers without gateway scopes to mutate phone control", async () => { + await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { + const res = await command.handler({ + ...createCommandContext("arm writes 30s"), + channel: "telegram", + senderIsOwner: true, + }); + + expect(res?.text ?? "").toContain("Phone control: armed"); + expect(writeConfigFile).toHaveBeenCalledTimes(1); + }); + }); + it("allows external channel callers with operator.admin to disarm phone control", async () => { await withRegisteredPhoneControl(async ({ command, writeConfigFile }) => { await command.handler({ diff --git a/extensions/phone-control/index.ts b/extensions/phone-control/index.ts index f6446f2ac54..9703c09134f 100644 --- a/extensions/phone-control/index.ts +++ b/extensions/phone-control/index.ts @@ -282,14 +282,15 @@ function parseGroup(raw: string | undefined): ArmGroup | null { return null; } -function requiresAdminToMutatePhoneControl( - channel: string, - gatewayClientScopes?: readonly string[], -): boolean { +function lacksAdminToMutatePhoneControl(params: { + senderIsOwner?: boolean; + gatewayClientScopes?: readonly string[]; +}): boolean { + const { senderIsOwner, gatewayClientScopes } = params; if (Array.isArray(gatewayClientScopes)) { return !gatewayClientScopes.includes(PHONE_ADMIN_SCOPE); } - return channel === "webchat"; + return senderIsOwner !== true; } function formatStatus(state: ArmStateFile | null): string { @@ -363,6 +364,7 @@ export default definePluginEntry({ name: "phone", description: "Arm/disarm high-risk phone node commands (camera/screen/writes).", acceptsArgs: true, + exposeSenderIsOwner: true, handler: async (ctx) => { const args = ctx.args?.trim() ?? ""; const tokens = args.split(/\s+/).filter(Boolean); @@ -382,7 +384,12 @@ export default definePluginEntry({ } if (action === "disarm") { - if (requiresAdminToMutatePhoneControl(ctx.channel, ctx.gatewayClientScopes)) { + if ( + lacksAdminToMutatePhoneControl({ + senderIsOwner: ctx.senderIsOwner, + gatewayClientScopes: ctx.gatewayClientScopes, + }) + ) { return { text: "⚠️ /phone disarm requires operator.admin.", }; @@ -404,7 +411,12 @@ export default definePluginEntry({ } if (action === "arm") { - if (requiresAdminToMutatePhoneControl(ctx.channel, ctx.gatewayClientScopes)) { + if ( + lacksAdminToMutatePhoneControl({ + senderIsOwner: ctx.senderIsOwner, + gatewayClientScopes: ctx.gatewayClientScopes, + }) + ) { return { text: "⚠️ /phone arm requires operator.admin.", }; diff --git a/src/gateway/server-methods/config.ts b/src/gateway/server-methods/config.ts index 20cec1f1b37..194f58f1f31 100644 --- a/src/gateway/server-methods/config.ts +++ b/src/gateway/server-methods/config.ts @@ -703,18 +703,15 @@ export const configHandlers: GatewayRequestHandlers = { respond(true, { ok: true, path: configPath }, undefined); } catch (error) { const errorMessage = formatConfigOpenError(error); - const isHeadlessError = errorMessage.includes("xdg-open") && errorMessage.includes("no method available"); + const isHeadlessError = + errorMessage.includes("xdg-open") && errorMessage.includes("no method available"); const detailedError = isHeadlessError ? `Cannot open file in headless environment. File path: ${configPath}. This environment appears to lack a graphical or terminal browser handler.` : `Failed to open config file: ${errorMessage}`; context?.logGateway?.warn( `config.openFile failed path=${sanitizeLookupPathForLog(configPath)}: ${errorMessage}`, ); - respond( - true, - { ok: false, path: configPath, error: detailedError }, - undefined, - ); + respond(true, { ok: false, path: configPath, error: detailedError }, undefined); } }, }; diff --git a/src/plugins/command-registration.ts b/src/plugins/command-registration.ts index b75116498fb..cc76916ca52 100644 --- a/src/plugins/command-registration.ts +++ b/src/plugins/command-registration.ts @@ -160,6 +160,12 @@ export function validatePluginCommandDefinition( : "Command requiredScopes contains unknown operator scope"; } } + if ( + command.exposeSenderIsOwner !== undefined && + typeof command.exposeSenderIsOwner !== "boolean" + ) { + return "Command exposeSenderIsOwner must be a boolean"; + } if (command.channels !== undefined) { if (!Array.isArray(command.channels)) { return "Command channels must be an array of channel ids"; @@ -308,7 +314,12 @@ export function pluginCommandSupportsChannel( export function registerPluginCommand( pluginId: string, command: OpenClawPluginCommandDefinition, - opts?: { pluginName?: string; pluginRoot?: string; allowReservedCommandNames?: boolean }, + opts?: { + pluginName?: string; + pluginRoot?: string; + allowReservedCommandNames?: boolean; + allowOwnerStatusExposure?: boolean; + }, ): CommandRegistrationResult { // Prevent registration while commands are being processed if (isPluginCommandRegistryLocked()) { @@ -363,6 +374,9 @@ export function registerPluginCommand( pluginId, pluginName: opts?.pluginName, pluginRoot: opts?.pluginRoot, + ...(opts?.allowOwnerStatusExposure === true && normalizedCommand.exposeSenderIsOwner === true + ? { trustedOwnerStatusExposure: true as const } + : {}), }); logVerbose(`Registered plugin command: ${key} (plugin: ${pluginId})`); return { ok: true }; diff --git a/src/plugins/command-registry-state.ts b/src/plugins/command-registry-state.ts index b8c9cd7d8bd..cc78273696b 100644 --- a/src/plugins/command-registry-state.ts +++ b/src/plugins/command-registry-state.ts @@ -11,6 +11,7 @@ export type RegisteredPluginCommand = OpenClawPluginCommandDefinition & { pluginId: string; pluginName?: string; pluginRoot?: string; + trustedOwnerStatusExposure?: true; }; type PluginCommandState = { @@ -59,6 +60,13 @@ export function isTrustedReservedCommandOwner(command: RegisteredPluginCommand): return command.ownership === "reserved"; } +export function canExposeSenderIsOwner(command: RegisteredPluginCommand): boolean { + return ( + (Array.isArray(command.requiredScopes) && command.requiredScopes.length > 0) || + command.trustedOwnerStatusExposure === true + ); +} + export function listRegisteredPluginCommands(): RegisteredPluginCommand[] { return Array.from(pluginCommands.values()); } diff --git a/src/plugins/commands.test.ts b/src/plugins/commands.test.ts index e8fb4ab5a83..b95879fdbc3 100644 --- a/src/plugins/commands.test.ts +++ b/src/plugins/commands.test.ts @@ -737,6 +737,109 @@ describe("registerPluginCommand", () => { expect(observedOwnerStatus).toBeUndefined(); }); + it("ignores owner status opt-in from direct plugin command registration", async () => { + let observedOwnerStatus: boolean | undefined; + registerPluginCommand("demo-plugin", { + name: "voice", + description: "Voice command", + exposeSenderIsOwner: true, + handler: async (ctx) => { + observedOwnerStatus = ctx.senderIsOwner; + return { text: "ok" }; + }, + }); + const match = requirePluginCommandMatch("/voice"); + + await executePluginCommand({ + command: match.command, + channel: "telegram", + isAuthorizedSender: true, + senderIsOwner: true, + commandBody: "/voice", + config: {}, + }); + + expect(observedOwnerStatus).toBeUndefined(); + }); + + it("ignores owner status opt-in from external plugin registry commands", async () => { + const pluginRegistry = createPluginRegistry({ + logger: { + info() {}, + warn() {}, + error() {}, + debug() {}, + }, + runtime: {} as PluginRuntime, + activateGlobalSideEffects: true, + }); + let observedOwnerStatus: boolean | undefined; + pluginRegistry.registerCommand( + { + ...createBundledPluginRecord("external-plugin"), + origin: "workspace", + source: "/workspace/external-plugin/index.ts", + rootDir: "/workspace/external-plugin", + }, + { + name: "external", + description: "External command", + exposeSenderIsOwner: true, + handler: async (ctx) => { + observedOwnerStatus = ctx.senderIsOwner; + return { text: "ok" }; + }, + }, + ); + const match = requirePluginCommandMatch("/external"); + + await executePluginCommand({ + command: match.command, + channel: "telegram", + isAuthorizedSender: true, + senderIsOwner: true, + commandBody: "/external", + config: {}, + }); + + expect(observedOwnerStatus).toBeUndefined(); + }); + + it("exposes owner status to trusted bundled plugin commands that opt in", async () => { + const pluginRegistry = createPluginRegistry({ + logger: { + info() {}, + warn() {}, + error() {}, + debug() {}, + }, + runtime: {} as PluginRuntime, + activateGlobalSideEffects: true, + }); + let observedOwnerStatus: boolean | undefined; + pluginRegistry.registerCommand(createBundledPluginRecord("phone-control"), { + name: "phone", + description: "Phone command", + exposeSenderIsOwner: true, + handler: async (ctx) => { + observedOwnerStatus = ctx.senderIsOwner; + return { text: "ok" }; + }, + }); + const match = requirePluginCommandMatch("/phone"); + + await executePluginCommand({ + command: match.command, + channel: "telegram", + isAuthorizedSender: true, + senderIsOwner: true, + commandBody: "/phone", + config: {}, + }); + + expect(observedOwnerStatus).toBe(true); + }); + it("allows command owners to run scoped plugin commands without gateway scopes", async () => { let observedOwnerStatus: boolean | undefined; const handler = vi.fn(async (ctx: { senderIsOwner?: boolean }) => { diff --git a/src/plugins/commands.ts b/src/plugins/commands.ts index 19badb6bdb8..7fb21bb6df6 100644 --- a/src/plugins/commands.ts +++ b/src/plugins/commands.ts @@ -22,6 +22,7 @@ import { validatePluginCommandDefinition, } from "./command-registration.js"; import { + canExposeSenderIsOwner, isTrustedReservedCommandOwner, listRegisteredPluginAgentPromptGuidance, pluginCommands, @@ -307,7 +308,7 @@ export async function executePluginCommand(params: { }); const effectiveAccountId = bindingConversation?.accountId ?? params.accountId; const senderIsOwnerForCommand = - requiredScopes.length > 0 || + canExposeSenderIsOwner(command) || (isTrustedReservedCommandOwner(command) && command.ownership === "reserved" && isReservedCommandName(command.name) && diff --git a/src/plugins/contracts/host-hooks.contract.test.ts b/src/plugins/contracts/host-hooks.contract.test.ts index fea874aeb81..71529658a69 100644 --- a/src/plugins/contracts/host-hooks.contract.test.ts +++ b/src/plugins/contracts/host-hooks.contract.test.ts @@ -1586,6 +1586,14 @@ describe("host-hook fixture plugin contract", () => { handler: () => ({ text: "unused" }), }), ).toBe("Command requiredScopes contains unknown operator scope: operator.unknown"); + expect( + validatePluginCommandDefinition({ + name: "invalid-owner-status-fixture", + description: "Invalid owner status exposure.", + exposeSenderIsOwner: "yes" as never, + handler: () => ({ text: "unused" }), + }), + ).toBe("Command exposeSenderIsOwner must be a boolean"); await expect( executePluginCommand({ diff --git a/src/plugins/registry.ts b/src/plugins/registry.ts index e1568fec91b..180c1d229df 100644 --- a/src/plugins/registry.ts +++ b/src/plugins/registry.ts @@ -1761,6 +1761,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { pluginName: record.name, pluginRoot: record.rootDir, allowReservedCommandNames, + allowOwnerStatusExposure: canClaimReservedCommandOwnership(record), }, ); if (!result.ok) { diff --git a/src/plugins/types.ts b/src/plugins/types.ts index c56fdc6fca1..8323dc7c41f 100644 --- a/src/plugins/types.ts +++ b/src/plugins/types.ts @@ -2054,6 +2054,8 @@ export type OpenClawPluginCommandDefinition = { requireAuth?: boolean; /** Operator scopes required by gateway clients; command owners may satisfy this on chat surfaces. */ requiredScopes?: OperatorScope[]; + /** Whether a trusted bundled handler needs owner status for subcommand-level authorization. */ + exposeSenderIsOwner?: boolean; /** * Allows a bundled plugin to claim a command name that is otherwise reserved * by core. External plugins cannot use this field.