Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2801: Make it explicit that event bodies are untrusted data #2801

Merged
merged 4 commits into from
Mar 28, 2021

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Oct 1, 2020

@richvdh richvdh added the proposal A matrix spec change proposal label Oct 1, 2020
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

proposals/2801-untrusted-event-data.md Show resolved Hide resolved
@richvdh
Copy link
Member Author

richvdh commented Mar 22, 2021

I think this has general agreement, so

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Mar 22, 2021

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 mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Mar 22, 2021
@richvdh richvdh mentioned this pull request Mar 22, 2021
@dbkr
Copy link
Member

dbkr commented Mar 22, 2021

✅ 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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mention the infeasiblity of alternative of requiring a new room version every time event specifications change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this covered under the previous heading? It's not really relevant to this heading.

@turt2live turt2live added this to Ready for FCP ticks in Spec Core Team Backlog Mar 23, 2021
@mscbot
Copy link
Collaborator

mscbot commented Mar 23, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Mar 23, 2021
Comment on lines 106 to 127

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(https://github.com/matrix-org/matrix-doc/pull/2801/files/5db56391f30621db3a7e36824417fc57ed5f169f#diff-db3e334e6547f917a79147c0c0df60709ac25caca1ae5aaf6822482f0e04d553R188-R189)

In short, yes we have considered it, but it doesn't change the general philosophy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-authored-by: Jonathan de Jong <jonathandejong02@gmail.com>
@anoadragon453 anoadragon453 moved this from Ready for FCP ticks to In FCP in Spec Core Team Backlog Mar 23, 2021
@mscbot
Copy link
Collaborator

mscbot commented Mar 28, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Mar 28, 2021
@turt2live turt2live merged commit ed690c8 into master Mar 28, 2021
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Mar 28, 2021
@turt2live turt2live moved this from In FCP to Done to some definition in Spec Core Team Backlog Mar 28, 2021
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Apr 6, 2021
@turt2live turt2live self-assigned this Apr 6, 2021
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Apr 6, 2021
@turt2live
Copy link
Member

Merged 🎉

richvdh pushed a commit that referenced this pull request Aug 23, 2021
…_data

MSC2801: Make it explicit that event bodies are untrusted data
richvdh pushed a commit that referenced this pull request Aug 27, 2021
…_data

MSC2801: Make it explicit that event bodies are untrusted data
@afranke afranke deleted the rav/proposal/untrusted_event_data branch September 22, 2021 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
Archived in project
Spec Core Team Backlog
  
Done to some definition
Development

Successfully merging this pull request may close these issues.

None yet

10 participants