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
2026-03-04 09:22:50 +08:00
parent 21e8d88c1d
commit b7589b32a8
5 changed files with 422 additions and 25 deletions
--- 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 `<relevant-memories>...</relevant-memories>` 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.
--- 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<FeishuConfig>) => 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();
+  });
 });
--- 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<string, unknown>;
+      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,
--- a/extensions/feishu/src/onboarding.test.ts
+++ 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;
+      }
+    }
+  });
+});
--- 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<string, unknown>;
+      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<string, unknown>).appId)
+        : isAppIdConfigured(feishuCfg?.appId);
+      const accountSecretConfigured = hasOwnAppSecret
+        ? hasConfiguredSecretInput((account as Record<string, unknown>).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),
      });
    }