From 2ad17098fecc6d64fb284d8694b25ef6b43ba331 Mon Sep 17 00:00:00 2001 From: Chunyue Wang <80630709+openperf@users.noreply.github.com> Date: Mon, 20 Apr 2026 14:59:09 +0800 Subject: [PATCH] fix(slack): tolerate unresolved channel SecretRef on outbound send path (#68954) Merged via squash. Prepared head SHA: f13c639c26df0d95b2ed1c90bb041d0d28123921 Co-authored-by: openperf <80630709+openperf@users.noreply.github.com> Co-authored-by: openperf <80630709+openperf@users.noreply.github.com> Reviewed-by: @openperf --- CHANGELOG.md | 1 + extensions/slack/src/accounts.test.ts | 210 ++++++++++++++++++++++++++ extensions/slack/src/accounts.ts | 75 ++++++--- extensions/slack/src/channel.ts | 4 + extensions/slack/src/send.ts | 11 ++ 5 files changed, 281 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aae47bbc88..fc93bfea0b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ Docs: https://docs.openclaw.ai - Telegram/setup: require numeric `allowFrom` user IDs during setup instead of offering unsupported `@username` DM resolution, and point operators to `from.id`/`getUpdates` for discovery. (#69191) Thanks @obviyus. - GitHub Copilot/onboarding: default GitHub Copilot setup to `claude-opus-4.6` and keep the bundled default model list aligned, so new Copilot setups no longer start on the older `gpt-4o` default. (#69207) Thanks @obviyus. - Gateway/status: separate reachability, capability, and read-probe reporting so connect-only or scope-limited sessions no longer look fully healthy, and normalize SSH targets entered as `ssh user@host`. (#69215) Thanks @obviyus. +- Slack: fix outbound replies failing with "unresolved SecretRef" for accounts configured via `file` or `exec` secret sources; the send path now tolerates the runtime snapshot retaining an unresolved channel SecretRef when a boot-resolved token override is already available. (#68954) Thanks @openperf. ## 2026.4.19-beta.2 diff --git a/extensions/slack/src/accounts.test.ts b/extensions/slack/src/accounts.test.ts index 6424792b77d..8fe0cd770a3 100644 --- a/extensions/slack/src/accounts.test.ts +++ b/extensions/slack/src/accounts.test.ts @@ -1,3 +1,4 @@ +import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import { describe, expect, it } from "vitest"; import { resolveSlackAccount } from "./accounts.js"; @@ -107,3 +108,212 @@ describe("resolveSlackAccount allowFrom precedence", () => { expect(resolved.config.dm?.allowFrom).toEqual(["U123"]); }); }); + +describe("resolveSlackAccount tolerateUnresolvedSecrets", () => { + // The static `SlackAccountConfig.botToken` type is `string` because it + // models the post-resolution shape, but the runtime cfg snapshot can still + // hold an unresolved `SecretRef` object for inactive channel targets (per + // the inspect/strict separation in #66818). Cast via `unknown` so the test + // can construct that runtime-only shape without weakening the production + // type. See #68237. + const cfgWithUnresolvedBotTokenRef = { + channels: { + slack: { + accounts: { + default: { + botToken: { source: "exec", provider: "default", id: "slack_bot_token" }, + allowFrom: ["U999"], + }, + }, + }, + }, + } as unknown as OpenClawConfig; + + it("throws by default when the snapshot still holds an unresolved SecretRef botToken", () => { + expect(() => + resolveSlackAccount({ + cfg: cfgWithUnresolvedBotTokenRef, + accountId: "default", + }), + ).toThrowError(/channels\.slack\.accounts\.default\.botToken/); + }); + + it("returns undefined credentials without throwing when tolerateUnresolvedSecrets is set", () => { + const resolved = resolveSlackAccount({ + cfg: cfgWithUnresolvedBotTokenRef, + accountId: "default", + tolerateUnresolvedSecrets: true, + }); + + expect(resolved.botToken).toBeUndefined(); + expect(resolved.botTokenSource).toBe("none"); + // Surrounding account info still resolves so callers with an explicit + // override (for example sendMessageSlack receiving opts.token) can keep + // operating. + expect(resolved.accountId).toBe("default"); + expect(resolved.config.allowFrom).toEqual(["U999"]); + }); + + it("still returns resolved string credentials in tolerant mode", () => { + const resolved = resolveSlackAccount({ + cfg: { + channels: { + slack: { + accounts: { + default: { botToken: "xoxb-resolved", appToken: "xapp-resolved" }, + }, + }, + }, + }, + accountId: "default", + tolerateUnresolvedSecrets: true, + }); + + expect(resolved.botToken).toBe("xoxb-resolved"); + expect(resolved.botTokenSource).toBe("config"); + expect(resolved.appToken).toBe("xapp-resolved"); + expect(resolved.appTokenSource).toBe("config"); + }); + + it("does not silently fall back to SLACK_*_TOKEN env vars in tolerant mode when all credentials are configured as SecretRef (credential confusion guard)", () => { + // Each credential is configured as a SecretRef. In tolerant mode none of + // them resolves, so per-credential env gating must block all three env + // vars; otherwise a stray `SLACK_*_TOKEN` would silently impersonate the + // operator-configured account (CWE-287 credential confusion). + const cfgAllSecretRefs = { + channels: { + slack: { + accounts: { + default: { + botToken: { source: "exec", provider: "default", id: "slack_bot_token" }, + appToken: { source: "exec", provider: "default", id: "slack_app_token" }, + userToken: { source: "exec", provider: "default", id: "slack_user_token" }, + }, + }, + }, + }, + } as unknown as OpenClawConfig; + const previousBotToken = process.env.SLACK_BOT_TOKEN; + const previousAppToken = process.env.SLACK_APP_TOKEN; + const previousUserToken = process.env.SLACK_USER_TOKEN; + process.env.SLACK_BOT_TOKEN = "xoxb-env-fallback"; + process.env.SLACK_APP_TOKEN = "xapp-env-fallback"; + process.env.SLACK_USER_TOKEN = "xoxp-env-fallback"; + try { + const resolved = resolveSlackAccount({ + cfg: cfgAllSecretRefs, + accountId: "default", + tolerateUnresolvedSecrets: true, + }); + + expect(resolved.botToken).toBeUndefined(); + expect(resolved.botTokenSource).toBe("none"); + expect(resolved.appToken).toBeUndefined(); + expect(resolved.appTokenSource).toBe("none"); + expect(resolved.userToken).toBeUndefined(); + expect(resolved.userTokenSource).toBe("none"); + } finally { + if (previousBotToken === undefined) { + delete process.env.SLACK_BOT_TOKEN; + } else { + process.env.SLACK_BOT_TOKEN = previousBotToken; + } + if (previousAppToken === undefined) { + delete process.env.SLACK_APP_TOKEN; + } else { + process.env.SLACK_APP_TOKEN = previousAppToken; + } + if (previousUserToken === undefined) { + delete process.env.SLACK_USER_TOKEN; + } else { + process.env.SLACK_USER_TOKEN = previousUserToken; + } + } + }); + + it("preserves SLACK_BOT_TOKEN env fallback in tolerant mode when no config token is set (env-only setups)", () => { + const previousBotToken = process.env.SLACK_BOT_TOKEN; + const previousAppToken = process.env.SLACK_APP_TOKEN; + process.env.SLACK_BOT_TOKEN = "xoxb-env-only"; + process.env.SLACK_APP_TOKEN = "xapp-env-only"; + try { + // No SecretRef and no string token configured for the default account: + // env fallback must still fire so env-only deployments (relying solely + // on SLACK_BOT_TOKEN / SLACK_APP_TOKEN) keep working when callers like + // `channel.ts` invoke sendMessageSlack without an explicit override. + const resolved = resolveSlackAccount({ + cfg: { + channels: { + slack: { + accounts: { + default: { allowFrom: ["U001"] }, + }, + }, + }, + }, + accountId: "default", + tolerateUnresolvedSecrets: true, + }); + + expect(resolved.botToken).toBe("xoxb-env-only"); + expect(resolved.botTokenSource).toBe("env"); + expect(resolved.appToken).toBe("xapp-env-only"); + expect(resolved.appTokenSource).toBe("env"); + } finally { + if (previousBotToken === undefined) { + delete process.env.SLACK_BOT_TOKEN; + } else { + process.env.SLACK_BOT_TOKEN = previousBotToken; + } + if (previousAppToken === undefined) { + delete process.env.SLACK_APP_TOKEN; + } else { + process.env.SLACK_APP_TOKEN = previousAppToken; + } + } + }); + + it("blocks env fallback per-credential: unresolved SecretRef on botToken does not leak SLACK_APP_TOKEN", () => { + const previousBotToken = process.env.SLACK_BOT_TOKEN; + const previousAppToken = process.env.SLACK_APP_TOKEN; + process.env.SLACK_BOT_TOKEN = "xoxb-env-bot"; + process.env.SLACK_APP_TOKEN = "xapp-env-app"; + try { + // botToken has an unresolved SecretRef (env fallback should be + // blocked), but appToken is unset (env fallback should still fire). + // This proves the gating is per-credential, not whole-account. + const resolved = resolveSlackAccount({ + cfg: { + channels: { + slack: { + accounts: { + default: { + botToken: { source: "exec", provider: "default", id: "slack_bot_token" }, + }, + }, + }, + }, + } as unknown as OpenClawConfig, + accountId: "default", + tolerateUnresolvedSecrets: true, + }); + + expect(resolved.botToken).toBeUndefined(); + expect(resolved.botTokenSource).toBe("none"); + // appToken was never configured → env fallback still fires. + expect(resolved.appToken).toBe("xapp-env-app"); + expect(resolved.appTokenSource).toBe("env"); + } finally { + if (previousBotToken === undefined) { + delete process.env.SLACK_BOT_TOKEN; + } else { + process.env.SLACK_BOT_TOKEN = previousBotToken; + } + if (previousAppToken === undefined) { + delete process.env.SLACK_APP_TOKEN; + } else { + process.env.SLACK_APP_TOKEN = previousAppToken; + } + } + }); +}); diff --git a/extensions/slack/src/accounts.ts b/extensions/slack/src/accounts.ts index 6e0faec4bb0..948e547212e 100644 --- a/extensions/slack/src/accounts.ts +++ b/extensions/slack/src/accounts.ts @@ -6,6 +6,7 @@ import { resolveMergedAccountConfig, type OpenClawConfig, } from "openclaw/plugin-sdk/account-resolution"; +import { isSecretRef, normalizeSecretInputString } from "openclaw/plugin-sdk/secret-input"; import { normalizeOptionalString } from "openclaw/plugin-sdk/text-runtime"; import type { SlackAccountSurfaceFields } from "./account-surface-fields.js"; import type { SlackAccountConfig } from "./runtime-api.js"; @@ -35,10 +36,8 @@ export function mergeSlackAccountConfig( accountId: string, ): SlackAccountConfig { return resolveMergedAccountConfig({ - channelConfig: cfg.channels?.slack as SlackAccountConfig | undefined, - accounts: cfg.channels?.slack?.accounts as - | Record> - | undefined, + channelConfig: cfg.channels?.slack as SlackAccountConfig, + accounts: cfg.channels?.slack?.accounts as Record>, accountId, }); } @@ -46,6 +45,28 @@ export function mergeSlackAccountConfig( export function resolveSlackAccount(params: { cfg: OpenClawConfig; accountId?: string | null; + /** + * When true, account-level credential reads (`botToken`, `appToken`, + * `userToken`) silently become `undefined` for unresolved `SecretRef` + * inputs instead of throwing. Default is false to preserve the strict + * behavior expected by boot-time provider initialization, which must + * surface unresolved channel SecretRefs loudly. + * + * Pass `true` from call sites that already have a separately-resolved + * credential override (for example `sendMessageSlack` receives an explicit + * `opts.token` derived from the boot-time monitor context) and only need + * the rest of the account info (account id, dm policy, channel settings, + * etc.). The downstream consumer's existing `if (!token)` guard still + * surfaces a clean "missing token" error when no explicit override is + * supplied either. + * + * Without this opt-in, an inactive `channels.slack.accounts.*.botToken` + * SecretRef left in the runtime snapshot (per the inspect/strict + * separation introduced in #66818) blows up the strict resolver path even + * though the actual send already has a valid boot-resolved token. See + * #68237. + */ + tolerateUnresolvedSecrets?: boolean; }): ResolvedSlackAccount { const accountId = normalizeAccountId( params.accountId ?? resolveDefaultSlackAccountId(params.cfg), @@ -54,22 +75,36 @@ export function resolveSlackAccount(params: { const merged = mergeSlackAccountConfig(params.cfg, accountId); const accountEnabled = merged.enabled !== false; const enabled = baseEnabled && accountEnabled; - const allowEnv = accountId === DEFAULT_ACCOUNT_ID; - const envBot = allowEnv ? resolveSlackBotToken(process.env.SLACK_BOT_TOKEN) : undefined; - const envApp = allowEnv ? resolveSlackAppToken(process.env.SLACK_APP_TOKEN) : undefined; - const envUser = allowEnv ? resolveSlackUserToken(process.env.SLACK_USER_TOKEN) : undefined; - const configBot = resolveSlackBotToken( - merged.botToken, - `channels.slack.accounts.${accountId}.botToken`, - ); - const configApp = resolveSlackAppToken( - merged.appToken, - `channels.slack.accounts.${accountId}.appToken`, - ); - const configUser = resolveSlackUserToken( - merged.userToken, - `channels.slack.accounts.${accountId}.userToken`, - ); + // Per-credential env-var fallback gating: in tolerant mode, only block + // the `SLACK_*_TOKEN` env fallback for credentials whose configured value + // is an unresolved `SecretRef` object. Otherwise (config field is a + // resolved string, or unset entirely) keep the original env fallback so + // legitimate env-only setups (no per-account config token, just + // `SLACK_BOT_TOKEN` in the process env) keep working. This avoids + // credential confusion (CWE-287) on misconfigured deployments where an + // unresolved SecretRef would otherwise be silently overridden by a stray + // env var, while preserving the env-only contract that callers like + // `extensions/slack/src/channel.ts` rely on when omitting `opts.token`. + const baseAllowEnv = accountId === DEFAULT_ACCOUNT_ID; + const tolerantMode = params.tolerateUnresolvedSecrets === true; + const blockBotEnv = tolerantMode && isSecretRef(merged.botToken); + const blockAppEnv = tolerantMode && isSecretRef(merged.appToken); + const blockUserEnv = tolerantMode && isSecretRef(merged.userToken); + const envBot = + baseAllowEnv && !blockBotEnv ? resolveSlackBotToken(process.env.SLACK_BOT_TOKEN) : undefined; + const envApp = + baseAllowEnv && !blockAppEnv ? resolveSlackAppToken(process.env.SLACK_APP_TOKEN) : undefined; + const envUser = + baseAllowEnv && !blockUserEnv ? resolveSlackUserToken(process.env.SLACK_USER_TOKEN) : undefined; + const configBot = tolerantMode + ? normalizeSecretInputString(merged.botToken) + : resolveSlackBotToken(merged.botToken, `channels.slack.accounts.${accountId}.botToken`); + const configApp = tolerantMode + ? normalizeSecretInputString(merged.appToken) + : resolveSlackAppToken(merged.appToken, `channels.slack.accounts.${accountId}.appToken`); + const configUser = tolerantMode + ? normalizeSecretInputString(merged.userToken) + : resolveSlackUserToken(merged.userToken, `channels.slack.accounts.${accountId}.userToken`); const botToken = configBot ?? envBot; const appToken = configApp ?? envApp; const userToken = configUser ?? envUser; diff --git a/extensions/slack/src/channel.ts b/extensions/slack/src/channel.ts index 81d4ba8bd40..3a1100b9208 100644 --- a/extensions/slack/src/channel.ts +++ b/extensions/slack/src/channel.ts @@ -170,6 +170,10 @@ async function resolveSlackSendContext(params: { const send = resolveOutboundSendDep(params.deps, "slack") ?? (await loadSlackSendRuntime()).sendMessageSlack; + // params.cfg is the scoped channel-dispatch config; channel credentials are + // expected to be resolved here (not a raw loadConfig() snapshot). Strict mode + // is intentional so boot-time misconfigurations surface loudly. See #68237 + // for the companion tolerant-mode path in sendMessageSlack itself. const account = resolveSlackAccount({ cfg: params.cfg, accountId: params.accountId }); const token = getTokenForOperation(account, "write"); const botToken = account.botToken?.trim(); diff --git a/extensions/slack/src/send.ts b/extensions/slack/src/send.ts index ae95dd12a10..988e95ccda3 100644 --- a/extensions/slack/src/send.ts +++ b/extensions/slack/src/send.ts @@ -322,9 +322,20 @@ export async function sendMessageSlack( throw new Error("Slack send requires text, blocks, or media"); } const cfg = opts.cfg ?? loadConfig(); + // Tolerate unresolved channel SecretRefs in the cfg snapshot here: the + // send path either receives an explicit `opts.token` (resolved at Slack + // monitor boot time and threaded through `ctx.botToken`) or surfaces the + // existing "Slack bot token missing" error via `resolveToken` below. The + // runtime snapshot can legitimately retain unresolved `channels.slack.*` + // SecretRefs (see the inspect/strict separation introduced in #66818) when + // the active account's secrets were not part of the agent-runtime base + // target set; failing the strict resolver here would block outbound + // replies even though `reactions.add` and inbound dispatch (which use the + // boot-resolved client/token directly) keep working. See #68237. const account = resolveSlackAccount({ cfg, accountId: opts.accountId, + tolerateUnresolvedSecrets: true, }); const token = resolveToken({ explicit: opts.token,