fix(matrix): align local DM trust openclaw#57124 thanks @w-sss

This commit is contained in:
Gustavo Madeira Santana 2026-03-30 17:57:13 -04:00
parent 19a3a30dc0
commit e2ff0d5e96
No known key found for this signature in database
8 changed files with 133 additions and 48 deletions

View File

@ -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

View File

@ -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", () => {

View File

@ -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,
};
}

View File

@ -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);
});
});

View File

@ -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,
};
}

View File

@ -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,

View File

@ -37,7 +37,7 @@ export function createDirectRoomTracker(client: MatrixClient, opts: DirectRoomTr
let hasSeededDmCache = false;
let cachedSelfUserId: string | null = null;
const joinedMembersCache = new Map<string, { members: string[]; ts: number }>();
const directMemberFlagCache = new Map<string, { isDirect: boolean; ts: number }>();
const directMemberFlagCache = new Map<string, { isDirect: boolean | null; ts: number }>();
const ensureSelfUserId = async (): Promise<string | null> => {
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(

View File

@ -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";