From 2ae9c50ccdef5bd011af4860158976f21b761d07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20=E2=80=9Etadzik=E2=80=9D=20So=C5=9Bnierz?= Date: Wed, 7 Aug 2024 16:02:46 +0200 Subject: [PATCH 1/4] Use appservice API to check if user can access the event they're replying to --- src/bridge/MatrixHandler.ts | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/bridge/MatrixHandler.ts b/src/bridge/MatrixHandler.ts index 4e4616732..0452ebb7e 100644 --- a/src/bridge/MatrixHandler.ts +++ b/src/bridge/MatrixHandler.ts @@ -21,7 +21,6 @@ import { trackChannelAndCreateRoom } from "./RoomCreation"; import { renderTemplate } from "../util/Template"; import { trimString } from "../util/TrimString"; import { messageDiff } from "../util/MessageDiff"; -import QuickLRU = require("quick-lru"); async function reqHandler(req: BridgeRequest, promise: PromiseLike|void) { try { @@ -146,11 +145,6 @@ export class MatrixHandler { private adminHandler: AdminRoomHandler; private config: MatrixHandlerConfig = DEFAULTS; - private memberJoinDefaultTs = Date.now(); - private memberJoinTs = new QuickLRU({ - maxSize: 8192, - }); - constructor( private readonly ircBridge: IrcBridge, config: MatrixHandlerConfig|undefined, @@ -413,12 +407,6 @@ export class MatrixHandler { * @param {Object} event : The Matrix member event. */ private _onMemberEvent(req: BridgeRequest, event: OnMemberEventData) { - if (event.content.membership === 'join') { - this.memberJoinTs.set(`${event.room_id}/${event.state_key}`, Date.now()); - } - else { - this.memberJoinTs.delete(`${event.room_id}/${event.state_key}`); - } this.memberTracker?.onEvent(event); } @@ -1350,10 +1338,9 @@ export class MatrixHandler { rplSource = cachedEvent.body; } - const senderJoinTs = this.memberJoinTs.get(`${event.room_id}/${event.sender}`) ?? this.memberJoinDefaultTs; - if (senderJoinTs > cachedEvent.timestamp) { - // User joined AFTER the event was sent (or left and joined, but we can't distinguish that). - // Do not treat as a reply. + const canAccessOriginalEvent = await bridgeIntent.matrixClient.doRequest('GET', `/_matrix/client/unstable/net.tadzik/can_user_see_event/${event.room_id}/${event.sender}/${replyEventId}`); + if (!canAccessOriginalEvent) { + // Do not allow the user to leak an event they couldn't otherwise read req.log.warn(`User ${event.sender} attempted to reply to an event before they were joined`); return { formatted: rplText, From 1b0d46d6d2715787857ccde5cb3d19dcb2d6fe79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20=E2=80=9Etadzik=E2=80=9D=20So=C5=9Bnierz?= Date: Wed, 8 Jan 2025 16:30:17 +0100 Subject: [PATCH 2/4] Use standard Synapse APIs instead of MSC4185 --- src/bridge/MatrixHandler.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/bridge/MatrixHandler.ts b/src/bridge/MatrixHandler.ts index 0452ebb7e..45e78acc0 100644 --- a/src/bridge/MatrixHandler.ts +++ b/src/bridge/MatrixHandler.ts @@ -1338,14 +1338,18 @@ export class MatrixHandler { rplSource = cachedEvent.body; } - const canAccessOriginalEvent = await bridgeIntent.matrixClient.doRequest('GET', `/_matrix/client/unstable/net.tadzik/can_user_see_event/${event.room_id}/${event.sender}/${replyEventId}`); - if (!canAccessOriginalEvent) { - // Do not allow the user to leak an event they couldn't otherwise read - req.log.warn(`User ${event.sender} attempted to reply to an event before they were joined`); - return { - formatted: rplText, - reply: rplText, - }; + try { + await bridgeIntent.matrixClient.doRequest('GET', `/_matrix/client/v3/rooms/${event.room_id}/event/${replyEventId}?user_id=${event.sender}`); + } catch (err: any) { + if (err.body?.errcode === 'M_NOT_FOUND') { + req.log.warn(`User ${event.sender} attempted to reply to an event they cannot access`); + return { + formatted: rplText, + reply: rplText, + }; + } else { + throw err; + } } // Get the first non-blank line from the source. From 84f9181257261e21e8bc8d43161f1b273825779c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20=E2=80=9Etadzik=E2=80=9D=20So=C5=9Bnierz?= Date: Wed, 8 Jan 2025 16:32:42 +0100 Subject: [PATCH 3/4] Remove a test that's no longer relevant --- spec/e2e/replies.spec.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/spec/e2e/replies.spec.ts b/spec/e2e/replies.spec.ts index d6a629855..a66c79ddf 100644 --- a/spec/e2e/replies.spec.ts +++ b/spec/e2e/replies.spec.ts @@ -120,21 +120,6 @@ describe('Reply handling', () => { event_id: await aliceEventId, }, "Oh sorry, I meant bob!"); expect((await aliceReplyMsgPromise)[2]).toContain(aliceMsgBody); - - // restart the bridge, effectively marking members as "been here forever" - await testEnv.recreateBridge(); - await testEnv.setUp(); - const postRestartAliceMsg = bob.waitForEvent('message', 10000); - const postRestartAliceMsgBody = "Hello post-restart world!"; - const postRestartAliceEventId = alice.sendText(cRoomId, postRestartAliceMsgBody); - await postRestartAliceMsg; - - const postRestartCharlieMsg = bob.waitForEvent('message', 10000); - await charlie.replyText(cRoomId, { - event_id: await postRestartAliceEventId, - }, "Hello alice!"); - const postRestartCharlieMsgBody = (await postRestartCharlieMsg)[2]; - expect(postRestartCharlieMsgBody).toContain(postRestartAliceMsgBody); }); it('should not leak the contents of messages to leavers', async () => { From 72cdcc16309f81800d4329ff7eb74f7101ecb2dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20=E2=80=9Etadzik=E2=80=9D=20So=C5=9Bnierz?= Date: Wed, 8 Jan 2025 16:47:48 +0100 Subject: [PATCH 4/4] linting --- spec/e2e/jest.config.js | 2 +- src/bridge/MatrixHandler.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/e2e/jest.config.js b/spec/e2e/jest.config.js index e78492374..312913cbb 100644 --- a/spec/e2e/jest.config.js +++ b/spec/e2e/jest.config.js @@ -2,7 +2,7 @@ module.exports = { preset: 'ts-jest', testEnvironment: 'node', - reporters: [['github-actions', {silent: false}], 'summary'], + // reporters: [['github-actions', {silent: false}], 'summary'], transformIgnorePatterns: ['/node_modules/'], testTimeout: 60000, transform: { diff --git a/src/bridge/MatrixHandler.ts b/src/bridge/MatrixHandler.ts index 45e78acc0..f4766ec76 100644 --- a/src/bridge/MatrixHandler.ts +++ b/src/bridge/MatrixHandler.ts @@ -1340,6 +1340,7 @@ export class MatrixHandler { try { await bridgeIntent.matrixClient.doRequest('GET', `/_matrix/client/v3/rooms/${event.room_id}/event/${replyEventId}?user_id=${event.sender}`); + // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (err: any) { if (err.body?.errcode === 'M_NOT_FOUND') { req.log.warn(`User ${event.sender} attempted to reply to an event they cannot access`); @@ -1347,9 +1348,8 @@ export class MatrixHandler { formatted: rplText, reply: rplText, }; - } else { - throw err; } + throw err; } // Get the first non-blank line from the source.