From 69ba2765de0a9aabb2351f83d0ef3d50f918a843 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 17 Jan 2026 09:01:43 +0000 Subject: [PATCH] refactor(security): harden CommandAuthorized plumbing --- extensions/zalo/src/monitor.ts | 9 ++++---- extensions/zalouser/src/monitor.ts | 9 ++++---- src/auto-reply/command-detection.ts | 8 +++++++ src/auto-reply/reply/abort.test.ts | 13 ++++++----- src/auto-reply/reply/abort.ts | 6 ++--- .../reply/dispatch-from-config.test.ts | 22 ++++++++++--------- src/auto-reply/reply/dispatch-from-config.ts | 4 ++-- src/auto-reply/reply/get-reply.ts | 14 ++++++------ src/auto-reply/reply/inbound-context.test.ts | 1 + src/auto-reply/reply/inbound-context.ts | 9 +++++--- src/auto-reply/reply/provider-dispatcher.ts | 6 ++--- src/auto-reply/reply/test-ctx.ts | 18 +++++++++++++++ src/auto-reply/templating.ts | 8 +++++++ src/slack/monitor/message-handler/prepare.ts | 9 ++++---- src/slack/monitor/message-handler/types.ts | 3 ++- src/web/auto-reply/monitor/process-message.ts | 9 ++------ 16 files changed, 92 insertions(+), 56 deletions(-) create mode 100644 src/auto-reply/reply/test-ctx.ts diff --git a/extensions/zalo/src/monitor.ts b/extensions/zalo/src/monitor.ts index 82df82cb1..3f9a064a9 100644 --- a/extensions/zalo/src/monitor.ts +++ b/extensions/zalo/src/monitor.ts @@ -2,8 +2,8 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import type { ResolvedZaloAccount } from "./accounts.js"; import { - hasInlineCommandTokens, isControlCommandMessage, + shouldComputeCommandAuthorized, } from "../../../src/auto-reply/command-detection.js"; import { finalizeInboundContext } from "../../../src/auto-reply/reply/inbound-context.js"; import { resolveCommandAuthorizedFromAuthorizers } from "../../../src/channels/command-gating.js"; @@ -443,16 +443,15 @@ async function processMessageWithPipeline(params: { const dmPolicy = account.config.dmPolicy ?? "pairing"; const configAllowFrom = (account.config.allowFrom ?? []).map((v) => String(v)); const rawBody = text?.trim() || (mediaPath ? "" : ""); - const shouldComputeCommandAuthorized = - isControlCommandMessage(rawBody, config) || hasInlineCommandTokens(rawBody); + const shouldComputeAuth = shouldComputeCommandAuthorized(rawBody, config); const storeAllowFrom = - !isGroup && (dmPolicy !== "open" || shouldComputeCommandAuthorized) + !isGroup && (dmPolicy !== "open" || shouldComputeAuth) ? await deps.readChannelAllowFromStore("zalo").catch(() => []) : []; const effectiveAllowFrom = [...configAllowFrom, ...storeAllowFrom]; const useAccessGroups = config.commands?.useAccessGroups !== false; const senderAllowedForCommands = isSenderAllowed(senderId, effectiveAllowFrom); - const commandAuthorized = shouldComputeCommandAuthorized + const commandAuthorized = shouldComputeAuth ? resolveCommandAuthorizedFromAuthorizers({ useAccessGroups, authorizers: [{ configured: effectiveAllowFrom.length > 0, allowed: senderAllowedForCommands }], diff --git a/extensions/zalouser/src/monitor.ts b/extensions/zalouser/src/monitor.ts index c64d9bb6d..b572a08a9 100644 --- a/extensions/zalouser/src/monitor.ts +++ b/extensions/zalouser/src/monitor.ts @@ -2,8 +2,8 @@ import type { ChildProcess } from "node:child_process"; import type { RuntimeEnv } from "../../../src/runtime.js"; import { - hasInlineCommandTokens, isControlCommandMessage, + shouldComputeCommandAuthorized, } from "../../../src/auto-reply/command-detection.js"; import { finalizeInboundContext } from "../../../src/auto-reply/reply/inbound-context.js"; import { resolveCommandAuthorizedFromAuthorizers } from "../../../src/channels/command-gating.js"; @@ -111,16 +111,15 @@ async function processMessage( const dmPolicy = account.config.dmPolicy ?? "pairing"; const configAllowFrom = (account.config.allowFrom ?? []).map((v) => String(v)); const rawBody = content.trim(); - const shouldComputeCommandAuthorized = - isControlCommandMessage(rawBody, config) || hasInlineCommandTokens(rawBody); + const shouldComputeAuth = shouldComputeCommandAuthorized(rawBody, config); const storeAllowFrom = - !isGroup && (dmPolicy !== "open" || shouldComputeCommandAuthorized) + !isGroup && (dmPolicy !== "open" || shouldComputeAuth) ? await deps.readChannelAllowFromStore("zalouser").catch(() => []) : []; const effectiveAllowFrom = [...configAllowFrom, ...storeAllowFrom]; const useAccessGroups = config.commands?.useAccessGroups !== false; const senderAllowedForCommands = isSenderAllowed(senderId, effectiveAllowFrom); - const commandAuthorized = shouldComputeCommandAuthorized + const commandAuthorized = shouldComputeAuth ? resolveCommandAuthorizedFromAuthorizers({ useAccessGroups, authorizers: [{ configured: effectiveAllowFrom.length > 0, allowed: senderAllowedForCommands }], diff --git a/src/auto-reply/command-detection.ts b/src/auto-reply/command-detection.ts index 226d82d54..83749e633 100644 --- a/src/auto-reply/command-detection.ts +++ b/src/auto-reply/command-detection.ts @@ -58,3 +58,11 @@ export function hasInlineCommandTokens(text?: string): boolean { if (!body.trim()) return false; return /(?:^|\s)[/!][a-z]/i.test(body); } + +export function shouldComputeCommandAuthorized( + text?: string, + cfg?: ClawdbotConfig, + options?: CommandNormalizeOptions, +): boolean { + return isControlCommandMessage(text, cfg, options) || hasInlineCommandTokens(text); +} diff --git a/src/auto-reply/reply/abort.test.ts b/src/auto-reply/reply/abort.test.ts index 4406a5a80..d5de914aa 100644 --- a/src/auto-reply/reply/abort.test.ts +++ b/src/auto-reply/reply/abort.test.ts @@ -6,6 +6,7 @@ import type { ClawdbotConfig } from "../../config/config.js"; import { isAbortTrigger, tryFastAbortFromMessage } from "./abort.js"; import { enqueueFollowupRun, getFollowupQueueDepth, type FollowupRun } from "./queue.js"; import { initSessionState } from "./session.js"; +import { buildTestCtx } from "./test-ctx.js"; vi.mock("../../agents/pi-embedded.js", () => ({ abortEmbeddedPiRun: vi.fn().mockReturnValue(true), @@ -67,7 +68,7 @@ describe("abort detection", () => { const cfg = { session: { store: storePath }, commands: { text: false } } as ClawdbotConfig; const result = await tryFastAbortFromMessage({ - ctx: { + ctx: buildTestCtx({ CommandBody: "/stop", RawBody: "/stop", CommandAuthorized: true, @@ -76,7 +77,7 @@ describe("abort detection", () => { Surface: "telegram", From: "telegram:123", To: "telegram:123", - }, + }), cfg, }); @@ -130,7 +131,7 @@ describe("abort detection", () => { expect(getFollowupQueueDepth(sessionKey)).toBe(1); const result = await tryFastAbortFromMessage({ - ctx: { + ctx: buildTestCtx({ CommandBody: "/stop", RawBody: "/stop", CommandAuthorized: true, @@ -139,7 +140,7 @@ describe("abort detection", () => { Surface: "telegram", From: "telegram:123", To: "telegram:123", - }, + }), cfg, }); @@ -187,7 +188,7 @@ describe("abort detection", () => { ]); const result = await tryFastAbortFromMessage({ - ctx: { + ctx: buildTestCtx({ CommandBody: "/stop", RawBody: "/stop", CommandAuthorized: true, @@ -196,7 +197,7 @@ describe("abort detection", () => { Surface: "telegram", From: "telegram:parent", To: "telegram:parent", - }, + }), cfg, }); diff --git a/src/auto-reply/reply/abort.ts b/src/auto-reply/reply/abort.ts index aec0f8a0e..6fc0f2881 100644 --- a/src/auto-reply/reply/abort.ts +++ b/src/auto-reply/reply/abort.ts @@ -11,7 +11,7 @@ import { import { parseAgentSessionKey } from "../../routing/session-key.js"; import { resolveCommandAuthorization } from "../command-auth.js"; import { normalizeCommandBody } from "../commands-registry.js"; -import type { MsgContext } from "../templating.js"; +import type { FinalizedMsgContext, MsgContext } from "../templating.js"; import { logVerbose } from "../../globals.js"; import { stripMentions, stripStructuralPrefixes } from "./mentions.js"; import { clearSessionQueues } from "./queue.js"; @@ -115,7 +115,7 @@ export function stopSubagentsForRequester(params: { } export async function tryFastAbortFromMessage(params: { - ctx: MsgContext; + ctx: FinalizedMsgContext; cfg: ClawdbotConfig; }): Promise<{ handled: boolean; aborted: boolean; stoppedSubagents?: number }> { const { ctx, cfg } = params; @@ -132,7 +132,7 @@ export async function tryFastAbortFromMessage(params: { const abortRequested = normalized === "/stop" || isAbortTrigger(stripped); if (!abortRequested) return { handled: false, aborted: false }; - const commandAuthorized = ctx.CommandAuthorized ?? false; + const commandAuthorized = ctx.CommandAuthorized; const auth = resolveCommandAuthorization({ ctx, cfg, diff --git a/src/auto-reply/reply/dispatch-from-config.test.ts b/src/auto-reply/reply/dispatch-from-config.test.ts index 2dac96ca1..b395e2bc1 100644 --- a/src/auto-reply/reply/dispatch-from-config.test.ts +++ b/src/auto-reply/reply/dispatch-from-config.test.ts @@ -4,6 +4,7 @@ import type { ClawdbotConfig } from "../../config/config.js"; import type { MsgContext } from "../templating.js"; import type { GetReplyOptions, ReplyPayload } from "../types.js"; import type { ReplyDispatcher } from "./reply-dispatcher.js"; +import { buildTestCtx } from "./test-ctx.js"; const mocks = vi.hoisted(() => ({ routeReply: vi.fn(async () => ({ ok: true, messageId: "mock" })), @@ -58,11 +59,12 @@ describe("dispatchReplyFromConfig", () => { mocks.routeReply.mockClear(); const cfg = {} as ClawdbotConfig; const dispatcher = createDispatcher(); - const ctx: MsgContext = { + const ctx = buildTestCtx({ Provider: "slack", + Surface: undefined, OriginatingChannel: "slack", OriginatingTo: "channel:C123", - }; + }); const replyResolver = async ( _ctx: MsgContext, @@ -83,13 +85,13 @@ describe("dispatchReplyFromConfig", () => { mocks.routeReply.mockClear(); const cfg = {} as ClawdbotConfig; const dispatcher = createDispatcher(); - const ctx: MsgContext = { + const ctx = buildTestCtx({ Provider: "slack", AccountId: "acc-1", MessageThreadId: 123, OriginatingChannel: "telegram", OriginatingTo: "telegram:999", - }; + }); const replyResolver = async ( _ctx: MsgContext, @@ -116,10 +118,10 @@ describe("dispatchReplyFromConfig", () => { }); const cfg = {} as ClawdbotConfig; const dispatcher = createDispatcher(); - const ctx: MsgContext = { + const ctx = buildTestCtx({ Provider: "telegram", Body: "/stop", - }; + }); const replyResolver = vi.fn(async () => ({ text: "hi" }) as ReplyPayload); await dispatchReplyFromConfig({ ctx, cfg, dispatcher, replyResolver }); @@ -138,10 +140,10 @@ describe("dispatchReplyFromConfig", () => { }); const cfg = {} as ClawdbotConfig; const dispatcher = createDispatcher(); - const ctx: MsgContext = { + const ctx = buildTestCtx({ Provider: "telegram", Body: "/stop", - }; + }); await dispatchReplyFromConfig({ ctx, @@ -161,12 +163,12 @@ describe("dispatchReplyFromConfig", () => { aborted: false, }); const cfg = {} as ClawdbotConfig; - const ctx: MsgContext = { + const ctx = buildTestCtx({ Provider: "whatsapp", OriginatingChannel: "whatsapp", OriginatingTo: "whatsapp:+15555550123", MessageSid: "msg-1", - }; + }); const replyResolver = vi.fn(async () => ({ text: "hi" }) as ReplyPayload); await dispatchReplyFromConfig({ diff --git a/src/auto-reply/reply/dispatch-from-config.ts b/src/auto-reply/reply/dispatch-from-config.ts index c7a766fed..391ab6c1d 100644 --- a/src/auto-reply/reply/dispatch-from-config.ts +++ b/src/auto-reply/reply/dispatch-from-config.ts @@ -1,7 +1,7 @@ import type { ClawdbotConfig } from "../../config/config.js"; import { logVerbose } from "../../globals.js"; import { getReplyFromConfig } from "../reply.js"; -import type { MsgContext } from "../templating.js"; +import type { FinalizedMsgContext } from "../templating.js"; import type { GetReplyOptions, ReplyPayload } from "../types.js"; import { formatAbortReplyText, tryFastAbortFromMessage } from "./abort.js"; import { shouldSkipDuplicateInbound } from "./inbound-dedupe.js"; @@ -14,7 +14,7 @@ export type DispatchFromConfigResult = { }; export async function dispatchReplyFromConfig(params: { - ctx: MsgContext; + ctx: FinalizedMsgContext; cfg: ClawdbotConfig; dispatcher: ReplyDispatcher; replyOptions?: Omit; diff --git a/src/auto-reply/reply/get-reply.ts b/src/auto-reply/reply/get-reply.ts index d14194c6c..1633f9355 100644 --- a/src/auto-reply/reply/get-reply.ts +++ b/src/auto-reply/reply/get-reply.ts @@ -73,25 +73,25 @@ export async function getReplyFromConfig( silentToken: SILENT_REPLY_TOKEN, log: defaultRuntime.log, }); - opts?.onTypingController?.(typing); + opts?.onTypingController?.(typing); - finalizeInboundContext(ctx); + const finalized = finalizeInboundContext(ctx); await applyMediaUnderstanding({ - ctx, + ctx: finalized, cfg, agentDir, activeModel: { provider, model }, }); - const commandAuthorized = ctx.CommandAuthorized ?? false; + const commandAuthorized = finalized.CommandAuthorized; resolveCommandAuthorization({ - ctx, + ctx: finalized, cfg, commandAuthorized, }); const sessionState = await initSessionState({ - ctx, + ctx: finalized, cfg, commandAuthorized, }); @@ -113,7 +113,7 @@ export async function getReplyFromConfig( } = sessionState; const directiveResult = await resolveReplyDirectives({ - ctx, + ctx: finalized, cfg, agentId, agentDir, diff --git a/src/auto-reply/reply/inbound-context.test.ts b/src/auto-reply/reply/inbound-context.test.ts index 5b2aa8473..58647176c 100644 --- a/src/auto-reply/reply/inbound-context.test.ts +++ b/src/auto-reply/reply/inbound-context.test.ts @@ -18,6 +18,7 @@ describe("finalizeInboundContext", () => { expect(out.RawBody).toBe("raw\nline"); expect(out.BodyForAgent).toBe("a\nb\nc"); expect(out.BodyForCommands).toBe("raw\nline"); + expect(out.CommandAuthorized).toBe(false); expect(out.ChatType).toBe("channel"); expect(out.ConversationLabel).toContain("Test"); }); diff --git a/src/auto-reply/reply/inbound-context.ts b/src/auto-reply/reply/inbound-context.ts index c4262f815..1af20f7e9 100644 --- a/src/auto-reply/reply/inbound-context.ts +++ b/src/auto-reply/reply/inbound-context.ts @@ -1,6 +1,6 @@ import { normalizeChatType } from "../../channels/chat-type.js"; import { resolveConversationLabel } from "../../channels/conversation-label.js"; -import type { MsgContext } from "../templating.js"; +import type { FinalizedMsgContext, MsgContext } from "../templating.js"; import { formatInboundBodyWithSenderMeta } from "./inbound-sender-meta.js"; import { normalizeInboundTextNewlines } from "./inbound-text.js"; @@ -19,7 +19,7 @@ function normalizeTextField(value: unknown): string | undefined { export function finalizeInboundContext>( ctx: T, opts: FinalizeInboundContextOptions = {}, -): T & MsgContext { +): T & FinalizedMsgContext { const normalized = ctx as T & MsgContext; normalized.Body = normalizeInboundTextNewlines( @@ -64,5 +64,8 @@ export function finalizeInboundContext>( body: normalized.BodyForAgent, }); - return normalized; + // Always set. Default-deny when upstream forgets to populate it. + normalized.CommandAuthorized = normalized.CommandAuthorized === true; + + return normalized as T & FinalizedMsgContext; } diff --git a/src/auto-reply/reply/provider-dispatcher.ts b/src/auto-reply/reply/provider-dispatcher.ts index 2da188722..68e2431d1 100644 --- a/src/auto-reply/reply/provider-dispatcher.ts +++ b/src/auto-reply/reply/provider-dispatcher.ts @@ -1,5 +1,5 @@ import type { ClawdbotConfig } from "../../config/config.js"; -import type { MsgContext } from "../templating.js"; +import type { FinalizedMsgContext } from "../templating.js"; import type { GetReplyOptions } from "../types.js"; import type { DispatchFromConfigResult } from "./dispatch-from-config.js"; import { dispatchReplyFromConfig } from "./dispatch-from-config.js"; @@ -11,7 +11,7 @@ import { } from "./reply-dispatcher.js"; export async function dispatchReplyWithBufferedBlockDispatcher(params: { - ctx: MsgContext; + ctx: FinalizedMsgContext; cfg: ClawdbotConfig; dispatcherOptions: ReplyDispatcherWithTypingOptions; replyOptions?: Omit; @@ -37,7 +37,7 @@ export async function dispatchReplyWithBufferedBlockDispatcher(params: { } export async function dispatchReplyWithDispatcher(params: { - ctx: MsgContext; + ctx: FinalizedMsgContext; cfg: ClawdbotConfig; dispatcherOptions: ReplyDispatcherOptions; replyOptions?: Omit; diff --git a/src/auto-reply/reply/test-ctx.ts b/src/auto-reply/reply/test-ctx.ts new file mode 100644 index 000000000..031ab662c --- /dev/null +++ b/src/auto-reply/reply/test-ctx.ts @@ -0,0 +1,18 @@ +import type { FinalizedMsgContext, MsgContext } from "../templating.js"; +import { finalizeInboundContext } from "./inbound-context.js"; + +export function buildTestCtx(overrides: Partial = {}): FinalizedMsgContext { + return finalizeInboundContext({ + Body: "", + CommandBody: "", + CommandSource: "text", + From: "whatsapp:+1000", + To: "whatsapp:+2000", + ChatType: "direct", + Provider: "whatsapp", + Surface: "whatsapp", + CommandAuthorized: false, + ...overrides, + }); +} + diff --git a/src/auto-reply/templating.ts b/src/auto-reply/templating.ts index c77dedd2c..6bf5d013e 100644 --- a/src/auto-reply/templating.ts +++ b/src/auto-reply/templating.ts @@ -103,6 +103,14 @@ export type MsgContext = { HookMessages?: string[]; }; +export type FinalizedMsgContext = Omit & { + /** + * Always set by finalizeInboundContext(). + * Default-deny: missing/undefined becomes false. + */ + CommandAuthorized: boolean; +}; + export type TemplateContext = MsgContext & { BodyStripped?: string; SessionId?: string; diff --git a/src/slack/monitor/message-handler/prepare.ts b/src/slack/monitor/message-handler/prepare.ts index 6ba4b2b1d..34b551623 100644 --- a/src/slack/monitor/message-handler/prepare.ts +++ b/src/slack/monitor/message-handler/prepare.ts @@ -1,6 +1,7 @@ import { resolveAckReaction } from "../../../agents/identity.js"; import { hasControlCommand } from "../../../auto-reply/command-detection.js"; import { shouldHandleTextCommands } from "../../../auto-reply/commands-registry.js"; +import type { FinalizedMsgContext } from "../../../auto-reply/templating.js"; import { formatInboundEnvelope, formatThreadStarterEnvelope, @@ -466,10 +467,10 @@ export async function prepareSlackMessage(params: { MediaPath: media?.path, MediaType: media?.contentType, MediaUrl: media?.path, - CommandAuthorized: commandAuthorized, - OriginatingChannel: "slack" as const, - OriginatingTo: slackTo, - }) satisfies Record; + CommandAuthorized: commandAuthorized, + OriginatingChannel: "slack" as const, + OriginatingTo: slackTo, + }) satisfies FinalizedMsgContext; const replyTarget = ctxPayload.To ?? undefined; if (!replyTarget) return null; diff --git a/src/slack/monitor/message-handler/types.ts b/src/slack/monitor/message-handler/types.ts index 7522c9fa2..e7b4b0807 100644 --- a/src/slack/monitor/message-handler/types.ts +++ b/src/slack/monitor/message-handler/types.ts @@ -1,4 +1,5 @@ import type { ResolvedAgentRoute } from "../../../routing/resolve-route.js"; +import type { FinalizedMsgContext } from "../../../auto-reply/templating.js"; import type { ResolvedSlackAccount } from "../../accounts.js"; import type { SlackMessageEvent } from "../../types.js"; import type { SlackChannelConfigResolved } from "../channel-config.js"; @@ -11,7 +12,7 @@ export type PreparedSlackMessage = { route: ResolvedAgentRoute; channelConfig: SlackChannelConfigResolved | null; replyTarget: string; - ctxPayload: Record; + ctxPayload: FinalizedMsgContext; isDirectMessage: boolean; isRoomish: boolean; historyKey: string; diff --git a/src/web/auto-reply/monitor/process-message.ts b/src/web/auto-reply/monitor/process-message.ts index 7081afe9d..170baeba5 100644 --- a/src/web/auto-reply/monitor/process-message.ts +++ b/src/web/auto-reply/monitor/process-message.ts @@ -16,10 +16,7 @@ import { import { dispatchReplyWithBufferedBlockDispatcher } from "../../../auto-reply/reply/provider-dispatcher.js"; import type { getReplyFromConfig } from "../../../auto-reply/reply.js"; import type { ReplyPayload } from "../../../auto-reply/types.js"; -import { - hasInlineCommandTokens, - isControlCommandMessage, -} from "../../../auto-reply/command-detection.js"; +import { shouldComputeCommandAuthorized } from "../../../auto-reply/command-detection.js"; import { finalizeInboundContext } from "../../../auto-reply/reply/inbound-context.js"; import { toLocationContext } from "../../../channels/location.js"; import type { loadConfig } from "../../../config/config.js"; @@ -232,9 +229,7 @@ export async function processMessage(params: { const textLimit = params.maxMediaTextChunkLimit ?? resolveTextChunkLimit(params.cfg, "whatsapp"); let didLogHeartbeatStrip = false; let didSendReply = false; - const shouldComputeCommandAuthorized = - isControlCommandMessage(params.msg.body, params.cfg) || hasInlineCommandTokens(params.msg.body); - const commandAuthorized = shouldComputeCommandAuthorized + const commandAuthorized = shouldComputeCommandAuthorized(params.msg.body, params.cfg) ? await resolveWhatsAppCommandAuthorized({ cfg: params.cfg, msg: params.msg }) : undefined; const configuredResponsePrefix = params.cfg.messages?.responsePrefix;