From f009e541816cbae1519fede2b40b0af68020b3ab Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 30 Nov 2022 12:54:37 +0000 Subject: [PATCH] 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 | 13 ++++++++++++ internal/pushrules/evaluate_test.go | 13 ++++++------ internal/pushrules/util.go | 31 +++++++++++++++++------------ internal/pushrules/util_test.go | 1 + 4 files changed, 39 insertions(+), 19 deletions(-) diff --git a/internal/pushrules/evaluate.go b/internal/pushrules/evaluate.go index df22cb04..4ff9939a 100644 --- a/internal/pushrules/evaluate.go +++ b/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 } diff --git a/internal/pushrules/evaluate_test.go b/internal/pushrules/evaluate_test.go index eabd0241..c5d5abd2 100644 --- a/internal/pushrules/evaluate_test.go +++ b/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, - // which implicitly means "*". - {"patternEmpty", "content", "", `{"content":{}}`, true}, + {"patternEmpty", "content", "", `{"content":{}}`, false}, {"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) } }) } diff --git a/internal/pushrules/util.go b/internal/pushrules/util.go index 8ab4eab9..fb9c05be 100644 --- a/internal/pushrules/util.go +++ b/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 - tweaks := map[string]interface{}{} + var kind ActionKind + var tweaks map[string]interface{} for _, a := range as { - if a.Kind == SetTweakAction { - tweaks[string(a.Tweak)] = a.Value - continue - } - if kind != UnknownAction { - return UnknownAction, nil, fmt.Errorf("got multiple primary actions: already had %q, got %s", kind, a.Kind) - } - kind = a.Kind - } + switch a.Kind { + case DontNotifyAction: + // Don't bother processing any further + return DontNotifyAction, nil, nil - if len(tweaks) == 0 { - tweaks = nil + case SetTweakAction: + 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 diff --git a/internal/pushrules/util_test.go b/internal/pushrules/util_test.go index a951c55a..89f8243d 100644 --- a/internal/pushrules/util_test.go +++ b/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"}}, {