From 63fc3c780bcd124ccdcbf549e28b854d824dd4a2 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Thu, 12 Mar 2026 04:23:09 +0000 Subject: [PATCH] Matrix: harden alias trust and log redaction --- .../matrix/src/channel.directory.test.ts | 28 ++++++++++ extensions/matrix/src/channel.ts | 18 ++++--- .../src/matrix/monitor/auto-join.test.ts | 51 ++++++++++++------- .../matrix/src/matrix/monitor/auto-join.ts | 44 ++++++++-------- .../matrix/src/matrix/monitor/config.test.ts | 41 +++++++++++++++ .../matrix/src/matrix/monitor/config.ts | 2 +- .../matrix/src/matrix/sdk/logger.test.ts | 25 +++++++++ extensions/matrix/src/matrix/sdk/logger.ts | 4 +- src/plugin-sdk/matrix.ts | 1 + 9 files changed, 163 insertions(+), 51 deletions(-) create mode 100644 extensions/matrix/src/matrix/sdk/logger.test.ts diff --git a/extensions/matrix/src/channel.directory.test.ts b/extensions/matrix/src/channel.directory.test.ts index 7b27a0cf2f8..ca87d59647e 100644 --- a/extensions/matrix/src/channel.directory.test.ts +++ b/extensions/matrix/src/channel.directory.test.ts @@ -258,6 +258,7 @@ describe("matrix directory", () => { }), ).toEqual([ '- Matrix rooms: groupPolicy="open" allows any room to trigger (mention-gated). Set channels.matrix.groupPolicy="allowlist" + channels.matrix.groups (and optionally channels.matrix.groupAllowFrom) to restrict rooms.', + '- Matrix invites: autoJoin="always" joins any invited room before message policy applies. Set channels.matrix.autoJoin="allowlist" + channels.matrix.autoJoinAllowlist (or channels.matrix.autoJoin="off") to restrict joins.', ]); expect( @@ -292,6 +293,33 @@ describe("matrix directory", () => { }), ).toEqual([ '- Matrix rooms: groupPolicy="open" allows any room to trigger (mention-gated). Set channels.matrix.accounts.assistant.groupPolicy="allowlist" + channels.matrix.accounts.assistant.groups (and optionally channels.matrix.accounts.assistant.groupAllowFrom) to restrict rooms.', + '- Matrix invites: autoJoin="always" joins any invited room before message policy applies. Set channels.matrix.accounts.assistant.autoJoin="allowlist" + channels.matrix.accounts.assistant.autoJoinAllowlist (or channels.matrix.accounts.assistant.autoJoin="off") to restrict joins.', + ]); + }); + + it("reports invite auto-join warnings even when room policy is restricted", () => { + expect( + matrixPlugin.security?.collectWarnings?.({ + cfg: { + channels: { + matrix: { + groupPolicy: "allowlist", + }, + }, + } as CoreConfig, + account: resolveMatrixAccount({ + cfg: { + channels: { + matrix: { + groupPolicy: "allowlist", + }, + }, + } as CoreConfig, + accountId: "default", + }), + }), + ).toEqual([ + '- Matrix invites: autoJoin="always" joins any invited room before message policy applies. Set channels.matrix.autoJoin="allowlist" + channels.matrix.autoJoinAllowlist (or channels.matrix.autoJoin="off") to restrict joins.', ]); }); diff --git a/extensions/matrix/src/channel.ts b/extensions/matrix/src/channel.ts index 465aaa8c6bb..2fc7335b713 100644 --- a/extensions/matrix/src/channel.ts +++ b/extensions/matrix/src/channel.ts @@ -158,13 +158,19 @@ export const matrixPlugin: ChannelPlugin = { groupPolicy: account.config.groupPolicy, defaultGroupPolicy, }); - if (groupPolicy !== "open") { - return []; - } const configPath = resolveMatrixConfigPath(cfg as CoreConfig, account.accountId); - return [ - `- Matrix rooms: groupPolicy="open" allows any room to trigger (mention-gated). Set ${configPath}.groupPolicy="allowlist" + ${configPath}.groups (and optionally ${configPath}.groupAllowFrom) to restrict rooms.`, - ]; + const warnings: string[] = []; + if (groupPolicy === "open") { + warnings.push( + `- Matrix rooms: groupPolicy="open" allows any room to trigger (mention-gated). Set ${configPath}.groupPolicy="allowlist" + ${configPath}.groups (and optionally ${configPath}.groupAllowFrom) to restrict rooms.`, + ); + } + if ((account.config.autoJoin ?? "always") === "always") { + warnings.push( + `- Matrix invites: autoJoin="always" joins any invited room before message policy applies. Set ${configPath}.autoJoin="allowlist" + ${configPath}.autoJoinAllowlist (or ${configPath}.autoJoin="off") to restrict joins.`, + ); + } + return warnings; }, }, groups: { diff --git a/extensions/matrix/src/matrix/monitor/auto-join.test.ts b/extensions/matrix/src/matrix/monitor/auto-join.test.ts index 94493100d9e..8cbc082ffd7 100644 --- a/extensions/matrix/src/matrix/monitor/auto-join.test.ts +++ b/extensions/matrix/src/matrix/monitor/auto-join.test.ts @@ -16,15 +16,14 @@ function createClientStub() { return client; }), joinRoom: vi.fn(async () => {}), - getRoomStateEvent: vi.fn(async () => ({})), + resolveRoom: vi.fn(async () => null), } as unknown as import("../sdk.js").MatrixClient; return { client, getInviteHandler: () => inviteHandler, joinRoom: (client as unknown as { joinRoom: ReturnType }).joinRoom, - getRoomStateEvent: (client as unknown as { getRoomStateEvent: ReturnType }) - .getRoomStateEvent, + resolveRoom: (client as unknown as { resolveRoom: ReturnType }).resolveRoom, }; } @@ -60,11 +59,8 @@ describe("registerMatrixAutoJoin", () => { }); it("ignores invites outside allowlist when autoJoin=allowlist", async () => { - const { client, getInviteHandler, joinRoom, getRoomStateEvent } = createClientStub(); - getRoomStateEvent.mockResolvedValue({ - alias: "#other:example.org", - alt_aliases: ["#else:example.org"], - }); + const { client, getInviteHandler, joinRoom, resolveRoom } = createClientStub(); + resolveRoom.mockResolvedValue(null); const accountConfig: MatrixConfig = { autoJoin: "allowlist", autoJoinAllowlist: ["#allowed:example.org"], @@ -86,12 +82,9 @@ describe("registerMatrixAutoJoin", () => { expect(joinRoom).not.toHaveBeenCalled(); }); - it("joins invite when alias matches allowlist", async () => { - const { client, getInviteHandler, joinRoom, getRoomStateEvent } = createClientStub(); - getRoomStateEvent.mockResolvedValue({ - alias: "#allowed:example.org", - alt_aliases: ["#backup:example.org"], - }); + it("joins invite when allowlisted alias resolves to the invited room", async () => { + const { client, getInviteHandler, joinRoom, resolveRoom } = createClientStub(); + resolveRoom.mockResolvedValue("!room:example.org"); const accountConfig: MatrixConfig = { autoJoin: "allowlist", autoJoinAllowlist: [" #allowed:example.org "], @@ -113,13 +106,33 @@ describe("registerMatrixAutoJoin", () => { expect(joinRoom).toHaveBeenCalledWith("!room:example.org"); }); - it("uses account-scoped auto-join settings for non-default accounts", async () => { - const { client, getInviteHandler, joinRoom, getRoomStateEvent } = createClientStub(); - getRoomStateEvent.mockResolvedValue({ - alias: "#ops-allowed:example.org", - alt_aliases: [], + it("does not trust room-provided alias claims for allowlist joins", async () => { + const { client, getInviteHandler, joinRoom, resolveRoom } = createClientStub(); + resolveRoom.mockResolvedValue("!different-room:example.org"); + + registerMatrixAutoJoin({ + client, + accountConfig: { + autoJoin: "allowlist", + autoJoinAllowlist: ["#allowed:example.org"], + }, + runtime: { + log: vi.fn(), + error: vi.fn(), + } as unknown as import("openclaw/plugin-sdk/matrix").RuntimeEnv, }); + const inviteHandler = getInviteHandler(); + expect(inviteHandler).toBeTruthy(); + await inviteHandler!("!room:example.org", {}); + + expect(joinRoom).not.toHaveBeenCalled(); + }); + + it("uses account-scoped auto-join settings for non-default accounts", async () => { + const { client, getInviteHandler, joinRoom, resolveRoom } = createClientStub(); + resolveRoom.mockResolvedValue("!room:example.org"); + registerMatrixAutoJoin({ client, accountConfig: { diff --git a/extensions/matrix/src/matrix/monitor/auto-join.ts b/extensions/matrix/src/matrix/monitor/auto-join.ts index 27f6a93e847..9df9d7f3b7b 100644 --- a/extensions/matrix/src/matrix/monitor/auto-join.ts +++ b/extensions/matrix/src/matrix/monitor/auto-join.ts @@ -17,9 +17,13 @@ export function registerMatrixAutoJoin(params: { runtime.log?.(message); }; const autoJoin = accountConfig.autoJoin ?? "always"; - const autoJoinAllowlist = new Set( - (accountConfig.autoJoinAllowlist ?? []).map((entry) => String(entry).trim()).filter(Boolean), - ); + const rawAllowlist = (accountConfig.autoJoinAllowlist ?? []) + .map((entry) => String(entry).trim()) + .filter(Boolean); + const autoJoinAllowlist = new Set(rawAllowlist); + const allowedRoomIds = new Set(rawAllowlist.filter((entry) => entry.startsWith("!"))); + const allowedAliases = rawAllowlist.filter((entry) => entry.startsWith("#")); + const resolvedAliasRoomIds = new Map(); if (autoJoin === "off") { return; @@ -31,31 +35,25 @@ export function registerMatrixAutoJoin(params: { logVerbose("matrix: auto-join enabled for allowlist invites"); } + const resolveAllowedAliasRoomId = async (alias: string): Promise => { + if (resolvedAliasRoomIds.has(alias)) { + return resolvedAliasRoomIds.get(alias) ?? null; + } + const resolved = await params.client.resolveRoom(alias); + resolvedAliasRoomIds.set(alias, resolved); + return resolved; + }; + // Handle invites directly so both "always" and "allowlist" modes share the same path. client.on("room.invite", async (roomId: string, _inviteEvent: unknown) => { if (autoJoin === "allowlist") { - let alias: string | undefined; - let altAliases: string[] = []; - try { - const aliasState = await client - .getRoomStateEvent(roomId, "m.room.canonical_alias", "") - .catch(() => null); - alias = aliasState && typeof aliasState.alias === "string" ? aliasState.alias : undefined; - altAliases = - aliasState && Array.isArray(aliasState.alt_aliases) - ? aliasState.alt_aliases - .map((entry) => (typeof entry === "string" ? entry.trim() : "")) - .filter(Boolean) - : []; - } catch { - // Ignore errors - } - + const allowedAliasRoomIds = await Promise.all( + allowedAliases.map(async (alias) => await resolveAllowedAliasRoomId(alias)), + ); const allowed = autoJoinAllowlist.has("*") || - autoJoinAllowlist.has(roomId) || - (alias ? autoJoinAllowlist.has(alias) : false) || - altAliases.some((value) => autoJoinAllowlist.has(value)); + allowedRoomIds.has(roomId) || + allowedAliasRoomIds.some((resolvedRoomId) => resolvedRoomId === roomId); if (!allowed) { logVerbose(`matrix: invite ignored (not in allowlist) room=${roomId}`); diff --git a/extensions/matrix/src/matrix/monitor/config.test.ts b/extensions/matrix/src/matrix/monitor/config.test.ts index 21d96a0abe4..f2a146879f7 100644 --- a/extensions/matrix/src/matrix/monitor/config.test.ts +++ b/extensions/matrix/src/matrix/monitor/config.test.ts @@ -153,4 +153,45 @@ describe("resolveMatrixMonitorConfig", () => { "matrix rooms must be room IDs or aliases (example: !room:server or #alias:server). Unresolved entries are ignored.", ); }); + + it("resolves exact room aliases to canonical room ids instead of trusting alias keys directly", async () => { + const runtime = createRuntime(); + const resolveTargets = vi.fn( + async ({ kind, inputs }: { inputs: string[]; kind: "user" | "group" }) => { + if (kind === "group") { + return inputs.map((input) => + input === "#allowed:example.org" + ? { input, resolved: true, id: "!allowed-room:example.org" } + : { input, resolved: false }, + ); + } + return []; + }, + ); + + const result = await resolveMatrixMonitorConfig({ + cfg: {} as CoreConfig, + accountId: "ops", + roomsConfig: { + "#allowed:example.org": { + allow: true, + }, + }, + runtime, + resolveTargets, + }); + + expect(result.roomsConfig).toEqual({ + "!allowed-room:example.org": { + allow: true, + }, + }); + expect(resolveTargets).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: "ops", + kind: "group", + inputs: ["#allowed:example.org"], + }), + ); + }); }); diff --git a/extensions/matrix/src/matrix/monitor/config.ts b/extensions/matrix/src/matrix/monitor/config.ts index 247c71e5e25..5a9086dd7ba 100644 --- a/extensions/matrix/src/matrix/monitor/config.ts +++ b/extensions/matrix/src/matrix/monitor/config.ts @@ -183,7 +183,7 @@ async function resolveMatrixMonitorRoomsConfig(params: { unresolved.push(entry); continue; } - if ((cleaned.startsWith("!") || cleaned.startsWith("#")) && cleaned.includes(":")) { + if (cleaned.startsWith("!") && cleaned.includes(":")) { if (!nextRooms[cleaned]) { nextRooms[cleaned] = roomConfig; } diff --git a/extensions/matrix/src/matrix/sdk/logger.test.ts b/extensions/matrix/src/matrix/sdk/logger.test.ts new file mode 100644 index 00000000000..b21168b6520 --- /dev/null +++ b/extensions/matrix/src/matrix/sdk/logger.test.ts @@ -0,0 +1,25 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ConsoleLogger, setMatrixConsoleLogging } from "./logger.js"; + +describe("ConsoleLogger", () => { + afterEach(() => { + setMatrixConsoleLogging(false); + vi.restoreAllMocks(); + }); + + it("redacts sensitive tokens in emitted log messages", () => { + setMatrixConsoleLogging(true); + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + + new ConsoleLogger().error( + "MatrixHttpClient", + "Authorization: Bearer 123456:abcdefghijklmnopqrstuvwxyzABCDEFG", + ); + + const message = spy.mock.calls[0]?.[0]; + expect(typeof message).toBe("string"); + expect(message).toContain("Authorization: Bearer"); + expect(message).not.toContain("123456:abcdefghijklmnopqrstuvwxyzABCDEFG"); + expect(message).toContain("***"); + }); +}); diff --git a/extensions/matrix/src/matrix/sdk/logger.ts b/extensions/matrix/src/matrix/sdk/logger.ts index a0637f17f8d..f3f08fe7cdc 100644 --- a/extensions/matrix/src/matrix/sdk/logger.ts +++ b/extensions/matrix/src/matrix/sdk/logger.ts @@ -1,5 +1,5 @@ import { format } from "node:util"; -import type { RuntimeLogger } from "openclaw/plugin-sdk/matrix"; +import { redactSensitiveText, type RuntimeLogger } from "openclaw/plugin-sdk/matrix"; import { getMatrixRuntime } from "../../runtime.js"; export type Logger = { @@ -35,7 +35,7 @@ function formatMessage(module: string, messageOrObject: unknown[]): string { if (messageOrObject.length === 0) { return `[${module}]`; } - return `[${module}] ${format(...messageOrObject)}`; + return redactSensitiveText(`[${module}] ${format(...messageOrObject)}`); } export class ConsoleLogger { diff --git a/src/plugin-sdk/matrix.ts b/src/plugin-sdk/matrix.ts index 84984d083a0..b727f388e5e 100644 --- a/src/plugin-sdk/matrix.ts +++ b/src/plugin-sdk/matrix.ts @@ -154,6 +154,7 @@ export { } from "../security/dm-policy-shared.js"; export { normalizeStringEntries } from "../shared/string-normalization.js"; export { formatDocsLink } from "../terminal/links.js"; +export { redactSensitiveText } from "../logging/redact.js"; export type { WizardPrompter } from "../wizard/prompts.js"; export { evaluateGroupRouteAccessForPolicy,