Allow 3rd party rules `check_event_allowed` callback to function for room version "12" creation events by jason-famedly · Pull Request #19768 · element-hq/synapse

jason-famedly · 2026-05-11T12:37:23Z

The change to room version "12" removes that a room ID is included in the room creation event, and is instead generated based on the event hash of said event. This seems to break the check_event_allowed() callback for this room version specifically for creation events due to an assert in the function that expects a room ID to be present.

Specifically ignore that assertion for creation events on rooms that support MSC4291(Room ID's as hashes) to fix this. This also needs to be checked for the correct assumptions about MSC4242 taken in it's second half(that creation events do not contain prev_state_events in the same way that they do not include prev_events)

Note this contains a commit that should be reverted before merging, that overrides the test so room version "12" is used

💡 This is the command line to run the specific test:
poetry run trial tests.rest.client.test_third_party_rules.ThirdPartyRulesTestCase.test_legacy_check_event_allowed

Failure looked like this before:

[ERROR]
Traceback (most recent call last):
  File "/home/jason/dev/famedly/synapse-upstreaming/tests/rest/client/test_third_party_rules.py", line 394, in test_legacy_check_event_allowed
    "/_matrix/client/r0/rooms/%s/send/m.room.message/1" % self.room_id,
builtins.AttributeError: 'ThirdPartyRulesTestCase' object has no attribute 'room_id'

tests.rest.client.test_third_party_rules.ThirdPartyRulesTestCase.test_legacy_check_event_allowed
===============================================================================

(There is another error in this TestCase when using room version "12", but it is unrelated to this and will be handled separately when fixing the tests themselves)

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)

…m V12 compliant

anoadragon453

Some minor changes below.

Thanks for taking on fixing this!

anoadragon453 · 2026-06-18T09:12:20Z

+            # The room_id should only be None for creation events using msc4291
+            # rooms(version "12" and newer)
+            if not (
+                builder.room_version.msc4291_room_ids_as_hashes
+                and builder.type == EventTypes.Create
+            ):
+                assert builder.room_id is not None


I don't think this if statement and assertion needs to live inside the try / except.

This assertion existed before, I was just trying to keep it and make it work. I do not mind losing it as long as mypy does not complain 😅 . Do you think the check should be kept but changed into a SynapseError instead so it can be reported back that the third party module might have made a naughty move?

Looking at the validate_builder() above, it actually does check inside that a room_id should not be missing on a non-creation event. 'm totally fine losing this. The lower ones below are for mypy more than anything else.

However, the below exception being raised is not caught. Checking the code paths that the parent function(create_new_client_event) calls, I see no places that catch a raw Exception(or any kind of exception at all, so it must propagate up to the rest response exception catchers).

How about adjusting this while we are here? Creating the Builder looks to just create a class, which shouldn't raise. The validate_builder() can raise directly a SynapseError. Perhaps that should just be allowed to percolate up, or at least update the e.msg as is done here(it wraps the message which on its own does not say it is from a third party module)? Otherwise we lose the associated error codes and everything 500's.

Unrelated thing discovered during this investigation
It actually appears that in `handlers.federation.on_make_knock_request()` it calls this `create_new_client_event()` which runs the third party `check_event_allowed()` prior to this rebuild process. It then calls the third party handler again. Perhaps instead of running the check twice it should just catch the `Exception`?

anoadragon453 · 2026-06-18T09:20:30Z

+        # Creation events using msc4291 rooms will not have a room_id, and will
+        # also not have prev_events nor prev_state_events.


I think this check is only relevant to MSC4242, rather than MSC4291? I get that MSC4242 technically builds on MSC4291 (and thus the create event won't have a room_id field). But I think we should be checking original_event.room_version.msc4242_state_dags here instead, no?

I can split the comment apart if that helps? original_event.room_version.msc4242_state_dags is checked a little further down in the next block. I think I was trying to cover 3 statements with one comment

I let this marinate for a while, and I think I don't follow which check you mean. I clarified the comment anyway and condensed it with the below, it was a little wordy

anoadragon453 · 2026-06-18T09:21:22Z

-        if original_event.room_version.msc4242_state_dags:
+        if (
+            original_event.room_version.msc4242_state_dags
+            and builder.room_id is not None


Seems to make more sense to swap out the builder.room_id check for builder.type == EventTypes.Create here. As only a create event won't have prev_state_events in a MSC4242-enabled room.

I can add that, if you like. But mypy is requiring the builder.room_id to not be optionally a string here.

synapse/handlers/message.py:2438: error: Argument 1 to "get_state_dag_extremities" of "EventFederationWorkerStore" has incompatible type "str | None"; expected "str" [arg-type]

er, well, I can add that builder.type != EventTypes.Create instead, as the inverse would make rather a mess 😅

jason-famedly · 2026-06-25T15:40:40Z

                "config": {},
-            }
+            },
+            "default_room_version": "12",


reminder: this needs to be removed before merging

jason-famedly added 3 commits May 8, 2026 11:25

fix _rebuild_event_after_third_party_rules() to be creation event/roo…

2d19bcd

…m V12 compliant

[revert] how to test this

2e4b2ee

changelog

d802d85

jason-famedly marked this pull request as ready for review May 11, 2026 13:34

jason-famedly requested a review from a team as a code owner May 11, 2026 13:34

MadLittleMods added the A-Modules label May 27, 2026

anoadragon453 requested changes Jun 18, 2026

View reviewed changes

jason-famedly commented Jun 23, 2026

View reviewed changes

Comment thread synapse/handlers/message.py Outdated

jason-famedly commented Jun 23, 2026

View reviewed changes

Comment thread synapse/handlers/message.py Outdated

apply some feedback and rearrange a few comments

cc29279

jason-famedly requested a review from anoadragon453 June 25, 2026 15:39

jason-famedly commented Jun 25, 2026

View reviewed changes

		# Creation events using msc4291 rooms will not have a room_id, and will
		# also not have prev_events nor prev_state_events.

Uh oh!

Conversation

jason-famedly commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

anoadragon453 left a comment

Choose a reason for hiding this comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jason-famedly commented May 11, 2026 •

edited

Loading