From 4af7d3009f10b1f2fb810784c1e491d9d3bee82b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Wed, 27 Mar 2024 10:39:26 +0100 Subject: [PATCH] Add tests for various forms of rich replies (#1799) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add tests for various forms of rich replies Signed-off-by: Tadeusz „tadzik” Sośnierz * Ensure leaving the channel and rejoining doesn't allow you to quote-reply messages you haven't seen * Fix braces --------- Signed-off-by: Tadeusz „tadzik” Sośnierz Co-authored-by: Will Hunt --- changelog.d/1799.misc | 1 + spec/e2e/replies.spec.ts | 187 +++++++++++++++++++++++++++++++ spec/integ/matrix-to-irc.spec.js | 18 +-- spec/util/e2e-test.ts | 7 ++ src/bridge/MatrixHandler.ts | 40 +++++-- 5 files changed, 237 insertions(+), 16 deletions(-) create mode 100644 changelog.d/1799.misc create mode 100644 spec/e2e/replies.spec.ts diff --git a/changelog.d/1799.misc b/changelog.d/1799.misc new file mode 100644 index 000000000..4a1f2393d --- /dev/null +++ b/changelog.d/1799.misc @@ -0,0 +1 @@ +Add tests for various forms of rich replies. diff --git a/spec/e2e/replies.spec.ts b/spec/e2e/replies.spec.ts new file mode 100644 index 000000000..652bacd32 --- /dev/null +++ b/spec/e2e/replies.spec.ts @@ -0,0 +1,187 @@ +import { TestIrcServer } from "matrix-org-irc"; +import { IrcBridgeE2ETest } from "../util/e2e-test"; +import { describe, expect, it } from "@jest/globals"; + + +describe('Reply handling', () => { + let testEnv: IrcBridgeE2ETest; + async function setupTestEnv(shortReplyTresholdSeconds: number) { + testEnv = await IrcBridgeE2ETest.createTestEnv({ + matrixLocalparts: [TestIrcServer.generateUniqueNick("alice"), TestIrcServer.generateUniqueNick("charlie")], + ircNicks: ['bob'], + traceToFile: true, + shortReplyTresholdSeconds, + }); + await testEnv.setUp(); + } + afterEach(() => { + return testEnv?.tearDown(); + }); + + it('should use short and long reply formats, depending on elapsed time', async () => { + await setupTestEnv(1); + + const channel = `#${TestIrcServer.generateUniqueNick("test")}`; + const { homeserver } = testEnv; + const [alice, charlie] = homeserver.users.map(u => u.client); + const { bob } = testEnv.ircTest.clients; + + await bob.join(channel); + + const adminRoomId = await testEnv.createAdminRoomHelper(alice); + const cRoomId = await testEnv.joinChannelHelper(alice, adminRoomId, channel); + await charlie.joinRoom(cRoomId); + + const bobUserId = `@irc_${bob.nick}:${homeserver.domain}`; + await alice.waitForRoomEvent( + {eventType: 'm.room.member', sender: bobUserId, stateKey: bobUserId, roomId: cRoomId} + ); + + // first message is always a bit delayed, so let's send&await it ahead of time before we get to testing + let bridgedMessage = bob.waitForEvent('message', 10000); + await alice.sendText(cRoomId, "warming up..."); + await bridgedMessage; + + const originalMessageBody = "Original message"; + bridgedMessage = bob.waitForEvent('message', 10000); + const originalMessageId = await alice.sendText(cRoomId, originalMessageBody); + await bridgedMessage; + + bridgedMessage = bob.waitForEvent('message', 10000); + await charlie.replyText(cRoomId, { + event_id: originalMessageId, + }, "Short reply"); + let ircMessage = await bridgedMessage; + + expect(ircMessage[2]).toContain("Short reply"); + expect(ircMessage[2]).not.toContain("Original message"); + + await new Promise(r => setTimeout(r, 1000)); + + bridgedMessage = bob.waitForEvent('message', 10000); + await charlie.replyText(cRoomId, { + event_id: originalMessageId, + }, "Long reply"); + ircMessage = await bridgedMessage; + + expect(ircMessage[2]).toContain("Long reply"); + expect(ircMessage[2]).toContain("Original message"); + }); + it('should not leak the contents of messages to new joiners', async () => { + await setupTestEnv(0); + + const channel = `#${TestIrcServer.generateUniqueNick("test")}`; + const { homeserver, ircBridge } = testEnv; + const [alice, charlie] = homeserver.users.map(u => u.client); + const { bob } = testEnv.ircTest.clients; + + // Create the channel + await bob.join(channel); + + const adminRoomId = await testEnv.createAdminRoomHelper(alice); + const cRoomId = await testEnv.joinChannelHelper(alice, adminRoomId, channel); + const roomName = await alice.getRoomStateEvent(cRoomId, 'm.room.name', ''); + expect(roomName.name).toEqual(channel); + + // And finally wait for bob to appear. + const bobUserId = `@irc_${bob.nick}:${homeserver.domain}`; + await alice.waitForRoomEvent( + {eventType: 'm.room.member', sender: bobUserId, stateKey: bobUserId, roomId: cRoomId} + ); + + // Send some messages + const aliceMsg = bob.waitForEvent('message', 10000); + const bobMsg = alice.waitForRoomEvent( + {eventType: 'm.room.message', sender: bobUserId, roomId: cRoomId} + ); + const aliceMsgBody = "Hello bib!"; + const aliceEventId = alice.sendText(cRoomId, aliceMsgBody); + await aliceMsg; + bob.say(channel, "Hi alice!"); + await bobMsg; + + // Now charlie joins and tries to reply to alice. + await charlie.joinRoom(cRoomId); + const charlieMsgIrcPromise = bob.waitForEvent('message', 10000); + await charlie.replyText(cRoomId, { + event_id: await aliceEventId, + }, "Sneaky reply to a message I have not seen"); + + // Safety check to ensure that we're using the long form reply format. + expect(ircBridge.config.ircService.matrixHandler?.shortReplyTresholdSeconds).toBe(0); + // The long form reply format should not contain alice's message. + const charlieIrcMessage = (await charlieMsgIrcPromise)[2]; + expect(charlieIrcMessage).not.toContain(aliceMsgBody); + + // Now check that alice can reply, as they have been in the room long enough. + const aliceReplyMsgPromise = bob.waitForEvent('message', 10000); + await alice.replyText(cRoomId, { + 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 () => { + await setupTestEnv(0); + + const channel = `#${TestIrcServer.generateUniqueNick("test")}`; + const { homeserver, ircBridge } = testEnv; + const [alice, charlie] = homeserver.users.map(u => u.client); + const { bob } = testEnv.ircTest.clients; + + // Create the channel + await bob.join(channel); + + const adminRoomId = await testEnv.createAdminRoomHelper(alice); + const cRoomId = await testEnv.joinChannelHelper(alice, adminRoomId, channel); + const roomName = await alice.getRoomStateEvent(cRoomId, 'm.room.name', ''); + expect(roomName.name).toEqual(channel); + + const bobUserId = `@irc_${bob.nick}:${homeserver.domain}`; + await alice.waitForRoomEvent( + {eventType: 'm.room.member', sender: bobUserId, stateKey: bobUserId, roomId: cRoomId} + ); + + await charlie.joinRoom(cRoomId); + await charlie.leaveRoom(cRoomId); + + // Send some messages + const aliceMsg = bob.waitForEvent('message', 10000); + const bobMsg = alice.waitForRoomEvent( + {eventType: 'm.room.message', sender: bobUserId, roomId: cRoomId} + ); + const aliceMsgBody = "Hello bib!"; + const aliceEventId = alice.sendText(cRoomId, aliceMsgBody); + await aliceMsg; + bob.say(channel, "Hi alice!"); + await bobMsg; + + // Now charlie joins and tries to reply to alice. + await charlie.joinRoom(cRoomId); + const charlieMsgIrcPromise = bob.waitForEvent('message', 10000); + await charlie.replyText(cRoomId, { + event_id: await aliceEventId, + }, "Sneaky reply to a message I have not seen"); + + // Safety check to ensure that we're using the long form reply format. + expect(ircBridge.config.ircService.matrixHandler?.shortReplyTresholdSeconds).toBe(0); + // The long form reply format should not contain alice's message. + const charlieIrcMessage = (await charlieMsgIrcPromise)[2]; + expect(charlieIrcMessage).not.toContain(aliceMsgBody); + }); +}); diff --git a/spec/integ/matrix-to-irc.spec.js b/spec/integ/matrix-to-irc.spec.js index 17d4c3b04..9aff5ede5 100644 --- a/spec/integ/matrix-to-irc.spec.js +++ b/spec/integ/matrix-to-irc.spec.js @@ -244,7 +244,7 @@ describe("Matrix-to-IRC message bridging", function() { room_id: roomMapping.roomId, sender: repliesUser.id, event_id: "$original:bar.com", - origin_server_ts: 1_000, + origin_server_ts: Date.now(), type: "m.room.message" }); const p = env.ircMock._whenClient(roomMapping.server, testUser.nick, "say", @@ -274,7 +274,7 @@ describe("Matrix-to-IRC message bridging", function() { }, sender: testUser.id, room_id: roomMapping.roomId, - origin_server_ts: 2_000, + origin_server_ts: Date.now(), type: "m.room.message" }); await p; @@ -290,7 +290,7 @@ describe("Matrix-to-IRC message bridging", function() { room_id: roomMapping.roomId, sender: repliesUser.id, event_id: "$original:bar.com", - origin_server_ts: 1_000, + origin_server_ts: Date.now(), type: "m.room.message" }); const p = env.ircMock._whenClient(roomMapping.server, testUser.nick, "say", @@ -320,7 +320,7 @@ describe("Matrix-to-IRC message bridging", function() { }, sender: testUser.id, room_id: roomMapping.roomId, - origin_server_ts: 1_000_000, + origin_server_ts: Date.now() + 1_000_000, type: "m.room.message" }); await p; @@ -381,7 +381,7 @@ describe("Matrix-to-IRC message bridging", function() { room_id: roomMapping.roomId, sender: repliesUser.id, event_id: "$original:bar.com", - origin_server_ts: 1_000, + origin_server_ts: Date.now(), type: "m.room.message" }); const p = env.ircMock._whenClient(roomMapping.server, testUser.nick, "say", @@ -411,7 +411,7 @@ describe("Matrix-to-IRC message bridging", function() { }, sender: testUser.id, room_id: roomMapping.roomId, - origin_server_ts: 1_000_000, + origin_server_ts: Date.now() + 1_000_000, type: "m.room.message" }); await p; @@ -465,7 +465,7 @@ describe("Matrix-to-IRC message bridging", function() { room_id: roomMapping.roomId, sender: repliesUser.id, event_id: "$first:bar.com", - origin_server_ts: 1_000, + origin_server_ts: Date.now(), type: "m.room.message" }) @@ -484,7 +484,7 @@ describe("Matrix-to-IRC message bridging", function() { room_id: roomMapping.roomId, sender: repliesUser.id, event_id: "$second:bar.com", - origin_server_ts: 1_000_000, + origin_server_ts: Date.now() + 1_000_000, type: "m.room.message" }); @@ -517,7 +517,7 @@ describe("Matrix-to-IRC message bridging", function() { }, sender: testUser.id, room_id: roomMapping.roomId, - origin_server_ts: 2_000_000, + origin_server_ts: Date.now() + 2_000_000, type: "m.room.message" }); diff --git a/spec/util/e2e-test.ts b/spec/util/e2e-test.ts index a94fcbff5..30b8fb085 100644 --- a/spec/util/e2e-test.ts +++ b/spec/util/e2e-test.ts @@ -11,6 +11,7 @@ import { expect } from "@jest/globals"; import dns from 'node:dns'; import fs from "node:fs/promises"; import { WriteStream, createWriteStream } from "node:fs"; +import { DEFAULTS as MatrixHandlerDefaults } from "../../src/bridge/MatrixHandler"; // Needed to make tests work on GitHub actions. Node 17+ defaults // to IPv6, and the homerunner domain resolves to IPv6, but the // runtime doesn't actually support IPv6 🤦 @@ -22,12 +23,14 @@ const DEFAULT_PORT = parseInt(process.env.IRC_TEST_PORT ?? '6667', 10); const DEFAULT_ADDRESS = process.env.IRC_TEST_ADDRESS ?? "127.0.0.1"; const IRCBRIDGE_TEST_REDIS_URL = process.env.IRCBRIDGE_TEST_REDIS_URL; + interface Opts { matrixLocalparts?: string[]; ircNicks?: string[]; timeout?: number; config?: Partial, traceToFile?: boolean, + shortReplyTresholdSeconds?: number, } const traceFilePath = '.e2e-traces'; @@ -235,6 +238,10 @@ export class IrcBridgeE2ETest { ircHandler: { powerLevelGracePeriodMs: 0, }, + matrixHandler: { + ...MatrixHandlerDefaults, + shortReplyTresholdSeconds: opts.shortReplyTresholdSeconds ?? 0, + }, servers: { localhost: { ...IrcServer.DEFAULT_CONFIG, diff --git a/src/bridge/MatrixHandler.ts b/src/bridge/MatrixHandler.ts index 1864ca7d0..0d6a405a4 100644 --- a/src/bridge/MatrixHandler.ts +++ b/src/bridge/MatrixHandler.ts @@ -21,6 +21,7 @@ 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 { @@ -52,7 +53,7 @@ export interface MatrixHandlerConfig { truncatedMessageTemplate: string; } -const DEFAULTS: MatrixHandlerConfig = { +export const DEFAULTS: MatrixHandlerConfig = { eventCacheSize: 4096, replySourceMaxLength: 32, shortReplyTresholdSeconds: 5 * 60, @@ -106,6 +107,7 @@ export interface OnMemberEventData { state_key: string; type: string; event_id: string; + origin_server_ts?: number; content: { displayname?: string; membership: string; @@ -135,6 +137,11 @@ 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, @@ -386,6 +393,12 @@ 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}`, event.origin_server_ts ?? Date.now()); + } + else { + this.memberJoinTs.delete(`${event.room_id}/${event.state_key}`); + } this.memberTracker?.onEvent(event); } @@ -1039,7 +1052,7 @@ export class MatrixHandler { // special handling for replies (and threads) if (event.content["m.relates_to"] && event.content["m.relates_to"]["m.in_reply_to"]) { const eventId = event.content["m.relates_to"]["m.in_reply_to"].event_id; - const reply = await this.textForReplyEvent(event, eventId, ircRoom); + const reply = await this.textForReplyEvent(req, event, eventId, ircRoom); if (reply !== null) { ircAction.text = reply.formatted; cacheBody = reply.reply; @@ -1260,8 +1273,10 @@ export class MatrixHandler { await this.ircBridge.getMatrixUser(ircUser); } - private async textForReplyEvent(event: MatrixMessageEvent, replyEventId: string, ircRoom: IrcRoom): - Promise<{formatted: string; reply: string}|null> { + private async textForReplyEvent( + req: BridgeRequest, event: MatrixMessageEvent, replyEventId: string, ircRoom: IrcRoom + ): Promise<{formatted: string; reply: string}|null> { + const bridgeIntent = this.ircBridge.getAppServiceBridge().getIntent(); // strips out the quotation of the original message, if needed const replyText = (body: string): string => { const REPLY_REGEX = /> <(.*?)>(.*?)\n\n([\s\S]*)/; @@ -1285,7 +1300,7 @@ export class MatrixHandler { if (!cachedEvent) { // Fallback to fetching from the homeserver. try { - const eventContent = await this.ircBridge.getAppServiceBridge().getIntent().getEvent( + const eventContent = await bridgeIntent.getEvent( event.room_id, replyEventId ); rplName = eventContent.sender; @@ -1317,6 +1332,17 @@ 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. + req.log.warn(`User ${event.sender} attempted to reply to an event before they were joined`); + return { + formatted: rplText, + reply: rplText, + }; + } + // Get the first non-blank line from the source. const lines = rplSource.split('\n').filter((line) => !/^\s*$/.test(line)) if (lines.length > 0) { @@ -1347,8 +1373,8 @@ export class MatrixHandler { } let replyTemplate: string; - const tresholdMs = (this.config.shortReplyTresholdSeconds) * 1000; - if (rplSource && event.origin_server_ts - cachedEvent.timestamp > tresholdMs) { + const thresholdMs = (this.config.shortReplyTresholdSeconds) * 1000; + if (rplSource && event.origin_server_ts - cachedEvent.timestamp > thresholdMs) { replyTemplate = this.config.longReplyTemplate; } else {