-
Notifications
You must be signed in to change notification settings - Fork 412
MSC4311: Ensuring the create event is available on invites #4311
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
Merged
+108
−0
Merged
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
b7ec690
MSC: Ensuring the create event is available on invites and knocks
turt2live a982f53
Clarify that we're not replacing auth checks on create events
turt2live 78a1387
Acknowledge disparity in unaffected room formats
turt2live 4971929
This isn't a word despite what spellcheck thinks
turt2live 7d2c5f8
add migration steps
turt2live eeb4640
Just use the normal stripped_state format
turt2live 05b8b49
Rewrite problem statement and solution components to match new scope
turt2live 5ac4656
review: improve intro
turt2live e651b61
review: clarify format
turt2live afff25e
review: fix knocks
turt2live 72f6a3a
review: fix room_id calculation
turt2live bef6540
review: rework migration wording
turt2live 2104c97
Fully format all events in stripped state
turt2live 05b3d96
Spelling
turt2live 249dec3
Make migration normative
turt2live d34f796
Apply suggestions from code review
turt2live b8ec2f6
Adjust requirements on servers
turt2live File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
# MSC4311: Ensuring the create event is available on invites and knocks | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
Historically, when processing an invite or knock, safety tooling would parse the room ID despite | ||
[being opaque](https://spec.matrix.org/v1.15/appendices/#room-ids) to determine the server which | ||
originally created the room. If that server was considered abusive, the invite/knock may be rejected | ||
or blocked early by the tooling. This approach is preferred because the user sending the invite may | ||
not be on the same server as the user who created the room, but is often still checked by safety | ||
tooling. | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
With [MSC4291](https://github.com/matrix-org/matrix-spec-proposals/pull/4291), room IDs lose their | ||
domain component, which can make the first check described above harder if not impossible. When | ||
speaking over federation however, [invites](https://spec.matrix.org/v1.15/server-server-api/#put_matrixfederationv1inviteroomideventid) | ||
and [knocks](https://spec.matrix.org/v1.15/server-server-api/#get_matrixfederationv1make_knockroomiduserid) | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
contain a concept of "stripped state events" which give (unsigned) context about the room for the | ||
consuming clients. The Client-Server API [Stripped State](https://spec.matrix.org/v1.15/client-server-api/#stripped-state) | ||
section has more information about what these 'events' are and how they work. | ||
|
||
This MSC aims to reintroduce the accessibility of the creator's server name to clients (and safety | ||
tooling) by requiring the `m.room.create` event *at least* be a full and proper PDU in affected room | ||
versions when transiting the Federation API via stripped state, allowing intermediary servers and | ||
clients to inspect the creator's domain name. | ||
|
||
|
||
## Proposal | ||
|
||
This proposal is split into the Federation API changes and Client-Server API changes for clarity. | ||
|
||
|
||
### Federation API | ||
|
||
For room versions affected by [MSC4291](https://github.com/matrix-org/matrix-spec-proposals/pull/4291), | ||
the `m.room.create` event MUST be included in [`invite_room_state`](https://spec.matrix.org/v1.15/server-server-api/#put_matrixfederationv1inviteroomideventid) | ||
and [`knock_room_state`](https://spec.matrix.org/v1.15/server-server-api/#get_matrixfederationv1make_knockroomiduserid) | ||
and MUST be a properly-formatted PDU according to that room version's event format specification. | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
If the `m.room.create` event is not present, not a PDU, or not for the room ID specified, the server | ||
MUST fail to continue processing the invite or knock. For invites, this is a `400 M_MISSING_PARAM` | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
standard Matrix error (new to the endpoint). For knocks, this means the server drops the `make_knock` | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
response and never completes a `send_knock`. For both operations, the associated Client-Server API | ||
request is failed with `500 M_BAD_STATE`. A 4xx error isn't used for the Client-Server API because | ||
there's nothing the client can materially do differently to fix that request. | ||
|
||
For room versions *not* affected by MSC4291, servers SHOULD include the properly-formatted `m.room.create` | ||
PDU. This is not made mandatory to avoid a situation where servers trust data that shouldn't be trusted | ||
for the reasons described by MSC4291. | ||
|
||
To determine whether a room is "affected" by MSC4291, servers MUST inspect the `room_id` rather than | ||
the create event's `room_version`. Specifically, a room ID with a domain component is *not* affected | ||
while one without a domain component (and happens to be `!<43 unpadded urlsafe base64 chars>`) *is* | ||
affected. This is done to ensure the server is not potentially confused by a malicious server providing | ||
a create event for a different, unaffected, room. | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
When a room is affected, the server MUST validate the `m.room.create` event as follows for the purposes | ||
of the above: | ||
|
||
1. If the event has a `room_id`, reject. | ||
2. If the event does not otherwise comply with the event format for its self-described room version, | ||
reject. | ||
3. If the event fails [signature checks](https://spec.matrix.org/v1.15/server-server-api/#validating-hashes-and-signatures-on-received-events), | ||
reject. The content hash check MAY be skipped as the event can safely be redacted prior to all of | ||
these checks. | ||
4. If the event's [reference hash](https://spec.matrix.org/v1.15/server-server-api/#calculating-the-reference-hash-for-an-event) | ||
does not match the event ID contained in the room ID, reject. | ||
5. Otherwise, allow. | ||
|
||
|
||
### Client-Server API | ||
|
||
Using the same "affected" calculation above, when serving `m.room.create` in [Stripped State](https://spec.matrix.org/v1.15/client-server-api/#stripped-state) | ||
to clients in affected room versions, servers MUST format that event in the same way it would for | ||
[`GET /rooms/:roomId/event/:eventId`](https://spec.matrix.org/v1.15/client-server-api/#get_matrixclientv3roomsroomideventeventid). | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
For unaffected room versions, servers SHOULD NOT format the create event, if known, any different than | ||
what stripped state already requires. This is to ensure clients do not accidentally rely on information | ||
which may be confusing to them, such as the "creator server name". This is especially true because the | ||
server is required to treat those events as untrusted for the reasons described in MSC4291. | ||
|
||
**Note**: "serving to clients" includes [pushing to Application Services](https://spec.matrix.org/v1.15/application-service-api/#pushing-events). | ||
|
||
|
||
## Potential issues | ||
|
||
* Not using the same format for both affected and unaffected room versions may lead to parser complexity. | ||
This is considered tolerable by this proposal to ensure that clients (and servers) have limited | ||
ability to confuse themselves. | ||
|
||
* This technique is not applied to other state events present in stripped state. A future MSC or | ||
series of MSCs is expected to address this particular concern. | ||
|
||
* Some server implementations allow safety tooling and other applications to hook into them between | ||
the Federation API and Client-Server API. Such implementations are encouraged to make the create | ||
event reasonably available in its full form to those applications. Typically, this will be an internal | ||
representation of the event which still has the capability to serialize down to a PDU. | ||
|
||
* For unaffected room versions, it's a little awkward that the Federation API SHOULD use PDU format, | ||
but the Client-Server API SHOULD NOT format the event as a real event. This proposal makes this | ||
choice to give *some* amount of information to intermediary tooling within the server, but can | ||
easily be modified to be a "SHOULD NOT" for both cases. | ||
|
||
|
||
## Alternatives | ||
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
This proposal fills a potential gap in information created by MSC4291, making the alternatives roughly | ||
equivalent to "don't do this". | ||
|
||
|
||
## Security considerations | ||
|
||
Security considerations are made throughout, especially in areas where a server may accidentally trust | ||
data it shouldn't. | ||
|
||
|
||
## Unstable prefix | ||
|
||
This proposal does not require an unstable prefix as the behaviour can be accomplished without overly | ||
affecting client or server implementations. | ||
|
||
|
||
## Migration | ||
|
||
*This section is non-normative for spec writing purposes. It only affects implementations which have | ||
implemented room version 12 upon its release.* | ||
|
||
Room version 12 contains MSC4291 and is expected to be used in production prior to this proposal | ||
becoming stable itself. To account for this, servers SHOULD treat "MUST" as "MAY" throughout this | ||
proposal until 1 full spec release cycle has passed since this MSC's own release in the specification. | ||
|
||
This translates to a timeline anywhere between 2 and 6 months, depending on ecosystem rollout. An | ||
example *possible* release schedule is: | ||
|
||
1. August 2025 - Matrix 1.16 is released with Room Version 12. | ||
2. October 2025 - Matrix 1.17 is released with this proposal; servers use "MAY" keywords. | ||
3. January 2026 - Matrix 1.18 is released; servers switch to "MUST" keywords. | ||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
Servers MAY switch to "MUST" keywords early if their local ecosystems are prepared for the change. | ||
|
||
|
||
## Dependencies | ||
|
||
This proposal requires [MSC4291](https://github.com/matrix-org/matrix-spec-proposals/pull/4291) in | ||
order to make any amount of sense. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation requirements:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruma (server; sending):
knock
intomembership
, allow non-stripped events to be represented in membership handshakes, unstable support for MSC4311 ruma/ruma#2171sync_events
, unstable support for MSC4311 ruma/ruma#2173There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meowlnir (client/bot replacing room ID parsing with creator parsing in invite antispam): maunium/meowlnir@eeccd4a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though servers haven't actually added the validation on the stripped state yet, per the migration section of the MSC, no one has indicated that it's a nightmare to do so. I'm inclined to consider that "implementation" for the purposes of starting FCP, but welcome (particularly SCT) opinions on whether that's valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Server returning and receiving: https://gitlab.com/famedly/conduit/-/commit/532b17ade86b4373db9f3bcca6af6815dd9b5b10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MSC now requires all stripped state events to be PDUs over federation. I believe element-hq/synapse#18822 and matrix-org/complement#796 demonstrate it's not impossible, even if the code itself is bad.