From 09faed6bd87b6f2f13ffd59290e6d02f2dac32f2 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Mar 2026 22:44:31 -0700 Subject: [PATCH] fix(gateway): gate internal command persistence mutations --- CHANGELOG.md | 1 + extensions/phone-control/index.test.ts | 88 +++++++++++++++++++ extensions/phone-control/index.ts | 10 +++ .../reply/directive-handling.fast-lane.ts | 2 + .../reply/directive-handling.impl.ts | 13 ++- .../reply/directive-handling.model.test.ts | 87 ++++++++++++++++++ .../reply/directive-handling.params.ts | 1 + .../reply/directive-handling.persist.ts | 13 ++- .../reply/directive-handling.shared.ts | 15 ++++ .../reply/get-reply-directives-apply.ts | 3 + 10 files changed, 229 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b324a5fbf3..895192a2535 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -281,6 +281,7 @@ Docs: https://docs.openclaw.ai - Gateway/bonjour: suppress the non-fatal `@homebridge/ciao` IPv4-loss assertion during interface churn so WiFi/VPN/sleep-wake changes no longer take down the gateway. (#38628, #47159, #52431) - Browser/launch: stop forcing an extra blank tab on browser launch so managed browser startup no longer opens an unwanted empty page. (#52451) Thanks @rogerdigital. - ACP/Codex session replay: preserve hidden assistant thinking when loading or rebinding existing ACP sessions so stored thought chunks do not replay into visible assistant text. Thanks @vincentkoc. +- Gateway/commands: keep internal `chat.send` slash-command UX while requiring `operator.admin` before internal callers can persist `/exec` defaults or mutate `phone-control` node policy through `/phone arm|disarm`. ### Breaking diff --git a/extensions/phone-control/index.test.ts b/extensions/phone-control/index.test.ts index d5af0de44de..f19d7ac188d 100644 --- a/extensions/phone-control/index.test.ts +++ b/extensions/phone-control/index.test.ts @@ -106,4 +106,92 @@ describe("phone-control plugin", () => { await fs.rm(stateDir, { recursive: true, force: true }); } }); + + it("blocks internal operator.write callers from mutating phone control", async () => { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-phone-control-test-")); + try { + let config: Record = { + gateway: { + nodes: { + allowCommands: [], + denyCommands: ["calendar.add", "contacts.add", "reminders.add", "sms.send"], + }, + }, + }; + const writeConfigFile = vi.fn(async (next: Record) => { + config = next; + }); + + let command: OpenClawPluginCommandDefinition | undefined; + registerPhoneControl.register( + createApi({ + stateDir, + getConfig: () => config, + writeConfig: writeConfigFile, + registerCommand: (nextCommand) => { + command = nextCommand; + }, + }), + ); + + if (!command) { + throw new Error("phone-control plugin did not register its command"); + } + + const res = await command.handler({ + ...createCommandContext("arm writes 30s"), + channel: "webchat", + gatewayClientScopes: ["operator.write"], + }); + + expect(String(res?.text ?? "")).toContain("requires operator.admin"); + expect(writeConfigFile).not.toHaveBeenCalled(); + } finally { + await fs.rm(stateDir, { recursive: true, force: true }); + } + }); + + it("allows internal operator.admin callers to mutate phone control", async () => { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-phone-control-test-")); + try { + let config: Record = { + gateway: { + nodes: { + allowCommands: [], + denyCommands: ["calendar.add", "contacts.add", "reminders.add", "sms.send"], + }, + }, + }; + const writeConfigFile = vi.fn(async (next: Record) => { + config = next; + }); + + let command: OpenClawPluginCommandDefinition | undefined; + registerPhoneControl.register( + createApi({ + stateDir, + getConfig: () => config, + writeConfig: writeConfigFile, + registerCommand: (nextCommand) => { + command = nextCommand; + }, + }), + ); + + if (!command) { + throw new Error("phone-control plugin did not register its command"); + } + + const res = await command.handler({ + ...createCommandContext("arm writes 30s"), + channel: "webchat", + gatewayClientScopes: ["operator.admin"], + }); + + expect(String(res?.text ?? "")).toContain("sms.send"); + expect(writeConfigFile).toHaveBeenCalledTimes(1); + } finally { + await fs.rm(stateDir, { recursive: true, force: true }); + } + }); }); diff --git a/extensions/phone-control/index.ts b/extensions/phone-control/index.ts index 1743e3faae5..1ebc407bf6e 100644 --- a/extensions/phone-control/index.ts +++ b/extensions/phone-control/index.ts @@ -358,6 +358,11 @@ export default definePluginEntry({ } if (action === "disarm") { + if (ctx.channel === "webchat" && !ctx.gatewayClientScopes?.includes("operator.admin")) { + return { + text: "⚠️ /phone disarm requires operator.admin for internal gateway callers.", + }; + } const res = await disarmNow({ api, stateDir, @@ -375,6 +380,11 @@ export default definePluginEntry({ } if (action === "arm") { + if (ctx.channel === "webchat" && !ctx.gatewayClientScopes?.includes("operator.admin")) { + return { + text: "⚠️ /phone arm requires operator.admin for internal gateway callers.", + }; + } const group = parseGroup(tokens[1]); if (!group) { return { text: `Usage: /phone arm [duration]\nGroups: ${formatGroupList()}` }; diff --git a/src/auto-reply/reply/directive-handling.fast-lane.ts b/src/auto-reply/reply/directive-handling.fast-lane.ts index 4635c4073f8..0e0425672b6 100644 --- a/src/auto-reply/reply/directive-handling.fast-lane.ts +++ b/src/auto-reply/reply/directive-handling.fast-lane.ts @@ -86,6 +86,8 @@ export async function applyInlineDirectivesFastLane( currentVerboseLevel, currentReasoningLevel, currentElevatedLevel, + surface: ctx.Surface, + gatewayClientScopes: ctx.GatewayClientScopes, }); if (sessionEntry?.providerOverride) { diff --git a/src/auto-reply/reply/directive-handling.impl.ts b/src/auto-reply/reply/directive-handling.impl.ts index 5fd0682ac93..80e92b09588 100644 --- a/src/auto-reply/reply/directive-handling.impl.ts +++ b/src/auto-reply/reply/directive-handling.impl.ts @@ -18,9 +18,11 @@ import { maybeHandleModelDirectiveInfo } from "./directive-handling.model.js"; import type { HandleDirectiveOnlyParams } from "./directive-handling.params.js"; import { maybeHandleQueueDirective } from "./directive-handling.queue-validation.js"; import { + canPersistInternalExecDirective, formatDirectiveAck, formatElevatedRuntimeHint, formatElevatedUnavailableText, + formatInternalExecPersistenceDeniedText, enqueueModeSwitchEvents, withOptions, } from "./directive-handling.shared.js"; @@ -92,6 +94,10 @@ export async function handleDirectiveOnly( sessionKey: params.sessionKey, }).sandboxed; const shouldHintDirectRuntime = directives.hasElevatedDirective && !runtimeIsSandboxed; + const allowInternalExecPersistence = canPersistInternalExecDirective({ + surface: params.surface, + gatewayClientScopes: params.gatewayClientScopes, + }); const modelInfo = await maybeHandleModelDirectiveInfo({ directives, @@ -344,7 +350,7 @@ export async function handleDirectiveOnly( elevatedChanged || (directives.elevatedLevel !== prevElevatedLevel && directives.elevatedLevel !== undefined); } - if (directives.hasExecDirective && directives.hasExecOptions) { + if (directives.hasExecDirective && directives.hasExecOptions && allowInternalExecPersistence) { if (directives.execHost) { sessionEntry.execHost = directives.execHost; } @@ -453,7 +459,7 @@ export async function handleDirectiveOnly( parts.push(formatElevatedRuntimeHint()); } } - if (directives.hasExecDirective && directives.hasExecOptions) { + if (directives.hasExecDirective && directives.hasExecOptions && allowInternalExecPersistence) { const execParts: string[] = []; if (directives.execHost) { execParts.push(`host=${directives.execHost}`); @@ -471,6 +477,9 @@ export async function handleDirectiveOnly( parts.push(formatDirectiveAck(`Exec defaults set (${execParts.join(", ")}).`)); } } + if (directives.hasExecDirective && directives.hasExecOptions && !allowInternalExecPersistence) { + parts.push(formatDirectiveAck(formatInternalExecPersistenceDeniedText())); + } if (shouldDowngradeXHigh) { parts.push( `Thinking level set to high (xhigh not supported for ${resolvedProvider}/${resolvedModel}).`, diff --git a/src/auto-reply/reply/directive-handling.model.test.ts b/src/auto-reply/reply/directive-handling.model.test.ts index f80ebecfc91..192f5566031 100644 --- a/src/auto-reply/reply/directive-handling.model.test.ts +++ b/src/auto-reply/reply/directive-handling.model.test.ts @@ -645,4 +645,91 @@ describe("handleDirectiveOnly model persist behavior (fixes #1435)", () => { expect(sessionEntry.thinkingLevel).toBe("off"); expect(sessionStore["agent:main:dm:1"]?.thinkingLevel).toBe("off"); }); + + it("blocks internal operator.write exec persistence in directive-only handling", async () => { + const directives = parseInlineDirectives( + "/exec host=node security=allowlist ask=always node=worker-1", + ); + const sessionEntry = createSessionEntry(); + const sessionStore = { [sessionKey]: sessionEntry }; + const result = await handleDirectiveOnly( + createHandleParams({ + directives, + sessionEntry, + sessionStore, + surface: "webchat", + gatewayClientScopes: ["operator.write"], + }), + ); + + expect(result?.text).toContain("operator.admin"); + expect(sessionEntry.execHost).toBeUndefined(); + expect(sessionEntry.execSecurity).toBeUndefined(); + expect(sessionEntry.execAsk).toBeUndefined(); + expect(sessionEntry.execNode).toBeUndefined(); + }); + + it("allows internal operator.admin exec persistence in directive-only handling", async () => { + const directives = parseInlineDirectives( + "/exec host=node security=allowlist ask=always node=worker-1", + ); + const sessionEntry = createSessionEntry(); + const sessionStore = { [sessionKey]: sessionEntry }; + const result = await handleDirectiveOnly( + createHandleParams({ + directives, + sessionEntry, + sessionStore, + surface: "webchat", + gatewayClientScopes: ["operator.admin"], + }), + ); + + expect(result?.text).toContain("Exec defaults set"); + expect(sessionEntry.execHost).toBe("node"); + expect(sessionEntry.execSecurity).toBe("allowlist"); + expect(sessionEntry.execAsk).toBe("always"); + expect(sessionEntry.execNode).toBe("worker-1"); + }); +}); + +describe("persistInlineDirectives internal exec scope gate", () => { + it("skips exec persistence for internal operator.write callers", async () => { + const allowedModelKeys = new Set(["anthropic/claude-opus-4-5", "openai/gpt-4o"]); + const directives = parseInlineDirectives( + "/exec host=node security=allowlist ask=always node=worker-1", + ); + const sessionEntry = { + sessionId: "s1", + updatedAt: Date.now(), + } as SessionEntry; + const sessionStore = { "agent:main:main": sessionEntry }; + + await persistInlineDirectives({ + directives, + cfg: baseConfig(), + sessionEntry, + sessionStore, + sessionKey: "agent:main:main", + storePath: "/tmp/sessions.json", + elevatedEnabled: true, + elevatedAllowed: true, + defaultProvider: "anthropic", + defaultModel: "claude-opus-4-5", + aliasIndex: baseAliasIndex(), + allowedModelKeys, + provider: "anthropic", + model: "claude-opus-4-5", + initialModelLabel: "anthropic/claude-opus-4-5", + formatModelSwitchEvent: (label) => `Switched to ${label}`, + agentCfg: undefined, + surface: "webchat", + gatewayClientScopes: ["operator.write"], + }); + + expect(sessionEntry.execHost).toBeUndefined(); + expect(sessionEntry.execSecurity).toBeUndefined(); + expect(sessionEntry.execAsk).toBeUndefined(); + expect(sessionEntry.execNode).toBeUndefined(); + }); }); diff --git a/src/auto-reply/reply/directive-handling.params.ts b/src/auto-reply/reply/directive-handling.params.ts index fd64e379d0c..0b14b536581 100644 --- a/src/auto-reply/reply/directive-handling.params.ts +++ b/src/auto-reply/reply/directive-handling.params.ts @@ -37,6 +37,7 @@ export type HandleDirectiveOnlyParams = HandleDirectiveOnlyCoreParams & { currentReasoningLevel?: ReasoningLevel; currentElevatedLevel?: ElevatedLevel; surface?: string; + gatewayClientScopes?: string[]; }; export type ApplyInlineDirectivesFastLaneParams = HandleDirectiveOnlyCoreParams & { diff --git a/src/auto-reply/reply/directive-handling.persist.ts b/src/auto-reply/reply/directive-handling.persist.ts index 3c6815a0f62..8ed7c7ce7ca 100644 --- a/src/auto-reply/reply/directive-handling.persist.ts +++ b/src/auto-reply/reply/directive-handling.persist.ts @@ -14,7 +14,10 @@ import { applyVerboseOverride } from "../../sessions/level-overrides.js"; import { applyModelOverrideToSessionEntry } from "../../sessions/model-overrides.js"; import { resolveModelSelectionFromDirective } from "./directive-handling.model-selection.js"; import type { InlineDirectives } from "./directive-handling.parse.js"; -import { enqueueModeSwitchEvents } from "./directive-handling.shared.js"; +import { + canPersistInternalExecDirective, + enqueueModeSwitchEvents, +} from "./directive-handling.shared.js"; import type { ElevatedLevel, ReasoningLevel } from "./directives.js"; export async function persistInlineDirectives(params: { @@ -37,6 +40,8 @@ export async function persistInlineDirectives(params: { initialModelLabel: string; formatModelSwitchEvent: (label: string, alias?: string) => string; agentCfg: NonNullable["defaults"] | undefined; + surface?: string; + gatewayClientScopes?: string[]; }): Promise<{ provider: string; model: string; contextTokens: number }> { const { directives, @@ -56,6 +61,10 @@ export async function persistInlineDirectives(params: { agentCfg, } = params; let { provider, model } = params; + const allowInternalExecPersistence = canPersistInternalExecDirective({ + surface: params.surface, + gatewayClientScopes: params.gatewayClientScopes, + }); const activeAgentId = sessionKey ? resolveSessionAgentId({ sessionKey, config: cfg }) : resolveDefaultAgentId(cfg); @@ -110,7 +119,7 @@ export async function persistInlineDirectives(params: { (directives.elevatedLevel !== prevElevatedLevel && directives.elevatedLevel !== undefined); updated = true; } - if (directives.hasExecDirective && directives.hasExecOptions) { + if (directives.hasExecDirective && directives.hasExecOptions && allowInternalExecPersistence) { if (directives.execHost) { sessionEntry.execHost = directives.execHost; updated = true; diff --git a/src/auto-reply/reply/directive-handling.shared.ts b/src/auto-reply/reply/directive-handling.shared.ts index 3b42f5dab6d..12c0cc306d6 100644 --- a/src/auto-reply/reply/directive-handling.shared.ts +++ b/src/auto-reply/reply/directive-handling.shared.ts @@ -1,5 +1,6 @@ import { formatCliCommand } from "../../cli/command-format.js"; import { SYSTEM_MARK, prefixSystemMessage } from "../../infra/system-message.js"; +import { isInternalMessageChannel } from "../../utils/message-channel.js"; import type { ElevatedLevel, ReasoningLevel } from "./directives.js"; export const formatDirectiveAck = (text: string): string => { @@ -13,6 +14,20 @@ export const withOptions = (line: string, options: string) => export const formatElevatedRuntimeHint = () => `${SYSTEM_MARK} Runtime is direct; sandboxing does not apply.`; +export const formatInternalExecPersistenceDeniedText = () => + "Exec defaults require operator.admin for internal gateway callers; skipped persistence."; + +export function canPersistInternalExecDirective(params: { + surface?: string; + gatewayClientScopes?: string[]; +}): boolean { + if (!isInternalMessageChannel(params.surface)) { + return true; + } + const scopes = params.gatewayClientScopes ?? []; + return scopes.includes("operator.admin"); +} + export const formatElevatedEvent = (level: ElevatedLevel) => { if (level === "full") { return "Elevated FULL — exec runs on host with auto-approval."; diff --git a/src/auto-reply/reply/get-reply-directives-apply.ts b/src/auto-reply/reply/get-reply-directives-apply.ts index c6c1ab7650a..7b891b417f4 100644 --- a/src/auto-reply/reply/get-reply-directives-apply.ts +++ b/src/auto-reply/reply/get-reply-directives-apply.ts @@ -213,6 +213,7 @@ export async function applyInlineDirectiveOverrides(params: { currentReasoningLevel, currentElevatedLevel, surface: ctx.Surface, + gatewayClientScopes: ctx.GatewayClientScopes, }); let statusReply: ReplyPayload | undefined; if (directives.hasStatusDirective && allowTextCommands && command.isAuthorizedSender) { @@ -317,6 +318,8 @@ export async function applyInlineDirectiveOverrides(params: { initialModelLabel, formatModelSwitchEvent, agentCfg, + surface: ctx.Surface, + gatewayClientScopes: ctx.GatewayClientScopes, }); provider = persisted.provider; model = persisted.model;