MSC4311: Use full PDU's in stripped state (like `invite_room_state`) over federation and always include `m.room.create` event by MadLittleMods · Pull Request #19723 · element-hq/synapse

MadLittleMods · 2026-04-23T17:23:09Z

Background

This PR was originally just trying to remove the flawed MSC4311 partial implementation as client side API's like /sync should still use stripped events. But it turns out we were just re-using the client logic for the federation side and things might break if we didn't include the full m.room.create event so this PR now introduces MSC4311 support to use full PDU's in the invite_room_state/knock_room_state in the federation API's.

The flawed implementation was originally introduced in 0eb7252 (no PR I assume because part of Hydra security fix)

Spawning from reviewing #19722 and noticing that we have TestMSC4311FullCreateEventOnStrippedState in Complement which already passes even though that test looks flawed:

I think this test is mixing up what MSC4311 proposes. Perhaps these were changes to the MSC that came after?

For the client API's like /sync, it only proposes that m.room.create is a required stripped state event.

For the federation API's, alongside requiring m.room.create, it also mandates using the full event PDU format for all events in the invite_room_state/knock_room_state on m.room.member events (in unsigned)

What does this PR do?

Always use stripped state for client API's
1. Remove flawed MSC4311 partial implementation (as explained above)
2. Sanitize stripped state when we receive events over federation
Use full PDU's when sending invite_room_state/knock_room_state over federation
Validate PDU's and warn when receiving invite_room_state/knock_room_state over federation

Complement tests: matrix-org/complement#796

Part of #19414

Dev notes

COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestMSC4311FullEventsOnStrippedStateFederation

COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestMSC4311StrippedStateClientAPI

COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run 'TestMSC4311StrippedStateClientAPI/parallel/`invite_state`_on_`/sync`_\(remote_invite\)'

Todo

Fix/remove TestMSC4311FullCreateEventOnStrippedState Complement test so it doesn't look for the full PDU in /sync.
- It looks like Update MSC4311 tests to conform to the spec and better coverage matrix-org/complement#796 tries to address this
Figure out whether we care about addressing knock_room_state in this PR
Validate knock_room_state
Figure out Sytest failures -> Use sender instead of user_id for any stripped state event usages matrix-org/sytest#1425
Merge Sytest changes, Use sender instead of user_id for any stripped state event usages matrix-org/sytest#1425
Merge Complement tests when we merge this PR, Update MSC4311 tests to conform to the spec and better coverage matrix-org/complement#796
Wait until 2027-06-01 to address Apply strict MSC4311 stripped state validation after 2027-06-01 #19943

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

…al-implementation

MadLittleMods · 2026-05-04T21:05:09Z

-        and event.get_state_key() == ""
-    ):
-        return event.get_pdu_json()
-


Removing the flawed MSC4311 partial implementation

MadLittleMods · 2026-05-04T21:07:05Z

