From d4e3babdcc09c122bb068311a4da16fa2f069e42 Mon Sep 17 00:00:00 2001 From: Taras Lukavyi Date: Tue, 24 Mar 2026 03:51:30 +0100 Subject: [PATCH] fix: command auth SecretRef resolution (#52791) (thanks @Lukavyi) * fix(command-auth): handle unresolved SecretRef in resolveAllowFrom * fix(command-auth): fall back to config allowlists * fix(command-auth): avoid duplicate resolution fallback * fix(command-auth): fail closed on invalid allowlists * fix(command-auth): isolate fallback resolution errors * fix: record command auth SecretRef landing notes (#52791) (thanks @Lukavyi) --------- Co-authored-by: Ayaan Zaidi --- CHANGELOG.md | 1 + src/auto-reply/command-auth.ts | 193 ++++++++++++++-- src/auto-reply/command-control.test.ts | 300 ++++++++++++++++++++++++- 3 files changed, 471 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff4eb6dfb2d..54ad9905b83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai - Infra/exec trust: preserve shell-multiplexer wrapper binaries for policy checks without breaking approved-command reconstruction, so BusyBox/ToyBox allowlist and audit flows bind to the real wrapper while execution plans stay coherent. (#53134) Thanks @vincentkoc. - LINE/runtime-api: pre-export overlapping runtime symbols before the `line-runtime` star export so jiti no longer throws `TypeError: Cannot redefine property` on startup. (#53221) Thanks @Drickon. - CLI/cron: make `openclaw cron add|edit --at ... --tz ` honor the requested local wall-clock time for offset-less one-shot datetimes, including DST boundaries, and keep `--tz` rejected for `--every`. (#53224) Thanks @RolfHegr. +- Commands/auth: stop slash-command authorization from crashing or dropping valid allowlists when channel `allowFrom` resolution hits unresolved SecretRef-backed accounts, and fail closed only for the affected provider inference path. (#52791) Thanks @Lukavyi. ## 2026.3.23 diff --git a/src/auto-reply/command-auth.ts b/src/auto-reply/command-auth.ts index 44e9f5f30db..18ba2d3967d 100644 --- a/src/auto-reply/command-auth.ts +++ b/src/auto-reply/command-auth.ts @@ -20,13 +20,16 @@ export type CommandAuthorization = { to?: string; }; -function resolveProviderFromContext(ctx: MsgContext, cfg: OpenClawConfig): ChannelId | undefined { +function resolveProviderFromContext( + ctx: MsgContext, + cfg: OpenClawConfig, +): { providerId: ChannelId | undefined; hadResolutionError: boolean } { const explicitMessageChannel = normalizeMessageChannel(ctx.Provider) ?? normalizeMessageChannel(ctx.Surface) ?? normalizeMessageChannel(ctx.OriginatingChannel); if (explicitMessageChannel === INTERNAL_MESSAGE_CHANNEL) { - return undefined; + return { providerId: undefined, hadResolutionError: false }; } const direct = normalizeAnyChannelId(explicitMessageChannel ?? undefined) ?? @@ -35,7 +38,7 @@ function resolveProviderFromContext(ctx: MsgContext, cfg: OpenClawConfig): Chann normalizeAnyChannelId(ctx.Surface) ?? normalizeAnyChannelId(ctx.OriginatingChannel); if (direct) { - return direct; + return { providerId: direct, hadResolutionError: false }; } const candidates = [ctx.From, ctx.To] .filter((value): value is string => Boolean(value?.trim())) @@ -43,35 +46,52 @@ function resolveProviderFromContext(ctx: MsgContext, cfg: OpenClawConfig): Chann for (const candidate of candidates) { const normalizedCandidateChannel = normalizeMessageChannel(candidate); if (normalizedCandidateChannel === INTERNAL_MESSAGE_CHANNEL) { - return undefined; + return { providerId: undefined, hadResolutionError: false }; } const normalized = normalizeAnyChannelId(normalizedCandidateChannel ?? undefined) ?? (normalizedCandidateChannel as ChannelId | undefined) ?? normalizeAnyChannelId(candidate); if (normalized) { - return normalized; + return { providerId: normalized, hadResolutionError: false }; } } const configured = listChannelPlugins() .map((plugin) => { - if (!plugin.config?.resolveAllowFrom) { - return null; - } - const allowFrom = plugin.config.resolveAllowFrom({ + const resolvedAllowFrom = resolveProviderAllowFrom({ + plugin, cfg, accountId: ctx.AccountId, }); - if (!Array.isArray(allowFrom) || allowFrom.length === 0) { + const allowFrom = formatAllowFromList({ + plugin, + cfg, + accountId: ctx.AccountId, + allowFrom: resolvedAllowFrom.allowFrom, + }); + if (allowFrom.length === 0) { return null; } - return plugin.id; + return { + providerId: plugin.id, + hadResolutionError: resolvedAllowFrom.hadResolutionError, + }; }) - .filter((value): value is ChannelId => Boolean(value)); + .filter( + ( + value, + ): value is { + providerId: ChannelId; + hadResolutionError: boolean; + } => Boolean(value), + ); if (configured.length === 1) { return configured[0]; } - return undefined; + return { + providerId: undefined, + hadResolutionError: configured.some((entry) => entry.hadResolutionError), + }; } function formatAllowFromList(params: { @@ -105,6 +125,63 @@ function normalizeAllowFromEntry(params: { return normalized.filter((entry) => entry.trim().length > 0); } +function resolveProviderAllowFrom(params: { + plugin?: ChannelPlugin; + cfg: OpenClawConfig; + accountId?: string | null; +}): { + allowFrom: Array; + hadResolutionError: boolean; +} { + const { plugin, cfg, accountId } = params; + const providerId = plugin?.id; + if (!plugin?.config?.resolveAllowFrom) { + return { + allowFrom: resolveFallbackAllowFrom({ cfg, providerId, accountId }), + hadResolutionError: false, + }; + } + + try { + const allowFrom = plugin.config.resolveAllowFrom({ cfg, accountId }); + if (allowFrom == null) { + return { + allowFrom: [], + hadResolutionError: false, + }; + } + if (!Array.isArray(allowFrom)) { + console.warn( + `[command-auth] resolveAllowFrom returned an invalid allowFrom for provider "${providerId}", falling back to config allowFrom: invalid_result`, + ); + return { + allowFrom: resolveFallbackAllowFrom({ cfg, providerId, accountId }), + hadResolutionError: true, + }; + } + return { + allowFrom, + hadResolutionError: false, + }; + } catch (err) { + console.warn( + `[command-auth] resolveAllowFrom threw for provider "${providerId}", falling back to config allowFrom: ${describeAllowFromResolutionError(err)}`, + ); + return { + allowFrom: resolveFallbackAllowFrom({ cfg, providerId, accountId }), + hadResolutionError: true, + }; + } +} + +function describeAllowFromResolutionError(err: unknown): string { + if (err instanceof Error) { + const name = err.name.trim(); + return name || "Error"; + } + return "unknown_error"; +} + function resolveOwnerAllowFromList(params: { plugin?: ChannelPlugin; cfg: OpenClawConfig; @@ -283,7 +360,9 @@ function resolveFallbackAllowFrom(params: { > | undefined; const channelCfg = channels?.[providerId]; - const accountCfg = params.accountId ? channelCfg?.accounts?.[params.accountId] : undefined; + const accountCfg = + resolveFallbackAccountConfig(channelCfg?.accounts, params.accountId) ?? + resolveFallbackDefaultAccountConfig(channelCfg); const allowFrom = accountCfg?.allowFrom ?? accountCfg?.dm?.allowFrom ?? @@ -292,6 +371,64 @@ function resolveFallbackAllowFrom(params: { return Array.isArray(allowFrom) ? allowFrom : []; } +function resolveFallbackAccountConfig( + accounts: + | Record< + string, + | { + allowFrom?: Array; + dm?: { allowFrom?: Array }; + } + | undefined + > + | undefined, + accountId?: string | null, +) { + const normalizedAccountId = accountId?.trim().toLowerCase(); + if (!accounts || !normalizedAccountId) { + return undefined; + } + const direct = accounts[normalizedAccountId]; + if (direct) { + return direct; + } + const matchKey = Object.keys(accounts).find( + (key) => key.trim().toLowerCase() === normalizedAccountId, + ); + return matchKey ? accounts[matchKey] : undefined; +} + +function resolveFallbackDefaultAccountConfig( + channelCfg: + | { + allowFrom?: Array; + dm?: { allowFrom?: Array }; + defaultAccount?: string; + accounts?: Record< + string, + | { + allowFrom?: Array; + dm?: { allowFrom?: Array }; + } + | undefined + >; + } + | undefined, +) { + const accounts = channelCfg?.accounts; + if (!accounts) { + return undefined; + } + const preferred = + resolveFallbackAccountConfig(accounts, channelCfg?.defaultAccount) ?? + resolveFallbackAccountConfig(accounts, "default"); + if (preferred) { + return preferred; + } + const definedAccounts = Object.values(accounts).filter(Boolean); + return definedAccounts.length === 1 ? definedAccounts[0] : undefined; +} + function resolveFallbackCommandOptions(providerId?: ChannelId): { enforceOwnerForCommands: boolean; } { @@ -306,7 +443,10 @@ export function resolveCommandAuthorization(params: { commandAuthorized: boolean; }): CommandAuthorization { const { ctx, cfg, commandAuthorized } = params; - const providerId = resolveProviderFromContext(ctx, cfg); + const { providerId, hadResolutionError: providerResolutionError } = resolveProviderFromContext( + ctx, + cfg, + ); const plugin = providerId ? getChannelPlugin(providerId) : undefined; const from = (ctx.From ?? "").trim(); const to = (ctx.To ?? "").trim(); @@ -319,18 +459,25 @@ export function resolveCommandAuthorization(params: { providerId, }); - const allowFromRaw = plugin?.config?.resolveAllowFrom - ? plugin.config.resolveAllowFrom({ cfg, accountId: ctx.AccountId }) - : resolveFallbackAllowFrom({ + const resolvedAllowFrom = providerResolutionError + ? { + allowFrom: resolveFallbackAllowFrom({ + cfg, + providerId, + accountId: ctx.AccountId, + }), + hadResolutionError: true, + } + : resolveProviderAllowFrom({ + plugin, cfg, - providerId, accountId: ctx.AccountId, }); const allowFromList = formatAllowFromList({ plugin, cfg, accountId: ctx.AccountId, - allowFrom: Array.isArray(allowFromRaw) ? allowFromRaw : [], + allowFrom: resolvedAllowFrom.allowFrom, }); const configOwnerAllowFromList = resolveOwnerAllowFromList({ plugin, @@ -347,7 +494,8 @@ export function resolveCommandAuthorization(params: { allowFrom: ctx.OwnerAllowFrom, }); const allowAll = - allowFromList.length === 0 || allowFromList.some((entry) => entry.trim() === "*"); + !resolvedAllowFrom.hadResolutionError && + (allowFromList.length === 0 || allowFromList.some((entry) => entry.trim() === "*")); const ownerCandidatesForCommands = allowAll ? [] : allowFromList.filter((entry) => entry !== "*"); if (!allowAll && ownerCandidatesForCommands.length === 0 && to) { @@ -423,7 +571,8 @@ export function resolveCommandAuthorization(params: { const matchedCommandsAllowFrom = commandsAllowFromList.length ? senderCandidates.find((candidate) => commandsAllowFromList.includes(candidate)) : undefined; - isAuthorizedSender = commandsAllowAll || Boolean(matchedCommandsAllowFrom); + isAuthorizedSender = + !providerResolutionError && (commandsAllowAll || Boolean(matchedCommandsAllowFrom)); } else { // Fall back to existing behavior isAuthorizedSender = commandAuthorized && isOwnerForCommands; diff --git a/src/auto-reply/command-control.test.ts b/src/auto-reply/command-control.test.ts index 9d5dc1de094..860e010ec86 100644 --- a/src/auto-reply/command-control.test.ts +++ b/src/auto-reply/command-control.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; import { createOutboundTestPlugin, createTestRegistry } from "../test-utils/channel-plugins.js"; @@ -181,6 +181,50 @@ describe("resolveCommandAuthorization", () => { expect(auth.isAuthorizedSender).toBe(true); }); + it("falls back to channel allowFrom when provider allowlist resolution throws", () => { + setActivePluginRegistry( + createTestRegistry([ + { + pluginId: "telegram", + plugin: { + ...createOutboundTestPlugin({ + id: "telegram", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => { + throw new Error("channels.telegram.botToken: unresolved SecretRef"); + }, + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + ]), + ); + const cfg = { + channels: { telegram: { allowFrom: ["123"] } }, + } as OpenClawConfig; + + const auth = resolveCommandAuthorization({ + ctx: { + Provider: "telegram", + Surface: "telegram", + From: "telegram:123", + SenderId: "123", + } as MsgContext, + cfg, + commandAuthorized: true, + }); + + expect(auth.ownerList).toEqual(["123"]); + expect(auth.senderIsOwner).toBe(true); + expect(auth.isAuthorizedSender).toBe(true); + }); + describe("commands.allowFrom", () => { const commandsAllowFromConfig = { commands: { @@ -443,6 +487,260 @@ describe("resolveCommandAuthorization", () => { expect(deniedAuth.isAuthorizedSender).toBe(false); }); + + it("fails closed when provider inference hits unresolved SecretRef allowlists", () => { + setActivePluginRegistry( + createTestRegistry([ + { + pluginId: "telegram", + plugin: { + ...createOutboundTestPlugin({ + id: "telegram", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => { + throw new Error("channels.telegram.botToken: unresolved SecretRef"); + }, + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + ]), + ); + + const cfg = { + commands: { + allowFrom: { + telegram: ["123"], + }, + }, + channels: { + telegram: { + allowFrom: ["123"], + }, + }, + } as OpenClawConfig; + + const auth = resolveCommandAuthorization({ + ctx: { + SenderId: "123", + } as MsgContext, + cfg, + commandAuthorized: false, + }); + + expect(auth.providerId).toBe("telegram"); + expect(auth.isAuthorizedSender).toBe(false); + }); + + it("does not let an unrelated provider resolution error poison inferred commands.allowFrom", () => { + setActivePluginRegistry( + createTestRegistry([ + { + pluginId: "telegram", + plugin: { + ...createOutboundTestPlugin({ + id: "telegram", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => ["123"], + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + { + pluginId: "slack", + plugin: { + ...createOutboundTestPlugin({ + id: "slack", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => { + throw new Error("channels.slack.token: unresolved SecretRef"); + }, + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + ]), + ); + + const auth = resolveCommandAuthorization({ + ctx: { + SenderId: "123", + } as MsgContext, + cfg: { + commands: { + allowFrom: { + telegram: ["123"], + }, + }, + channels: { + telegram: { + allowFrom: ["123"], + }, + }, + } as OpenClawConfig, + commandAuthorized: false, + }); + + expect(auth.providerId).toBe("telegram"); + expect(auth.isAuthorizedSender).toBe(true); + }); + + it("preserves default-account allowFrom on SecretRef fallback", () => { + setActivePluginRegistry( + createTestRegistry([ + { + pluginId: "telegram", + plugin: { + ...createOutboundTestPlugin({ + id: "telegram", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => { + throw new Error("channels.telegram.botToken: unresolved SecretRef"); + }, + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + ]), + ); + + const auth = resolveCommandAuthorization({ + ctx: { + Provider: "telegram", + Surface: "telegram", + SenderId: "123", + } as MsgContext, + cfg: { + channels: { + telegram: { + accounts: { + default: { + allowFrom: ["123"], + }, + }, + }, + }, + } as OpenClawConfig, + commandAuthorized: true, + }); + + expect(auth.ownerList).toEqual(["123"]); + expect(auth.isAuthorizedSender).toBe(true); + }); + + it("treats undefined allowFrom as an open channel, not a resolution failure", () => { + setActivePluginRegistry( + createTestRegistry([ + { + pluginId: "discord", + plugin: { + ...createOutboundTestPlugin({ + id: "discord", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => undefined, + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + ]), + ); + + const auth = resolveCommandAuthorization({ + ctx: { + Provider: "discord", + Surface: "discord", + SenderId: "123", + } as MsgContext, + cfg: { + channels: { + discord: {}, + }, + } as OpenClawConfig, + commandAuthorized: true, + }); + + expect(auth.isAuthorizedSender).toBe(true); + }); + + it("does not log raw resolution messages from thrown allowFrom errors", () => { + setActivePluginRegistry( + createTestRegistry([ + { + pluginId: "telegram", + plugin: { + ...createOutboundTestPlugin({ + id: "telegram", + outbound: { deliveryMode: "direct" }, + }), + config: { + listAccountIds: () => ["default"], + resolveAccount: () => ({}), + resolveAllowFrom: () => { + throw new Error("SECRET-TOKEN-123"); + }, + formatAllowFrom: ({ allowFrom }: { allowFrom: Array }) => + allowFrom.map((entry) => String(entry).trim()).filter(Boolean), + }, + }, + source: "test", + }, + ]), + ); + + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + resolveCommandAuthorization({ + ctx: { + Provider: "telegram", + Surface: "telegram", + SenderId: "123", + } as MsgContext, + cfg: { + channels: { + telegram: { + allowFrom: ["123"], + }, + }, + } as OpenClawConfig, + commandAuthorized: true, + }); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0] ?? "")).toContain("Error"); + expect(String(warn.mock.calls[0]?.[0] ?? "")).not.toContain("SECRET-TOKEN-123"); + } finally { + warn.mockRestore(); + } + }); }); it("grants senderIsOwner for internal channel with operator.admin scope", () => {