Push rule evaluation tweaks (#2897)

This tweaks push rule evaluation:

1. to be more strict around pattern matching and to not match empty
patterns
3. to bail if we come across a `dont_notify`, since cycles after that
are wasted
4. refactors `ActionsToTweaks` to make a bit more sense

internal/pushrules/evaluate.go

@@ -145,6 +145,11 @@ func conditionMatches(cond *Condition, event *gomatrixserverlib.Event, ec Evalua
 }
 
 func patternMatches(key, pattern string, event *gomatrixserverlib.Event) (bool, error) {
+	// It doesn't make sense for an empty pattern to match anything.
+	if pattern == "" {
+		return false, nil
+	}
+
 	re, err := globToRegexp(pattern)
 	if err != nil {
 		return false, err
@@ -154,12 +159,20 @@ func patternMatches(key, pattern string, event *gomatrixserverlib.Event) (bool,
 	if err = json.Unmarshal(event.JSON(), &eventMap); err != nil {
 		return false, fmt.Errorf("parsing event: %w", err)
 	}
+	// From the spec:
+	// "If the property specified by key is completely absent from
+	// the event, or does not have a string value, then the condition
+	// will not match, even if pattern is *."
 	v, err := lookupMapPath(strings.Split(key, "."), eventMap)
 	if err != nil {
 		// An unknown path is a benign error that shouldn't stop rule
 		// processing. It's just a non-match.
 		return false, nil
 	}
+	if _, ok := v.(string); !ok {
+		// A non-string never matches.
+		return false, nil
+	}
 
 	return re.MatchString(fmt.Sprint(v)), nil
 }

internal/pushrules/evaluate_test.go

@@ -111,7 +111,10 @@ func TestConditionMatches(t *testing.T) {
 		{"empty", Condition{}, `{}`, false},
 		{"empty", Condition{Kind: "unknownstring"}, `{}`, false},
 
-		{"eventMatch", Condition{Kind: EventMatchCondition, Key: "content"}, `{"content":{}}`, true},
+		// Neither of these should match because `content` is not a full string match,
+		// and `content.body` is not a string value.
+		{"eventMatch", Condition{Kind: EventMatchCondition, Key: "content"}, `{"content":{}}`, false},
+		{"eventBodyMatch", Condition{Kind: EventMatchCondition, Key: "content.body", Is: "3"}, `{"content":{"body": 3}}`, false},
 
 		{"displayNameNoMatch", Condition{Kind: ContainsDisplayNameCondition}, `{"content":{"body":"something without displayname"}}`, false},
 		{"displayNameMatch", Condition{Kind: ContainsDisplayNameCondition}, `{"content":{"body":"hello Dear User, how are you?"}}`, true},
@@ -137,7 +140,7 @@ func TestConditionMatches(t *testing.T) {
 				t.Fatalf("conditionMatches failed: %v", err)
 			}
 			if got != tst.Want {
-				t.Errorf("conditionMatches: got %v, want %v", got, tst.Want)
+				t.Errorf("conditionMatches: got %v, want %v on %s", got, tst.Want, tst.Name)
 			}
 		})
 	}
@@ -161,9 +164,7 @@ func TestPatternMatches(t *testing.T) {
 	}{
 		{"empty", "", "", `{}`, false},
 
-		// Note that an empty pattern contains no wildcard characters,
+		{"patternEmpty", "content", "", `{"content":{}}`, false},
-		// which implicitly means "*".
-		{"patternEmpty", "content", "", `{"content":{}}`, true},
 
 		{"literal", "content.creator", "acreator", `{"content":{"creator":"acreator"}}`, true},
 		{"substring", "content.creator", "reat", `{"content":{"creator":"acreator"}}`, true},
@@ -178,7 +179,7 @@ func TestPatternMatches(t *testing.T) {
 				t.Fatalf("patternMatches failed: %v", err)
 			}
 			if got != tst.Want {
-				t.Errorf("patternMatches: got %v, want %v", got, tst.Want)
+				t.Errorf("patternMatches: got %v, want %v on %s", got, tst.Want, tst.Name)
 			}
 		})
 	}

internal/pushrules/util.go

@@ -11,22 +11,27 @@ import (
 // kind and a tweaks map. Returns a nil map if it would have been
 // empty.
 func ActionsToTweaks(as []*Action) (ActionKind, map[string]interface{}, error) {
-	kind := UnknownAction
+	var kind ActionKind
-	tweaks := map[string]interface{}{}
+	var tweaks map[string]interface{}
 
 	for _, a := range as {
-		if a.Kind == SetTweakAction {
+		switch a.Kind {
-			tweaks[string(a.Tweak)] = a.Value
+		case DontNotifyAction:
-			continue
+			// Don't bother processing any further
-		}
+			return DontNotifyAction, nil, nil
-		if kind != UnknownAction {
-			return UnknownAction, nil, fmt.Errorf("got multiple primary actions: already had %q, got %s", kind, a.Kind)
-		}
-		kind = a.Kind
-	}
 
-	if len(tweaks) == 0 {
+		case SetTweakAction:
-		tweaks = nil
+			if tweaks == nil {
+				tweaks = map[string]interface{}{}
+			}
+			tweaks[string(a.Tweak)] = a.Value
+
+		default:
+			if kind != UnknownAction {
+				return UnknownAction, nil, fmt.Errorf("got multiple primary actions: already had %q, got %s", kind, a.Kind)
+			}
+			kind = a.Kind
+		}
 	}
 
 	return kind, tweaks, nil

internal/pushrules/util_test.go

@@ -17,6 +17,7 @@ func TestActionsToTweaks(t *testing.T) {
 		{"empty", nil, UnknownAction, nil},
 		{"zero", []*Action{{}}, UnknownAction, nil},
 		{"onlyPrimary", []*Action{{Kind: NotifyAction}}, NotifyAction, nil},
+		{"onlyPrimaryDontNotify", []*Action{{Kind: DontNotifyAction}}, DontNotifyAction, nil},
 		{"onlyTweak", []*Action{{Kind: SetTweakAction, Tweak: HighlightTweak}}, UnknownAction, map[string]interface{}{"highlight": nil}},
 		{"onlyTweakWithValue", []*Action{{Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"}}, UnknownAction, map[string]interface{}{"sound": "default"}},
 		{