diff --git a/CHANGELOG.md b/CHANGELOG.md index f579d47b16f..0ab17a67944 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Docs: https://docs.openclaw.ai - Security/Elevated: match `tools.elevated.allowFrom` against sender identities only (not recipient `ctx.To`), closing a recipient-token bypass for `/elevated` authorization. (#11022) Thanks @coygeek. - Config/Memory: allow `"mistral"` in `agents.defaults.memorySearch.provider` and `agents.defaults.memorySearch.fallback` schema validation. (#14934) Thanks @ThomsenDrake. - Security/Feishu: enforce ID-only allowlist matching for DM/group sender authorization, normalize Feishu ID prefixes during checks, and ignore mutable display names so display-name collisions cannot satisfy allowlist entries. This ships in the next npm release. Thanks @jiseoung for reporting. +- Security/Group policy: harden `channels.*.groups.*.toolsBySender` matching by requiring explicit sender-key types (`id:`, `e164:`, `username:`, `name:`), preventing cross-identifier collisions across mutable/display-name fields while keeping legacy untyped keys on a deprecated ID-only path. This ships in the next npm release. Thanks @jiseoung for reporting. - Feishu/Commands: in group chats, command authorization now falls back to top-level `channels.feishu.allowFrom` when per-group `allowFrom` is not set, so `/command` no longer gets blocked by an unintended empty allowlist. (#23756) - Feishu/Plugins: restore bundled Feishu SDK availability for global installs and strip `openclaw: workspace:*` from plugin `devDependencies` during plugin-version sync so npm-installed Feishu plugins do not fail dependency install. (#23611, #23645, #23603) - Plugins/Install: strip `workspace:*` devDependency entries from copied plugin manifests before `npm install --omit=dev`, preventing `EUNSUPPORTEDPROTOCOL` install failures for npm-published channel plugins (including Feishu and MS Teams). diff --git a/docs/channels/groups.md b/docs/channels/groups.md index 00118c546b5..de848243c9c 100644 --- a/docs/channels/groups.md +++ b/docs/channels/groups.md @@ -254,7 +254,10 @@ Notes: Some channel configs support restricting which tools are available **inside a specific group/room/channel**. - `tools`: allow/deny tools for the whole group. -- `toolsBySender`: per-sender overrides within the group (keys are sender IDs/usernames/emails/phone numbers depending on the channel). Use `"*"` as a wildcard. +- `toolsBySender`: per-sender overrides within the group. + Use explicit key prefixes: + `id:`, `e164:`, `username:`, `name:`, and `"*"` wildcard. + Legacy unprefixed keys are still accepted and matched as `id:` only. Resolution order (most specific wins): @@ -274,7 +277,7 @@ Example (Telegram): "-1001234567890": { tools: { deny: ["exec", "read", "write"] }, toolsBySender: { - "123456789": { alsoAllow: ["exec"] }, + "id:123456789": { alsoAllow: ["exec"] }, }, }, }, diff --git a/docs/channels/irc.md b/docs/channels/irc.md index 56bca6490d3..7496f574c4e 100644 --- a/docs/channels/irc.md +++ b/docs/channels/irc.md @@ -163,7 +163,7 @@ Use `toolsBySender` to apply a stricter policy to `"*"` and a looser one to your "*": { deny: ["group:runtime", "group:fs", "gateway", "nodes", "cron", "browser"], }, - eigen: { + "id:eigen": { deny: ["gateway", "nodes", "cron"], }, }, @@ -176,7 +176,9 @@ Use `toolsBySender` to apply a stricter policy to `"*"` and a looser one to your Notes: -- `toolsBySender` keys can be a nick (e.g. `"eigen"`) or a full hostmask (`"eigen!~eigen@174.127.248.171"`) for stronger identity matching. +- `toolsBySender` keys should use `id:` for IRC sender identity values: + `id:eigen` or `id:eigen!~eigen@174.127.248.171` for stronger matching. +- Legacy unprefixed keys are still accepted and matched as `id:` only. - The first matching sender policy wins; `"*"` is the wildcard fallback. For more on group access vs mention-gating (and how they interact), see: [/channels/groups](/channels/groups). diff --git a/docs/channels/msteams.md b/docs/channels/msteams.md index 2232582610a..d8b9f0af865 100644 --- a/docs/channels/msteams.md +++ b/docs/channels/msteams.md @@ -469,6 +469,8 @@ Key settings (see `/gateway/configuration` for shared channel patterns): - `channels.msteams.teams..channels..requireMention`: per-channel override. - `channels.msteams.teams..channels..tools`: per-channel tool policy overrides (`allow`/`deny`/`alsoAllow`). - `channels.msteams.teams..channels..toolsBySender`: per-channel per-sender tool policy overrides (`"*"` wildcard supported). +- `toolsBySender` keys should use explicit prefixes: + `id:`, `e164:`, `username:`, `name:` (legacy unprefixed keys still map to `id:` only). - `channels.msteams.sharePointSiteId`: SharePoint site ID for file uploads in group chats/channels (see [Sending files in group chats](#sending-files-in-group-chats)). ## Routing & Sessions diff --git a/docs/channels/slack.md b/docs/channels/slack.md index 35ade5d1add..beb79a511fc 100644 --- a/docs/channels/slack.md +++ b/docs/channels/slack.md @@ -191,6 +191,8 @@ For actions/directory reads, user token can be preferred when configured. For wr - `skills` - `systemPrompt` - `tools`, `toolsBySender` + - `toolsBySender` key format: `id:`, `e164:`, `username:`, `name:`, or `"*"` wildcard + (legacy unprefixed keys still map to `id:` only) diff --git a/src/agents/pi-tools-agent-config.test.ts b/src/agents/pi-tools-agent-config.test.ts index 9b84b48815f..f2c7f46b2a8 100644 --- a/src/agents/pi-tools-agent-config.test.ts +++ b/src/agents/pi-tools-agent-config.test.ts @@ -379,7 +379,7 @@ describe("Agent-specific tool filtering", () => { "*": { tools: { allow: ["read"] }, toolsBySender: { - alice: { allow: ["read", "exec"] }, + "id:alice": { allow: ["read", "exec"] }, }, }, }, @@ -417,7 +417,7 @@ describe("Agent-specific tool filtering", () => { groups: { "*": { toolsBySender: { - admin: { allow: ["read", "exec"] }, + "id:admin": { allow: ["read", "exec"] }, }, }, locked: { diff --git a/src/channels/plugins/group-mentions.test.ts b/src/channels/plugins/group-mentions.test.ts index 0d6a6dca24d..a737808a131 100644 --- a/src/channels/plugins/group-mentions.test.ts +++ b/src/channels/plugins/group-mentions.test.ts @@ -20,7 +20,7 @@ const cfg = { requireMention: false, tools: { allow: ["message.send"] }, toolsBySender: { - "user:alice": { allow: ["sessions.list"] }, + "id:user:alice": { allow: ["sessions.list"] }, }, }, "*": { @@ -109,14 +109,14 @@ describe("group mentions (discord)", () => { requireMention: false, tools: { allow: ["message.guild"] }, toolsBySender: { - "user:guild-admin": { allow: ["sessions.list"] }, + "id:user:guild-admin": { allow: ["sessions.list"] }, }, channels: { "123": { requireMention: true, tools: { allow: ["message.channel"] }, toolsBySender: { - "user:channel-admin": { deny: ["exec"] }, + "id:user:channel-admin": { deny: ["exec"] }, }, }, }, diff --git a/src/config/group-policy.test.ts b/src/config/group-policy.test.ts index 6aa584d6969..8151f36363b 100644 --- a/src/config/group-policy.test.ts +++ b/src/config/group-policy.test.ts @@ -1,6 +1,6 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "./config.js"; -import { resolveChannelGroupPolicy } from "./group-policy.js"; +import { resolveChannelGroupPolicy, resolveToolsBySender } from "./group-policy.js"; describe("resolveChannelGroupPolicy", () => { it("fails closed when groupPolicy=allowlist and groups are missing", () => { @@ -90,3 +90,139 @@ describe("resolveChannelGroupPolicy", () => { expect(policy.allowed).toBe(false); }); }); + +describe("resolveToolsBySender", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("matches typed sender IDs", () => { + expect( + resolveToolsBySender({ + toolsBySender: { + "id:user:alice": { allow: ["exec"] }, + "*": { deny: ["exec"] }, + }, + senderId: "user:alice", + }), + ).toEqual({ allow: ["exec"] }); + }); + + it("does not allow senderName collisions to match id keys", () => { + const victimId = "f4ce8a7d-1111-2222-3333-444455556666"; + expect( + resolveToolsBySender({ + toolsBySender: { + [`id:${victimId}`]: { allow: ["exec", "fs.read"] }, + "*": { deny: ["exec"] }, + }, + senderId: "attacker-real-id", + senderName: victimId, + senderUsername: "attacker", + }), + ).toEqual({ deny: ["exec"] }); + }); + + it("treats untyped legacy keys as senderId only", () => { + const warningSpy = vi.spyOn(process, "emitWarning").mockImplementation(() => undefined); + const victimId = "legacy-owner-id"; + expect( + resolveToolsBySender({ + toolsBySender: { + [victimId]: { allow: ["exec"] }, + "*": { deny: ["exec"] }, + }, + senderId: "attacker-real-id", + senderName: victimId, + }), + ).toEqual({ deny: ["exec"] }); + + expect( + resolveToolsBySender({ + toolsBySender: { + [victimId]: { allow: ["exec"] }, + "*": { deny: ["exec"] }, + }, + senderId: victimId, + senderName: "attacker", + }), + ).toEqual({ allow: ["exec"] }); + expect(warningSpy).toHaveBeenCalledTimes(1); + }); + + it("matches username keys only against senderUsername", () => { + expect( + resolveToolsBySender({ + toolsBySender: { + "username:alice": { allow: ["exec"] }, + "*": { deny: ["exec"] }, + }, + senderId: "alice", + senderUsername: "other-user", + }), + ).toEqual({ deny: ["exec"] }); + + expect( + resolveToolsBySender({ + toolsBySender: { + "username:alice": { allow: ["exec"] }, + "*": { deny: ["exec"] }, + }, + senderId: "other-id", + senderUsername: "@alice", + }), + ).toEqual({ allow: ["exec"] }); + }); + + it("matches e164 and name only when explicitly typed", () => { + expect( + resolveToolsBySender({ + toolsBySender: { + "e164:+15550001111": { allow: ["exec"] }, + "name:owner": { deny: ["exec"] }, + }, + senderE164: "+15550001111", + senderName: "owner", + }), + ).toEqual({ allow: ["exec"] }); + }); + + it("prefers id over username over name", () => { + expect( + resolveToolsBySender({ + toolsBySender: { + "id:alice": { deny: ["exec"] }, + "username:alice": { allow: ["exec"] }, + "name:alice": { allow: ["read"] }, + }, + senderId: "alice", + senderUsername: "alice", + senderName: "alice", + }), + ).toEqual({ deny: ["exec"] }); + }); + + it("emits one deprecation warning per legacy key", () => { + const warningSpy = vi.spyOn(process, "emitWarning").mockImplementation(() => undefined); + const legacyKey = "legacy-warning-key"; + const policy = { + [legacyKey]: { allow: ["exec"] }, + "*": { deny: ["exec"] }, + }; + + resolveToolsBySender({ + toolsBySender: policy, + senderId: "other-id", + }); + resolveToolsBySender({ + toolsBySender: policy, + senderId: "other-id", + }); + + expect(warningSpy).toHaveBeenCalledTimes(1); + expect(String(warningSpy.mock.calls[0]?.[0])).toContain(`toolsBySender key "${legacyKey}"`); + expect(warningSpy.mock.calls[0]?.[1]).toMatchObject({ + code: "OPENCLAW_TOOLS_BY_SENDER_UNTYPED_KEY", + }); + }); +}); diff --git a/src/config/group-policy.ts b/src/config/group-policy.ts index a188a824bed..d1f1245701d 100644 --- a/src/config/group-policy.ts +++ b/src/config/group-policy.ts @@ -50,15 +50,140 @@ export type GroupToolPolicySender = { senderE164?: string | null; }; -function normalizeSenderKey(value: string): string { +type SenderKeyType = "id" | "e164" | "username" | "name"; + +const SENDER_KEY_TYPES: SenderKeyType[] = ["id", "e164", "username", "name"]; +const warnedLegacyToolsBySenderKeys = new Set(); + +type ParsedSenderPolicyKey = + | { kind: "wildcard" } + | { kind: "typed"; type: SenderKeyType; key: string }; + +type SenderPolicyBuckets = Record>; + +function normalizeSenderKey( + value: string, + options: { + stripLeadingAt?: boolean; + } = {}, +): string { const trimmed = value.trim(); if (!trimmed) { return ""; } - const withoutAt = trimmed.startsWith("@") ? trimmed.slice(1) : trimmed; + const withoutAt = options.stripLeadingAt && trimmed.startsWith("@") ? trimmed.slice(1) : trimmed; return withoutAt.toLowerCase(); } +function normalizeTypedSenderKey(value: string, type: SenderKeyType): string { + return normalizeSenderKey(value, { + stripLeadingAt: type === "username", + }); +} + +function normalizeLegacySenderKey(value: string): string { + return normalizeSenderKey(value, { + stripLeadingAt: true, + }); +} + +function parseTypedSenderKey(rawKey: string): { type: SenderKeyType; value: string } | undefined { + const lowered = rawKey.toLowerCase(); + for (const type of SENDER_KEY_TYPES) { + const prefix = `${type}:`; + if (!lowered.startsWith(prefix)) { + continue; + } + return { + type, + value: rawKey.slice(prefix.length), + }; + } + return undefined; +} + +function warnLegacyToolsBySenderKey(rawKey: string) { + const trimmed = rawKey.trim(); + if (!trimmed || warnedLegacyToolsBySenderKeys.has(trimmed)) { + return; + } + warnedLegacyToolsBySenderKeys.add(trimmed); + process.emitWarning( + `toolsBySender key "${trimmed}" is deprecated. Use explicit prefixes (id:, e164:, username:, name:). Legacy unprefixed keys are matched as id only.`, + { + type: "DeprecationWarning", + code: "OPENCLAW_TOOLS_BY_SENDER_UNTYPED_KEY", + }, + ); +} + +function parseSenderPolicyKey(rawKey: string): ParsedSenderPolicyKey | undefined { + const trimmed = rawKey.trim(); + if (!trimmed) { + return undefined; + } + if (trimmed === "*") { + return { kind: "wildcard" }; + } + const typed = parseTypedSenderKey(trimmed); + if (typed) { + const key = normalizeTypedSenderKey(typed.value, typed.type); + if (!key) { + return undefined; + } + return { + kind: "typed", + type: typed.type, + key, + }; + } + + // Backward-compatible fallback: untyped keys now map to immutable sender IDs only. + warnLegacyToolsBySenderKey(trimmed); + const key = normalizeLegacySenderKey(trimmed); + if (!key) { + return undefined; + } + return { + kind: "typed", + type: "id", + key, + }; +} + +function createSenderPolicyBuckets(): SenderPolicyBuckets { + return { + id: new Map(), + e164: new Map(), + username: new Map(), + name: new Map(), + }; +} + +function normalizeCandidate(value: string | null | undefined, type: SenderKeyType): string { + const trimmed = value?.trim(); + if (!trimmed) { + return ""; + } + return normalizeTypedSenderKey(trimmed, type); +} + +function normalizeSenderIdCandidates(value: string | null | undefined): string[] { + const trimmed = value?.trim(); + if (!trimmed) { + return []; + } + const typed = normalizeTypedSenderKey(trimmed, "id"); + const legacy = normalizeLegacySenderKey(trimmed); + if (!typed) { + return legacy ? [legacy] : []; + } + if (!legacy || legacy === typed) { + return [typed]; + } + return [typed, legacy]; +} + export function resolveToolsBySender( params: { toolsBySender?: GroupToolPolicyBySenderConfig; @@ -73,44 +198,49 @@ export function resolveToolsBySender( return undefined; } - const normalized = new Map(); + const buckets = createSenderPolicyBuckets(); let wildcard: GroupToolPolicyConfig | undefined; for (const [rawKey, policy] of entries) { if (!policy) { continue; } - const key = normalizeSenderKey(rawKey); - if (!key) { + const parsed = parseSenderPolicyKey(rawKey); + if (!parsed) { continue; } - if (key === "*") { + if (parsed.kind === "wildcard") { wildcard = policy; continue; } - if (!normalized.has(key)) { - normalized.set(key, policy); + const bucket = buckets[parsed.type]; + if (!bucket.has(parsed.key)) { + bucket.set(parsed.key, policy); } } - const candidates: string[] = []; - const pushCandidate = (value?: string | null) => { - const trimmed = value?.trim(); - if (!trimmed) { - return; + for (const senderIdCandidate of normalizeSenderIdCandidates(params.senderId)) { + const match = buckets.id.get(senderIdCandidate); + if (match) { + return match; } - candidates.push(trimmed); - }; - pushCandidate(params.senderId); - pushCandidate(params.senderE164); - pushCandidate(params.senderUsername); - pushCandidate(params.senderName); - - for (const candidate of candidates) { - const key = normalizeSenderKey(candidate); - if (!key) { - continue; + } + const senderE164 = normalizeCandidate(params.senderE164, "e164"); + if (senderE164) { + const match = buckets.e164.get(senderE164); + if (match) { + return match; } - const match = normalized.get(key); + } + const senderUsername = normalizeCandidate(params.senderUsername, "username"); + if (senderUsername) { + const match = buckets.username.get(senderUsername); + if (match) { + return match; + } + } + const senderName = normalizeCandidate(params.senderName, "name"); + if (senderName) { + const match = buckets.name.get(senderName); if (match) { return match; } diff --git a/src/config/types.tools.ts b/src/config/types.tools.ts index 1cf81f771ac..c6aeba12949 100644 --- a/src/config/types.tools.ts +++ b/src/config/types.tools.ts @@ -176,6 +176,18 @@ export type GroupToolPolicyConfig = { deny?: string[]; }; +/** + * Per-sender overrides. + * + * Prefer explicit key prefixes: + * - id: + * - e164: + * - username: + * - name: + * - * (wildcard) + * + * Legacy unprefixed keys are supported for backward compatibility and are matched as senderId only. + */ export type GroupToolPolicyBySenderConfig = Record; export type ExecToolConfig = {