| 18 Sep 2024 |
|  @joshsimmons:matrix.org changed their profile picture. | 12:32:16 |
 bkil | To HS developers. If you want to test your HS implementation with a room created with a malformed create event, please join #Office of the Matrix.org Foundation . HS other than Synapse seems to have a problem with it, so it may be a good idea to open a ticket in the respective repository. | 22:05:19 |
| 19 Sep 2024 |
 Cat | Malformed predecessor mapping specifically | 06:20:45 |
| Cat | So it’s an end user supplied bit of data. | 06:21:03 |
|  @ugp:matrix.org left the room. | 06:24:27 |
 Till | In reply to @bkil:matrix.org
To HS developers. If you want to test your HS implementation with a room created with a malformed create event, please join #Office of the Matrix.org Foundation . HS other than Synapse seems to have a problem with it, so it may be a good idea to open a ticket in the respective repository. No.. | 07:32:09 |
| Till | The room needs to be fixed | 07:32:20 |
| Till | https://spec.matrix.org/latest/client-server-api/#mroomcreate | 07:32:47 |
| Cat | Theres enough broken rooms out in the wild. | 07:59:25 |
| Cat | That some level of handling is required. | 07:59:52 |
| Cat | I know rooms that have no event ID linked but only a precursor. And then we have this new one. | 08:00:15 |
| Cat | After all its a end user supplied field that lacks validation for years in Synapse. | 08:00:30 |
| Till | And there always will be broken rooms, if we don't enforce validity of events. So, my stance is, neither Dendrite nor Conduit (and forks) are doing something wrong by rejecting the create event. | 08:10:36 |
| Cat | Tbh the fix for this long term is have Synapse enforce Required fields. | 08:21:36 |
 neilalexander | Synapse should never have accepted the event to begin with. Regardless of whether it is optional or not, the field is known to the spec as having a specific type when it is present | 08:47:37 |
| Cat | In reply to @neilalexander:dendrite.matrix.org
Synapse should never have accepted the event to begin with. Regardless of whether it is optional or not, the field is known to the spec as having a specific type when it is present it has required properties if present so yes i agree Synapse should enforce that. | 08:48:48 |
| neilalexander | I'm really tired of having to beat the "interoperability only works if we actually play by the same rules" argument but it applies here yet again, and it's mostly unfortunate/ironic that it's the Foundation's poster room that is broken because of an interop issue when the Foundation are supposed to be the ones driving interoperability | 08:51:18 |
| neilalexander | * I'm really tired of having to beat the "interoperability only works if we actually play by the same rules" drum but it applies here yet again, and it's mostly unfortunate/ironic that it's the Foundation's poster room that is broken because of an interop issue when the Foundation are supposed to be the ones driving interoperability | 08:51:47 |
| neilalexander | So IMO neither Dendrite nor Conduit are "broken" here and do not need to be fixed | 08:52:34 |
 TravisR | There's multiple issues here, which don't make a single project at fault. I don't think it's beneficial to create sides here. | 08:56:57 |
| neilalexander | To be clear, I understand human error is a thing and it's ultimately a mistake — they do happen — but I just don't think it's useful to say that this is a good litmus test for maintainers and then expect this to be fixed in implementations that are actually working properly. Otherwise this is just going to become one of those "de facto" bugs that everyone implements and then there's no guard for the next time it happens | 08:59:03 |
| TravisR | The bug is that this is being treated as an authorization failure when it's a validation failure. The server should (at best) be redacting it and serving that to the client, not failing to join. Meanwhile, client-server APIs should be preventing invalid data from getting through. | 09:01:25 |
| TravisR | We can also improve the spec to avoid the situation a bit more clearly in the future. | 09:01:43 |
 xiretza | In reply to @neilalexander:dendrite.matrix.org
To be clear, I understand human error is a thing and it's ultimately a mistake — they do happen — but I just don't think it's useful to say that this is a good litmus test for maintainers and then expect this to be fixed in implementations that are actually working properly. Otherwise this is just going to become one of those "de facto" bugs that everyone implements and then there's no guard for the next time it happens it's still a good test for "does your server blow up completely from a single malformed event in a room" | 09:01:45 |
| neilalexander | In reply to @travis:t2l.io
The bug is that this is being treated as an authorization failure when it's a validation failure. The server should (at best) be redacting it and serving that to the client, not failing to join. Meanwhile, client-server APIs should be preventing invalid data from getting through. Well, it's not even getting to the stage of authorisation failure in real terms because it just fails to unmarshal the JSON to begin with, because the struct has shape A and the JSON document has shape B and the parser quite rightly goes "hey WTF is this". Equally error handling is not something that is well prescribed in the spec either, and redacting to try and "fix" the event is a slippery slope as well that could lead to unintended side effects later | 09:06:08 |
| neilalexander | In reply to @xiretza:xiretza.xyz
it's still a good test for "does your server blow up completely from a single malformed event in a room" It's a good test for "does your server blow up completely from the one single auth event that everything in the room must reference all of the time always", sure, but not really a good test for much else | 09:06:57 |
| xiretza | In reply to @neilalexander:dendrite.matrix.org
It's a good test for "does your server blow up completely from the one single auth event that everything in the room must reference all of the time always", sure, but not really a good test for much else exactly, I see lots of opportunities for incorrect error handling with such a fundamental event | 09:07:45 |
| Till |
eventcontent_test.go:253: eventauth: unparseable create event content: json: cannot unmarshal string into Go struct field CreateContent.predecessor of type gomatrixserverlib.PreviousRoom
| 09:08:39 |
| neilalexander | I would use the power levels as a good example here. The redaction algorithm specifies that we should keep certain keys so that the PL event continues to "work", except if one of those keys is an invalid format, it's still going to fail to unmarshal anyway. Expecting the JSON parser to keep certain valid keys and throw away other invalid ones is a tall order because it's extremely uncommon for parsers to work that way, so what do we do? Throw away the whole event (safely), or mangle it until it's accepted but can no longer be guaranteed to do what we think it should do (unsafely)? | 09:09:23 |
 tulir | PL events are validated as a part of event auth, room create content is not | 09:10:04 |
