diff --git a/src/Mjolnir.ts b/src/Mjolnir.ts index c82aece..3015b70 100644 --- a/src/Mjolnir.ts +++ b/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( diff --git a/src/commands/CommandHandler.ts b/src/commands/CommandHandler.ts index dedebb4..97bde07 100644 --- a/src/commands/CommandHandler.ts +++ b/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 - Enables a particular protection\n" + "!mjolnir disable - Disables a particular protection\n" + - "!mjolnir set . [value] - Change a projection setting\n" + - "!mjolnir get [protection] - List protection settings\n" + + "!mjolnir config set . [value] - Change a projection setting\n" + + "!mjolnir config add . [value] - Add a value to a list protection setting\n" + + "!mjolnir config remove . [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 - Adds a protected room (may cause high server load)\n" + "!mjolnir rooms remove - Removes a protected room\n" + diff --git a/src/commands/ProtectionsCommands.ts b/src/commands/ProtectionsCommands.ts index 8659757..601f2fb 100644 --- a/src/commands/ProtectionsCommands.ts +++ b/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 { 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 - } 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; + // no specific protectionName provided, show all of them. + + // sort output by protection name + pickProtections.sort(); } 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 += `
  • ${protectionName}.${settingName}: ${htmlEscape(value)}
    • ` } } diff --git a/src/protections/BasicFlooding.ts b/src/protections/BasicFlooding.ts index c926b48..3f9cb3f 100644 --- a/src/protections/BasicFlooding.ts +++ b/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() { diff --git a/src/protections/ProtectionSettings.ts b/src/protections/ProtectionSettings.ts index 71a445c..2ef332a 100644 --- a/src/protections/ProtectionSettings.ts +++ b/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 { } export interface IProtectionListSetting extends IProtectionSetting { /* + * 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 { - return ("addValue" in object && "removeValue" in object); -} -class ProtectionSetting implements IProtectionSetting { +class AbstractProtectionSetting implements IProtectionSetting { value: TValue fromString(data: string): TChange | undefined { throw new Error("not Implemented"); @@ -68,7 +71,7 @@ class ProtectionSetting implements IProtectionSetting extends ProtectionSetting implements IProtectionListSetting { +class AbstractProtectionListSetting extends AbstractProtectionSetting implements IProtectionListSetting { addValue(data: TChange): TValue { throw new Error("not Implemented"); } @@ -76,13 +79,17 @@ class ProtectionListSetting extends ProtectionSetting { + return ("addValue" in object && "removeValue" in object); +} -export class StringProtectionSetting extends ProtectionSetting { + +export class StringProtectionSetting extends AbstractProtectionSetting { value = ""; fromString = (data) => data; validate = (data) => true; } -export class StringListProtectionSetting extends ProtectionListSetting { +export class StringListProtectionSetting extends AbstractProtectionListSetting { value: string[] = []; fromString = (data) => data; validate = (data) => true; @@ -95,7 +102,7 @@ export class StringListProtectionSetting extends ProtectionListSetting { +export class NumberProtectionSetting extends AbstractProtectionSetting { min: number|undefined; max: number|undefined; diff --git a/src/protections/protections.ts b/src/protections/protections.ts index cbd7ee8..356bc9e 100644 --- a/src/protections/protections.ts +++ b/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(), }, diff --git a/test/integration/protectionSettingsTest.ts b/test/integration/protectionSettingsTest.ts index b84e376..3b84511 100644 --- a/test/integration/protectionSettingsTest.ts +++ b/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("test", { test2: 2 }); - const settings = await this.mjolnir.getProtectionSettings("test"); - //assert.equal(settings["test1"], 1); + await this.mjolnir.setProtectionSettings("HPUjKN", { test1: 1 }); + await this.mjolnir.setProtectionSettings("HPUjKN", { test2: 2 }); + const settings = await this.mjolnir.getProtectionSettings("HPUjKN"); + 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 remove 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 oXzT0E.test asd"}) await reply - assert.deepEqual(await this.mjolnir.getProtectionSettings("test"), { "test": [] }); + assert.deepEqual(await this.mjolnir.getProtectionSettings("oXzT0E"), { "test": [] }); }); });