+
+        return [strip_event(e) for e in state_to_include.values()]
+
+    async def get_stripped_room_state_ids_from_event_context(


Split some logic out of get_stripped_room_state_from_event_context(...) into get_stripped_room_state_ids_from_event_context(...)

This way we can use the same event selection logic but fetch/format them however we see fit.

MadLittleMods · 2026-05-04T23:25:37Z

-        # TODO(paul): assert that room_id/event_id parsed from path actually
-        #   match those given in content


Removed as we actually do this now in FederationServer.on_invite_request(...) (added in this PR)

…al-implementation Conflicts: rust/src/room_versions.rs

MadLittleMods · 2026-05-22T00:09:04Z

+            # Add the invite itself
+            #
+            # FIXME: Doesn't seem to be in the spec
+            invited_state.append(strip_event(room.invite))


Found that we were accidentally providing the full invite event in this case. This is just something that's been happening since inception (2015)

The Complement tests check to make sure the client sees stripped state which is how I caught this.

Additionally, just documenting this existing behavior.

To note, the stripped state event fields cover the use cases described in matrix-org/synapse#6739 (comment). The client has the invite sender to show who invited you.

MSC4319 currently proposes that we "SHOULD" keep the full event behavior for at-least one spec version (discussed in matrix-org/matrix-spec-proposals#4319 (comment))

I find this pretty dubious given stripped state already covers the use cases described in matrix-org/synapse#6739 (comment) and only Draupnir was given as an example that relies on event_id (MSC discussion: matrix-org/matrix-spec-proposals#4319 (comment))

The desire for more information like event_id, origin_server_ts, etc is real but relying on this existing unspecced snowflake behavior is not something we should worry about. And it completely flies in the face of MSC4311 which still says clients should receive stripped state.

erikjohnston · 2026-07-22T13:32:52Z

        state_res: StateResolutionVersions::V2_1,
        msc4289_creator_power_enabled: true,
        msc4291_room_ids_as_hashes: true,
+        msc4311_stripped_state: true,


Will this not break invites across new and old Synapses for v12 rooms?

How so? We don't strictly validate anything (just warn when it fails validation in v12 rooms).

See "What does this PR do?" in the PR description.

Ah, my bad. Feels very odd to have an option that merely causes things to log. Especially given that initially this will log for almost every invite in v12. I wonder if its worth it for now?

It's house-keeping for the room versions. We use it today and will use it in the future.

erikjohnston · 2026-07-22T13:48:22Z

+                and err.errcode == Codes.MISSING_PARAM
+            ):
+                raise SynapseError(
+                    500,


If we're not doing a 4xx response then we shouldn't use 500 as it's not an internal server error. I'm not really sure what the closest HTTP code for "this server can't perform the action requested", it's half a 4xx and half a 5xx.

TBH it feels quite similar to the 400 above, "User's homeserver does not support this room version".

As the comment above states, the MSC tells us this should be a 5xx error.

I chose 500 as the other server is saying our homeserver is doing something wrong.

And the description sounds perfect:

The HTTP 500 Internal Server Error server error response status code indicates that the server encountered an unexpected condition that prevented it from fulfilling the request.

-- https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/500

In other words, our homeserver implementation is preventing the invite from going through. Nothing the user can do.

Hmm, bleurgh. The problem is that in practice returning a 500 is surprising behaviour for almost everyone: they expect a 500 to indicate there is a bug/misconfiguration in the server. Clients/users will retry when they hit a 500 as they assume it may be a transient problem.

Generally, regardless of what the web specs imply, clients view 5xx as possibly transient server issues, while 4xx as a final failure. Most 4xx are actually things the user can't do anything about, e.g. 403 or 404.

Concretely: returning a 500 will cause confusion for server operators (and us) as that should be reserved for bugs, and clients will keep retrying this endpoint. Intermediate proxies do sometimes infer uptime via 5xx errors (e.g. returning 502 often causes this).

Have taken this up wit the Spec Core Team.

MSC is up at: matrix-org/matrix-spec-proposals#4528

erikjohnston · 2026-07-22T14:02:45Z

+        state_events = await self.store.get_events(state_ids)
+        assert set(state_ids) == set(state_events.keys()), (
+            "We should have all events available that were set as stripped state."
+        )


Hmmm, this is technically a behaviour change where we are recalculating the stripped state here rather than using the stripped state we calculated at event creation.

Generally, I'm wondering if we should change the format of what we store as invite_room_state in the event json in the DB, rather than having this somewhat weird thing going where we have to recalculate it. Not really sure how tractable it is, but you can imagine a world where we store in a separate table the stripped state event IDs for locally generated events.

🤷 Future plans

See #19723 (comment)

… 'NoneType'`

…al-implementation

…c/test_rooms_invites.SlidingSyncRoomsInvitesTestCase to use 'stripped state' to compare against responses instead of fleshing out a full PDU

erikjohnston · 2026-07-29T10:53:55Z

+                    "(either one could be at fault).",
+                    Codes.UNKNOWN,
+                    additional_fields={
+                        "cause": err.msg,


Err, there's little point returning this to clients as nothing will read it. It can also be confusing if devs interpret it as the what this server is complaining about to the client

erikjohnston · 2026-07-29T10:58:41Z

+                    f"Received {HTTPStatus.BAD_REQUEST} {Codes.MISSING_PARAM} response from remote homeserver "
+                    "while trying to send the invite over federation. This indicates a compatibility problem "
+                    "between your homeserver and the homeserver you're trying to send the invite to "
+                    "(either one could be at fault).",


Sorry, missed this last time but I don't think this is a useful user facing error, it's too verbose and technical.

I think one of the following maybe:

Could not send the invite to the user's server.

The user's server rejected the invite.

Your server cannot send invite's to the remote server

We want something that the user can understand the net result: namely the invite failed for some reason.

as a user, not knowing why the invite failed (beyond "computer said no") would be more frustrating than the confusion caused by a technical error that I can ask for help with. As a server admin, it'd be even more annoying because I would then have to open a terminal and scrape through logs to try and find out more information.
Since this is explicitly only caused by an incompatibility, if you want non-technical, I think something more akin to

The user's server did not understand our invite and may be outdated

would make sense. It's simple, concise, but still explains what the problem was, and a possible solution. It's also similar to the error returned when the remote server doesn't support the room version, just above.

I'm in agreeance on providing enough info so that the error is useful to understand and for others (like us when people ask for help).

I've updated the error message to start off more plainly: The other user's server did not understand our invite. [...]

Fair that we should communicate that it is likely an incompatibility issue.

The other user's server did not understand our invite. [...]

I think this is confusing to people who aren't technical, how can an invite be not "understood"?

The problem is that these are user visible and therefore affect the UX. In practice giving "complicated" error messages simply causes more confusion for the vast majority of users with little benefit (even to technical users). Especially in this case where nothing we're telling the user in the error messages is actually useful information. The fact that the remote server rejected it with a 400 M_MISSING_PARAM is pretty meaningless?

The overarching problem here is that we simply don't know why the other side rejected the invite, and so we can't give a useful error message to the user, just an unsatisfactory "something went went wrong".

I think the information that is useful for the user is:

The invite failed.

It's not the user's fault.

Retrying immediately won't help.

So perhaps something like:

Cannot send the invite to the user's server; it was rejected. (This could be due to a compatibility issue between the servers).

It's not great, but I think says all that is useful to convey in a more friendly language. It's also unique enough that if more information is required server admins can scrape the logs easily.

Better phrasing is good but we're losing too much information in my opinion.

The problem needs to be clear. Forcing a random user to get their homeserver admin to log dive is not nice. It's undue burden on a homeserver admin and not possible in unresponsive admin scenarios.

If someone posts that error message in Synapse Admins and asks why, my next question is what did the other server respond with? If I looked specifically at the Synapse code, I can deduce it in the this case because it corresponds to one specific scenario but it still requires me to dig. That's just not necessary. Then the next step to look at is the two homeserver versions and see what they're running.

These are the same questions I would have as a user if this happened to me. Knowing that the other server responded with 400 M_MISSING_PARAM is useful to me. It doesn't matter that either my homeserver or the other homeserver needs to change which I don't control.

I've updated to this (separated the simple explanation from the details with \n\n):

f"Invite was rejected by the recipient's server.\n\n" f"The remote homeserver ({destination}) returned {HTTPStatus.BAD_REQUEST} {Codes.MISSING_PARAM} " "which indicates a compatibility problem between your homeserver and the " "homeserver you're trying to send the invite to (either one could be at fault).",

To better illustrate, this is a Hulu error. This drives me crazy

We're having trouble playing...

Rest assured, we're working on it. In the meantime, it may help if you restart the video.

Need help? Visit help.hulu.com/support in a browser

Hulu Error Code: P-DEV318
05/31/2022 20:14:41 PM PDT

I do agree that we shouldn't return a completely vacuous error. However, we do need to balance utility between super-power users and UX for normal users.

To that end: I don't really see what specifying {HTTPStatus.BAD_REQUEST} {Codes.MISSING_PARAM} in the error response gives users or helps with diagnostics. The proposed error response gives the information needed: one of the two servers is probably out-of-date. If that is not the case then you'll need to dig into the logs anyway to figure out what is wrong, you're not going to be able to tell just by the error codes.

I wouldn't be that adverse to adding extra information if our error messages had an extra collapsed-by-default section for "Advanced Information", but we don't have that.

Plus, generally we're reluctant to give out too much information to users (like stack traces), as they can too easily leak information we'd rather wasn't. At which point for most cases you're going to need to look at the logs anyway. (Not to mention that any remedies here require involvement by the server admins)

From your three point criteria: the invite failed, it's not the user's fault, retrying won't help. A hedged/vague message asserts those. A specific one demonstrates them. Reassurance without specifics isn't believed. Everyone has been told "sorry, something went wrong" by a system that was in fact blaming them or hiding the fact that they could have changed the outcome by trying something different. The details help close those loops and let the person stop second-guessing themselves.

The \n\n separation means the details cost the non-technical user nothing as they read the first sentence and stop, which is effectively your ideal way to provide "Advanced Information" anyway.

I think we've struck a good balance with the last iteration.

erikjohnston · 2026-07-29T13:54:07Z

+                and err.errcode == Codes.MISSING_PARAM
+            ):
+                raise SynapseError(
+                    500,


Hmm, bleurgh. The problem is that in practice returning a 500 is surprising behaviour for almost everyone: they expect a 500 to indicate there is a bug/misconfiguration in the server. Clients/users will retry when they hit a 500 as they assume it may be a transient problem.

Generally, regardless of what the web specs imply, clients view 5xx as possibly transient server issues, while 4xx as a final failure. Most 4xx are actually things the user can't do anything about, e.g. 403 or 404.

Concretely: returning a 500 will cause confusion for server operators (and us) as that should be reserved for bugs, and clients will keep retrying this endpoint. Intermediate proxies do sometimes infer uptime via 5xx errors (e.g. returning 502 often causes this).

Have taken this up wit the Spec Core Team.

…al-implementation

…tand our invite.` See #19723 (comment)

…c/test_rooms_invites.SlidingSyncRoomsInvitesTestCase to use 'stripped state' to compare against responses instead of fleshing out a full PDU

…al-implementation

See #19723 (comment)

MadLittleMods

LLM-assisted review

MadLittleMods · 2026-08-04T03:40:07Z

+            #
+            # We do this after the above checks to make sure it's a valid event
+            # from this room.
+            if pdu.type == EventTypes.Create:


Needs check for state_key == ""

MadLittleMods · 2026-08-04T03:53:48Z

+            # Validate signature/hashes
+            try:
+                pdu = await self.federation_client._check_sigs_and_hash(
+                    room_version, pdu
+                )


hmmm, MSC4311 says it only cares about when an event would "fail signature checks" but it links to the spec about validating hashes and signatures.

As a note, when the hash check fails, _check_sigs_and_hash doesn't actually raise, it just redacts the event. I guess if we care about checking hashes, we should be using the actual potentially redacted PDU returned.

This also runs the policy server and spam checker callbacks over the PDU's which we don't need/want here as far as I can tell.

MadLittleMods · 2026-08-04T04:02:22Z

+        #
+        # Parse/validate `knock_room_state`
+        try:
+            stripped_room_state = await self._parse_stripped_room_state(


Trying to clarify whether the all or nothing behavior of invite validation should also apply to knocks. MSC4311 currently says that for knocks, it should just remove individual events and continue

See matrix-org/matrix-spec-proposals#4311 (comment)

MadLittleMods · 2026-08-04T04:07:07Z

+        "Stripped" state means that only the `type`, `state_key`, `content` and `sender` keys
+        are included from each state event.


Not really relevant to this function which is only about getting the state event IDs.

Suggested change

"Stripped" state means that only the `type`, `state_key`, `content` and `sender` keys

are included from each state event.

MadLittleMods · 2026-08-04T04:13:43Z

-            # Ensure the event has been stripped
-            self.assertNotIn("signatures", event)


check_knock_room_state_against_room_state(...) is actually used in federation and client-side tests.

This could be still relevant for the client tests

…c/test_rooms_invites.SlidingSyncRoomsInvitesTestCase to use 'stripped state' to compare against responses instead of fleshing out a full PDU

Client side API's should still use stripped events with MSC4311

bdfeb90

MadLittleMods added A-Sync A-Spec-Compliance labels Apr 23, 2026

MadLittleMods mentioned this pull request Apr 23, 2026

Room version 12 tests matrix-org/complement#791

Merged

MadLittleMods changed the title ~~Client side API's like /sync should still use stripped events with MSC4311~~ Remove flawed MSC4311 partial implementation: Client side API's like /sync should still use stripped events Apr 23, 2026

MadLittleMods changed the title ~~Remove flawed MSC4311 partial implementation: Client side API's like /sync should still use stripped events~~ Remove flawed MSC4311 partial implementation: Client-side API's like /sync should still use stripped events Apr 23, 2026

Add changelog

43a11f5

MadLittleMods mentioned this pull request Apr 23, 2026

MSC4311: invites and knocks should contain the create event #19722

Merged

3 tasks

MadLittleMods added 4 commits May 1, 2026 18:23

Merge branch 'develop' into madlittlemods/remove-flawed-msc4311-parti…

7f25120

…al-implementation

Non-working: Use full PDU's for invite_room_state in federation

6026aaa

Iteration that uses current state

e0eb224

Use get_stripped_room_state_ids_from_event_context

3464ec8

Invite event should be stripped when included in invite_state

f96c008

MadLittleMods commented May 4, 2026

View reviewed changes

MadLittleMods added 3 commits May 4, 2026 17:54

Sanitize invite_room_state received over federation

a088aa8

Fix test lints

336b686

Updated comment

22f4f20

MadLittleMods commented May 4, 2026

View reviewed changes

MadLittleMods changed the title ~~Remove flawed MSC4311 partial implementation: Client-side API's like /sync should still use stripped events~~ MSC4311: Use full PDU's in stripped state (like invite_room_state) over federation and always include m.room.create event May 4, 2026

MadLittleMods added 5 commits May 21, 2026 18:07

Merge branch 'develop' into madlittlemods/remove-flawed-msc4311-parti…

cce5dc7

…al-implementation Conflicts: rust/src/room_versions.rs

Make the TODO more obvious

7e379e7

Placeholder arg docstring

92d0d8b

Better logic/reasoning

76b4905

Better comment flow

5103f1b

MadLittleMods commented May 22, 2026

View reviewed changes

erikjohnston reviewed Jul 22, 2026

View reviewed changes

MadLittleMods commented Jul 22, 2026

View reviewed changes

Comment thread synapse/storage/databases/main/events_worker.py Outdated

MadLittleMods added 5 commits July 22, 2026 14:14

Cross-link MSC4319

26bf739

Move parse/serialize as methods of StrippedStateEvent

2822f3c

See #19723 (comment)

Rename parse to from_json_dict

3ce6900

Rename serialize to as_json_dict

19d8a7a

Fix `builtins.TypeError: unsupported operand type(s) for |: 'str' and…

ecab104

… 'NoneType'`

MadLittleMods commented Jul 28, 2026

View reviewed changes

Comment thread changelog.d/19723.bugfix

MadLittleMods added 2 commits July 28, 2026 18:29

MSC4319 is now merged

eed3071

Merge branch 'develop' into madlittlemods/remove-flawed-msc4311-parti…

f751b5f

…al-implementation

MadLittleMods requested a review from erikjohnston July 28, 2026 23:31

erikjohnston reviewed Jul 29, 2026

View reviewed changes

MadLittleMods added 2 commits July 29, 2026 14:29

Merge branch 'develop' into madlittlemods/remove-flawed-msc4311-parti…

e06b311

…al-implementation

Start error off more plainly: `The other user's server did not unders…

e301cdd

…tand our invite.` See #19723 (comment)

MadLittleMods requested a review from erikjohnston July 29, 2026 20:11

MadLittleMods added 3 commits August 3, 2026 12:02

Merge branch 'develop' into madlittlemods/remove-flawed-msc4311-parti…

bc73bb0

…al-implementation

Merge branch 'develop' into madlittlemods/remove-flawed-msc4311-parti…

0baa67b

…al-implementation

Iterate on error

4f70688

See #19723 (comment)

MadLittleMods requested review from erikjohnston and removed request for erikjohnston August 3, 2026 21:00

MadLittleMods commented Aug 4, 2026

View reviewed changes

MadLittleMods mentioned this pull request Aug 11, 2026

MSC4528: M_INCOMPATIBLE_SERVER error code matrix-org/matrix-spec-proposals#4528

Open

half-duplex mentioned this pull request Aug 17, 2026

hope.net homeserver problems half-duplex/hopebot#20

Open


		return [strip_event(e) for e in state_to_include.values()]

		async def get_stripped_room_state_ids_from_event_context(

		# TODO(paul): assert that room_id/event_id parsed from path actually
		# match those given in content

		"Stripped" state means that only the `type`, `state_key`, `content` and `sender` keys
		are included from each state event.

		# Ensure the event has been stripped
		self.assertNotIn("signatures", event)

Uh oh!

Conversation

MadLittleMods commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

What does this PR do?

Dev notes

Todo

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

We're having trouble playing...

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MadLittleMods commented Apr 23, 2026 •

edited

Loading

MadLittleMods May 4, 2026 •

edited

Loading

MadLittleMods May 22, 2026 •

edited

Loading

MadLittleMods Jul 22, 2026 •

edited

Loading

MadLittleMods Jul 28, 2026 •

edited

Loading

MadLittleMods Aug 3, 2026 •

edited

Loading