From bbab94c1fead9a6fb3db44e605140b2edfa15620 Mon Sep 17 00:00:00 2001 From: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> Date: Sun, 1 Mar 2026 20:51:45 -0600 Subject: [PATCH] security(feishu): bind doc create grants to trusted requester context (#31184) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> --- CHANGELOG.md | 1 + extensions/feishu/src/doc-schema.ts | 10 +- extensions/feishu/src/docx.test.ts | 108 ++++++++++-------- extensions/feishu/src/docx.ts | 70 +++++++----- .../openclaw-tools.plugin-context.test.ts | 28 +++++ src/agents/openclaw-tools.ts | 2 + src/plugins/types.ts | 4 + src/security/audit.test.ts | 4 +- src/security/audit.ts | 6 +- 9 files changed, 142 insertions(+), 91 deletions(-) create mode 100644 src/agents/openclaw-tools.plugin-context.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 51f1ddec2d9..d6430f91ea4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,6 +127,7 @@ Docs: https://docs.openclaw.ai - Telegram/Group allowlist ordering: evaluate chat allowlist before sender allowlist enforcement so explicitly allowlisted groups are not fail-closed by empty sender allowlists. Landed from contributor PR #30680 by @openperf. Thanks @openperf. - Telegram/Multi-account group isolation: prevent channel-level `groups` config from leaking across Telegram accounts in multi-account setups, avoiding cross-account group routing drops. Landed from contributor PR #30677 by @YUJIE2002. Thanks @YUJIE2002. - Telegram/Voice caption overflow fallback: recover from `sendVoice` caption length errors by re-sending voice without caption and delivering text separately so replies are not lost. Landed from contributor PR #31131 by @Sid-Qin. Thanks @Sid-Qin. +- Feishu/Doc create permissions: remove caller-controlled owner fields from `feishu_doc` create and bind optional grant behavior to trusted Feishu requester context (`grant_to_requester`), preventing principal selection via tool arguments. (#31184) Thanks @Takhoffman. - Routing/Binding peer-kind parity: treat `peer.kind` `group` and `channel` as equivalent for binding scope matching (while keeping `direct` separate) so Slack/public channel bindings do not silently fall through. Landed from contributor PR #31135 by @Sid-Qin. Thanks @Sid-Qin. - Cron/Store EBUSY fallback: retry `rename` on `EBUSY` and use `copyFile` fallback on Windows when replacing cron store files so busy-file contention no longer causes false write failures. (#16932) Thanks @sudhanva-chakra. - Agents/FS workspace default: honor documented host file-tool default `tools.fs.workspaceOnly=false` when unset so host `write`/`edit` calls are not incorrectly workspace-restricted unless explicitly enabled. Landed from contributor PR #31128 by @SaucePackets. Thanks @SaucePackets. diff --git a/extensions/feishu/src/doc-schema.ts b/extensions/feishu/src/doc-schema.ts index b2c63582868..e2c0a56f23c 100644 --- a/extensions/feishu/src/doc-schema.ts +++ b/extensions/feishu/src/doc-schema.ts @@ -29,12 +29,10 @@ export const FeishuDocSchema = Type.Union([ action: Type.Literal("create"), title: Type.String({ description: "Document title" }), folder_token: Type.Optional(Type.String({ description: "Target folder token (optional)" })), - owner_open_id: Type.Optional( - Type.String({ description: "Open ID of the user to grant ownership permission" }), - ), - owner_perm_type: Type.Optional( - Type.Union([Type.Literal("view"), Type.Literal("edit"), Type.Literal("full_access")], { - description: "Permission type (default: full_access)", + grant_to_requester: Type.Optional( + Type.Boolean({ + description: + "Grant edit permission to the trusted requesting Feishu user from runtime context (default: true).", }), ), }), diff --git a/extensions/feishu/src/docx.test.ts b/extensions/feishu/src/docx.test.ts index 5fdfec208de..665f4309a52 100644 --- a/extensions/feishu/src/docx.test.ts +++ b/extensions/feishu/src/docx.test.ts @@ -340,7 +340,7 @@ describe("feishu_doc image fetch hardening", () => { consoleErrorSpy.mockRestore(); }); - it("reports owner permission details when grant succeeds", async () => { + it("create grants permission only to trusted Feishu requester", async () => { const registerTool = vi.fn(); registerFeishuDocTools({ config: { @@ -357,27 +357,35 @@ describe("feishu_doc image fetch hardening", () => { const feishuDocTool = registerTool.mock.calls .map((call) => call[0]) - .map((tool) => (typeof tool === "function" ? tool({}) : tool)) + .map((tool) => + typeof tool === "function" + ? tool({ messageChannel: "feishu", requesterSenderId: "ou_123" }) + : tool, + ) .find((tool) => tool.name === "feishu_doc"); expect(feishuDocTool).toBeDefined(); const result = await feishuDocTool.execute("tool-call", { action: "create", title: "Demo", - owner_open_id: "ou_123", - owner_perm_type: "edit", }); - expect(permissionMemberCreateMock).toHaveBeenCalled(); - expect(result.details.owner_permission_added).toBe(true); - expect(result.details.owner_open_id).toBe("ou_123"); - expect(result.details.owner_perm_type).toBe("edit"); + expect(result.details.document_id).toBe("doc_created"); + expect(result.details.requester_permission_added).toBe(true); + expect(result.details.requester_open_id).toBe("ou_123"); + expect(result.details.requester_perm_type).toBe("edit"); + expect(permissionMemberCreateMock).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + member_type: "openid", + member_id: "ou_123", + perm: "edit", + }), + }), + ); }); - it("does not report owner permission details when grant fails", async () => { - const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); - permissionMemberCreateMock.mockRejectedValueOnce(new Error("permission denied")); - + it("create skips requester grant when trusted requester identity is unavailable", async () => { const registerTool = vi.fn(); registerFeishuDocTools({ config: { @@ -394,43 +402,7 @@ describe("feishu_doc image fetch hardening", () => { const feishuDocTool = registerTool.mock.calls .map((call) => call[0]) - .map((tool) => (typeof tool === "function" ? tool({}) : tool)) - .find((tool) => tool.name === "feishu_doc"); - expect(feishuDocTool).toBeDefined(); - - const result = await feishuDocTool.execute("tool-call", { - action: "create", - title: "Demo", - owner_open_id: "ou_123", - owner_perm_type: "edit", - }); - - expect(permissionMemberCreateMock).toHaveBeenCalled(); - expect(result.details.owner_permission_added).toBeUndefined(); - expect(result.details.owner_open_id).toBeUndefined(); - expect(result.details.owner_perm_type).toBeUndefined(); - expect(consoleWarnSpy).toHaveBeenCalled(); - consoleWarnSpy.mockRestore(); - }); - - it("skips permission grant when owner_open_id is omitted", async () => { - const registerTool = vi.fn(); - registerFeishuDocTools({ - config: { - channels: { - feishu: { - appId: "app_id", - appSecret: "app_secret", - }, - }, - } as any, - logger: { debug: vi.fn(), info: vi.fn() } as any, - registerTool, - } as any); - - const feishuDocTool = registerTool.mock.calls - .map((call) => call[0]) - .map((tool) => (typeof tool === "function" ? tool({}) : tool)) + .map((tool) => (typeof tool === "function" ? tool({ messageChannel: "feishu" }) : tool)) .find((tool) => tool.name === "feishu_doc"); expect(feishuDocTool).toBeDefined(); @@ -440,7 +412,43 @@ describe("feishu_doc image fetch hardening", () => { }); expect(permissionMemberCreateMock).not.toHaveBeenCalled(); - expect(result.details.owner_permission_added).toBeUndefined(); + expect(result.details.requester_permission_added).toBe(false); + expect(result.details.requester_permission_skipped_reason).toContain("trusted requester"); + }); + + it("create never grants permissions when grant_to_requester is false", async () => { + const registerTool = vi.fn(); + registerFeishuDocTools({ + config: { + channels: { + feishu: { + appId: "app_id", + appSecret: "app_secret", + }, + }, + } as any, + logger: { debug: vi.fn(), info: vi.fn() } as any, + registerTool, + } as any); + + const feishuDocTool = registerTool.mock.calls + .map((call) => call[0]) + .map((tool) => + typeof tool === "function" + ? tool({ messageChannel: "feishu", requesterSenderId: "ou_123" }) + : tool, + ) + .find((tool) => tool.name === "feishu_doc"); + expect(feishuDocTool).toBeDefined(); + + const result = await feishuDocTool.execute("tool-call", { + action: "create", + title: "Demo", + grant_to_requester: false, + }); + + expect(permissionMemberCreateMock).not.toHaveBeenCalled(); + expect(result.details.requester_permission_added).toBeUndefined(); }); it("returns an error when create response omits document_id", async () => { diff --git a/extensions/feishu/src/docx.ts b/extensions/feishu/src/docx.ts index eecbcbbe314..db14e8a91ba 100644 --- a/extensions/feishu/src/docx.ts +++ b/extensions/feishu/src/docx.ts @@ -751,8 +751,7 @@ async function createDoc( client: Lark.Client, title: string, folderToken?: string, - ownerOpenId?: string, - ownerPermType: "view" | "edit" | "full_access" = "full_access", + options?: { grantToRequester?: boolean; requesterOpenId?: string }, ) { const res = await client.docx.document.create({ data: { title, folder_token: folderToken }, @@ -765,23 +764,32 @@ async function createDoc( if (!docToken) { throw new Error("Document creation succeeded but no document_id was returned"); } - let ownerPermissionAdded = false; + const shouldGrantToRequester = options?.grantToRequester !== false; + const requesterOpenId = options?.requesterOpenId?.trim(); + const requesterPermType: "edit" = "edit"; - // Auto add owner permission if ownerOpenId is provided - if (docToken && ownerOpenId) { - try { - await client.drive.permissionMember.create({ - path: { token: docToken }, - params: { type: "docx", need_notification: false }, - data: { - member_type: "openid", - member_id: ownerOpenId, - perm: ownerPermType, - }, - }); - ownerPermissionAdded = true; - } catch (err) { - console.warn("Failed to add owner permission (non-critical):", err); + let requesterPermissionAdded = false; + let requesterPermissionSkippedReason: string | undefined; + let requesterPermissionError: string | undefined; + + if (shouldGrantToRequester) { + if (!requesterOpenId) { + requesterPermissionSkippedReason = "trusted requester identity unavailable"; + } else { + try { + await client.drive.permissionMember.create({ + path: { token: docToken }, + params: { type: "docx", need_notification: false }, + data: { + member_type: "openid", + member_id: requesterOpenId, + perm: requesterPermType, + }, + }); + requesterPermissionAdded = true; + } catch (err) { + requesterPermissionError = err instanceof Error ? err.message : String(err); + } } } @@ -789,12 +797,15 @@ async function createDoc( document_id: docToken, title: doc?.title, url: `https://feishu.cn/docx/${docToken}`, - ...(ownerOpenId && - ownerPermissionAdded && { - owner_permission_added: true, - owner_open_id: ownerOpenId, - owner_perm_type: ownerPermType, + ...(shouldGrantToRequester && { + requester_permission_added: requesterPermissionAdded, + ...(requesterOpenId && { requester_open_id: requesterOpenId }), + requester_perm_type: requesterPermType, + ...(requesterPermissionSkippedReason && { + requester_permission_skipped_reason: requesterPermissionSkippedReason, }), + ...(requesterPermissionError && { requester_permission_error: requesterPermissionError }), + }), }; } @@ -1251,6 +1262,8 @@ export function registerFeishuDocTools(api: OpenClawPluginApi) { api.registerTool( (ctx) => { const defaultAccountId = ctx.agentAccountId; + const trustedRequesterOpenId = + ctx.messageChannel === "feishu" ? ctx.requesterSenderId?.trim() || undefined : undefined; return { name: "feishu_doc", label: "Feishu Doc", @@ -1297,13 +1310,10 @@ export function registerFeishuDocTools(api: OpenClawPluginApi) { ); case "create": return json( - await createDoc( - client, - p.title, - p.folder_token, - p.owner_open_id, - p.owner_perm_type, - ), + await createDoc(client, p.title, p.folder_token, { + grantToRequester: p.grant_to_requester, + requesterOpenId: trustedRequesterOpenId, + }), ); case "list_blocks": return json(await listBlocks(client, p.doc_token)); diff --git a/src/agents/openclaw-tools.plugin-context.test.ts b/src/agents/openclaw-tools.plugin-context.test.ts new file mode 100644 index 00000000000..45c30bf7ece --- /dev/null +++ b/src/agents/openclaw-tools.plugin-context.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it, vi } from "vitest"; + +const resolvePluginToolsMock = vi.fn(() => []); + +vi.mock("../plugins/tools.js", () => ({ + resolvePluginTools: (params: unknown) => resolvePluginToolsMock(params), +})); + +import { createOpenClawTools } from "./openclaw-tools.js"; + +describe("createOpenClawTools plugin context", () => { + it("forwards trusted requester sender identity to plugin tool context", () => { + createOpenClawTools({ + config: {} as never, + requesterSenderId: "trusted-sender", + senderIsOwner: true, + }); + + expect(resolvePluginToolsMock).toHaveBeenCalledWith( + expect.objectContaining({ + context: expect.objectContaining({ + requesterSenderId: "trusted-sender", + senderIsOwner: true, + }), + }), + ); + }); +}); diff --git a/src/agents/openclaw-tools.ts b/src/agents/openclaw-tools.ts index 22140d167a9..9626d68d1af 100644 --- a/src/agents/openclaw-tools.ts +++ b/src/agents/openclaw-tools.ts @@ -187,6 +187,8 @@ export function createOpenClawTools(options?: { sessionKey: options?.agentSessionKey, messageChannel: options?.agentChannel, agentAccountId: options?.agentAccountId, + requesterSenderId: options?.requesterSenderId ?? undefined, + senderIsOwner: options?.senderIsOwner ?? undefined, sandboxed: options?.sandboxed, }, existingToolNames: new Set(tools.map((tool) => tool.name)), diff --git a/src/plugins/types.ts b/src/plugins/types.ts index 2f3ea097ed2..7589c785c70 100644 --- a/src/plugins/types.ts +++ b/src/plugins/types.ts @@ -63,6 +63,10 @@ export type OpenClawPluginToolContext = { sessionKey?: string; messageChannel?: string; agentAccountId?: string; + /** Trusted sender id from inbound context (runtime-provided, not tool args). */ + requesterSenderId?: string; + /** Whether the trusted sender is an owner. */ + senderIsOwner?: boolean; sandboxed?: boolean; }; diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index c92aa40520e..da8abcd9ff2 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -1266,7 +1266,7 @@ describe("security audit", () => { ); }); - it("warns when Feishu doc tool is enabled because create supports owner_open_id", async () => { + it("warns when Feishu doc tool is enabled because create can grant requester access", async () => { const cfg: OpenClawConfig = { channels: { feishu: { @@ -1280,7 +1280,7 @@ describe("security audit", () => { expectFinding(res, "channels.feishu.doc_owner_open_id", "warn"); }); - it("does not warn for Feishu owner_open_id when doc tools are disabled", async () => { + it("does not warn for Feishu doc grant risk when doc tools are disabled", async () => { const cfg: OpenClawConfig = { channels: { feishu: { diff --git a/src/security/audit.ts b/src/security/audit.ts index 1febc036f52..749b0fe6b22 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -519,11 +519,11 @@ function collectGatewayConfigFindings( findings.push({ checkId: "channels.feishu.doc_owner_open_id", severity: "warn", - title: "Feishu doc create can grant owner permissions", + title: "Feishu doc create can grant requester permissions", detail: - 'channels.feishu tools include "doc"; feishu_doc action "create" accepts owner_open_id and can grant document access to that user.', + 'channels.feishu tools include "doc"; feishu_doc action "create" can grant document access to the trusted requesting Feishu user.', remediation: - "Disable channels.feishu.tools.doc when not needed, and restrict tool access so untrusted prompts cannot set owner_open_id.", + "Disable channels.feishu.tools.doc when not needed, and restrict tool access for untrusted prompts.", }); }