Should v3+ room version events containing an event_id field be rejected?

[anoadragon453]

Link to problem area: https://spec.matrix.org/v1.12/rooms/v3/#event-format

Issue

We received this pull request to Synapse which ignores incoming room v3+ events over federation if they contain an event_id field. The rationale being that events in rooms with room version v3+ have their event ID derived from the non-redactable contents of the event.

Given that the event_id field in such an event would thus be fed into that hashing algorithm, in order to generate itself, this event structure appears improbable. In addition, if such a field exists in the event, it's likely that in practice the sending homeserver is accidentally sending an event constructed for a room v1/v2 room, instead of the v3+ room it's intended to be placed in.

Should the spec clarify that an event received with an event_id field already included, destined for a v3+ room, should thus not be accepted by the receiving homeserver?

[anoadragon453]

Related discussion: matrix-org/synapse#7543, #365.

From matrix-org/synapse#7543, the Synapse devs opted to silently drop what they call "invalid" events (those that can't be parsed/added to the local DAG).

In the case of the pull request I mentioned; if the event_id was provided in the original JSON, we use it to inform the homeserver that we weren't able to correctly parse its event via the pdus response field. Otherwise, the event is indeed silently dropped.

[richvdh]

Should v3+ room version events containing an event_id field be rejected?

My gut instinct here is "yes, absolutely". I think the more interesting question is: "... and if so, what should a server's behaviour be when encountering such an event?".

From matrix-org/synapse#7543, the Synapse devs opted to silently drop what they call "invalid" events (those that can't be parsed/added to the local DAG).

"Opted to" makes it sound like that is what already happens. As far as I can tell, it has not yet happened (though it does look like 2020-vintage-us decided it was the right thing to do. 2024-vintage-me isn't quite so convinced, but nm).

[S7evinK]

As mentioned in #synapse-dev, I don't think the event should be rejected and added with an error, as the event may otherwise be valid.

For Dendrite the "bogus" event_id is fine, as outlined here, as it removes the event_id field (and other) from the incoming JSON. Similar, if Dendrite were to unmarshal directly onto a struct which only contains the allowed fields for an event in room version v3+, the event_id would be dropped. I think the Rust implementations are also unmarshalling onto a struct and thus have the same behavior of dropping the event_id or any superfluous fields.

So TLDR: I think implementations should try to work with the given event, even if it may contain extra fields. So in the case of element-hq/synapse#17893, I think the correct way to fix the problem is to simply ignore the event_id field and see if it is still valid instead of returning with an error.

[neilalexander]

Dendrite relevant code:

https://github.com/matrix-org/gomatrixserverlib/blob/dbd5f31fefc031633c3418165e4ef6d343e03999/eventV2.go#L134-L138

None of the event_id, unsigned, outlier, destinations or age_ts fields are fed into the hashing algorithm or signature checks, they are stripped very early on in event processing. We had to do this in order to work around Synapse's leaky behaviour in the past and therefore I don't think there's really any option anymore but to spec the behaviour and continue to do so.

Now we might have auth events etc that contain these fields as a result, so it would be breaking if a new participant server to a room were to start rejecting these events just for the hell of it when today's behaviour would validate them perfectly fine.

[morguldir]

Note this section from the v3 spec: https://spec.matrix.org/v1.12/rooms/v3/#event-format

When events are sent over federation, the event_id field is no longer included

Additionally synapse has never been able to process such an event, see matrix-org/synapse@84af577#diff-a58f64c39f2c387f7e8820f3bcb82784cfa370358a660fc435ba0a5e51a2e778R251

[kegsay]

The only valid reason I see here why we should process these events is the fact that there may be extra fields on the event which we care about.

Those extra fields may be important, even if we (the server) don't know about it. Ideally we would be pushing new room versions if we suddenly want to read new fields though. The risks implementation confusion for calculated fields which reside on the event, in this case uniquely the event_id field: we don't want to use the wire-format and instead want to calculate it from the hash.

Overall, I agree with @neilalexander and @S7evinK here: we should continue to handle those events as-is and remove the keys we know about. We should do much better here and reject/ignore/drop those events, but that would need a new room version.