Centralize event/object schema checking

[ShadowJonathan]

Description:

A simple internal enhancement/feature request;

1. create a internal module that houses valid spec-schema for events (in the form of python objects or classes)
2. ...with additional logic embedded to "soft-adjust" or plain fail when incoming objects does not (strictly) match the schema
3. then adjust internal mechanisms to validate incoming (dirty) objects based on that
   • do the same with objects from the database, before using these.

Outgoing events/objects should use these classes to ensure their constructed format is automatically sterilised to the right schema.

[ShadowJonathan]

It seems #8357, #8340, #8339, #8323, #8307, #7925, #7924, #7905, #6879, #7543, matrix-org/matrix-spec-proposals#2445, #7699, #6999, matrix-org/matrix-spec#629, #8298, #8094, #7916, matrix-org/matrix-spec-proposals#1505, matrix-org/matrix-spec-proposals#1831 and #5432 are related to this.

Some honourable mentions: #8319, #8188, #8209, #8126, #8350, #8324, #8291, #6766, and #8106

[richvdh]

whilst validation of events is certainly a problem for us, I think this needs more consideration as to how it will work. In particular much of the problem is with events that are received over federation remote servers, and I don't think you have considered how to handle them. In particular, we cannot just reject them without breaking the room event graphs.

I'm also unconvinced that many of the issues you have linked would be fixed by such a mechanism, though I don't propose that we go through and debate them.

[ShadowJonathan]

In particular much of the problem is with events that are received over federation remote servers, and I don't think you have considered how to handle them. In particular, we cannot just reject them without breaking the room event graphs.

This is indeed a large problem, though there has to be a line drawn where synapse accepts events from servers, and which it rejects from unreasonable, buggy, or plain malicious servers, to disallow them injecting events into the room event graphs.

I'd be willing to discuss how this is done, but for now, I think being more stricter with what synapse accepts from other servers would be more healthy for the whole ecosystem. Probably by grandfathering these loose restrictions into previous room versions (and noting that down in the spec), and putting more strict restrictions in future room versions.

[joepie91]

In particular much of the problem is with events that are received over federation remote servers, and I don't think you have considered how to handle them. In particular, we cannot just reject them without breaking the room event graphs.

While true, not rejecting invalid events in some manner would likely have the same result on a longer timeline; if there are events which effectively contain undefined behaviour (because they do not comply with the spec), and different HS implementations handle them differently, the event graphs between those implementations would likely also diverge over time. The various hotpatches to deal with this in implementations would then effectively create a "shadow spec".

This was already discussed in one of the rooms a while ago, and there the suggestion of "make future room versions stricter" was also raised, as the probably-least-invasive solution to this problem.

[richvdh]

yes the only way to restrict what events are acceptable is with new room versions: that's a topic for matrix-doc, not here; in particular https://github.com/matrix-org/matrix-doc/issues/1646.

... and since typically we want to start using new features without tying them to room version upgrades, you still end up having to deal with "malformed" events.

We've been discussing validation internally quite a lot recently. I suggest people avoid wasting too much time on it for now, because you're just saying things that we're already considering. We'll say more in the next couple of days. In the meantime, I'm going to close this as I think it presents a particular solution to a problem, and I don't think it's the correct solution.

[joepie91]

... and since typically we want to start using new features without tying them to room version upgrades, you still end up having to deal with "malformed" events.

The argument isn't to embed event validation schemas in a room version; those schemas are already defined in the spec. Rather, the argument is to create a new room version which explicitly states that events which fail the current event schema, whatever that is at that moment in time, should be rejected. This would be a one-time change introducing strictness into the protocol explicitly, and future spec changes could be made as normal.

We've been discussing validation internally quite a lot recently.

Can this discussion be followed / contributed to somewhere? There are multiple people (including protocol implementors) with concerns about event validation, and this seems like the sort of thing that should solicit feedback from the community, rather than just being decided on "internally".

[richvdh]

@joepie91 that won't work.

@erikjohnston @neilisfragile can you share our thoughts on this please?

[richvdh]

right, seems I got volunteered here. See #8445 for the synapse-specific side of this, and matrix-org/matrix-spec-proposals#2801 for ideas affecting the matrix protocol as a whole.