mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-31 04:34:55 +00:00
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
This commit is contained in:
committed by
GitHub
parent
629fc2f8f0
commit
91a4635bdc
@@ -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.
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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.",
|
||||
};
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
},
|
||||
};
|
||||
|
||||
@@ -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 };
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
|
||||
@@ -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 }) => {
|
||||
|
||||
@@ -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) &&
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -1761,6 +1761,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) {
|
||||
pluginName: record.name,
|
||||
pluginRoot: record.rootDir,
|
||||
allowReservedCommandNames,
|
||||
allowOwnerStatusExposure: canClaimReservedCommandOwnership(record),
|
||||
},
|
||||
);
|
||||
if (!result.ok) {
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user