Add Protection against mention flooding #210

MTRNord · 2022-02-06T09:12:50Z

This basically tries to solve the current way of spam.

This is an adapted wordfilter protection. At the time of writing, this is not tested anywhere.

It only matches mxid based mentions currently. Not other mentions per default push rules.

TODO

Finish the warning branch
Test it (I have a docker of this branch at coreharbor.kubernetes.midnightthoughts.space/matrixdotorg/mjolnir:mentionflood-protection )

Signed-off-by: Marcel Radzio <mtrnord[at]nordgedanken.dev>

Yoric

Thanks a lot for this PR!

src/protections/MentionFlood.ts

MTRNord · 2022-02-06T11:02:55Z

Seems like my vscode likes to reformat stuff differently somehow :/ I can revert the unrelated formatting changes if needed.

src/protections/MentionFlood.ts

Yoric · 2022-02-14T09:39:34Z

Oops, looks like CI was waiting for approval before launching.

Let's see how it goes.

jesopo

big thank for the PR

src/protections/ProtectionSettings.ts

src/protections/MentionFlood.ts

cremesk · 2022-03-01T08:55:33Z

can we merge it please? i really need this protection :D

Yoric · 2022-03-07T09:15:25Z

@MTRNord Just back from trip, I'll try and review this today or tomorrow.

Yoric

PR looks good, could you add a test?

src/protections/MentionFlood.ts

src/protections/ProtectionSettings.ts

Yoric · 2022-03-08T10:56:57Z

@MTRNord I realize I wasn't clear: when I write "test", I mean a new integration test, see e.g. test/integration.

…ling due to pills

…s pills and also dont fail on null being returned by String.prototype.match

…values

…T_MINUTES_BEFORE_TRUSTING

Yoric · 2022-04-07T07:17:26Z

@MTRNord What's the status of this PR?

MTRNord · 2022-04-07T07:19:21Z

@MTRNord What's the status of this PR?

Sorry, I nearly have the tests done, but there is some small thing I still need to do. Just didn't get to it yet due to other work related tasks that took higher priority. Want to do the rest of the code on the weekend.

Yoric · 2022-04-07T07:23:53Z

Don't hesitate to yell if you need help!

Gnuxie

Made some observations about the test, you don't need to act on them but they're just things that would keep them cleaner this or next time

Gnuxie · 2022-05-04T14:55:45Z

test/integration/protections/MentionFloodTest.ts

+        } finally {
+            await moderator.stop();
+            await userA.stop();
+        }


doesn't matter too much but since you stop these clients in the afterEach hook you should be able to take out these try/finally blocks

Gnuxie · 2022-05-04T15:12:57Z

test/integration/protections/MentionFloodTest.ts

+        const userA = await newTestUser({ name: { contains: "a" } });
+
+        this.moderator = moderator;
+        this.userA = userA;


nitpick but userA doesn't tell us what this use is doing. they could unrelated to moderation of the room, they could just be a participant, they could even be a user who is going to be spamming, who is userA?

Gnuxie · 2022-05-04T15:26:37Z

test/integration/protections/protectionUtils.ts

