From e2ff0d5e961890b95bdc3f994c2381de4d2dfe24 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Mon, 30 Mar 2026 17:57:13 -0400 Subject: [PATCH] fix(matrix): align local DM trust openclaw#57124 thanks @w-sss --- CHANGELOG.md | 1 + .../src/matrix/direct-management.test.ts | 47 +++++++++++++++++-- .../matrix/src/matrix/direct-management.ts | 8 +++- .../matrix/src/matrix/direct-room.test.ts | 18 +++++++ extensions/matrix/src/matrix/direct-room.ts | 27 ++--------- .../matrix/src/matrix/monitor/direct.test.ts | 23 ++++++++- .../matrix/src/matrix/monitor/direct.ts | 21 +++------ .../matrix/src/matrix/send/targets.test.ts | 36 ++++++++++++-- 8 files changed, 133 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df5bb2226fc..9e543975c5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,7 @@ Docs: https://docs.openclaw.ai - Diffs: fall back to plain text when `lang` hints are invalid during diff render and viewer hydration, so bad or stale language values no longer break the diff viewer. (#57902) Thanks @gumadeiras. - Doctor/plugins: skip false Matrix legacy-helper warnings when no migration plans exist, and keep bundled `enabledByDefault` plugins in the gateway startup set. (#57931) Thanks @dinakars777. - Matrix/CLI send: start one-off Matrix send clients before outbound delivery so `openclaw message send --channel matrix` restores E2EE in encrypted rooms instead of sending plain events. (#57936) Thanks @gumadeiras. +- Matrix/direct rooms: stop trusting remote `is_direct`, honor explicit local `is_direct: false` for discovered DM candidates, and avoid extra member-state lookups for shared rooms so DM routing and repair stay aligned. (#57124) Thanks @w-sss. ## 2026.3.28 diff --git a/extensions/matrix/src/matrix/direct-management.test.ts b/extensions/matrix/src/matrix/direct-management.test.ts index c3c6eb7c459..42b12fabe4e 100644 --- a/extensions/matrix/src/matrix/direct-management.test.ts +++ b/extensions/matrix/src/matrix/direct-management.test.ts @@ -64,13 +64,15 @@ describe("inspectMatrixDirectRooms", () => { expect(result.discoveredStrictRoomIds).toEqual(["!fresh:example.org"]); }); - it("prefers discovered rooms marked direct in member state over plain strict rooms", async () => { + it("prefers discovered rooms marked direct in local member state over plain strict rooms", async () => { const client = createClient({ getJoinedRooms: vi.fn(async () => ["!fallback:example.org", "!explicit:example.org"]), getJoinedRoomMembers: vi.fn(async () => ["@bot:example.org", "@alice:example.org"]), - getRoomStateEvent: vi.fn(async (roomId: string, _eventType: string, userId: string) => ({ - is_direct: roomId === "!explicit:example.org" && userId === "@alice:example.org", - })), + getRoomStateEvent: vi.fn(async (roomId: string, _eventType: string, userId: string) => + roomId === "!explicit:example.org" && userId === "@bot:example.org" + ? { is_direct: true } + : {}, + ), }); const result = await inspectMatrixDirectRooms({ @@ -84,6 +86,43 @@ describe("inspectMatrixDirectRooms", () => { "!explicit:example.org", ]); }); + + it("ignores remote member-state direct flags when ranking discovered rooms", async () => { + const client = createClient({ + getJoinedRooms: vi.fn(async () => ["!fallback:example.org", "!remote-marked:example.org"]), + getJoinedRoomMembers: vi.fn(async () => ["@bot:example.org", "@alice:example.org"]), + getRoomStateEvent: vi.fn(async (roomId: string, _eventType: string, userId: string) => + roomId === "!remote-marked:example.org" && userId === "@alice:example.org" + ? { is_direct: true } + : {}, + ), + }); + + const result = await inspectMatrixDirectRooms({ + client, + remoteUserId: "@alice:example.org", + }); + + expect(result.activeRoomId).toBe("!fallback:example.org"); + }); + + it("does not treat discovered rooms with local is_direct false as active DMs", async () => { + const client = createClient({ + getJoinedRooms: vi.fn(async () => ["!blocked:example.org"]), + getJoinedRoomMembers: vi.fn(async () => ["@bot:example.org", "@alice:example.org"]), + getRoomStateEvent: vi.fn(async (_roomId: string, _eventType: string, userId: string) => ({ + is_direct: userId === "@bot:example.org" ? false : undefined, + })), + }); + + const result = await inspectMatrixDirectRooms({ + client, + remoteUserId: "@alice:example.org", + }); + + expect(result.activeRoomId).toBeNull(); + expect(result.discoveredStrictRoomIds).toEqual([]); + }); }); describe("repairMatrixDirectRooms", () => { diff --git a/extensions/matrix/src/matrix/direct-management.ts b/extensions/matrix/src/matrix/direct-management.ts index 2ed6c8366dd..db63018c62b 100644 --- a/extensions/matrix/src/matrix/direct-management.ts +++ b/extensions/matrix/src/matrix/direct-management.ts @@ -92,8 +92,12 @@ async function classifyDirectRoomCandidate(params: { return { roomId: params.roomId, joinedMembers: evidence.joinedMembers, - strict: evidence.strict, - explicit: evidence.strict && (params.source === "account-data" || evidence.viaMemberState), + strict: + evidence.strict && (params.source === "account-data" || evidence.memberStateFlag !== false), + explicit: + evidence.strict && + (params.source === "account-data" || evidence.memberStateFlag !== false) && + (params.source === "account-data" || evidence.viaMemberState), source: params.source, }; } diff --git a/extensions/matrix/src/matrix/direct-room.test.ts b/extensions/matrix/src/matrix/direct-room.test.ts index dfc482bcc48..0179d2a1e65 100644 --- a/extensions/matrix/src/matrix/direct-room.test.ts +++ b/extensions/matrix/src/matrix/direct-room.test.ts @@ -40,4 +40,22 @@ describe("inspectMatrixDirectRoomEvidence", () => { expect(getUserId).toHaveBeenCalledTimes(1); expect(result.strict).toBe(true); }); + + it("records only the local member-state direct flag", async () => { + const client = createClient({ + getRoomStateEvent: vi.fn(async (_roomId: string, _eventType: string, stateKey: string) => + stateKey === "@bot:example.org" ? { is_direct: false } : { is_direct: true }, + ), + }); + + const result = await inspectMatrixDirectRoomEvidence({ + client, + roomId: "!dm:example.org", + remoteUserId: "@alice:example.org", + }); + + expect(result.strict).toBe(true); + expect(result.memberStateFlag).toBe(false); + expect(result.viaMemberState).toBe(false); + }); }); diff --git a/extensions/matrix/src/matrix/direct-room.ts b/extensions/matrix/src/matrix/direct-room.ts index a2d48524191..b3bc71e5872 100644 --- a/extensions/matrix/src/matrix/direct-room.ts +++ b/extensions/matrix/src/matrix/direct-room.ts @@ -21,28 +21,10 @@ export function isStrictDirectMembership(params: { selfUserId?: string | null; remoteUserId?: string | null; joinedMembers?: readonly string[] | null; - isDirectFlag?: boolean | null; }): boolean { const selfUserId = trimMaybeString(params.selfUserId); const remoteUserId = trimMaybeString(params.remoteUserId); const joinedMembers = params.joinedMembers ?? []; - - // Only trust is_direct from local user's membership state (selfUserId). - // Remote user's is_direct is NOT trusted (CWE-285: authorization bypass). - - // When local user's is_direct=true, verify it's a true 2-person DM - if (params.isDirectFlag === true) { - return Boolean( - selfUserId && - remoteUserId && - joinedMembers.length === 2 && - joinedMembers.includes(selfUserId) && - joinedMembers.includes(remoteUserId), - ); - } - - // When is_direct=false or null, fall back to strict 2-member check - // This prevents attackers from forcing non-DM classification with is_direct=false return Boolean( selfUserId && remoteUserId && @@ -93,6 +75,7 @@ export type MatrixDirectRoomEvidence = { joinedMembers: string[] | null; strict: boolean; viaMemberState: boolean; + memberStateFlag: boolean | null; }; export async function inspectMatrixDirectRoomEvidence(params: { @@ -116,15 +99,15 @@ export async function inspectMatrixDirectRoomEvidence(params: { joinedMembers, strict: false, viaMemberState: false, + memberStateFlag: null, }; } + const memberStateFlag = await hasDirectMatrixMemberFlag(params.client, params.roomId, selfUserId); return { joinedMembers, strict, - viaMemberState: - (await hasDirectMatrixMemberFlag(params.client, params.roomId, params.remoteUserId)) === - true || - (await hasDirectMatrixMemberFlag(params.client, params.roomId, selfUserId)) === true, + viaMemberState: memberStateFlag === true, + memberStateFlag, }; } diff --git a/extensions/matrix/src/matrix/monitor/direct.test.ts b/extensions/matrix/src/matrix/monitor/direct.test.ts index 36d53322c3e..ded86edc496 100644 --- a/extensions/matrix/src/matrix/monitor/direct.test.ts +++ b/extensions/matrix/src/matrix/monitor/direct.test.ts @@ -145,9 +145,10 @@ describe("createDirectRoomTracker", () => { expect(client.getRoomStateEvent).not.toHaveBeenCalled(); }); - it("treats sender is_direct member state as a DM signal", async () => { + it("does not treat sender is_direct member state as a DM signal", async () => { const client = createMockClient({ isDm: false, + dmCacheAvailable: true, stateEvents: { "!room:example.org|m.room.member|@alice:example.org": { is_direct: true }, }, @@ -159,7 +160,7 @@ describe("createDirectRoomTracker", () => { roomId: "!room:example.org", senderId: "@alice:example.org", }), - ).resolves.toBe(true); + ).resolves.toBe(false); }); it("treats self is_direct member state as a DM signal", async () => { @@ -179,6 +180,24 @@ describe("createDirectRoomTracker", () => { ).resolves.toBe(true); }); + it("treats self is_direct false member state as a non-DM signal", async () => { + const client = createMockClient({ + isDm: false, + dmCacheAvailable: false, + stateEvents: { + "!room:example.org|m.room.member|@bot:example.org": { is_direct: false }, + }, + }); + const tracker = createDirectRoomTracker(client); + + await expect( + tracker.isDirectMessage({ + roomId: "!room:example.org", + senderId: "@alice:example.org", + }), + ).resolves.toBe(false); + }); + it("does not classify 2-member rooms whose sender is not a joined member when falling back", async () => { const client = createMockClient({ isDm: false, diff --git a/extensions/matrix/src/matrix/monitor/direct.ts b/extensions/matrix/src/matrix/monitor/direct.ts index 822cce66755..39e21fccab1 100644 --- a/extensions/matrix/src/matrix/monitor/direct.ts +++ b/extensions/matrix/src/matrix/monitor/direct.ts @@ -37,7 +37,7 @@ export function createDirectRoomTracker(client: MatrixClient, opts: DirectRoomTr let hasSeededDmCache = false; let cachedSelfUserId: string | null = null; const joinedMembersCache = new Map(); - const directMemberFlagCache = new Map(); + const directMemberFlagCache = new Map(); const ensureSelfUserId = async (): Promise => { if (cachedSelfUserId) { @@ -113,19 +113,10 @@ export function createDirectRoomTracker(client: MatrixClient, opts: DirectRoomTr const { roomId, senderId } = params; const selfUserId = params.selfUserId ?? (await ensureSelfUserId()); const joinedMembers = await resolveJoinedMembers(roomId); - - // Check is_direct flag from local user's membership state only. - // Do NOT trust remote sender's is_direct (CWE-285: improper authorization). - // In Matrix, m.room.member.content.is_direct is set by each member themselves, - // so a malicious remote user could manipulate it to bypass DM/room policies. - const directViaSelf = await resolveDirectMemberFlag(roomId, selfUserId); - const isDirectFlag: boolean | null = directViaSelf; - const strictDirectMembership = isStrictDirectMembership({ selfUserId, remoteUserId: senderId, joinedMembers, - isDirectFlag, }); try { @@ -143,13 +134,15 @@ export function createDirectRoomTracker(client: MatrixClient, opts: DirectRoomTr } if (strictDirectMembership) { - const directViaState = - (await resolveDirectMemberFlag(roomId, senderId)) || - (await resolveDirectMemberFlag(roomId, selfUserId)); - if (directViaState) { + const directViaSelf = await resolveDirectMemberFlag(roomId, selfUserId); + if (directViaSelf === true) { log(`matrix: dm detected via member state room=${roomId}`); return true; } + if (directViaSelf === false) { + log(`matrix: dm rejected via member state room=${roomId}`); + return false; + } if (!hasSeededDmCache) { log( diff --git a/extensions/matrix/src/matrix/send/targets.test.ts b/extensions/matrix/src/matrix/send/targets.test.ts index 1308dec7619..149139bf842 100644 --- a/extensions/matrix/src/matrix/send/targets.test.ts +++ b/extensions/matrix/src/matrix/send/targets.test.ts @@ -51,7 +51,7 @@ describe("resolveMatrixRoomId", () => { ); }); - it("prefers joined rooms marked direct in member state over plain strict rooms", async () => { + it("prefers joined rooms marked direct in local member state over plain strict rooms", async () => { const userId = "@fallback:example.org"; const client = { getAccountData: vi.fn().mockRejectedValue(new Error("nope")), @@ -60,9 +60,11 @@ describe("resolveMatrixRoomId", () => { getJoinedRoomMembers: vi.fn().mockResolvedValue(["@bot:example.org", userId]), getRoomStateEvent: vi .fn() - .mockImplementation(async (roomId: string, _eventType: string, stateKey: string) => ({ - is_direct: roomId === "!explicit:example.org" && stateKey === userId, - })), + .mockImplementation(async (roomId: string, _eventType: string, stateKey: string) => + roomId === "!explicit:example.org" && stateKey === "@bot:example.org" + ? { is_direct: true } + : {}, + ), setAccountData: vi.fn().mockResolvedValue(undefined), } as unknown as MatrixClient; @@ -75,6 +77,32 @@ describe("resolveMatrixRoomId", () => { ); }); + it("ignores remote member-state direct flags when resolving a direct room", async () => { + const userId = "@fallback:example.org"; + const client = { + getAccountData: vi.fn().mockRejectedValue(new Error("nope")), + getUserId: vi.fn().mockResolvedValue("@bot:example.org"), + getJoinedRooms: vi + .fn() + .mockResolvedValue(["!fallback:example.org", "!remote-marked:example.org"]), + getJoinedRoomMembers: vi.fn().mockResolvedValue(["@bot:example.org", userId]), + getRoomStateEvent: vi + .fn() + .mockImplementation(async (roomId: string, _eventType: string, stateKey: string) => + roomId === "!remote-marked:example.org" && stateKey === userId ? { is_direct: true } : {}, + ), + setAccountData: vi.fn().mockResolvedValue(undefined), + } as unknown as MatrixClient; + + const resolved = await resolveMatrixRoomId(client, userId); + + expect(resolved).toBe("!fallback:example.org"); + expect(client.setAccountData).toHaveBeenCalledWith( + EventType.Direct, + expect.objectContaining({ [userId]: ["!fallback:example.org"] }), + ); + }); + it("continues when a room member lookup fails", async () => { const userId = "@continue:example.org"; const roomId = "!good:example.org";