nits picked

src/Mjolnir.ts

@@ -403,7 +403,7 @@ export class Mjolnir {
     }
 
     /*
-     * Read org.matrix.mjolnir.setting state object, find any saved settings for
+     * Read org.matrix.mjolnir.setting state event, find any saved settings for
      * the requested protectionName, then iterate and validate against their parser
      * counterparts in IProtection.settings and return those which validate
      *
@@ -411,17 +411,17 @@ export class Mjolnir {
      * @returns Every saved setting for this protectionName that has a valid value
      */
     public async getProtectionSettings(protectionName: string): Promise<{ [setting: string]: any }> {
-        const settingDefinitions = PROTECTIONS[protectionName].factory().settings;
         let savedSettings: { [setting: string]: any } = {}
         try {
             savedSettings = await this.client.getRoomStateEvent(
                 this.managementRoomId, 'org.matrix.mjolnir.setting', protectionName
-            )
+            );
         } catch {
             // setting does not exist, return empty object
             return savedSettings;
         }
 
+        const settingDefinitions = PROTECTIONS[protectionName].factory().settings;
         const validatedSettings: { [setting: string]: any } = {}
         for (let [key, value] of Object.entries(savedSettings)) {
             if (
@@ -433,6 +433,12 @@ export class Mjolnir {
                     && settingDefinitions[key].validate(value)
             ) {
                 validatedSettings[key] = value;
+            } else {
+                await logMessage(
+                    LogLevel.WARN,
+                    "getProtectionSetting",
+                    `Tried to read ${protectionName}.${key} and got invalid value ${value}`
+                );
             }
         }
         return validatedSettings;
@@ -453,13 +459,14 @@ export class Mjolnir {
         for (let [key, value] of Object.entries(changedSettings)) {
             if (!(key in settingDefinitions)) {
                 throw new ProtectionSettingValidationError(`Failed to find protection setting by name: ${key}`);
-            } else if (typeof(settingDefinitions[key].value) !== typeof(value)) {
-                throw new ProtectionSettingValidationError(`Invalid type for protection setting: ${key} (${typeof(value)})`);
-            } else if (!settingDefinitions[key].validate(value)) {
-                throw new ProtectionSettingValidationError(`Invalid value for protection setting: ${key} (${value})`);
-            } else {
-                validatedSettings[key] = value;
             }
+            if (typeof(settingDefinitions[key].value) !== typeof(value)) {
+                throw new ProtectionSettingValidationError(`Invalid type for protection setting: ${key} (${typeof(value)})`);
+            }
+            if (!settingDefinitions[key].validate(value)) {
+                throw new ProtectionSettingValidationError(`Invalid value for protection setting: ${key} (${value})`);
+            }
+            validatedSettings[key] = value;
         }
 
         await this.client.sendStateEvent(

src/commands/CommandHandler.ts

@@ -131,8 +131,10 @@ export async function handleCommand(roomId: string, event: any, mjolnir: Mjolnir
                 "!mjolnir protections                                                - List all available protections\n" +
                 "!mjolnir enable <protection>                                        - Enables a particular protection\n" +
                 "!mjolnir disable <protection>                                       - Disables a particular protection\n" +
-                "!mjolnir set <protection>.<setting> [value]                         - Change a projection setting\n" +
+                "!mjolnir config set <protection>.<setting> [value]                  - Change a projection setting\n" +
-                "!mjolnir get [protection]                                           - List protection settings\n" +
+                "!mjolnir config add <protection>.<setting> [value]                  - Add a value to a list protection setting\n" +
+                "!mjolnir config remove <protection>.<setting> [value]               - Remove a value from a list protection setting\n" +
+                "!mjolnir config get [protection]                                    - List protection settings\n" +
                 "!mjolnir rooms                                                      - Lists all the protected rooms\n" +
                 "!mjolnir rooms add <room alias/ID>                                  - Adds a protected room (may cause high server load)\n" +
                 "!mjolnir rooms remove <room alias/ID>                               - Removes a protected room\n" +

src/commands/ProtectionsCommands.ts

@@ -41,16 +41,28 @@ enum ConfigAction {
     Remove
 }
 
+/*
+ * Process a given ConfigAction against a given protection setting
+ *
+ * @param mjolnir Current Mjolnir instance
+ * @param parts Arguments given to the command being processed
+ * @param action Which ConfigAction to do to the provided protection setting
+ * @returns Command success or failure message
+ */
 async function _execConfigChangeProtection(mjolnir: Mjolnir, parts: string[], action: ConfigAction): Promise<string> {
     const [protectionName, ...settingParts] = parts[0].split(".");
     const protection = PROTECTIONS[protectionName];
-    if (protection === undefined) return `Unknown protection ${protectionName}`;
+    if (protection === undefined) {
+        return `Unknown protection ${protectionName}`;
+    }
 
     const defaultSettings = protection.factory().settings
     const settingName = settingParts[0];
     const stringValue = parts[1];
 
-    if (!(settingName in defaultSettings)) return `Unknown setting ${settingName}`;
+    if (!(settingName in defaultSettings)) {
+        return `Unknown setting ${settingName}`;
+    }
 
     const parser = defaultSettings[settingName];
     // we don't need to validate `value`, because mjolnir.setProtectionSettings does
@@ -135,18 +147,20 @@ export async function execConfigRemoveProtection(roomId: string, event: any, mjo
  */
 export async function execConfigGetProtection(roomId: string, event: any, mjolnir: Mjolnir, parts: string[]) {
     let pickProtections = Object.keys(PROTECTIONS);
-    // this means the output is sorted by protection name
-    pickProtections.sort();
 
     if (parts.length < 3) {
-        // no specific protectionName provided, show all of them
+        // no specific protectionName provided, show all of them.
-    } else if (!pickProtections.includes(parts[0])) {
+
-        const errMsg = `Unknown protection: ${parts[0]}`;
+        // sort output by protection name
-        const errReply = RichReply.createFor(roomId, event, errMsg, errMsg);
+        pickProtections.sort();
-        errReply["msgtype"] = "m.notice";
-        await mjolnir.client.sendMessage(roomId, errReply);
-        return;
     } else {
+        if (!pickProtections.includes(parts[0])) {
+            const errMsg = `Unknown protection: ${parts[0]}`;
+            const errReply = RichReply.createFor(roomId, event, errMsg, errMsg);
+            errReply["msgtype"] = "m.notice";
+            await mjolnir.client.sendMessage(roomId, errReply);
+            return;
+        }
         pickProtections = [parts[0]];
     }
 
@@ -175,6 +189,9 @@ export async function execConfigGetProtection(roomId: string, event: any, mjolni
                 value = savedSettings[settingName]
 
             text += `* ${protectionName}.${settingName}: ${value}`;
+            // `protectionName` and `settingName` are user-provided but
+            // validated against the names of existing protections and their
+            // settings, so XSS is avoided for these already
             html += `<li><code>${protectionName}.${settingName}</code>: <code>${htmlEscape(value)}</code></li>`
         }
     }

src/protections/BasicFlooding.ts

@@ -21,7 +21,8 @@ import { LogLevel, LogService } from "matrix-bot-sdk";
 import { logMessage } from "../LogProxy";
 import config from "../config";
 
-export const MAX_PER_MINUTE = 10; // if this is exceeded, we'll ban the user for spam and redact their messages
+// if this is exceeded, we'll ban the user for spam and redact their messages
+export const DEFAULT_MAX_PER_MINUTE = 10;
 const TIMESTAMP_THRESHOLD = 30000; // 30s out of phase
 
 export class BasicFlooding implements IProtection {
@@ -29,7 +30,7 @@ export class BasicFlooding implements IProtection {
     private lastEvents: { [roomId: string]: { [userId: string]: { originServerTs: number, eventId: string }[] } } = {};
     private recentlyBanned: string[] = [];
 
-    maxPerMinute = new NumberProtectionSetting(MAX_PER_MINUTE);
+    maxPerMinute = new NumberProtectionSetting(DEFAULT_MAX_PER_MINUTE);
     settings = {};
 
     constructor() {

src/protections/ProtectionSettings.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2021 The Matrix.org Foundation C.I.C.
+Copyright 2022 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.
@@ -44,19 +44,22 @@ export interface IProtectionSetting<TChange, TValue> {
 }
 export interface IProtectionListSetting<TChange, TValue> extends IProtectionSetting<TChange, TValue> {
     /*
+     * Add `data` to the current setting value, and return that new object
      *
+     * @param data Value to add to the current setting value
+     * @returns The potential new value of this setting object
      */
     addValue(data: TChange): TValue;
     /*
+     * Remove `data` from the current setting value, and return that new object
      *
+     * @param data Value to remove from the current setting value
+     * @returns The potential new value of this setting object
      */
     removeValue(data: TChange): TValue;
 }
-export function isListSetting(object: any): object is IProtectionListSetting<any, any> {
-    return ("addValue" in object && "removeValue" in object);
-}
 
-class ProtectionSetting<TChange, TValue> implements IProtectionSetting<TChange, TValue> {
+class AbstractProtectionSetting<TChange, TValue> implements IProtectionSetting<TChange, TValue> {
     value: TValue
     fromString(data: string): TChange | undefined {
         throw new Error("not Implemented");
@@ -68,7 +71,7 @@ class ProtectionSetting<TChange, TValue> implements IProtectionSetting<TChange,
         this.value = data;
     }
 }
-class ProtectionListSetting<TChange, TValue> extends ProtectionSetting<TChange, TValue> implements IProtectionListSetting<TChange, TValue> {
+class AbstractProtectionListSetting<TChange, TValue> extends AbstractProtectionSetting<TChange, TValue> implements IProtectionListSetting<TChange, TValue> {
     addValue(data: TChange): TValue {
         throw new Error("not Implemented");
     }
@@ -76,13 +79,17 @@ class ProtectionListSetting<TChange, TValue> extends ProtectionSetting<TChange,
         throw new Error("not Implemented");
     }
 }
+export function isListSetting(object: any): object is IProtectionListSetting<any, any> {
+    return ("addValue" in object && "removeValue" in object);
+}
 
-export class StringProtectionSetting extends ProtectionSetting<string, string> {
+
+export class StringProtectionSetting extends AbstractProtectionSetting<string, string> {
     value = "";
     fromString = (data) => data;
     validate = (data) => true;
 }
-export class StringListProtectionSetting extends ProtectionListSetting<string, string[]> {
+export class StringListProtectionSetting extends AbstractProtectionListSetting<string, string[]> {
     value: string[] = [];
     fromString = (data) => data;
     validate = (data) => true;
@@ -95,7 +102,7 @@ export class StringListProtectionSetting extends ProtectionListSetting<string, s
     }
 }
 
-export class NumberProtectionSetting extends ProtectionSetting<number, number> {
+export class NumberProtectionSetting extends AbstractProtectionSetting<number, number> {
     min: number|undefined;
     max: number|undefined;
 

src/protections/protections.ts

@@ -16,7 +16,7 @@ limitations under the License.
 
 import { FirstMessageIsImage } from "./FirstMessageIsImage";
 import { IProtection } from "./IProtection";
-import { BasicFlooding, MAX_PER_MINUTE } from "./BasicFlooding";
+import { BasicFlooding, DEFAULT_MAX_PER_MINUTE } from "./BasicFlooding";
 import { WordList } from "./WordList";
 import { MessageIsVoice } from "./MessageIsVoice";
 import { MessageIsMedia } from "./MessageIsMedia";
@@ -28,7 +28,7 @@ export const PROTECTIONS: PossibleProtections = {
         factory: () => new FirstMessageIsImage(),
     },
     [new BasicFlooding().name]: {
-        description: "If a user posts more than " + MAX_PER_MINUTE + " messages in 60s they'll be " +
+        description: "If a user posts more than " + DEFAULT_MAX_PER_MINUTE + " messages in 60s they'll be " +
             "banned for spam. This does not publish the ban to any of your ban lists.",
         factory: () => new BasicFlooding(),
     },

test/integration/protectionSettingsTest.ts

@@ -26,28 +26,28 @@ describe("Test: Protection settings", function() {
     it("Mjolnir successfully saves valid protection setting values", async function() {
         this.timeout(20000);
 
-        PROTECTIONS["test"] = {
+        PROTECTIONS["05OVMS"] = {
             description: "A test protection",
             factory: () => new class implements IProtection {
-                name = "test";
+                name = "05OVMS";
                 async handleEvent(mjolnir: Mjolnir, roomId: string, event: any) {};
                 settings = { test: new NumberProtectionSetting(3) };
             }
         };
 
-        await this.mjolnir.setProtectionSettings("test", { test: 123 });
+        await this.mjolnir.setProtectionSettings("05OVMS", { test: 123 });
         assert.equal(
-            (await this.mjolnir.getProtectionSettings("test"))["test"],
+            (await this.mjolnir.getProtectionSettings("05OVMS"))["test"],
             123
         );
     });
     it("Mjolnir should accumulate changed settings", async function() {
         this.timeout(20000);
 
-        PROTECTIONS["test"] = {
+        PROTECTIONS["HPUjKN"] = {
             description: "A test protection",
             factory: () => new class implements IProtection {
-                name = "test";
+                name = "HPUjKN";
                 async handleEvent(mjolnir: Mjolnir, roomId: string, event: any) {};
                 settings = {
                     test1: new NumberProtectionSetting(3),
@@ -56,20 +56,20 @@ describe("Test: Protection settings", function() {
             }
         };
 
-        await this.mjolnir.setProtectionSettings("test", { test1: 1 });
+        await this.mjolnir.setProtectionSettings("HPUjKN", { test1: 1 });
-        await this.mjolnir.setProtectionSettings("test", { test2: 2 });
+        await this.mjolnir.setProtectionSettings("HPUjKN", { test2: 2 });
-        const settings = await this.mjolnir.getProtectionSettings("test");
+        const settings = await this.mjolnir.getProtectionSettings("HPUjKN");
-        //assert.equal(settings["test1"], 1);
+        assert.equal(settings["test1"], 1);
         assert.equal(settings["test2"], 2);
     });
     it("Mjolnir responds to !set correctly", async function() {
         this.timeout(20000);
         await client.joinRoom(config.managementRoom);
 
-        PROTECTIONS["test"] = {
+        PROTECTIONS["JY2TPN"] = {
             description: "A test protection",
             factory: () => new class implements IProtection {
-                name = "test";
+                name = "JY2TPN";
                 async handleEvent(mjolnir: Mjolnir, roomId: string, event: any) {};
                 settings = { test: new StringProtectionSetting() };
             }
@@ -78,26 +78,26 @@ describe("Test: Protection settings", function() {
 
         let reply = new Promise((resolve, reject) => {
             client.on('room.message', noticeListener(this.mjolnir.managementRoomId, (event) => {
-                if (event.content.body.includes("Changed test.test ")) {
+                if (event.content.body.includes("Changed JY2TPN.test ")) {
                     resolve(event);
                 }
             }))
         });
 
-        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config set test.test asd"})
+        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config set JY2TPN.test asd"})
         await reply
 
-        const settings = await this.mjolnir.getProtectionSettings("test");
+        const settings = await this.mjolnir.getProtectionSettings("JY2TPN");
         assert.equal(settings["test"], "asd");
     });
     it("Mjolnir adds a value to a list setting", async function() {
         this.timeout(20000);
         await client.joinRoom(config.managementRoom);
 
-        PROTECTIONS["test"] = {
+        PROTECTIONS["r33XyT"] = {
             description: "A test protection",
             factory: () => new class implements IProtection {
-                name = "test";
+                name = "r33XyT";
                 async handleEvent(mjolnir: Mjolnir, roomId: string, event: any) {};
                 settings = { test: new StringListProtectionSetting() };
             }
@@ -106,25 +106,25 @@ describe("Test: Protection settings", function() {
 
         let reply = new Promise((resolve, reject) => {
             client.on('room.message', noticeListener(this.mjolnir.managementRoomId, (event) => {
-                if (event.content.body.includes("Changed test.test ")) {
+                if (event.content.body.includes("Changed r33XyT.test ")) {
                     resolve(event);
                 }
             }))
         });
 
-        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config add test.test asd"})
+        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config add r33XyT.test asd"})
         await reply
 
-        assert.deepEqual(await this.mjolnir.getProtectionSettings("test"), { "test": ["asd"] });
+        assert.deepEqual(await this.mjolnir.getProtectionSettings("r33XyT"), { "test": ["asd"] });
     });
     it("Mjolnir removes a value from a list setting", async function() {
         this.timeout(20000);
         await client.joinRoom(config.managementRoom);
 
-        PROTECTIONS["test"] = {
+        PROTECTIONS["oXzT0E"] = {
             description: "A test protection",
             factory: () => new class implements IProtection {
-                name = "test";
+                name = "oXzT0E";
                 async handleEvent(mjolnir: Mjolnir, roomId: string, event: any) {};
                 settings = { test: new StringListProtectionSetting() };
             }
@@ -134,7 +134,7 @@ describe("Test: Protection settings", function() {
         let reply = new Promise((resolve, reject) => {
             let i = 0;
             client.on('room.message', noticeListener(this.mjolnir.managementRoomId, (event) => {
-                if (event.content.body.includes("Changed test.test ")) {
+                if (event.content.body.includes("Changed oXzT0E.test ")) {
                     if (++i == 2) {
                         resolve(event);
                     }
@@ -142,11 +142,11 @@ describe("Test: Protection settings", function() {
             }))
         });
 
-        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config add test.test asd"})
+        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config add oXzT0E.test asd"})
-        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config remove test.test asd"})
+        await client.sendMessage(this.mjolnir.managementRoomId, {msgtype: "m.text", body: "!mjolnir config remove oXzT0E.test asd"})
         await reply
 
-        assert.deepEqual(await this.mjolnir.getProtectionSettings("test"), { "test": [] });
+        assert.deepEqual(await this.mjolnir.getProtectionSettings("oXzT0E"), { "test": [] });
     });
 });