+import { MatrixClient } from "matrix-bot-sdk";
+
+/**
+ * Returns a promise that resolves to the first event while sending events.
+ * @param client A MatrixClient that is already in the targetRoom. We will use it to listen for the event produced by targetEventThunk.
+ * This function assumes that the start() has already been called on the client.
+ * @param targetRoom The room to listen for the reply in.
+ * @param produceEvents A function that produces an events when called.
+ * @returns The first event.
+ */
+export async function getFirstMessage(client: MatrixClient, targetRoom: string, produceEvents: () => Promise<any>): Promise<any> {
+    let reactionEvents = [];
+    const addEvent = function (roomId, event) {
+        if (roomId !== targetRoom) return;
+        if (event.type !== 'm.room.message') return;
+        reactionEvents.push(event);
+    };
+    let targetCb;
+    try {
+        client.on('room.event', addEvent);
+        await produceEvents();
+        for (let event of reactionEvents) {
+            return event;
+        }
+        return await new Promise(resolve => {
+            targetCb = function (roomId, event) {
+                if (roomId !== targetRoom) return;
+                if (event.type !== 'm.room.message') return;
+                resolve(event);
+            };
+            client.on('room.event', targetCb);
+        });
+    } finally {
+        client.removeListener('room.event', addEvent);
+        if (targetCb) {
+            client.removeListener('room.event', targetCb);
+        }
+    }
+}
+
+/**
+ * Returns true if the user got changed to the wanted membership type.
+ * @param client A MatrixClient that is already in the targetRoom. We will use it to listen for the event produced by targetEventThunk.
+ * This function assumes that the start() has already been called on the client.
+ * @param targetRoom The room to listen for the reply in.
+ * @param userId The user that should get checked for.
+ * @param membership The membership type that the user should get.
+ * @returns The first event.
+ */
+export async function checkMembershipChange(client: MatrixClient, targetRoom: string, userId: string, membership: string): Promise<boolean> {
+    let membershipEvents = [];
+    const addEvent = function (roomId, event) {
+        if (roomId !== targetRoom) return;
+        if (event.type !== 'm.room.member') return;
+        membershipEvents.push(event);
+    };
+    let targetCb;
+    try {
+        client.on('room.event', addEvent);
+        for (let event of membershipEvents) {
+            if (event.state_key == userId && event.content.membership == membership) {
+                return true;
+            } else if (event.state_key == userId && event.content.membership != membership) {
+                return false;
+            }
+        }
+        return await new Promise(resolve => {
+            targetCb = function (roomId, event) {
+                if (roomId !== targetRoom) return;
+                if (event.type !== 'm.room.member') return;
+                if (event.state_key == userId && event.content.membership == membership) {
+                    resolve(true);
+                } else if (event.state_key == userId && event.content.membership != membership) {
+                    resolve(false);
+                }
+            };
+            client.on('room.event', targetCb);
+        });
+    } finally {
+        client.removeListener('room.event', addEvent);
+        if (targetCb) {
+            client.removeListener('room.event', targetCb);
+        }
+    }
+}


I know it's not your fault/related to this PR necessarily, but i am a little worried now about how this pattern is being used everywhere https://github.com/MTRNord/mjolnir/blob/MTRNord/mention-flood/test/integration/commands/commandUtils.ts#L5-L97

Yoric · 2022-05-09T08:43:50Z

Sorry for the delay, I'm just back from vacation, I'll try and review this today.

Yoric

Quite good!
You'll need a few more changes, but we're getting there.

Yoric · 2022-05-09T08:45:26Z

src/protections/MentionFlood.ts

+const DEFAULT_ACTION = "ban";
+
+const LOCALPART_REGEX = "[0-9a-z-.=_/]+";
+// https://github.com/johno/domain-regex/blob/8a6984c8fa1fe8481a4b99be0fa7f2a01ee17517/index.js


Nit: Could you clarify this comment?

e.g.

/** * A regex to extract a domain name. * Based on https:// ... */

Same things below.

Yoric · 2022-05-09T08:45:48Z

src/protections/MentionFlood.ts

+const LOCALPART_REGEX = "[0-9a-z-.=_/]+";
+// https://github.com/johno/domain-regex/blob/8a6984c8fa1fe8481a4b99be0fa7f2a01ee17517/index.js
+const DOMAIN_REGEX = "(\\b((?=[a-z0-9-]{1,63}\\.)(xn--)?[a-z0-9]+(-[a-z0-9]+)*\\.)+[a-z]{2,63}\\b)";
+// https://stackoverflow.com/a/5284410


Nit: one line between the various const would help with readability.

Yoric · 2022-05-09T08:46:45Z

src/protections/MentionFlood.ts

