diff --git a/CHANGELOG.md b/CHANGELOG.md index 08e89fb20..696a1b60e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,7 @@ Docs: https://docs.openclaw.ai - Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin. - Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek. - Security/Skills: harden skill installer metadata parsing by rejecting unsafe installer specs (brew/node/go/uv/download) and constrain plugin-declared skill directories to the plugin root (including symlink-escape checks), with regression coverage. +- Discord/DM command auth: unify DM allowlist + pairing-store authorization across message preflight and native command interactions so DM command gating is consistent for `open`/`pairing`/`allowlist` policies. - ACP/Harness thread spawn routing: force ACP harness thread creation through `sessions_spawn` (`runtime: "acp"`, `thread: true`) and explicitly forbid `message action=thread-create` for ACP harness requests, avoiding misrouted `Unknown channel` errors. (#30957) Thanks @dutifulbob. - Docs/ACP permissions: document the correct `permissionMode` default (`approve-reads`) and clarify non-interactive permission failure behavior/troubleshooting guidance. (#31044) Thanks @barronlroth. - Security/Logging utility hardening: remove `eval`-based command execution from `scripts/clawlog.sh`, switch to argv-safe command construction, and escape predicate literals for user-supplied search/category filters to block local command/predicate injection paths. diff --git a/src/discord/monitor/dm-command-auth.test.ts b/src/discord/monitor/dm-command-auth.test.ts new file mode 100644 index 000000000..041eb82b4 --- /dev/null +++ b/src/discord/monitor/dm-command-auth.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it } from "vitest"; +import { resolveDiscordDmCommandAccess } from "./dm-command-auth.js"; + +describe("resolveDiscordDmCommandAccess", () => { + const sender = { + id: "123", + name: "alice", + tag: "alice#0001", + }; + + it("allows open DMs and keeps command auth enabled without allowlist entries", async () => { + const result = await resolveDiscordDmCommandAccess({ + accountId: "default", + dmPolicy: "open", + configuredAllowFrom: [], + sender, + allowNameMatching: false, + useAccessGroups: true, + readStoreAllowFrom: async () => [], + }); + + expect(result.decision).toBe("allow"); + expect(result.commandAuthorized).toBe(true); + }); + + it("marks command auth true when sender is allowlisted", async () => { + const result = await resolveDiscordDmCommandAccess({ + accountId: "default", + dmPolicy: "open", + configuredAllowFrom: ["discord:123"], + sender, + allowNameMatching: false, + useAccessGroups: true, + readStoreAllowFrom: async () => [], + }); + + expect(result.decision).toBe("allow"); + expect(result.commandAuthorized).toBe(true); + }); + + it("returns pairing decision and unauthorized command auth for unknown senders", async () => { + const result = await resolveDiscordDmCommandAccess({ + accountId: "default", + dmPolicy: "pairing", + configuredAllowFrom: ["discord:456"], + sender, + allowNameMatching: false, + useAccessGroups: true, + readStoreAllowFrom: async () => [], + }); + + expect(result.decision).toBe("pairing"); + expect(result.commandAuthorized).toBe(false); + }); + + it("authorizes sender from pairing-store allowlist entries", async () => { + const result = await resolveDiscordDmCommandAccess({ + accountId: "default", + dmPolicy: "pairing", + configuredAllowFrom: [], + sender, + allowNameMatching: false, + useAccessGroups: true, + readStoreAllowFrom: async () => ["discord:123"], + }); + + expect(result.decision).toBe("allow"); + expect(result.commandAuthorized).toBe(true); + }); + + it("keeps open DM command auth true when access groups are disabled", async () => { + const result = await resolveDiscordDmCommandAccess({ + accountId: "default", + dmPolicy: "open", + configuredAllowFrom: [], + sender, + allowNameMatching: false, + useAccessGroups: false, + readStoreAllowFrom: async () => [], + }); + + expect(result.decision).toBe("allow"); + expect(result.commandAuthorized).toBe(true); + }); +}); diff --git a/src/discord/monitor/dm-command-auth.ts b/src/discord/monitor/dm-command-auth.ts new file mode 100644 index 000000000..45700abe3 --- /dev/null +++ b/src/discord/monitor/dm-command-auth.ts @@ -0,0 +1,87 @@ +import { resolveCommandAuthorizedFromAuthorizers } from "../../channels/command-gating.js"; +import { + readStoreAllowFromForDmPolicy, + resolveDmGroupAccessWithLists, + type DmGroupAccessDecision, +} from "../../security/dm-policy-shared.js"; +import { normalizeDiscordAllowList, resolveDiscordAllowListMatch } from "./allow-list.js"; + +const DISCORD_ALLOW_LIST_PREFIXES = ["discord:", "user:", "pk:"]; + +export type DiscordDmPolicy = "open" | "pairing" | "allowlist" | "disabled"; + +export type DiscordDmCommandAccess = { + decision: DmGroupAccessDecision; + reason: string; + commandAuthorized: boolean; + allowMatch: ReturnType | { allowed: false }; +}; + +export async function resolveDiscordDmCommandAccess(params: { + accountId: string; + dmPolicy: DiscordDmPolicy; + configuredAllowFrom: string[]; + sender: { id: string; name?: string; tag?: string }; + allowNameMatching: boolean; + useAccessGroups: boolean; + readStoreAllowFrom?: () => Promise; +}): Promise { + const storeAllowFrom = params.readStoreAllowFrom + ? await params.readStoreAllowFrom().catch(() => []) + : await readStoreAllowFromForDmPolicy({ + provider: "discord", + accountId: params.accountId, + dmPolicy: params.dmPolicy, + }); + + const access = resolveDmGroupAccessWithLists({ + isGroup: false, + dmPolicy: params.dmPolicy, + allowFrom: params.configuredAllowFrom, + groupAllowFrom: [], + storeAllowFrom, + isSenderAllowed: (allowEntries) => { + const allowList = normalizeDiscordAllowList(allowEntries, DISCORD_ALLOW_LIST_PREFIXES); + const allowMatch = allowList + ? resolveDiscordAllowListMatch({ + allowList, + candidate: params.sender, + allowNameMatching: params.allowNameMatching, + }) + : { allowed: false }; + return allowMatch.allowed; + }, + }); + + const commandAllowList = normalizeDiscordAllowList( + access.effectiveAllowFrom, + DISCORD_ALLOW_LIST_PREFIXES, + ); + const allowMatch = commandAllowList + ? resolveDiscordAllowListMatch({ + allowList: commandAllowList, + candidate: params.sender, + allowNameMatching: params.allowNameMatching, + }) + : { allowed: false }; + + const commandAuthorized = resolveCommandAuthorizedFromAuthorizers({ + useAccessGroups: params.useAccessGroups, + authorizers: [ + { + configured: access.effectiveAllowFrom.length > 0, + allowed: allowMatch.allowed, + }, + ], + modeWhenAccessGroupsOff: "configured", + }); + const effectiveCommandAuthorized = + access.decision === "allow" && params.dmPolicy === "open" ? true : commandAuthorized; + + return { + decision: access.decision, + reason: access.reason, + commandAuthorized: effectiveCommandAuthorized, + allowMatch, + }; +} diff --git a/src/discord/monitor/message-handler.preflight.ts b/src/discord/monitor/message-handler.preflight.ts index 2777ba01b..5a13bb1b6 100644 --- a/src/discord/monitor/message-handler.preflight.ts +++ b/src/discord/monitor/message-handler.preflight.ts @@ -28,7 +28,6 @@ import { buildPairingReply } from "../../pairing/pairing-messages.js"; import { upsertChannelPairingRequest } from "../../pairing/pairing-store.js"; import { resolveAgentRoute } from "../../routing/resolve-route.js"; import { DEFAULT_ACCOUNT_ID, resolveAgentIdFromSessionKey } from "../../routing/session-key.js"; -import { readStoreAllowFromForDmPolicy } from "../../security/dm-policy-shared.js"; import { fetchPluralKitMessageInfo } from "../pluralkit.js"; import { sendMessageDiscord } from "../send.js"; import { @@ -36,13 +35,13 @@ import { isDiscordGroupAllowedByPolicy, normalizeDiscordAllowList, normalizeDiscordSlug, - resolveDiscordAllowListMatch, resolveDiscordChannelConfigWithFallback, resolveDiscordGuildEntry, resolveDiscordMemberAccessState, resolveDiscordShouldRequireMention, resolveGroupDmAllow, } from "./allow-list.js"; +import { resolveDiscordDmCommandAccess } from "./dm-command-auth.js"; import { formatDiscordUserTag, resolveDiscordSystemLocation, @@ -174,6 +173,7 @@ export async function preflightDiscordMessage( } const dmPolicy = params.discordConfig?.dmPolicy ?? params.discordConfig?.dm?.policy ?? "pairing"; + const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; const resolvedAccountId = params.accountId ?? DEFAULT_ACCOUNT_ID; let commandAuthorized = true; if (isDirectMessage) { @@ -181,69 +181,61 @@ export async function preflightDiscordMessage( logVerbose("discord: drop dm (dmPolicy: disabled)"); return null; } - if (dmPolicy !== "open") { - const storeAllowFrom = await readStoreAllowFromForDmPolicy({ - provider: "discord", - accountId: resolvedAccountId, - dmPolicy, - }); - const effectiveAllowFrom = [...(params.allowFrom ?? []), ...storeAllowFrom]; - const allowList = normalizeDiscordAllowList(effectiveAllowFrom, ["discord:", "user:", "pk:"]); - const allowMatch = allowList - ? resolveDiscordAllowListMatch({ - allowList, - candidate: { - id: sender.id, - name: sender.name, - tag: sender.tag, - }, - allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig), - }) - : { allowed: false }; - const allowMatchMeta = formatAllowlistMatchMeta(allowMatch); - const permitted = allowMatch.allowed; - if (!permitted) { - commandAuthorized = false; - if (dmPolicy === "pairing") { - const { code, created } = await upsertChannelPairingRequest({ - channel: "discord", - id: author.id, - accountId: resolvedAccountId, - meta: { - tag: formatDiscordUserTag(author), - name: author.username ?? undefined, - }, - }); - if (created) { - logVerbose( - `discord pairing request sender=${author.id} tag=${formatDiscordUserTag(author)} (${allowMatchMeta})`, - ); - try { - await sendMessageDiscord( - `user:${author.id}`, - buildPairingReply({ - channel: "discord", - idLine: `Your Discord user id: ${author.id}`, - code, - }), - { - token: params.token, - rest: params.client.rest, - accountId: params.accountId, - }, - ); - } catch (err) { - logVerbose(`discord pairing reply failed for ${author.id}: ${String(err)}`); - } - } - } else { + const dmAccess = await resolveDiscordDmCommandAccess({ + accountId: resolvedAccountId, + dmPolicy, + configuredAllowFrom: params.allowFrom ?? [], + sender: { + id: sender.id, + name: sender.name, + tag: sender.tag, + }, + allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig), + useAccessGroups, + }); + commandAuthorized = dmAccess.commandAuthorized; + if (dmAccess.decision !== "allow") { + const allowMatchMeta = formatAllowlistMatchMeta( + dmAccess.allowMatch.allowed ? dmAccess.allowMatch : undefined, + ); + if (dmAccess.decision === "pairing") { + const { code, created } = await upsertChannelPairingRequest({ + channel: "discord", + id: author.id, + accountId: resolvedAccountId, + meta: { + tag: formatDiscordUserTag(author), + name: author.username ?? undefined, + }, + }); + if (created) { logVerbose( - `Blocked unauthorized discord sender ${sender.id} (dmPolicy=${dmPolicy}, ${allowMatchMeta})`, + `discord pairing request sender=${author.id} tag=${formatDiscordUserTag(author)} (${allowMatchMeta})`, ); + try { + await sendMessageDiscord( + `user:${author.id}`, + buildPairingReply({ + channel: "discord", + idLine: `Your Discord user id: ${author.id}`, + code, + }), + { + token: params.token, + rest: params.client.rest, + accountId: params.accountId, + }, + ); + } catch (err) { + logVerbose(`discord pairing reply failed for ${author.id}: ${String(err)}`); + } } - return null; + } else { + logVerbose( + `Blocked unauthorized discord sender ${sender.id} (dmPolicy=${dmPolicy}, ${allowMatchMeta})`, + ); } - commandAuthorized = true; + return null; } } @@ -598,7 +590,6 @@ export async function preflightDiscordMessage( { allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig) }, ) : false; - const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; const commandGate = resolveControlCommandGate({ useAccessGroups, authorizers: [ diff --git a/src/discord/monitor/native-command.ts b/src/discord/monitor/native-command.ts index feeb89f2d..3a21118c5 100644 --- a/src/discord/monitor/native-command.ts +++ b/src/discord/monitor/native-command.ts @@ -50,7 +50,6 @@ import { upsertChannelPairingRequest } from "../../pairing/pairing-store.js"; import { resolveAgentRoute } from "../../routing/resolve-route.js"; import { resolveAgentIdFromSessionKey } from "../../routing/session-key.js"; import { buildUntrustedChannelMetadata } from "../../security/channel-metadata.js"; -import { readStoreAllowFromForDmPolicy } from "../../security/dm-policy-shared.js"; import { chunkItems } from "../../utils/chunk-items.js"; import { withTimeout } from "../../utils/with-timeout.js"; import { loadWebMedia } from "../../web/media.js"; @@ -65,6 +64,7 @@ import { resolveDiscordMemberAccessState, resolveDiscordOwnerAllowFrom, } from "./allow-list.js"; +import { resolveDiscordDmCommandAccess } from "./dm-command-auth.js"; import { resolveDiscordChannelInfo } from "./message-utils.js"; import { readDiscordModelPickerRecentModels, @@ -1357,56 +1357,44 @@ async function dispatchDiscordCommandInteraction(params: { await respond("Discord DMs are disabled."); return; } - if (dmPolicy !== "open") { - const storeAllowFrom = await readStoreAllowFromForDmPolicy({ - provider: "discord", - accountId, - dmPolicy, - }); - const effectiveAllowFrom = [ - ...(discordConfig?.allowFrom ?? discordConfig?.dm?.allowFrom ?? []), - ...storeAllowFrom, - ]; - const allowList = normalizeDiscordAllowList(effectiveAllowFrom, ["discord:", "user:", "pk:"]); - const permitted = allowList - ? allowListMatches( - allowList, - { - id: sender.id, - name: sender.name, - tag: sender.tag, - }, - { allowNameMatching: isDangerousNameMatchingEnabled(discordConfig) }, - ) - : false; - if (!permitted) { - commandAuthorized = false; - if (dmPolicy === "pairing") { - const { code, created } = await upsertChannelPairingRequest({ - channel: "discord", - id: user.id, - accountId, - meta: { - tag: sender.tag, - name: sender.name, - }, - }); - if (created) { - await respond( - buildPairingReply({ - channel: "discord", - idLine: `Your Discord user id: ${user.id}`, - code, - }), - { ephemeral: true }, - ); - } - } else { - await respond("You are not authorized to use this command.", { ephemeral: true }); + const dmAccess = await resolveDiscordDmCommandAccess({ + accountId, + dmPolicy, + configuredAllowFrom: discordConfig?.allowFrom ?? discordConfig?.dm?.allowFrom ?? [], + sender: { + id: sender.id, + name: sender.name, + tag: sender.tag, + }, + allowNameMatching: isDangerousNameMatchingEnabled(discordConfig), + useAccessGroups, + }); + commandAuthorized = dmAccess.commandAuthorized; + if (dmAccess.decision !== "allow") { + if (dmAccess.decision === "pairing") { + const { code, created } = await upsertChannelPairingRequest({ + channel: "discord", + id: user.id, + accountId, + meta: { + tag: sender.tag, + name: sender.name, + }, + }); + if (created) { + await respond( + buildPairingReply({ + channel: "discord", + idLine: `Your Discord user id: ${user.id}`, + code, + }), + { ephemeral: true }, + ); } - return; + } else { + await respond("You are not authorized to use this command.", { ephemeral: true }); } - commandAuthorized = true; + return; } } if (!isDirectMessage) {