MSC2801: Make it explicit that event bodies are untrusted data #2801
Conversation
turt2live
added
kind:core
| Firstly, let's examine the reasons why such malformed events can exist. Some of | ||
| these reasons may appear to have trivial solutions; these are discussed below. | ||
|
|
||
| 1. Most obviously, Synapse as it currently stands does very little validation |
There was a problem hiding this comment.
presumably the reason for Synapse being listed here is because it's the historical reference for the spec, though these days there are other implementations which do much stronger validation?
There was a problem hiding this comment.
Synapse is listed here because its current behaviour is the lowest common denominator: as long as there exist servers which exhibit Synapse's current behaviour (as there will be unless we somehow spec otherwise), clients and users must handle that behaviour.
|
I think this has general agreement, so @mscbot fcp merge |
|
Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged people: Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
mscbot
added
disposition-merge
proposed-final-comment-period
|
✅ and just to say thank you for taking the trouble to get complete reasoning-through of the problem down: it's super helpful to have the exact analysis of this documented somewhere. |
|
|
||
| Even if all the servers in the room can be upgraded at once, what about any | ||
| `m.room.member` events which were sent before the rule change? | ||
|
|
There was a problem hiding this comment.
Mention the infeasiblity of alternative of requiring a new room version every time event specifications change
There was a problem hiding this comment.
Isn't this covered under the previous heading? It's not really relevant to this heading.
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
mscbot
added
final-comment-period
|
|
||
| As outlined above, one of the big problems in this area is how we deal with | ||
| events sent over federation; in particular, if subsets of the servers in a room | ||
| have different ideas as to which events are "valid", then their concepts of the | ||
| room state can begin to drift, and the room can eventually become | ||
| "split-brained". This makes it hard to simply say, for example, | ||
| "`m.room.member` events with a non-string `displayname` are invald and should | ||
| not form part of the room state": we have a risk that some servers will accept | ||
| the event, and some will not. | ||
|
|
||
| One approach to solving this is via [room | ||
| versions](https://matrix.org/docs/spec/index#room-versions). By specifying that | ||
| a change of rules only applies for a future room version, we can eliminate this | ||
| potential disagreement. | ||
|
|
||
| The process of changing a room from one version to another is intrusive, not | ||
| least because it requires that all servers in a room support the new room | ||
| version (or risk being locked out). For that reason, it is extremely | ||
| undesirable that any new feature require a new room version: whenever possible, | ||
| it should be possible to use new features in existing rooms. It therefore | ||
| follows that we cannot rely on room versions to provide validation of event | ||
| data. |
There was a problem hiding this comment.
There is another alternative that only requires auth-changing events such as redaction, PL change, role change, kick and ban to be well formed with no extensions, and all others be untrusted. Have you considered that?
There was a problem hiding this comment.
This is the sort of thing that is meant by
It may be possible to assert that specific, known cases can be treated as
trusted data, but these should be called out as specific cases.
In short, yes we have considered it, but it doesn't change the general philosophy.
There was a problem hiding this comment.
I believe that's covered by the second to last sentence in the MSC: https://github.com/matrix-org/matrix-doc/pull/2801/files#diff-db3e334e6547f917a79147c0c0df60709ac25caca1ae5aaf6822482f0e04d553R188-R189
Co-authored-by: Jonathan de Jong <jonathandejong02@gmail.com>
|
The final comment period, with a disposition to merge, as per the review above, is now complete. |
mscbot
added
finished-final-comment-period
and removed
disposition-merge
final-comment-period
turt2live
added
spec-pr-missing
turt2live
added
spec-pr-in-review
turt2live
added
merged
|
Merged 🎉 |
…_data MSC2801: Make it explicit that event bodies are untrusted data
…_data MSC2801: Make it explicit that event bodies are untrusted data
Rendered