From 15bf93e613668fb749eb8c51664e0394b31544ff Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 23 Mar 2023 16:24:59 +0100 Subject: [PATCH 1/4] Validate and filter m.direct before use --- src/utils/DMRoomMap.ts | 11 +++- src/utils/dm/filterValidMDirect.ts | 69 +++++++++++++++++++++ test/utils/DMRoomMap-test.ts | 73 ++++++++++++++++++---- test/utils/dm/filterValidMDirect-test.ts | 77 ++++++++++++++++++++++++ 4 files changed, 217 insertions(+), 13 deletions(-) create mode 100644 src/utils/dm/filterValidMDirect.ts create mode 100644 test/utils/dm/filterValidMDirect-test.ts diff --git a/src/utils/DMRoomMap.ts b/src/utils/DMRoomMap.ts index 97c4b6b11ce..9726ceafac9 100644 --- a/src/utils/DMRoomMap.ts +++ b/src/utils/DMRoomMap.ts @@ -23,6 +23,7 @@ import { MatrixEvent } from "matrix-js-sdk/src/models/event"; import { Optional } from "matrix-events-sdk"; import { MatrixClientPeg } from "../MatrixClientPeg"; +import { filterValidMDirect } from "./dm/filterValidMDirect"; /** * Class that takes a Matrix Client and flips the m.direct map @@ -44,8 +45,14 @@ export default class DMRoomMap { // see onAccountData this.hasSentOutPatchDirectAccountDataPatch = false; - const mDirectEvent = matrixClient.getAccountData(EventType.Direct)?.getContent() ?? {}; - this.mDirectEvent = { ...mDirectEvent }; // copy as we will mutate + const mDirectRawContent = matrixClient.getAccountData(EventType.Direct)?.getContent() ?? {}; + const { valid, filteredContent } = filterValidMDirect(mDirectRawContent); + + if (!valid) { + logger.warn("Invalid m.direct content occurred", mDirectRawContent); + } + + this.mDirectEvent = filteredContent; } /** diff --git a/src/utils/dm/filterValidMDirect.ts b/src/utils/dm/filterValidMDirect.ts new file mode 100644 index 00000000000..d848da0040a --- /dev/null +++ b/src/utils/dm/filterValidMDirect.ts @@ -0,0 +1,69 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +interface FilterValidMDirectResult { + /** Whether the entire content is valid */ + valid: boolean; + /** Filtered content with only the valid parts */ + filteredContent: Record; +} + +/** + * Filter m.direct content to be compliant to https://spec.matrix.org/v1.6/client-server-api/#mdirect. + * + * @param content - Raw event content to be filerted + * @returns value as a flag whether to content was valid. + * filteredContent with only values from the content that are spec compliant. + */ +export const filterValidMDirect = (content: unknown): FilterValidMDirectResult => { + if (content === null || typeof content !== "object") { + return { + valid: false, + filteredContent: {}, + }; + } + + const filteredContent = new Map(); + let valid = true; + + for (const [userId, roomIds] of Object.entries(content)) { + if (typeof userId !== "string") { + valid = false; + continue; + } + + if (!Array.isArray(roomIds)) { + valid = false; + continue; + } + + const filteredRoomIds: string[] = []; + filteredContent.set(userId, filteredRoomIds); + roomIds.forEach((roomId: unknown) => { + if (typeof roomId === "string") { + filteredRoomIds.push(roomId); + return; + } + + valid = false; + }); + } + + return { + valid, + filteredContent: Object.fromEntries(filteredContent.entries()), + }; +}; diff --git a/test/utils/DMRoomMap-test.ts b/test/utils/DMRoomMap-test.ts index 528bd0778c9..87a966a2035 100644 --- a/test/utils/DMRoomMap-test.ts +++ b/test/utils/DMRoomMap-test.ts @@ -15,18 +15,18 @@ limitations under the License. */ import { mocked, Mocked } from "jest-mock"; -import { EventType, IContent, MatrixClient } from "matrix-js-sdk/src/matrix"; +import { logger } from "matrix-js-sdk/src/logger"; +import { EventType, IContent, MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix"; import DMRoomMap from "../../src/utils/DMRoomMap"; import { mkEvent, stubClient } from "../test-utils"; - describe("DMRoomMap", () => { const roomId1 = "!room1:example.com"; const roomId2 = "!room2:example.com"; const roomId3 = "!room3:example.com"; const roomId4 = "!room4:example.com"; - const mDirectContent = { + const validMDirectContent = { "user@example.com": [roomId1, roomId2], "@user:example.com": [roomId1, roomId3, roomId4], "@user2:example.com": [] as string[], @@ -35,20 +35,71 @@ describe("DMRoomMap", () => { let client: Mocked; let dmRoomMap: DMRoomMap; + const mkMDirectEvent = (content: any): MatrixEvent => { + return mkEvent({ + event: true, + type: EventType.Direct, + user: client.getSafeUserId(), + content: content, + }); + }; + beforeEach(() => { client = mocked(stubClient()); + jest.spyOn(logger, "warn"); + }); - const mDirectEvent = mkEvent({ - event: true, + describe("when m.direct has valid content", () => { + beforeEach(() => { + client.getAccountData.mockReturnValue(mkMDirectEvent(validMDirectContent)); + dmRoomMap = new DMRoomMap(client); + }); + + it("getRoomIds should return the room Ids", () => { + expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId2, roomId3, roomId4])); + }); + }); + + describe("when m.direct content contains the entire event", () => { + const mDirectContentContent = { type: EventType.Direct, - user: client.getSafeUserId(), - content: mDirectContent, + content: validMDirectContent, + }; + + beforeEach(() => { + client.getAccountData.mockReturnValue(mkMDirectEvent(mDirectContentContent)); + dmRoomMap = new DMRoomMap(client); + }); + + it("should log the invalid content", () => { + expect(logger.warn).toHaveBeenCalledWith("Invalid m.direct content occurred", mDirectContentContent); + }); + + it("getRoomIds should return an empty list", () => { + expect(dmRoomMap.getRoomIds()).toEqual(new Set([])); }); - client.getAccountData.mockReturnValue(mDirectEvent); - dmRoomMap = new DMRoomMap(client); }); - it("getRoomIds should return the room Ids", () => { - expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId2, roomId3, roomId4])); + describe("when partially crap m.direct content appears", () => { + const partiallyCrapContent = { + "hello": 23, + "@user1:example.com": [] as string[], + "@user2:example.com": [roomId1, roomId2], + "@user3:example.com": "room1, room2, room3", + "@user4:example.com": [roomId4], + }; + + beforeEach(() => { + client.getAccountData.mockReturnValue(mkMDirectEvent(partiallyCrapContent)); + dmRoomMap = new DMRoomMap(client); + }); + + it("should log the invalid content", () => { + expect(logger.warn).toHaveBeenCalledWith("Invalid m.direct content occurred", partiallyCrapContent); + }); + + it("getRoomIds should only return the valid items", () => { + expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId2, roomId4])); + }); }); }); diff --git a/test/utils/dm/filterValidMDirect-test.ts b/test/utils/dm/filterValidMDirect-test.ts new file mode 100644 index 00000000000..333b1feb4ab --- /dev/null +++ b/test/utils/dm/filterValidMDirect-test.ts @@ -0,0 +1,77 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { filterValidMDirect } from "../../../src/utils/dm/filterValidMDirect"; + +const roomId1 = "!room1:example.com"; +const roomId2 = "!room2:example.com"; +const userId1 = "@user1:example.com"; +const userId2 = "@user2:example.com"; +const userId3 = "@user3:example.com"; + +describe("filterValidMDirect", () => { + it("should return an empty object as valid content", () => { + expect(filterValidMDirect({})).toEqual({ + valid: true, + filteredContent: {}, + }); + }); + + it("should return valid content", () => { + expect( + filterValidMDirect({ + [userId1]: [roomId1, roomId2], + [userId2]: [roomId1], + }), + ).toEqual({ + valid: true, + filteredContent: { + [userId1]: [roomId1, roomId2], + [userId2]: [roomId1], + }, + }); + }); + + it("should return an empy object for null", () => { + expect(filterValidMDirect(null)).toEqual({ + valid: false, + filteredContent: {}, + }); + }); + + it("should return an empy object for a non-object", () => { + expect(filterValidMDirect(23)).toEqual({ + valid: false, + filteredContent: {}, + }); + }); + + it("should only return valid content", () => { + const invalidContent = { + [userId1]: [23], + [userId2]: [roomId2], + [userId3]: "room1", + }; + + expect(filterValidMDirect(invalidContent)).toEqual({ + valid: false, + filteredContent: { + [userId1]: [], + [userId2]: [roomId2], + }, + }); + }); +}); From 776f61aa2ea3aafa2534d462141c89b417f45c54 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Fri, 24 Mar 2023 09:54:10 +0100 Subject: [PATCH 2/4] Also validate on update --- src/utils/DMRoomMap.ts | 29 +++++++++++++++++++-------- test/utils/DMRoomMap-test.ts | 39 ++++++++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/utils/DMRoomMap.ts b/src/utils/DMRoomMap.ts index 9726ceafac9..1f95f5be61a 100644 --- a/src/utils/DMRoomMap.ts +++ b/src/utils/DMRoomMap.ts @@ -46,13 +46,7 @@ export default class DMRoomMap { this.hasSentOutPatchDirectAccountDataPatch = false; const mDirectRawContent = matrixClient.getAccountData(EventType.Direct)?.getContent() ?? {}; - const { valid, filteredContent } = filterValidMDirect(mDirectRawContent); - - if (!valid) { - logger.warn("Invalid m.direct content occurred", mDirectRawContent); - } - - this.mDirectEvent = filteredContent; + this.setMDirectFromContent(mDirectRawContent); } /** @@ -91,9 +85,28 @@ export default class DMRoomMap { this.matrixClient.removeListener(ClientEvent.AccountData, this.onAccountData); } + /** + * Filter m.direct content to contain only valid data and then sets it. + * Logs if invalid m.direct content occurs. + * {@link filterValidMDirect} + * + * @param content - Raw m.direct content + */ + private setMDirectFromContent(content: unknown): void { + const { valid, filteredContent } = filterValidMDirect(content); + + if (!valid) { + logger.warn("Invalid m.direct content occurred", content); + } + + this.mDirectEvent = filteredContent; + } + private onAccountData = (ev: MatrixEvent): void => { + console.log("onAccountData"); + if (ev.getType() == EventType.Direct) { - this.mDirectEvent = { ...ev.getContent() }; // copy as we will mutate + this.setMDirectFromContent(ev.getContent()); this.userToRooms = null; this.roomToUser = null; } diff --git a/test/utils/DMRoomMap-test.ts b/test/utils/DMRoomMap-test.ts index 87a966a2035..62f02251bb9 100644 --- a/test/utils/DMRoomMap-test.ts +++ b/test/utils/DMRoomMap-test.ts @@ -16,10 +16,10 @@ limitations under the License. import { mocked, Mocked } from "jest-mock"; import { logger } from "matrix-js-sdk/src/logger"; -import { EventType, IContent, MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix"; +import { ClientEvent, EventType, IContent, MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix"; import DMRoomMap from "../../src/utils/DMRoomMap"; -import { mkEvent, stubClient } from "../test-utils"; +import { flushPromises, mkEvent, stubClient } from "../test-utils"; describe("DMRoomMap", () => { const roomId1 = "!room1:example.com"; const roomId2 = "!room2:example.com"; @@ -53,11 +53,46 @@ describe("DMRoomMap", () => { beforeEach(() => { client.getAccountData.mockReturnValue(mkMDirectEvent(validMDirectContent)); dmRoomMap = new DMRoomMap(client); + dmRoomMap.start(); }); it("getRoomIds should return the room Ids", () => { expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId2, roomId3, roomId4])); }); + + describe("and there is an update with valid data", () => { + beforeEach(() => { + client.emit( + ClientEvent.AccountData, + mkMDirectEvent({ + "@user:example.com": [roomId1, roomId3], + }), + ); + }); + + it("getRoomIds should return the new room Ids", () => { + expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId3])); + }); + }); + + describe("and there is an update with invalid data", () => { + const partiallyInvalidContent = { + "@user1:example.com": [roomId1, roomId3], + "@user2:example.com": "room2, room3", + }; + + beforeEach(() => { + client.emit(ClientEvent.AccountData, mkMDirectEvent(partiallyInvalidContent)); + }); + + it("getRoomIds should return the valid room Ids", () => { + expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId3])); + }); + + it("should log the invalid content", () => { + expect(logger.warn).toHaveBeenCalledWith("Invalid m.direct content occurred", partiallyInvalidContent); + }); + }); }); describe("when m.direct content contains the entire event", () => { From a203c130c0b7d93f12349e8656f715272fee53e9 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Fri, 24 Mar 2023 10:11:13 +0100 Subject: [PATCH 3/4] Fix lint issue --- test/utils/DMRoomMap-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/DMRoomMap-test.ts b/test/utils/DMRoomMap-test.ts index 62f02251bb9..03591ae54a3 100644 --- a/test/utils/DMRoomMap-test.ts +++ b/test/utils/DMRoomMap-test.ts @@ -19,7 +19,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { ClientEvent, EventType, IContent, MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix"; import DMRoomMap from "../../src/utils/DMRoomMap"; -import { flushPromises, mkEvent, stubClient } from "../test-utils"; +import { mkEvent, stubClient } from "../test-utils"; describe("DMRoomMap", () => { const roomId1 = "!room1:example.com"; const roomId2 = "!room2:example.com"; From 74649b9e245578e287014e6c8f3b4fab361d2073 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Fri, 24 Mar 2023 10:37:51 +0100 Subject: [PATCH 4/4] Use plain loop --- src/utils/dm/filterValidMDirect.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/utils/dm/filterValidMDirect.ts b/src/utils/dm/filterValidMDirect.ts index d848da0040a..a0cfabae025 100644 --- a/src/utils/dm/filterValidMDirect.ts +++ b/src/utils/dm/filterValidMDirect.ts @@ -52,14 +52,14 @@ export const filterValidMDirect = (content: unknown): FilterValidMDirectResult = const filteredRoomIds: string[] = []; filteredContent.set(userId, filteredRoomIds); - roomIds.forEach((roomId: unknown) => { + + for (const roomId of roomIds) { if (typeof roomId === "string") { filteredRoomIds.push(roomId); - return; + } else { + valid = false; } - - valid = false; - }); + } } return {