| 19 Sep 2024 |
 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 |
| neilalexander | In reply to @tulir:maunium.net
PL events are validated as a part of event auth, room create content is not That's not the point, the problem is before we get to event auth | 09:10:19 |
| neilalexander | The problem is deciding whether or not we can understand and interpret the event enough to do anything with it, before event auth happens | 09:11:02 |
 NotHSDevNico | Arguably the create event is validated during the join, so it wouldn't cause a room split if you add extra validation, so it doesn't have harmful sideeffects to validate it there, apart from that you can't join rooms, that violate the spec. Which sounds sensible to me, most Matrix rooms are way too sloppy | 09:12:41 |
| neilalexander | And my point is that the redaction algorithm is not a magical fix, because all that does is tell us to keep certain keys and throw away others. It doesn't guarantee those keys we kept will be any more valid than before, or what to do with them if they aren't | 09:12:43 |
| neilalexander | In reply to @deepbluev7:neko.dev
Arguably the create event is validated during the join, so it wouldn't cause a room split if you add extra validation, so it doesn't have harmful sideeffects to validate it there, apart from that you can't join rooms, that violate the spec. Which sounds sensible to me, most Matrix rooms are way too sloppy Indeed, broken rooms are broken rooms, they should just be really broken so that people stop expecting broken rooms to work | 09:13:32 |
 Kegan | In reply to @neilalexander:dendrite.matrix.org
So IMO neither Dendrite nor Conduit are "broken" here and do not need to be fixed I agree with this. The event was malformed. Plz send correct events. | 09:14:54 |
 Gnuxie 💜🐝 | 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. Is there anywhere in the spec makes the recommendation to redact authorization events when they fall to validate? Because that also has other implications for protected fields. | 09:16:16 |
| Gnuxie 💜🐝 | * Is there anywhere in the spec makes the recommendation to redact authorization events when they fall to validate? Because taken literally that also has other implications for protected fields. | 09:17:16 |
| Gnuxie 💜🐝 | * Is there anywhere the spec makes the recommendation to redact authorization events when they fall to validate? Because taken literally that also has other implications for protected fields. | 09:17:43 |
 richvdh | In reply to @kegan:matrix.org
I agree with this. The event was malformed. Plz send correct events. Isn't this https://github.com/matrix-org/matrix-spec-proposals/pull/2801 writ large? | 09:19:18 |
| NotHSDevNico | In reply to @richvdh:sw1v.org
Isn't this https://github.com/matrix-org/matrix-spec-proposals/pull/2801 writ large? No, this is about some servers accepting events that don't follow the spec, which in turn makes them unusable by clients. It doesn't matter if those events are trusted or not, they shouldn't end up in rooms unless they are sent by servers trying to do bad stuff | 09:21:54 |
| NotHSDevNico | The predecessor field in that room is broken, it can't be used by clients to paginate to the previous room unless they implement off-spec behaviour to fix that | 09:23:28 |
 Cat | Its not broken. its literally invalid. | 09:23:40 |
| Gnuxie 💜🐝 | Servers doing bad stuff like break the room. I think the argument to redact the event is valid, but you have to double down and make that explicit in the spec. | 09:23:53 |
| NotHSDevNico | In reply to @cat:feline.support
Its not broken. its literally invalid. That's needlessly pedantic, it is broken because it doesn't do what it should do. | 09:24:11 |
| Cat | Being broken would be missing the event ID field. But that is literally invalid as its not remotely looking like the correct data. | 09:24:12 |
| neilalexander | In reply to @gnu_ponut:matrix.org
Servers doing bad stuff like break the room. I think the argument to redact the event is valid, but you have to double down and make that explicit in the spec. It wouldn't have helped in this case: the redaction algorithm is a no-op for create events. And it still doesn't prescribe anything about the types of fields, it just says "keep these keys and throw away everything else" | 09:24:26 |
| neilalexander | You can keep fields that the redaction algorithm says you should keep and they could still be the wrong type | 09:24:52 |
| Gnuxie 💜🐝 | In reply to @neilalexander:dendrite.matrix.org
It wouldn't have helped in this case: the redaction algorithm is a no-op for create events. And it still doesn't prescribe anything about the types of fields, it just says "keep these keys and throw away everything else" Ahh, that makes sense | 09:25:25 |
| NotHSDevNico | Seems like the create event also broke showing the room version properly in Nheko... :D | 09:26:01 |
| xiretza | is there any place in the spec that tells you to redact things in an attempt to fix invalid data? where is this idea coming from?
| 09:26:33 |
| Gnuxie 💜🐝 | In reply to @neilalexander:dendrite.matrix.org
You can keep fields that the redaction algorithm says you should keep and they could still be the wrong type I guess for other events you could revert to wherever behaviour is specified for when the event is missing, but that's sketchy. | 09:27:05 |
| NotHSDevNico | In reply to @xiretza:xiretza.xyz
is there any place in the spec that tells you to redact things in an attempt to fix invalid data? where is this idea coming from?
Events are redacted before authing them, afaik | 09:27:06 |