+import { htmlEscape } from "../utils";
+import { BooleanProtectionSetting, DurationMSProtectionSetting, NumberProtectionSetting, OptionListProtectionSetting } from "./ProtectionSettings";
+
+const DEFAULT_MINUTES_BEFORE_TRUSTING = 20 * 60 * 1000;


Nit: Doesn't look like minutes to me.

Yoric · 2022-05-09T08:56:49Z

src/protections/MentionFlood.ts

+
+    settings = {
+        // Time in which this protection takes action on users
+        minutesBeforeTrusting: new DurationMSProtectionSetting(DEFAULT_MINUTES_BEFORE_TRUSTING),


Definitely not minutes.

Yoric · 2022-05-09T09:05:40Z

src/protections/MentionFlood.ts

+            if (minsBeforeTrusting > 0) {
+                const joinTime = mjolnir.roomJoins.getUserJoin({ roomId: roomId, userId: event['sender'] });
+                // If we know the user and have its time we check if.
+                // Otherwise we assume a bug and still mark them as suspect just to make sure.


This means that everybody who joined before the start of Mjölnir is considered suspect. I believe that you should rather take the opposite policy and assume that if we don't have a joinTime, the user has been there for a long time.

Yoric · 2022-05-09T09:16:28Z

src/protections/MentionFlood.ts

+        if (event['type'] === 'm.room.message') {
+            const message: string = content['formatted_body'] || content['body'] || "";
+
+            // Check conditions first


Nit: Could you mention that we check conditions first because it's fast?

Yoric · 2022-05-09T09:17:09Z

src/protections/MentionFlood.ts

+        const content = event['content'] || {};
+        const minsBeforeTrusting = this.settings.minutesBeforeTrusting.value;
+
+        if (event['type'] === 'm.room.message') {


Nit: I think it would be a little more readable if you did the opposite, e.g.

if (event['type'] !== 'm.room.message') { return; }

Yoric · 2022-05-09T10:58:36Z

test/integration/protections/MentionFloodTest.ts

+                return await moderator.sendMessage(this.mjolnir.managementRoomId, { msgtype: 'm.text', body: `!mjolnir enable MentionFlood` });
+            });
+        } finally {
+            await moderator.stop();


Unfortunately, there are cases in which this finally will not be executed. In particular, in case of timeout, or if the call to joinRoom fails for some reason. This causes massive breakages in our test suite.

Instead of this finally, you need to declare the moderator as:

this.moderator = await newTester(...)

And add

afterEach(async function() { await this.moderator?.stop(); // also, any other client, e.g. `userA`, `userB` });

towards the beginning or end of this describe.

Yoric · 2022-05-09T10:59:25Z

test/integration/protections/MentionFloodTest.ts

+        // Create users
+        const mjolnir = config.RUNTIME.client!;
+        const mjolnirUserId = await mjolnir.getUserId();
+        const moderator = await newTestUser({ name: { contains: "moderator" } });


Here, too, turn this into this.moderator = ... and get rid of the finally.

Yoric · 2022-05-09T11:17:02Z

test/integration/protections/protectionUtils.ts

+            client.removeListener('room.event', targetCb);
+        }
+    }
+}


Nit: Missing newline.

Yoric self-requested a review February 6, 2022 09:21

Yoric suggested changes Feb 6, 2022

View reviewed changes

TheTimeWalker mentioned this pull request Feb 6, 2022

Mitigate spammers using mass mentions #211

Closed

MTRNord commented Feb 6, 2022

View reviewed changes

src/protections/MentionFlood.ts Show resolved Hide resolved

Yoric requested a review from jesopo February 6, 2022 12:33

MTRNord force-pushed the MTRNord/mention-flood branch from a0e913d to b7bdb05 Compare February 7, 2022 14:01

jesopo reviewed Feb 15, 2022

View reviewed changes

src/protections/ProtectionSettings.ts Outdated Show resolved Hide resolved

src/protections/MentionFlood.ts Show resolved Hide resolved

src/protections/MentionFlood.ts Outdated Show resolved Hide resolved

MTRNord requested a review from jesopo February 22, 2022 20:32

jesopo approved these changes Mar 2, 2022

View reviewed changes

MTRNord requested a review from Yoric March 3, 2022 14:52

Yoric reviewed Mar 7, 2022

View reviewed changes

MTRNord added 14 commits March 8, 2022 14:42

Add Protection against mention flooding

1309426

Move settings to the class settings object

edba358

Make justJoined a Map

e7daad8

Add redact and action settings

526e333

Make sure to escape the event_id data

ea1f908

Fix escaping the correct values in log messages

09f1dc7

Fix regex and make sure we dont use the formatted body to prevent fai…

7727108

…ling due to pills

Revert previous changes to config.ts

25de265

Make sure to default run on formatted_body due to how element-web doe…

28b278b

…s pills and also dont fail on null being returned by String.prototype.match

Add function to the warning action

8822bc1

Make sure to correctly escape the regex

01e53f4

Remove historic mxid regex part

8d4bd29

Update MentionFlood protection to work with 813741c

fd755c1

Make sure we dont assume false on invalid input

6483e39

MTRNord added 3 commits March 8, 2022 14:43

Stricter type action by adding a setting that only allows predefined …

433a17a

…values

Apply removal of LogProxy to MentionFlood Protection

6993f96

Use DurationMSProtectionSetting and add setting comments

219ecce

MTRNord force-pushed the MTRNord/mention-flood branch from d337ff4 to 219ecce Compare March 8, 2022 13:44

MTRNord added 3 commits March 8, 2022 14:49

Move to using mjolnir.roomJoins.getUserJoin and fix missuse of DEFAUL…

2575703

…T_MINUTES_BEFORE_TRUSTING

Update MentionFlood description text

c197cdf

Add missing newline

ea9d586

Add tests for mention flooding

1f1ba03

MTRNord requested a review from Yoric May 1, 2022 12:33

Gnuxie reviewed May 4, 2022

View reviewed changes

Yoric suggested changes May 9, 2022

View reviewed changes

MTRNord closed this Jun 23, 2022

MTRNord deleted the MTRNord/mention-flood branch June 23, 2022 11:14

ShadowJonathan mentioned this pull request Jun 23, 2022

Mention flood protection #309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Protection against mention flooding #210

Add Protection against mention flooding #210

MTRNord commented Feb 6, 2022 •

edited

Loading

Yoric left a comment

MTRNord commented Feb 6, 2022

Yoric commented Feb 14, 2022

jesopo left a comment

cremesk commented Mar 1, 2022

Yoric commented Mar 7, 2022

Yoric left a comment

Yoric commented Mar 8, 2022 •

edited

Loading

Yoric commented Apr 7, 2022

MTRNord commented Apr 7, 2022

Yoric commented Apr 7, 2022

Gnuxie left a comment

Gnuxie May 4, 2022

Gnuxie May 4, 2022

Gnuxie May 4, 2022 •

edited

Loading

Yoric commented May 9, 2022

Yoric left a comment

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Yoric May 9, 2022

Add Protection against mention flooding #210

Add Protection against mention flooding #210

Conversation

MTRNord commented Feb 6, 2022 • edited Loading

TODO

Yoric left a comment

Choose a reason for hiding this comment

MTRNord commented Feb 6, 2022

Yoric commented Feb 14, 2022

jesopo left a comment

Choose a reason for hiding this comment

cremesk commented Mar 1, 2022

Yoric commented Mar 7, 2022

Yoric left a comment

Choose a reason for hiding this comment

Yoric commented Mar 8, 2022 • edited Loading

Yoric commented Apr 7, 2022

MTRNord commented Apr 7, 2022

Yoric commented Apr 7, 2022

Gnuxie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnuxie May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Yoric commented May 9, 2022

Yoric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MTRNord commented Feb 6, 2022 •

edited

Loading

Yoric commented Mar 8, 2022 •

edited

Loading

Gnuxie May 4, 2022 •

edited

Loading