diff --git a/.eslintrc.json b/.eslintrc.json index 5e00870d..b83be258 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -1,6 +1,4 @@ { - "parser": "@typescript-eslint/parser", - "plugins": ["@typescript-eslint"], "parserOptions": { "ecmaVersion": 9, "ecmaFeatures": { @@ -11,7 +9,6 @@ "node": true, "jasmine": true }, - "extends": ["plugin:@typescript-eslint/recommended"], "rules": { "consistent-return": "error", "curly": "error", @@ -81,5 +78,13 @@ "@typescript-eslint/explicit-module-boundary-types": "off", "@typescript-eslint/no-explicit-any": "warn", "@typescript-eslint/no-shadow": ["error"] - } -} \ No newline at end of file + }, + "overrides": [ + { + "files": ["*.ts"], + "parser": "@typescript-eslint/parser", + "plugins": ["@typescript-eslint"], + "extends": ["plugin:@typescript-eslint/recommended"] + } + ] +} diff --git a/changelog.d/329.feature b/changelog.d/329.feature new file mode 100644 index 00000000..7f46aaf1 --- /dev/null +++ b/changelog.d/329.feature @@ -0,0 +1 @@ +Validate that the sender of a message edit matches the original sender. diff --git a/spec/.eslintrc.json b/spec/.eslintrc.json new file mode 100644 index 00000000..8fb25edd --- /dev/null +++ b/spec/.eslintrc.json @@ -0,0 +1,4 @@ +{ + "strict": "off", + "no-var": "off" +} diff --git a/spec/integ/bridge.spec.js b/spec/integ/bridge.spec.js index 214f648b..2514d6aa 100644 --- a/spec/integ/bridge.spec.js +++ b/spec/integ/bridge.spec.js @@ -10,8 +10,10 @@ const BOT_USER_ID = `@${BOT_LOCALPART}:${HS_DOMAIN}`; const TEST_USER_DB_PATH = __dirname + "/test-users.db"; const TEST_ROOM_DB_PATH = __dirname + "/test-rooms.db"; +const TEST_EVENT_DB_PATH = __dirname + "/test-events.db"; const UserBridgeStore = require("../..").UserBridgeStore; const RoomBridgeStore = require("../..").RoomBridgeStore; +const EventBridgeStore = require("../..").EventBridgeStore; const MatrixUser = require("../..").MatrixUser; const RemoteUser = require("../..").RemoteUser; const MatrixRoom = require("../..").MatrixRoom; @@ -23,7 +25,7 @@ const deferPromise = require("../../lib/utils/promiseutil").defer; describe("Bridge", function() { var bridge, bridgeCtrl, appService, clientFactory, appServiceRegistration; - var roomStore, userStore, clients; + var roomStore, userStore, eventStore, clients; beforeEach( /** @this */ @@ -97,16 +99,19 @@ describe("Bridge", function() { Promise.all([ loadDatabase(TEST_USER_DB_PATH, UserBridgeStore), - loadDatabase(TEST_ROOM_DB_PATH, RoomBridgeStore) - ]).then(([userDb, roomDb]) => { + loadDatabase(TEST_ROOM_DB_PATH, RoomBridgeStore), + loadDatabase(TEST_EVENT_DB_PATH, EventBridgeStore) + ]).then(([userDb, roomDb, eventDb]) => { userStore = userDb; roomStore = roomDb; + eventStore = eventDb; bridge = new Bridge({ homeserverUrl: HS_URL, domain: HS_DOMAIN, registration: appServiceRegistration, userStore: userDb, roomStore: roomDb, + eventStore: eventDb, controller: bridgeCtrl, clientFactory: clientFactory }); @@ -129,6 +134,12 @@ describe("Bridge", function() { catch (e) { // do nothing } + try { + fs.unlinkSync(TEST_EVENT_DB_PATH); + } + catch (e) { + // do nothing + } }); describe("onUserQuery", function() { @@ -319,6 +330,137 @@ describe("Bridge", function() { expect(bridgeCtrl.onEvent).not.toHaveBeenCalled(); }); + describe('opts.eventValidation.validateEditSender', () => { + async function setupBridge(eventValidation) { + const bridge = new Bridge({ + homeserverUrl: HS_URL, + domain: HS_DOMAIN, + registration: appServiceRegistration, + userStore: userStore, + roomStore: roomStore, + controller: bridgeCtrl, + clientFactory: clientFactory, + disableContext: true, + eventValidation + }); + await bridge.run(101, {}, appService); + + return bridge; + } + + function createMessageEditEvent(sender) { + const event = { + content: { + body: ' * my message edit', + 'm.new_content': { body: 'my message edit', msgtype: 'm.text' }, + 'm.relates_to': { + event_id: '$ZrXenSQt4TbtHnMclrWNJdiP7SrRCSdl3tAYS81H2bs', + rel_type: 'm.replace' + }, + msgtype: 'm.text' + }, + event_id: '$tagvjsXZqBOBWtHijq2qg0Un-uqVunrFLxiJyOIVGQ8', + room_id: '!dtJaPyDtsoOLTgJVmy:my.matrix.host', + sender, + type: 'm.room.message', + }; + + return event; + } + + let botClient; + beforeEach(async () => { + botClient = clients["bot"]; + botClient.getJoinedRoomMembers.and.returnValue(Promise.resolve({ + joined: { + [botClient.credentials.userId]: { + display_name: "bot" + } + } + })); + + // Mock onEvent callback + bridgeCtrl.onEvent.and.callFake(function(req) { req.resolve(); }); + }); + + describe('when enabled', () => { + beforeEach(async () => { + bridge = await setupBridge({ + validateEditSender: { + allowEventOnLookupFail: false, + } + }) + }); + + it("should suppress the event if the edit is coming from a different person than the original message", async() => { + const event = createMessageEditEvent('@root:my.matrix.host'); + + botClient.fetchRoomEvent.and.returnValue(Promise.resolve({ + event_id: '$ZrXenSQt4TbtHnMclrWNJdiP7SrRCSdl3tAYS81H2bs', + // The original message has different sender than the edit event + sender: '@some-other-user:different.host', + })); + + await appService.emit("event", event); + expect(bridgeCtrl.onEvent).not.toHaveBeenCalled(); + }); + + it("should emit event when the edit sender matches the original message sender", async() => { + const event = createMessageEditEvent('@root:my.matrix.host'); + + botClient.fetchRoomEvent.and.returnValue(Promise.resolve({ + event_id: '$ZrXenSQt4TbtHnMclrWNJdiP7SrRCSdl3tAYS81H2bs', + // The original message sender is the same as the edit event + sender: '@root:my.matrix.host', + })); + + await appService.emit("event", event); + expect(bridgeCtrl.onEvent).toHaveBeenCalled(); + }); + }); + + it('allowEventOnLookupFail=true should still emit event when failed to fetch original event', async() => { + const event = createMessageEditEvent('@root:my.matrix.host'); + + bridge = await setupBridge({ + validateEditSender: { + // Option of interest for this test is here! + allowEventOnLookupFail: true, + } + }) + + const botClient = clients["bot"]; + botClient.getJoinedRoomMembers.and.returnValue(Promise.resolve({ + joined: { + [botClient.credentials.userId]: { + display_name: "bot" + } + } + })); + botClient.fetchRoomEvent.and.returnValue(Promise.reject(new Error('Some problem fetching original event'))); + + await appService.emit("event", event); + expect(bridgeCtrl.onEvent).toHaveBeenCalled(); + }) + + describe('when disabled', () => { + it("should emit event even when the edit sender does NOT match the original message sender", async() => { + const event = createMessageEditEvent('@root:my.matrix.host'); + + bridge = await setupBridge(undefined) + + botClient.fetchRoomEvent.and.returnValue(Promise.resolve({ + event_id: '$ZrXenSQt4TbtHnMclrWNJdiP7SrRCSdl3tAYS81H2bs', + // The original message has different sender than the edit event + sender: '@some-other-user:different.host', + })); + + await appService.emit("event", event); + expect(bridgeCtrl.onEvent).toHaveBeenCalled(); + }); + }) + }); + it("should invoke the user-supplied onEvent function with the right args", function(done) { var event = { @@ -488,6 +630,11 @@ describe("Bridge", function() { expect(bridge.getUserStore()).toEqual(userStore); }); + it("should be able to getEventStore", async() => { + await bridge.run(101, {}, appService); + expect(bridge.getEventStore()).toEqual(eventStore); + }); + it("should be able to getRequestFactory", async() => { await bridge.run(101, {}, appService); expect(bridge.getRequestFactory()).toBeDefined(); @@ -792,7 +939,7 @@ function mkMockMatrixClient(uid) { var client = jasmine.createSpyObj( "MatrixClient", [ "register", "joinRoom", "credentials", "createRoom", "setDisplayName", - "setAvatarUrl", "_http" + "setAvatarUrl", "fetchRoomEvent", "getJoinedRoomMembers", "_http" ] ); // Shim requests to authedRequestWithPrefix to register() if it is diff --git a/src/bridge.ts b/src/bridge.ts index 09865a17..26083531 100644 --- a/src/bridge.ts +++ b/src/bridge.ts @@ -250,6 +250,19 @@ export interface BridgeOpts { homeserverUrl: string; store: ClientEncryptionStore; }; + + eventValidation?: { + /** + * Should we validate that the sender of an edit matches the parent event. + */ + validateEditSender?: { + /** + * If the parent edit event could not be found, + * should the event be rejected. + */ + allowEventOnLookupFail: boolean; + }; + }; } interface VettedBridgeOpts { @@ -382,6 +395,11 @@ interface VettedBridgeOpts { homeserverUrl: string; store: ClientEncryptionStore; }; + eventValidation?: { + validateEditSender?: { + allowEventOnLookupFail: boolean; + }; + }; } export class Bridge { @@ -462,6 +480,11 @@ export class Bridge { }, logRequestOutcome: opts.logRequestOutcome ?? true, suppressEcho: opts.suppressEcho ?? true, + eventValidation: opts.hasOwnProperty("eventValidation") ? opts.eventValidation : { + validateEditSender: { + allowEventOnLookupFail: false + } + } }; this.queue = EventQueue.create(this.opts.queue, this.onConsume.bind(this)); @@ -1195,6 +1218,59 @@ export class Bridge { } } + /** + * Find a member for a given room. This method will fetch the joined members + * from the homeserver if the cache doesn't have it stored. + * @param preferBot Should we prefer the bot user over a ghost user + * @returns {Promise} The userID of the member. + */ + public async getAnyASMemberInRoom(roomId: string, preferBot = true) { + if (!this.registration) { + throw Error('Registration must be defined before you can call this'); + } + let members = this.membershipCache.getMembersForRoom(roomId, "join"); + if (!members) { + if (!this.appServiceBot) { + throw Error('AS Bot not defined yet'); + } + members = Object.keys(await this.appServiceBot.getJoinedMembers(roomId)); + } + if (preferBot && members?.includes(this.botUserId)) { + return this.botUserId; + } + const reg = this.registration; + return members.find((u) => reg.isUserMatch(u, false)) || null; + } + + private async validateEditEvent(event: WeakEvent, parentEventId: string, allowEventOnLookupFail: boolean) { + try { + const roomMember = await this.getAnyASMemberInRoom(event.room_id); + if (!roomMember) { + throw Error('No member in room, cannot handle edit'); + } + const relatedEvent = await this.getIntent(roomMember).getEvent( + event.room_id, + parentEventId, + true + ); + // Only allow edits from the same sender + if (relatedEvent.sender !== event.sender) { + log.warn( + `Rejecting ${event.event_id}: Message edit sender did NOT match the original message (${parentEventId})` + ); + return false; + } + } + catch (ex) { + if (!allowEventOnLookupFail) { + log.warn(`Rejecting ${event.event_id}: Unable to fetch parent event ${parentEventId}`, ex); + return false; + } + log.warn(`Allowing event ${event.event_id}: Unable to fetch parent event ${parentEventId}`, ex); + } + return true; + } + // returns a Promise for the request linked to this event for testing. private async onEvent(event: WeakEvent) { if (!this.registration) { @@ -1214,6 +1290,22 @@ export class Bridge { return null; } + // eslint-disable-next-line camelcase + const relatesTo = event.content?.['m.relates_to'] as { event_id?: string; rel_type: "m.replace";}|undefined; + const editOptions = this.opts.eventValidation?.validateEditSender; + + if ( + event.type === 'm.room.message' && + relatesTo?.rel_type === 'm.replace' && + relatesTo.event_id && + editOptions + ) { + // Event rejected. + if (!await this.validateEditEvent(event, relatesTo.event_id, editOptions.allowEventOnLookupFail)) { + return null; + } + } + if (this.roomUpgradeHandler && this.opts.roomUpgradeOpts && this.appServiceBot) { // m.room.tombstone is the event that signals a room upgrade. if (event.type === "m.room.tombstone" && isCanonicalState && this.roomUpgradeHandler) {