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 <hi@obviy.us>
This commit is contained in:
Taras Lukavyi
2026-03-24 03:51:30 +01:00
committed by GitHub
parent 5cd8d43af9
commit d4e3babdcc
3 changed files with 471 additions and 23 deletions

View File

@@ -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<string | number>;
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<string | number>;
dm?: { allowFrom?: Array<string | number> };
}
| 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<string | number>;
dm?: { allowFrom?: Array<string | number> };
defaultAccount?: string;
accounts?: Record<
string,
| {
allowFrom?: Array<string | number>;
dm?: { allowFrom?: Array<string | number> };
}
| 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;