fix(security): keep DM pairing allowlists out of group auth

This commit is contained in:
Peter Steinberger
2026-02-26 12:58:06 +01:00
parent d08dafb08f
commit 8bdda7a651
15 changed files with 194 additions and 54 deletions

View File

@@ -95,6 +95,7 @@ Docs: https://docs.openclaw.ai
- Security/Slack member + message subtype events: gate `member_*` plus `message_changed`/`message_deleted`/`thread_broadcast` system-event enqueue through shared sender authorization so DM `dmPolicy`/`allowFrom` and channel `users` allowlists are enforced consistently for non-message ingress; message subtype system events now fail closed when sender identity is missing, with regression coverage. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Telegram group allowlist: fail closed for group sender authorization by removing DM pairing-store fallback from group allowlist evaluation; group sender access now requires explicit `groupAllowFrom` or per-group/per-topic `allowFrom`. (#25988) Thanks @bmendonca3.
- Security/DM-group allowlist boundaries: keep DM pairing-store approvals DM-only by removing pairing-store inheritance from group sender authorization in LINE and Mattermost message preflight, and by centralizing shared DM/group allowlist composition so group checks never include pairing-store entries. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Slack interactions: enforce channel/DM authorization and modal actor binding (`private_metadata.userId`) before enqueueing `block_action`/`view_submission`/`view_closed` system events, with regression coverage for unauthorized senders and missing/mismatched actor metadata. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
- Security/Nextcloud Talk: drop replayed signed webhook events with persistent per-account replay dedupe across restarts, and reject unexpected webhook backend origins when account base URL is configured. Thanks @aristorechina for reporting.
- Security/Nextcloud Talk: reject unsigned webhook traffic before full body reads, reducing unauthenticated request-body exposure, with auth-order regression coverage. (#26118) Thanks @bmendonca3.

View File

@@ -184,6 +184,7 @@ Notes:
- `groupPolicy` is separate from mention-gating (which requires @mentions).
- WhatsApp/Telegram/Signal/iMessage/Microsoft Teams/Zalo: use `groupAllowFrom` (fallback: explicit `allowFrom`).
- DM pairing approvals (`*-allowFrom` store entries) apply to DM access only; group sender authorization stays explicit to group allowlists.
- Discord: allowlist uses `channels.discord.guilds.<id>.channels`.
- Slack: allowlist uses `channels.slack.channels`.
- Matrix: allowlist uses `channels.matrix.groups` (room IDs, aliases, or names). Use `channels.matrix.groupAllowFrom` to restrict senders; per-room `users` allowlists are also supported.

View File

@@ -0,0 +1,37 @@
import { describe, expect, it } from "vitest";
import { resolveMattermostEffectiveAllowFromLists } from "./monitor.js";
describe("mattermost monitor authz", () => {
it("keeps DM allowlist merged with pairing-store entries", () => {
const resolved = resolveMattermostEffectiveAllowFromLists({
dmPolicy: "pairing",
allowFrom: ["@trusted-user"],
groupAllowFrom: ["@group-owner"],
storeAllowFrom: ["user:attacker"],
});
expect(resolved.effectiveAllowFrom).toEqual(["trusted-user", "attacker"]);
});
it("uses explicit groupAllowFrom without pairing-store inheritance", () => {
const resolved = resolveMattermostEffectiveAllowFromLists({
dmPolicy: "pairing",
allowFrom: ["@trusted-user"],
groupAllowFrom: ["@group-owner"],
storeAllowFrom: ["user:attacker"],
});
expect(resolved.effectiveGroupAllowFrom).toEqual(["group-owner"]);
});
it("does not inherit pairing-store entries into group allowlist", () => {
const resolved = resolveMattermostEffectiveAllowFromLists({
dmPolicy: "pairing",
allowFrom: ["@trusted-user"],
storeAllowFrom: ["user:attacker"],
});
expect(resolved.effectiveAllowFrom).toEqual(["trusted-user", "attacker"]);
expect(resolved.effectiveGroupAllowFrom).toEqual(["trusted-user"]);
});
});

View File

@@ -18,6 +18,7 @@ import {
isDangerousNameMatchingEnabled,
resolveControlCommandGate,
resolveDmGroupAccessWithLists,
resolveEffectiveAllowFromLists,
resolveAllowlistProviderRuntimeGroupPolicy,
resolveDefaultGroupPolicy,
resolveChannelMediaMaxBytes,
@@ -150,6 +151,23 @@ function normalizeAllowList(entries: Array<string | number>): string[] {
return Array.from(new Set(normalized));
}
export function resolveMattermostEffectiveAllowFromLists(params: {
allowFrom?: Array<string | number> | null;
groupAllowFrom?: Array<string | number> | null;
storeAllowFrom?: Array<string | number> | null;
dmPolicy?: string | null;
}): {
effectiveAllowFrom: string[];
effectiveGroupAllowFrom: string[];
} {
return resolveEffectiveAllowFromLists({
allowFrom: normalizeAllowList(params.allowFrom ?? []),
groupAllowFrom: normalizeAllowList(params.groupAllowFrom ?? []),
storeAllowFrom: normalizeAllowList(params.storeAllowFrom ?? []),
dmPolicy: params.dmPolicy,
});
}
function isSenderAllowed(params: {
senderId: string;
senderName?: string;
@@ -400,20 +418,18 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
senderId;
const rawText = post.message?.trim() || "";
const dmPolicy = account.config.dmPolicy ?? "pairing";
const configAllowFrom = normalizeAllowList(account.config.allowFrom ?? []);
const configGroupAllowFrom = normalizeAllowList(account.config.groupAllowFrom ?? []);
const storeAllowFrom = normalizeAllowList(
dmPolicy === "allowlist"
? []
: await core.channel.pairing.readAllowFromStore("mattermost").catch(() => []),
);
const effectiveAllowFrom = Array.from(new Set([...configAllowFrom, ...storeAllowFrom]));
const effectiveGroupAllowFrom = Array.from(
new Set([
...(configGroupAllowFrom.length > 0 ? configGroupAllowFrom : configAllowFrom),
...storeAllowFrom,
]),
);
const { effectiveAllowFrom, effectiveGroupAllowFrom } =
resolveMattermostEffectiveAllowFromLists({
dmPolicy,
allowFrom: account.config.allowFrom,
groupAllowFrom: account.config.groupAllowFrom,
storeAllowFrom,
});
const allowTextCommands = core.channel.commands.shouldHandleTextCommands({
cfg,
surface: "mattermost",

View File

@@ -1,10 +1,15 @@
import { describe, expect, it } from "vitest";
import { firstDefined, isSenderIdAllowed, mergeAllowFromSources } from "./allow-from.js";
import {
firstDefined,
isSenderIdAllowed,
mergeDmAllowFromSources,
resolveGroupAllowFromSources,
} from "./allow-from.js";
describe("mergeAllowFromSources", () => {
describe("mergeDmAllowFromSources", () => {
it("merges, trims, and filters empty values", () => {
expect(
mergeAllowFromSources({
mergeDmAllowFromSources({
allowFrom: [" line:user:abc ", "", 123],
storeAllowFrom: [" ", "telegram:456"],
}),
@@ -13,7 +18,7 @@ describe("mergeAllowFromSources", () => {
it("excludes pairing-store entries when dmPolicy is allowlist", () => {
expect(
mergeAllowFromSources({
mergeDmAllowFromSources({
allowFrom: ["+1111"],
storeAllowFrom: ["+2222", "+3333"],
dmPolicy: "allowlist",
@@ -23,7 +28,7 @@ describe("mergeAllowFromSources", () => {
it("keeps pairing-store entries for non-allowlist policies", () => {
expect(
mergeAllowFromSources({
mergeDmAllowFromSources({
allowFrom: ["+1111"],
storeAllowFrom: ["+2222"],
dmPolicy: "pairing",
@@ -32,6 +37,26 @@ describe("mergeAllowFromSources", () => {
});
});
describe("resolveGroupAllowFromSources", () => {
it("prefers explicit group allowlist", () => {
expect(
resolveGroupAllowFromSources({
allowFrom: ["owner"],
groupAllowFrom: ["group-owner", " group-admin "],
}),
).toEqual(["group-owner", "group-admin"]);
});
it("falls back to DM allowlist when group allowlist is unset/empty", () => {
expect(
resolveGroupAllowFromSources({
allowFrom: [" owner ", "", "owner2"],
groupAllowFrom: [],
}),
).toEqual(["owner", "owner2"]);
});
});
describe("firstDefined", () => {
it("returns the first non-undefined value", () => {
expect(firstDefined(undefined, undefined, "x", "y")).toBe("x");

View File

@@ -1,6 +1,6 @@
export function mergeAllowFromSources(params: {
export function mergeDmAllowFromSources(params: {
allowFrom?: Array<string | number>;
storeAllowFrom?: string[];
storeAllowFrom?: Array<string | number>;
dmPolicy?: string;
}): string[] {
const storeEntries = params.dmPolicy === "allowlist" ? [] : (params.storeAllowFrom ?? []);
@@ -9,6 +9,17 @@ export function mergeAllowFromSources(params: {
.filter(Boolean);
}
export function resolveGroupAllowFromSources(params: {
allowFrom?: Array<string | number>;
groupAllowFrom?: Array<string | number>;
}): string[] {
const scoped =
params.groupAllowFrom && params.groupAllowFrom.length > 0
? params.groupAllowFrom
: (params.allowFrom ?? []);
return scoped.map((value) => String(value).trim()).filter(Boolean);
}
export function firstDefined<T>(...values: Array<T | undefined>) {
for (const value of values) {
if (typeof value !== "undefined") {

View File

@@ -1,4 +1,8 @@
import { firstDefined, isSenderIdAllowed, mergeAllowFromSources } from "../channels/allow-from.js";
import {
firstDefined,
isSenderIdAllowed,
mergeDmAllowFromSources,
} from "../channels/allow-from.js";
export type NormalizedAllowFrom = {
entries: string[];
@@ -27,11 +31,11 @@ export const normalizeAllowFrom = (list?: Array<string | number>): NormalizedAll
};
};
export const normalizeAllowFromWithStore = (params: {
export const normalizeDmAllowFromWithStore = (params: {
allowFrom?: Array<string | number>;
storeAllowFrom?: string[];
dmPolicy?: string;
}): NormalizedAllowFrom => normalizeAllowFrom(mergeAllowFromSources(params));
}): NormalizedAllowFrom => normalizeAllowFrom(mergeDmAllowFromSources(params));
export const isSenderAllowed = (params: {
allow: NormalizedAllowFrom;

View File

@@ -182,6 +182,41 @@ describe("handleLineWebhookEvents", () => {
expect(processMessage).toHaveBeenCalledTimes(1);
});
it("blocks group sender that is only present in pairing-store allowlist", async () => {
const processMessage = vi.fn();
readAllowFromStoreMock.mockResolvedValueOnce(["user-paired"]);
const event = {
type: "message",
message: { id: "m3b", type: "text", text: "hi" },
replyToken: "reply-token",
timestamp: Date.now(),
source: { type: "group", groupId: "group-1", userId: "user-paired" },
mode: "active",
webhookEventId: "evt-3b",
deliveryContext: { isRedelivery: false },
} as MessageEvent;
await handleLineWebhookEvents([event], {
cfg: {
channels: { line: { groupPolicy: "allowlist", groupAllowFrom: ["user-owner"] } },
},
account: {
accountId: "default",
enabled: true,
channelAccessToken: "token",
channelSecret: "secret",
tokenSource: "config",
config: { groupPolicy: "allowlist", groupAllowFrom: ["user-owner"] },
},
runtime: createRuntime(),
mediaMaxBytes: 1,
processMessage,
});
expect(buildLineMessageContextMock).not.toHaveBeenCalled();
expect(processMessage).not.toHaveBeenCalled();
});
it("blocks group messages when wildcard group config disables groups", async () => {
const processMessage = vi.fn();
const event = {

View File

@@ -21,7 +21,12 @@ import {
upsertChannelPairingRequest,
} from "../pairing/pairing-store.js";
import type { RuntimeEnv } from "../runtime.js";
import { firstDefined, isSenderAllowed, normalizeAllowFromWithStore } from "./bot-access.js";
import {
firstDefined,
isSenderAllowed,
normalizeAllowFrom,
normalizeDmAllowFromWithStore,
} from "./bot-access.js";
import {
getLineSourceInfo,
buildLineMessageContext,
@@ -117,7 +122,7 @@ async function shouldProcessLineEvent(
const dmPolicy = account.config.dmPolicy ?? "pairing";
const storeAllowFrom = await readChannelAllowFromStore("line").catch(() => []);
const effectiveDmAllow = normalizeAllowFromWithStore({
const effectiveDmAllow = normalizeDmAllowFromWithStore({
allowFrom: account.config.allowFrom,
storeAllowFrom,
dmPolicy,
@@ -132,11 +137,9 @@ async function shouldProcessLineEvent(
account.config.groupAllowFrom,
fallbackGroupAllowFrom,
);
const effectiveGroupAllow = normalizeAllowFromWithStore({
allowFrom: groupAllowFrom,
storeAllowFrom,
dmPolicy,
});
// Group authorization stays explicit to group allowlists and must not
// inherit DM pairing-store identities.
const effectiveGroupAllow = normalizeAllowFrom(groupAllowFrom);
const defaultGroupPolicy = resolveDefaultGroupPolicy(cfg);
const { groupPolicy, providerMissingFallbackApplied } =
resolveAllowlistProviderRuntimeGroupPolicy({

View File

@@ -41,7 +41,7 @@ describe("security/dm-policy-shared", () => {
storeAllowFrom: [" owner3 ", ""],
});
expect(lists.effectiveAllowFrom).toEqual(["owner", "owner2", "owner3"]);
expect(lists.effectiveGroupAllowFrom).toEqual(["group:abc", "owner3"]);
expect(lists.effectiveGroupAllowFrom).toEqual(["group:abc"]);
});
it("falls back to DM allowlist for groups when groupAllowFrom is empty", () => {
@@ -51,7 +51,7 @@ describe("security/dm-policy-shared", () => {
storeAllowFrom: [" owner2 "],
});
expect(lists.effectiveAllowFrom).toEqual(["owner", "owner2"]);
expect(lists.effectiveGroupAllowFrom).toEqual(["owner", "owner2"]);
expect(lists.effectiveGroupAllowFrom).toEqual(["owner"]);
});
it("excludes storeAllowFrom when dmPolicy is allowlist", () => {
@@ -65,7 +65,7 @@ describe("security/dm-policy-shared", () => {
expect(lists.effectiveGroupAllowFrom).toEqual(["group:abc"]);
});
it("includes storeAllowFrom when dmPolicy is pairing", () => {
it("keeps group allowlist explicit when dmPolicy is pairing", () => {
const lists = resolveEffectiveAllowFromLists({
allowFrom: ["+1111"],
groupAllowFrom: [],
@@ -73,7 +73,7 @@ describe("security/dm-policy-shared", () => {
dmPolicy: "pairing",
});
expect(lists.effectiveAllowFrom).toEqual(["+1111", "+2222"]);
expect(lists.effectiveGroupAllowFrom).toEqual(["+1111", "+2222"]);
expect(lists.effectiveGroupAllowFrom).toEqual(["+1111"]);
});
it("resolves access + effective allowlists in one shared call", () => {
@@ -89,7 +89,7 @@ describe("security/dm-policy-shared", () => {
expect(resolved.decision).toBe("allow");
expect(resolved.reason).toBe("dmPolicy=pairing (allowlisted)");
expect(resolved.effectiveAllowFrom).toEqual(["owner", "paired-user"]);
expect(resolved.effectiveGroupAllowFrom).toEqual(["group:room", "paired-user"]);
expect(resolved.effectiveGroupAllowFrom).toEqual(["group:room"]);
});
it("keeps allowlist mode strict in shared resolver (no pairing-store fallback)", () => {

View File

@@ -1,3 +1,4 @@
import { mergeDmAllowFromSources, resolveGroupAllowFromSources } from "../channels/allow-from.js";
import type { ChannelId } from "../channels/plugins/types.js";
import { readChannelAllowFromStore } from "../pairing/pairing-store.js";
import { normalizeStringEntries } from "../shared/string-normalization.js";
@@ -11,21 +12,23 @@ export function resolveEffectiveAllowFromLists(params: {
effectiveAllowFrom: string[];
effectiveGroupAllowFrom: string[];
} {
const configAllowFrom = normalizeStringEntries(
Array.isArray(params.allowFrom) ? params.allowFrom : undefined,
const allowFrom = Array.isArray(params.allowFrom) ? params.allowFrom : undefined;
const groupAllowFrom = Array.isArray(params.groupAllowFrom) ? params.groupAllowFrom : undefined;
const storeAllowFrom = Array.isArray(params.storeAllowFrom) ? params.storeAllowFrom : undefined;
const effectiveAllowFrom = normalizeStringEntries(
mergeDmAllowFromSources({
allowFrom,
storeAllowFrom,
dmPolicy: params.dmPolicy ?? undefined,
}),
);
const configGroupAllowFrom = normalizeStringEntries(
Array.isArray(params.groupAllowFrom) ? params.groupAllowFrom : undefined,
// Group auth is explicit (groupAllowFrom fallback allowFrom). Pairing store is DM-only.
const effectiveGroupAllowFrom = normalizeStringEntries(
resolveGroupAllowFromSources({
allowFrom,
groupAllowFrom,
}),
);
const storeAllowFrom =
params.dmPolicy === "allowlist"
? []
: normalizeStringEntries(
Array.isArray(params.storeAllowFrom) ? params.storeAllowFrom : undefined,
);
const effectiveAllowFrom = normalizeStringEntries([...configAllowFrom, ...storeAllowFrom]);
const groupBase = configGroupAllowFrom.length > 0 ? configGroupAllowFrom : configAllowFrom;
const effectiveGroupAllowFrom = normalizeStringEntries([...groupBase, ...storeAllowFrom]);
return { effectiveAllowFrom, effectiveGroupAllowFrom };
}

View File

@@ -1,4 +1,8 @@
import { firstDefined, isSenderIdAllowed, mergeAllowFromSources } from "../channels/allow-from.js";
import {
firstDefined,
isSenderIdAllowed,
mergeDmAllowFromSources,
} from "../channels/allow-from.js";
import type { AllowlistMatch } from "../channels/allowlist-match.js";
import { createSubsystemLogger } from "../logging/subsystem.js";
@@ -53,11 +57,11 @@ export const normalizeAllowFrom = (list?: Array<string | number>): NormalizedAll
};
};
export const normalizeAllowFromWithStore = (params: {
export const normalizeDmAllowFromWithStore = (params: {
allowFrom?: Array<string | number>;
storeAllowFrom?: string[];
dmPolicy?: string;
}): NormalizedAllowFrom => normalizeAllowFrom(mergeAllowFromSources(params));
}): NormalizedAllowFrom => normalizeAllowFrom(mergeDmAllowFromSources(params));
export const isSenderAllowed = (params: {
allow: NormalizedAllowFrom;

View File

@@ -28,7 +28,7 @@ import { resolveThreadSessionKeys } from "../routing/session-key.js";
import { withTelegramApiErrorLogging } from "./api-logging.js";
import {
isSenderAllowed,
normalizeAllowFromWithStore,
normalizeDmAllowFromWithStore,
type NormalizedAllowFrom,
} from "./bot-access.js";
import type { TelegramMediaRef } from "./bot-message-context.js";
@@ -615,7 +615,7 @@ export const registerTelegramHandlers = ({
return { allowed: false, reason: "direct-disabled" };
}
if (dmPolicy !== "open") {
const effectiveDmAllow = normalizeAllowFromWithStore({
const effectiveDmAllow = normalizeDmAllowFromWithStore({
allowFrom,
storeAllowFrom,
dmPolicy,
@@ -1273,7 +1273,7 @@ export const registerTelegramHandlers = ({
effectiveGroupAllow,
hasGroupAllowOverride,
} = eventAuthContext;
const effectiveDmAllow = normalizeAllowFromWithStore({
const effectiveDmAllow = normalizeDmAllowFromWithStore({
allowFrom,
storeAllowFrom,
dmPolicy,

View File

@@ -40,7 +40,7 @@ import {
firstDefined,
isSenderAllowed,
normalizeAllowFrom,
normalizeAllowFromWithStore,
normalizeDmAllowFromWithStore,
} from "./bot-access.js";
import {
buildGroupLabel,
@@ -195,7 +195,7 @@ export const buildTelegramMessageContext = async ({
: null;
const sessionKey = threadKeys?.sessionKey ?? baseSessionKey;
const mentionRegexes = buildMentionRegexes(cfg, route.agentId);
const effectiveDmAllow = normalizeAllowFromWithStore({ allowFrom, storeAllowFrom, dmPolicy });
const effectiveDmAllow = normalizeDmAllowFromWithStore({ allowFrom, storeAllowFrom, dmPolicy });
const groupAllowOverride = firstDefined(topicConfig?.allowFrom, groupConfig?.allowFrom);
// Group sender checks are explicit and must not inherit DM pairing-store entries.
const effectiveGroupAllow = normalizeAllowFrom(groupAllowOverride ?? groupAllowFrom);

View File

@@ -41,7 +41,7 @@ import { resolveAgentRoute } from "../routing/resolve-route.js";
import { resolveThreadSessionKeys } from "../routing/session-key.js";
import type { RuntimeEnv } from "../runtime.js";
import { withTelegramApiErrorLogging } from "./api-logging.js";
import { isSenderAllowed, normalizeAllowFromWithStore } from "./bot-access.js";
import { isSenderAllowed, normalizeDmAllowFromWithStore } from "./bot-access.js";
import {
buildCappedTelegramMenuCommands,
buildPluginTelegramMenuCommands,
@@ -251,7 +251,7 @@ async function resolveTelegramCommandAuth(params: {
}
}
const dmAllow = normalizeAllowFromWithStore({
const dmAllow = normalizeDmAllowFromWithStore({
allowFrom: allowFrom,
storeAllowFrom,
dmPolicy: telegramCfg.dmPolicy ?? "pairing",