Implement MSC3873 to handle escaped dots in push rule keys (#3134)

* Add comments.

* Implment MSC3873 to handle escaped dots in keys.

* Add some comments about tests.

* Clarify spec behavior.

* Fix typo.

* Don't manually iterate string.

* Clean-up tests.

* Simplify tests.

* Add more tests & fix bug with empty parts.

* Add more edge cases.

* Add a regular expression solution.

This is ~80% slower than the basic split(".").

* Split on a simpler regular expression.

This is ~50% slower than a simple split(".").

* Remove redundant case in regex.

* Enable sticky regex.

* Rollback use of regex.

* Cache values in the PushProcessor.

* Use more each in tests.

* Pre-calculate the key parts instead of caching them.

* Fix typo.

* Switch back to external cache, but clean out obsolete cached values.

* Remove obsolete property.

* Remove more obsolete properties.

spec/unit/pushprocessor.spec.ts

@@ -1,6 +1,6 @@
 import * as utils from "../test-utils/test-utils";
 import { IActionsObject, PushProcessor } from "../../src/pushprocessor";
-import { EventType, IContent, MatrixClient, MatrixEvent } from "../../src";
+import { ConditionKind, EventType, IContent, MatrixClient, MatrixEvent } from "../../src";
 
 describe("NotificationService", function () {
     const testUserId = "@ali:matrix.org";
@@ -287,6 +287,13 @@ describe("NotificationService", function () {
         expect(actions.tweaks.highlight).toEqual(true);
     });
 
+    // TODO: This is not spec compliant behaviour.
+    //
+    // See https://spec.matrix.org/v1.5/client-server-api/#conditions-1 which
+    // describes pattern should glob:
+    //
+    // 1. * matches 0 or more characters;
+    // 2. ? matches exactly one character
     it("should bing on character group ([abc]) bing words.", function () {
         testEvent.event.content!.body = "Ping!";
         let actions = pushProcessor.actionsForEvent(testEvent);
@@ -296,12 +303,14 @@ describe("NotificationService", function () {
         expect(actions.tweaks.highlight).toEqual(true);
     });
 
+    // TODO: This is not spec compliant behaviour. (See above.)
     it("should bing on character range ([a-z]) bing words.", function () {
         testEvent.event.content!.body = "I ate 6 pies";
         const actions = pushProcessor.actionsForEvent(testEvent);
         expect(actions.tweaks.highlight).toEqual(true);
     });
 
+    // TODO: This is not spec compliant behaviour. (See above.)
     it("should bing on character negation ([!a]) bing words.", function () {
         testEvent.event.content!.body = "boke";
         let actions = pushProcessor.actionsForEvent(testEvent);
@@ -330,6 +339,8 @@ describe("NotificationService", function () {
     // invalid
 
     it("should gracefully handle bad input.", function () {
+        // The following body is an object (not a string) and thus is invalid
+        // for matching against.
         testEvent.event.content!.body = { foo: "bar" };
         const actions = pushProcessor.actionsForEvent(testEvent);
         expect(actions.tweaks.highlight).toEqual(false);
@@ -493,4 +504,82 @@ describe("NotificationService", function () {
             });
         });
     });
+
+    it.each([
+        // The properly escaped key works.
+        { key: "content.m\\.test.foo", pattern: "bar", expected: true },
+        // An unescaped version does not match.
+        { key: "content.m.test.foo", pattern: "bar", expected: false },
+        // Over escaping does not match.
+        { key: "content.m\\.test\\.foo", pattern: "bar", expected: false },
+        // Escaping backslashes should match.
+        { key: "content.m\\\\example", pattern: "baz", expected: true },
+        // An unnecessary escape sequence leaves the backslash and still matches.
+        { key: "content.m\\example", pattern: "baz", expected: true },
+    ])("test against escaped dotted paths '$key'", ({ key, pattern, expected }) => {
+        testEvent = utils.mkEvent({
+            type: "m.room.message",
+            room: testRoomId,
+            user: "@alfred:localhost",
+            event: true,
+            content: {
+                // A dot in the field name.
+                "m.test": { foo: "bar" },
+                // A backslash in a field name.
+                "m\\example": "baz",
+            },
+        });
+
+        expect(
+            pushProcessor.ruleMatchesEvent(
+                {
+                    rule_id: "rule1",
+                    actions: [],
+                    conditions: [
+                        {
+                            kind: ConditionKind.EventMatch,
+                            key: key,
+                            pattern: pattern,
+                        },
+                    ],
+                    default: false,
+                    enabled: true,
+                },
+                testEvent,
+            ),
+        ).toBe(expected);
+    });
+});
+
+describe("Test PushProcessor.partsForDottedKey", function () {
+    it.each([
+        // A field with no dots.
+        ["m", ["m"]],
+        // Simple dotted fields.
+        ["m.foo", ["m", "foo"]],
+        ["m.foo.bar", ["m", "foo", "bar"]],
+        // Backslash is used as an escape character.
+        ["m\\.foo", ["m.foo"]],
+        ["m\\\\.foo", ["m\\", "foo"]],
+        ["m\\\\\\.foo", ["m\\.foo"]],
+        ["m\\\\\\\\.foo", ["m\\\\", "foo"]],
+        ["m\\foo", ["m\\foo"]],
+        ["m\\\\foo", ["m\\foo"]],
+        ["m\\\\\\foo", ["m\\\\foo"]],
+        ["m\\\\\\\\foo", ["m\\\\foo"]],
+        // Ensure that escapes at the end don't cause issues.
+        ["m.foo\\", ["m", "foo\\"]],
+        ["m.foo\\\\", ["m", "foo\\"]],
+        ["m.foo\\.", ["m", "foo."]],
+        ["m.foo\\\\.", ["m", "foo\\", ""]],
+        ["m.foo\\\\\\.", ["m", "foo\\."]],
+        // Empty parts (corresponding to properties which are an empty string) are allowed.
+        [".m", ["", "m"]],
+        ["..m", ["", "", "m"]],
+        ["m.", ["m", ""]],
+        ["m..", ["m", "", ""]],
+        ["m..foo", ["m", "", "foo"]],
+    ])("partsFotDottedKey for %s", (path: string, expected: string[]) => {
+        expect(PushProcessor.partsForDottedKey(path)).toStrictEqual(expected);
+    });
 });

src/client.ts

@@ -8499,10 +8499,22 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      */
     public getPushRules(): Promise<IPushRules> {
         return this.http.authedRequest<IPushRules>(Method.Get, "/pushrules/").then((rules: IPushRules) => {
-            return PushProcessor.rewriteDefaultRules(rules);
+            this.setPushRules(rules);
+            return this.pushRules!;
         });
     }
 
+    /**
+     * Update the push rules for the account. This should be called whenever
+     * updated push rules are available.
+     */
+    public setPushRules(rules: IPushRules): void {
+        // Fix-up defaults, if applicable.
+        this.pushRules = PushProcessor.rewriteDefaultRules(rules);
+        // Pre-calculate any necessary caches.
+        this.pushProcessor.updateCachedPushRuleKeys(this.pushRules);
+    }
+
     /**
      * @returns Promise which resolves: an empty object `{}`
      * @returns Rejects: with an error response.

src/pushprocessor.ts

@@ -123,6 +123,12 @@ export class PushProcessor {
      */
     public constructor(private readonly client: MatrixClient) {}
 
+    /**
+     * Maps the original key from the push rules to a list of property names
+     * after unescaping.
+     */
+    private readonly parsedKeys = new Map<string, string[]>();
+
     /**
      * Convert a list of actions into a object with the actions as keys and their values
      * @example
@@ -162,7 +168,7 @@ export class PushProcessor {
         if (!newRules) newRules = {} as IPushRules;
         if (!newRules.global) newRules.global = {} as PushRuleSet;
         if (!newRules.global.override) newRules.global.override = [];
-        if (!newRules.global.override) newRules.global.underride = [];
+        if (!newRules.global.underride) newRules.global.underride = [];
 
         // Merge the client-level defaults with the ones from the server
         const globalOverrides = newRules.global.override;
@@ -202,6 +208,53 @@ export class PushProcessor {
         return newRules;
     }
 
+    /**
+     * Pre-caches the parsed keys for push rules and cleans out any obsolete cache
+     * entries. Should be called after push rules are updated.
+     * @param newRules - The new push rules.
+     */
+    public updateCachedPushRuleKeys(newRules: IPushRules): void {
+        // These lines are mostly to make the tests happy. We shouldn't run into these
+        // properties missing in practice.
+        if (!newRules) newRules = {} as IPushRules;
+        if (!newRules.global) newRules.global = {} as PushRuleSet;
+        if (!newRules.global.override) newRules.global.override = [];
+        if (!newRules.global.room) newRules.global.room = [];
+        if (!newRules.global.sender) newRules.global.sender = [];
+        if (!newRules.global.underride) newRules.global.underride = [];
+
+        // Process the 'key' property on event_match conditions pre-cache the
+        // values and clean-out any unused values.
+        const toRemoveKeys = new Set(this.parsedKeys.keys());
+        for (const ruleset of [
+            newRules.global.override,
+            newRules.global.room,
+            newRules.global.sender,
+            newRules.global.underride,
+        ]) {
+            for (const rule of ruleset) {
+                if (!rule.conditions) {
+                    continue;
+                }
+
+                for (const condition of rule.conditions) {
+                    if (condition.kind !== ConditionKind.EventMatch) {
+                        continue;
+                    }
+
+                    // Ensure we keep this key.
+                    toRemoveKeys.delete(condition.key);
+
+                    // Pre-process the key.
+                    this.parsedKeys.set(condition.key, PushProcessor.partsForDottedKey(condition.key));
+                }
+            }
+        }
+        // Any keys that were previously cached, but are no longer needed should
+        // be removed.
+        toRemoveKeys.forEach((k) => this.parsedKeys.delete(k));
+    }
+
     private static cachedGlobToRegex: Record<string, RegExp> = {}; // $glob: RegExp
 
     private matchingRuleFromKindSet(ev: MatrixEvent, kindset: PushRuleSet): IAnnotatedPushRule | null {
@@ -433,25 +486,99 @@ export class PushProcessor {
         return PushProcessor.cachedGlobToRegex[glob];
     }
 
+    /**
+     * Parse the key into the separate fields to search by splitting on
+     * unescaped ".", and then removing any escape characters.
+     *
+     * @param str - The key of the push rule condition: a dotted field.
+     * @returns The unescaped parts to fetch.
+     * @internal
+     */
+    public static partsForDottedKey(str: string): string[] {
+        const result = [];
+
+        // The current field and whether the previous character was the escape
+        // character (a backslash).
+        let part = "";
+        let escaped = false;
+
+        // Iterate over each character, and decide whether to append to the current
+        // part (following the escape rules) or to start a new part (based on the
+        // field separator).
+        for (const c of str) {
+            // If the previous character was the escape character (a backslash)
+            // then decide what to append to the current part.
+            if (escaped) {
+                if (c === "\\" || c === ".") {
+                    // An escaped backslash or dot just gets added.
+                    part += c;
+                } else {
+                    // A character that shouldn't be escaped gets the backslash prepended.
+                    part += "\\" + c;
+                }
+                // This always resets being escaped.
+                escaped = false;
+                continue;
+            }
+
+            if (c == ".") {
+                // The field separator creates a new part.
+                result.push(part);
+                part = "";
+            } else if (c == "\\") {
+                // A backslash adds no characters, but starts an escape sequence.
+                escaped = true;
+            } else {
+                // Otherwise, just add the current character.
+                part += c;
+            }
+        }
+
+        // Ensure the final part is included. If there's an open escape sequence
+        // it should be included.
+        if (escaped) {
+            part += "\\";
+        }
+        result.push(part);
+
+        return result;
+    }
+
+    /**
+     * For a dotted field and event, fetch the value at that position, if one
+     * exists.
+     *
+     * @param key - The key of the push rule condition: a dotted field to fetch.
+     * @param ev - The matrix event to fetch the field from.
+     * @returns The value at the dotted path given by key.
+     */
     private valueForDottedKey(key: string, ev: MatrixEvent): any {
-        const parts = key.split(".");
+        // The key should already have been parsed via updateCachedPushRuleKeys,
+        // but if it hasn't (maybe via an old consumer of the SDK which hasn't
+        // been updated?) then lazily calculate it here.
+        let parts = this.parsedKeys.get(key);
+        if (parts === undefined) {
+            parts = PushProcessor.partsForDottedKey(key);
+            this.parsedKeys.set(key, parts);
+        }
         let val: any;
 
         // special-case the first component to deal with encrypted messages
         const firstPart = parts[0];
+        let currentIndex = 0;
         if (firstPart === "content") {
             val = ev.getContent();
-            parts.shift();
+            ++currentIndex;
         } else if (firstPart === "type") {
             val = ev.getType();
-            parts.shift();
+            ++currentIndex;
         } else {
             // use the raw event for any other fields
             val = ev.event;
         }
 
-        while (parts.length > 0) {
-            const thisPart = parts.shift()!;
+        for (; currentIndex < parts.length; ++currentIndex) {
+            const thisPart = parts[currentIndex];
             if (isNullOrUndefined(val[thisPart])) {
                 return null;
             }

src/sliding-sync-sdk.ts

@@ -43,7 +43,6 @@ import {
 } from "./sliding-sync";
 import { EventType } from "./@types/event";
 import { IPushRules } from "./@types/PushRules";
-import { PushProcessor } from "./pushprocessor";
 import { RoomStateEvent } from "./models/room-state";
 import { RoomMemberEvent } from "./models/room-member";
 
@@ -262,7 +261,7 @@ class ExtensionAccountData implements Extension<ExtensionAccountDataRequest, Ext
             // (see sync) before syncing over the network.
             if (accountDataEvent.getType() === EventType.PushRules) {
                 const rules = accountDataEvent.getContent<IPushRules>();
-                this.client.pushRules = PushProcessor.rewriteDefaultRules(rules);
+                this.client.setPushRules(rules);
             }
             const prevEvent = prevEventsMap[accountDataEvent.getType()];
             this.client.emit(ClientEvent.AccountData, accountDataEvent, prevEvent);

src/sync.ts

@@ -32,7 +32,6 @@ import * as utils from "./utils";
 import { IDeferred } from "./utils";
 import { Filter } from "./filter";
 import { EventTimeline } from "./models/event-timeline";
-import { PushProcessor } from "./pushprocessor";
 import { logger } from "./logger";
 import { InvalidStoreError, InvalidStoreState } from "./errors";
 import { ClientEvent, IStoredClientOpts, MatrixClient, PendingEventOrdering, ResetTimelineCallback } from "./client";
@@ -1162,7 +1161,7 @@ export class SyncApi {
                 // (see sync) before syncing over the network.
                 if (accountDataEvent.getType() === EventType.PushRules) {
                     const rules = accountDataEvent.getContent<IPushRules>();
-                    client.pushRules = PushProcessor.rewriteDefaultRules(rules);
+                    client.setPushRules(rules);
                 }
                 const prevEvent = prevEventsMap[accountDataEvent.getType()!];
                 client.emit(ClientEvent.AccountData, accountDataEvent, prevEvent);