MSC3862: event_match (almost) anything

[Johennes]

Rendered

Implementations:

matrix-org/synapse#13466

[turt2live]

We should clarify that it's a requirement to encode "as written" or whatever terminology is required to say that encoding to 123e^7 is invalid (as some int-to-string functions like to do on large numbers).

[turt2live]

An added concern is I don't think any amount of documentation will save us from folks using language-specific features to encode numbers, which can easily lead to improper notation. The implementation likely wouldn't notice until it's too late, and generally cause the same sorts of problems we saw with Canonical JSON: implementations differing in how strict they are to the spec, leading to mismatched notifications.

[Johennes]

We should clarify that it's a requirement to encode "as written" or whatever terminology is required to say that encoding to 123e^7 is invalid (as some int-to-string functions like to do on large numbers).

This made me think, what if somebody used the exponential form in the JSON? E.g. should "pattern": 123e7 be converted into "123e7"?

[turt2live]

I don't believe it's allowable by Canonical JSON, but if it is then ugh/yay.

[Johennes]

Ah, you're right. It isn't. I've added a corresponding sub-bullet to explain the conversion.

[turt2live]

From a client perspective, I don't think either this MSC or #3758 is particularly troublesome, though personally I do lean more towards this MSC as it shows what push rules "should" be: matches on events irrespective of data type (there's no reason to punish someone for "42" when they meant 42, at least not in this subsystem). However, the appeal of a clean slate to "fix" push rules with 3758 is equally as high.

Both are equally complex to implement and would be considered a breaking change: here, clients won't agree with servers on what the notification counts are (not that they do already...^[1]), and with 3758 it's a whole new condition type. In essence, both MSCs would be writing a new condition type into the client code-wise.

^[1]Element Android for instance doesn't handle keyword notifications, leading to nasty surprises when the user moves from mobile to desktop.

[richvdh]

matches on events irrespective of data type (there's no reason to punish someone for "42" when they meant 42, at least not in this subsystem)

Do you really think so? It seems to me that this sort of "sloppy" matching could lead to things like #2223, where the push rules end up matching something different to the rest of the system. (That was the cause of security vulnerabilities in synapse and matrix-js-sdk.)

[richvdh]

I think my primary concern with #3758 is that a new push rule condition type might be more likely to break clients than extending the existing one. If @turt2live's opinion, as a client developer, is that there's not much to choose, my inclination is to go with the more complete, and more elegant, #3758.

[ara4n]

I also prefer #3758, as it avoids casting everything to strings, and the dotted notation is more powerful.

[turt2live]

matches on events irrespective of data type (there's no reason to punish someone for "42" when they meant 42, at least not in this subsystem)

Do you really think so? It seems to me that this sort of "sloppy" matching could lead to things like #2223, where the push rules end up matching something different to the rest of the system. (That was the cause of security vulnerabilities in synapse and matrix-js-sdk.)

It's not a strong opinion, tbh. For things like notifications it'd be a nice-to-have to support typeless matches, leaving the type validation for elsewhere (whether an event can be rendered, etc). It has been the cause of some security vulnerabilities, but push rules in general are an open avenue for multiple different kinds of implementation-specific attack to begin with. It would be a challenge to keep the "sloppiness" to the push rules though: maintaining a higher standard for other subsystems to counteract the typelessness is not traditionally something that's gone well.

[Johennes]

Closing this since #3758 is moving ahead.