From b7589b32a8e68a0cd40da34bfcd2e6efdad35c7c Mon Sep 17 00:00:00 2001 From: LiaoyuanNing Date: Wed, 4 Mar 2026 09:22:50 +0800 Subject: [PATCH] fix(feishu): support SecretRef-style env credentials in account resolver (#30903) Merged via squash. Prepared head SHA: d3d0a18f173e999070dae4ff01423dadd2804a9c Co-authored-by: LiaoyuanNing <259494737+LiaoyuanNing@users.noreply.github.com> Co-authored-by: joshavant <830519+joshavant@users.noreply.github.com> Reviewed-by: @joshavant --- CHANGELOG.md | 1 + extensions/feishu/src/accounts.test.ts | 187 +++++++++++++++++++++++ extensions/feishu/src/accounts.ts | 60 ++++++-- extensions/feishu/src/onboarding.test.ts | 147 ++++++++++++++++++ extensions/feishu/src/onboarding.ts | 52 +++++-- 5 files changed, 422 insertions(+), 25 deletions(-) create mode 100644 extensions/feishu/src/onboarding.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 540ecde21..06c04ecce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -530,6 +530,7 @@ Docs: https://docs.openclaw.ai - Slack/Legacy streaming config: map boolean `channels.slack.streaming=false` to unified streaming mode `off` (with `nativeStreaming=false`) so legacy configs correctly disable draft preview/native streaming instead of defaulting to `partial`. (#25990) Thanks @chilu18. - Slack/Socket reconnect reliability: reconnect Socket Mode after disconnect/start failures using bounded exponential backoff with abort-aware waits, while preserving clean shutdown behavior and adding disconnect/error helper tests. (#27232) Thanks @pandego. - Memory/QMD update+embed output cap: discard captured stdout for `qmd update` and `qmd embed` runs (while keeping stderr diagnostics) so large index progress output no longer fails sync with `produced too much output` during boot/refresh. (#28900; landed from contributor PR #23311 by @haitao-sjsu) Thanks @haitao-sjsu. +- Feishu/Onboarding SecretRef guards: avoid direct `.trim()` calls on object-form `appId`/`appSecret` in onboarding credential checks, keep status semantics strict when an account explicitly sets empty `appId` (no fallback to top-level `appId`), recognize env SecretRef `appId`/`appSecret` as configured so readiness is accurate, and preserve unresolved SecretRef errors in default account resolution for actionable diagnostics. (#30903) Thanks @LiaoyuanNing. - Onboarding/Custom providers: raise default custom-provider model context window to the runtime hard minimum (16k) and auto-heal existing custom model entries below that threshold during reconfiguration, preventing immediate `Model context window too small (4096 tokens)` failures. (#21653) Thanks @r4jiv007. - Web UI/Assistant text: strip internal `...` scaffolding from rendered assistant messages (while preserving code-fence literals), preventing memory-context leakage in chat output for models that echo internal blocks. (#29851) Thanks @Valkster70. - Dashboard/Sessions: allow authenticated Control UI clients to delete and patch sessions while still blocking regular webchat clients from session mutation RPCs, fixing Dashboard session delete failures. (#21264) Thanks @jskoiz. diff --git a/extensions/feishu/src/accounts.test.ts b/extensions/feishu/src/accounts.test.ts index 3fd9f1fba..bc04d4c56 100644 --- a/extensions/feishu/src/accounts.test.ts +++ b/extensions/feishu/src/accounts.test.ts @@ -3,7 +3,11 @@ import { resolveDefaultFeishuAccountId, resolveDefaultFeishuAccountSelection, resolveFeishuAccount, + resolveFeishuCredentials, } from "./accounts.js"; +import type { FeishuConfig } from "./types.js"; + +const asConfig = (value: Partial) => value as FeishuConfig; describe("resolveDefaultFeishuAccountId", () => { it("prefers channels.feishu.defaultAccount when configured", () => { @@ -98,6 +102,148 @@ describe("resolveDefaultFeishuAccountId", () => { }); }); +describe("resolveFeishuCredentials", () => { + it("throws unresolved SecretRef errors by default for unsupported secret sources", () => { + expect(() => + resolveFeishuCredentials( + asConfig({ + appId: "cli_123", + appSecret: { source: "file", provider: "default", id: "path/to/secret" } as never, + }), + ), + ).toThrow(/unresolved SecretRef/i); + }); + + it("returns null (without throwing) when unresolved SecretRef is allowed", () => { + const creds = resolveFeishuCredentials( + asConfig({ + appId: "cli_123", + appSecret: { source: "file", provider: "default", id: "path/to/secret" } as never, + }), + { allowUnresolvedSecretRef: true }, + ); + + expect(creds).toBeNull(); + }); + + it("throws unresolved SecretRef error when env SecretRef points to missing env var", () => { + const key = "FEISHU_APP_SECRET_MISSING_TEST"; + const prev = process.env[key]; + delete process.env[key]; + try { + expect(() => + resolveFeishuCredentials( + asConfig({ + appId: "cli_123", + appSecret: { source: "env", provider: "default", id: key } as never, + }), + ), + ).toThrow(/unresolved SecretRef/i); + } finally { + if (prev === undefined) { + delete process.env[key]; + } else { + process.env[key] = prev; + } + } + }); + + it("resolves env SecretRef objects when unresolved refs are allowed", () => { + const key = "FEISHU_APP_SECRET_TEST"; + const prev = process.env[key]; + process.env[key] = " secret_from_env "; + + try { + const creds = resolveFeishuCredentials( + asConfig({ + appId: "cli_123", + appSecret: { source: "env", provider: "default", id: key } as never, + }), + { allowUnresolvedSecretRef: true }, + ); + + expect(creds).toEqual({ + appId: "cli_123", + appSecret: "secret_from_env", + encryptKey: undefined, + verificationToken: undefined, + domain: "feishu", + }); + } finally { + if (prev === undefined) { + delete process.env[key]; + } else { + process.env[key] = prev; + } + } + }); + + it("resolves env SecretRef with custom provider alias when unresolved refs are allowed", () => { + const key = "FEISHU_APP_SECRET_CUSTOM_PROVIDER_TEST"; + const prev = process.env[key]; + process.env[key] = " secret_from_env_alias "; + + try { + const creds = resolveFeishuCredentials( + asConfig({ + appId: "cli_123", + appSecret: { source: "env", provider: "corp-env", id: key } as never, + }), + { allowUnresolvedSecretRef: true }, + ); + + expect(creds?.appSecret).toBe("secret_from_env_alias"); + } finally { + if (prev === undefined) { + delete process.env[key]; + } else { + process.env[key] = prev; + } + } + }); + + it("preserves unresolved SecretRef diagnostics for env refs in default mode", () => { + const key = "FEISHU_APP_SECRET_POLICY_TEST"; + const prev = process.env[key]; + process.env[key] = "secret_from_env"; + try { + expect(() => + resolveFeishuCredentials( + asConfig({ + appId: "cli_123", + appSecret: { source: "env", provider: "default", id: key } as never, + }), + ), + ).toThrow(/unresolved SecretRef/i); + } finally { + if (prev === undefined) { + delete process.env[key]; + } else { + process.env[key] = prev; + } + } + }); + + it("trims and returns credentials when values are valid strings", () => { + const creds = resolveFeishuCredentials( + asConfig({ + appId: " cli_123 ", + appSecret: " secret_456 ", + encryptKey: " enc ", + verificationToken: " vt ", + }), + ); + + expect(creds).toEqual({ + appId: "cli_123", + appSecret: "secret_456", + encryptKey: "enc", + verificationToken: "vt", + domain: "feishu", + }); + }); +}); + describe("resolveFeishuAccount", () => { it("uses top-level credentials with configured default account id even without account map entry", () => { const cfg = { @@ -158,4 +304,45 @@ describe("resolveFeishuAccount", () => { expect(account.selectionSource).toBe("explicit"); expect(account.appId).toBe("cli_default"); }); + + it("surfaces unresolved SecretRef errors in account resolution", () => { + expect(() => + resolveFeishuAccount({ + cfg: { + channels: { + feishu: { + accounts: { + main: { + appId: "cli_123", + appSecret: { source: "file", provider: "default", id: "path/to/secret" }, + } as never, + }, + }, + }, + } as never, + accountId: "main", + }), + ).toThrow(/unresolved SecretRef/i); + }); + + it("does not throw when account name is non-string", () => { + expect(() => + resolveFeishuAccount({ + cfg: { + channels: { + feishu: { + accounts: { + main: { + name: { bad: true }, + appId: "cli_123", + appSecret: "secret_456", + } as never, + }, + }, + }, + } as never, + accountId: "main", + }), + ).not.toThrow(); + }); }); diff --git a/extensions/feishu/src/accounts.ts b/extensions/feishu/src/accounts.ts index d91890691..39194cda0 100644 --- a/extensions/feishu/src/accounts.ts +++ b/extensions/feishu/src/accounts.ts @@ -129,27 +129,54 @@ export function resolveFeishuCredentials( verificationToken?: string; domain: FeishuDomain; } | null { - const appId = cfg?.appId?.trim(); - const appSecret = options?.allowUnresolvedSecretRef - ? normalizeSecretInputString(cfg?.appSecret) - : normalizeResolvedSecretInputString({ - value: cfg?.appSecret, - path: "channels.feishu.appSecret", - }); + const normalizeString = (value: unknown): string | undefined => { + if (typeof value !== "string") { + return undefined; + } + const trimmed = value.trim(); + return trimmed ? trimmed : undefined; + }; + + const resolveSecretLike = (value: unknown, path: string): string | undefined => { + const asString = normalizeString(value); + if (asString) { + return asString; + } + + // In relaxed/onboarding paths only: allow direct env SecretRef reads for UX. + // Default resolution path must preserve unresolved-ref diagnostics/policy semantics. + if (options?.allowUnresolvedSecretRef && typeof value === "object" && value !== null) { + const rec = value as Record; + const source = normalizeString(rec.source)?.toLowerCase(); + const id = normalizeString(rec.id); + if (source === "env" && id) { + const envValue = normalizeString(process.env[id]); + if (envValue) { + return envValue; + } + } + } + + if (options?.allowUnresolvedSecretRef) { + return normalizeSecretInputString(value); + } + return normalizeResolvedSecretInputString({ value, path }); + }; + + const appId = resolveSecretLike(cfg?.appId, "channels.feishu.appId"); + const appSecret = resolveSecretLike(cfg?.appSecret, "channels.feishu.appSecret"); + if (!appId || !appSecret) { return null; } return { appId, appSecret, - encryptKey: cfg?.encryptKey?.trim() || undefined, - verificationToken: - (options?.allowUnresolvedSecretRef - ? normalizeSecretInputString(cfg?.verificationToken) - : normalizeResolvedSecretInputString({ - value: cfg?.verificationToken, - path: "channels.feishu.verificationToken", - })) || undefined, + encryptKey: normalizeString(cfg?.encryptKey), + verificationToken: resolveSecretLike( + cfg?.verificationToken, + "channels.feishu.verificationToken", + ), domain: cfg?.domain ?? "feishu", }; } @@ -186,13 +213,14 @@ export function resolveFeishuAccount(params: { // Resolve credentials from merged config const creds = resolveFeishuCredentials(merged); + const accountName = (merged as FeishuAccountConfig).name; return { accountId, selectionSource, enabled, configured: Boolean(creds), - name: (merged as FeishuAccountConfig).name?.trim() || undefined, + name: typeof accountName === "string" ? accountName.trim() || undefined : undefined, appId: creds?.appId, appSecret: creds?.appSecret, encryptKey: creds?.encryptKey, diff --git a/extensions/feishu/src/onboarding.test.ts b/extensions/feishu/src/onboarding.test.ts new file mode 100644 index 000000000..dbb714485 --- /dev/null +++ b/extensions/feishu/src/onboarding.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it, vi } from "vitest"; + +vi.mock("./probe.js", () => ({ + probeFeishu: vi.fn(async () => ({ ok: false, error: "mocked" })), +})); + +import { feishuOnboardingAdapter } from "./onboarding.js"; + +const baseConfigureContext = { + runtime: {} as never, + accountOverrides: {}, + shouldPromptAccountIds: false, + forceAllowFrom: false, +}; + +const baseStatusContext = { + accountOverrides: {}, +}; + +describe("feishuOnboardingAdapter.configure", () => { + it("does not throw when config appId/appSecret are SecretRef objects", async () => { + const text = vi + .fn() + .mockResolvedValueOnce("cli_from_prompt") + .mockResolvedValueOnce("secret_from_prompt") + .mockResolvedValueOnce("oc_group_1"); + + const prompter = { + note: vi.fn(async () => undefined), + text, + confirm: vi.fn(async () => true), + select: vi.fn( + async ({ initialValue }: { initialValue?: string }) => initialValue ?? "allowlist", + ), + } as never; + + await expect( + feishuOnboardingAdapter.configure({ + cfg: { + channels: { + feishu: { + appId: { source: "env", id: "FEISHU_APP_ID", provider: "default" }, + appSecret: { source: "env", id: "FEISHU_APP_SECRET", provider: "default" }, + }, + }, + } as never, + prompter, + ...baseConfigureContext, + }), + ).resolves.toBeTruthy(); + }); +}); + +describe("feishuOnboardingAdapter.getStatus", () => { + it("does not fallback to top-level appId when account explicitly sets empty appId", async () => { + const status = await feishuOnboardingAdapter.getStatus({ + cfg: { + channels: { + feishu: { + appId: "top_level_app", + accounts: { + main: { + appId: "", + appSecret: "secret_123", + }, + }, + }, + }, + } as never, + ...baseStatusContext, + }); + + expect(status.configured).toBe(false); + }); + + it("treats env SecretRef appId as not configured when env var is missing", async () => { + const appIdKey = "FEISHU_APP_ID_STATUS_MISSING_TEST"; + const appSecretKey = "FEISHU_APP_SECRET_STATUS_MISSING_TEST"; + const prevAppId = process.env[appIdKey]; + const prevAppSecret = process.env[appSecretKey]; + delete process.env[appIdKey]; + process.env[appSecretKey] = "secret_env_456"; + + try { + const status = await feishuOnboardingAdapter.getStatus({ + cfg: { + channels: { + feishu: { + appId: { source: "env", id: appIdKey, provider: "default" }, + appSecret: { source: "env", id: appSecretKey, provider: "default" }, + }, + }, + } as never, + ...baseStatusContext, + }); + + expect(status.configured).toBe(false); + } finally { + if (prevAppId === undefined) { + delete process.env[appIdKey]; + } else { + process.env[appIdKey] = prevAppId; + } + if (prevAppSecret === undefined) { + delete process.env[appSecretKey]; + } else { + process.env[appSecretKey] = prevAppSecret; + } + } + }); + + it("treats env SecretRef appId/appSecret as configured in status", async () => { + const appIdKey = "FEISHU_APP_ID_STATUS_TEST"; + const appSecretKey = "FEISHU_APP_SECRET_STATUS_TEST"; + const prevAppId = process.env[appIdKey]; + const prevAppSecret = process.env[appSecretKey]; + process.env[appIdKey] = "cli_env_123"; + process.env[appSecretKey] = "secret_env_456"; + + try { + const status = await feishuOnboardingAdapter.getStatus({ + cfg: { + channels: { + feishu: { + appId: { source: "env", id: appIdKey, provider: "default" }, + appSecret: { source: "env", id: appSecretKey, provider: "default" }, + }, + }, + } as never, + ...baseStatusContext, + }); + + expect(status.configured).toBe(true); + } finally { + if (prevAppId === undefined) { + delete process.env[appIdKey]; + } else { + process.env[appIdKey] = prevAppId; + } + if (prevAppSecret === undefined) { + delete process.env[appSecretKey]; + } else { + process.env[appSecretKey] = prevAppSecret; + } + } + }); +}); diff --git a/extensions/feishu/src/onboarding.ts b/extensions/feishu/src/onboarding.ts index 163ea0506..00a4165b4 100644 --- a/extensions/feishu/src/onboarding.ts +++ b/extensions/feishu/src/onboarding.ts @@ -19,6 +19,14 @@ import type { FeishuConfig } from "./types.js"; const channel = "feishu" as const; +function normalizeString(value: unknown): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const trimmed = value.trim(); + return trimmed || undefined; +} + function setFeishuDmPolicy(cfg: ClawdbotConfig, dmPolicy: DmPolicy): ClawdbotConfig { const allowFrom = dmPolicy === "open" @@ -169,20 +177,43 @@ export const feishuOnboardingAdapter: ChannelOnboardingAdapter = { channel, getStatus: async ({ cfg }) => { const feishuCfg = cfg.channels?.feishu as FeishuConfig | undefined; + + const isAppIdConfigured = (value: unknown): boolean => { + const asString = normalizeString(value); + if (asString) { + return true; + } + if (!value || typeof value !== "object") { + return false; + } + const rec = value as Record; + const source = normalizeString(rec.source)?.toLowerCase(); + const id = normalizeString(rec.id); + if (source === "env" && id) { + return Boolean(normalizeString(process.env[id])); + } + return hasConfiguredSecretInput(value); + }; + const topLevelConfigured = Boolean( - feishuCfg?.appId?.trim() && hasConfiguredSecretInput(feishuCfg?.appSecret), + isAppIdConfigured(feishuCfg?.appId) && hasConfiguredSecretInput(feishuCfg?.appSecret), ); + const accountConfigured = Object.values(feishuCfg?.accounts ?? {}).some((account) => { if (!account || typeof account !== "object") { return false; } - const accountAppId = - typeof account.appId === "string" ? account.appId.trim() : feishuCfg?.appId?.trim(); - const accountSecretConfigured = - hasConfiguredSecretInput(account.appSecret) || - hasConfiguredSecretInput(feishuCfg?.appSecret); - return Boolean(accountAppId && accountSecretConfigured); + const hasOwnAppId = Object.prototype.hasOwnProperty.call(account, "appId"); + const hasOwnAppSecret = Object.prototype.hasOwnProperty.call(account, "appSecret"); + const accountAppIdConfigured = hasOwnAppId + ? isAppIdConfigured((account as Record).appId) + : isAppIdConfigured(feishuCfg?.appId); + const accountSecretConfigured = hasOwnAppSecret + ? hasConfiguredSecretInput((account as Record).appSecret) + : hasConfiguredSecretInput(feishuCfg?.appSecret); + return Boolean(accountAppIdConfigured && accountSecretConfigured); }); + const configured = topLevelConfigured || accountConfigured; const resolvedCredentials = resolveFeishuCredentials(feishuCfg, { allowUnresolvedSecretRef: true, @@ -224,7 +255,9 @@ export const feishuOnboardingAdapter: ChannelOnboardingAdapter = { allowUnresolvedSecretRef: true, }); const hasConfigSecret = hasConfiguredSecretInput(feishuCfg?.appSecret); - const hasConfigCreds = Boolean(feishuCfg?.appId?.trim() && hasConfigSecret); + const hasConfigCreds = Boolean( + typeof feishuCfg?.appId === "string" && feishuCfg.appId.trim() && hasConfigSecret, + ); const canUseEnv = Boolean( !hasConfigCreds && process.env.FEISHU_APP_ID?.trim() && process.env.FEISHU_APP_SECRET?.trim(), ); @@ -265,7 +298,8 @@ export const feishuOnboardingAdapter: ChannelOnboardingAdapter = { appSecretProbeValue = appSecretResult.resolvedValue; appId = await promptFeishuAppId({ prompter, - initialValue: feishuCfg?.appId?.trim() || process.env.FEISHU_APP_ID?.trim(), + initialValue: + normalizeString(feishuCfg?.appId) ?? normalizeString(process.env.FEISHU_APP_ID), }); }