-
Notifications
You must be signed in to change notification settings - Fork 522
Unfreeze event dict after check_event_allowed module callbacks #19547
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
base: develop
Are you sure you want to change the base?
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1 @@ | ||||||
| Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to add your own handle here as well
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps better 🤷 :
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |||||
| from synapse.storage.roommember import ProfileInfo | ||||||
| from synapse.types import Requester, StateMap | ||||||
| from synapse.util.async_helpers import delay_cancellation, maybe_awaitable | ||||||
| from synapse.util.frozenutils import unfreeze | ||||||
|
|
||||||
| if TYPE_CHECKING: | ||||||
| from synapse.server import HomeServer | ||||||
|
|
@@ -288,6 +289,9 @@ async def check_event_allowed( | |||||
| # the hashes and signatures. | ||||||
| event.freeze() | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also update the |
||||||
|
|
||||||
| allow = True | ||||||
| event_dict = None | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| for callback in self._check_event_allowed_callbacks: | ||||||
| try: | ||||||
| res, replacement_data = await delay_cancellation( | ||||||
|
|
@@ -313,11 +317,18 @@ async def check_event_allowed( | |||||
| # Return if the event shouldn't be allowed or if the module came up with a | ||||||
| # replacement dict for the event. | ||||||
| if res is False: | ||||||
| return res, None | ||||||
| allow = False | ||||||
| break | ||||||
| elif isinstance(replacement_data, dict): | ||||||
| return True, replacement_data | ||||||
| event_dict = replacement_data | ||||||
| break | ||||||
|
|
||||||
| # Unfreeze the event dict to avoid potential issues with frozen dicts further | ||||||
| # down the code. Pydantic for example is not happy with frozen dicts. | ||||||
| # cf https://github.com/element-hq/synapse/issues/18117 | ||||||
| event._dict = unfreeze(event._dict) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initial implementation was adding an
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should create new The location in this file vs being
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even one step further, we could use a context manager pattern for this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably do this in a Would be good to have a test for this (module that throws an error) |
||||||
|
|
||||||
| return True, None | ||||||
| return allow, event_dict | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an aside, I wonder if we have to be careful about someone passing back a frozen Not something we necessarily have to concern ourselves with this PR but may be another problem. Perhaps we just need to tighten up the docstring details on what we expect passed back. And hopefully, this doesn't naturally happen from someone taking the |
||||||
|
|
||||||
| async def on_create_room( | ||||||
| self, requester: Requester, config: dict, is_requester_admin: bool | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # | ||
| # This file is licensed under the Affero General Public License (AGPL) version 3. | ||
| # | ||
| # Copyright (C) 2026 The Matrix.org Foundation C.I.C. | ||
| # | ||
| # This program is free software: you can redistribute it and/or modify | ||
| # it under the terms of the GNU Affero General Public License as | ||
| # published by the Free Software Foundation, either version 3 of the | ||
| # License, or (at your option) any later version. | ||
| # | ||
| # See the GNU Affero General Public License for more details: | ||
| # <https://www.gnu.org/licenses/agpl-3.0.html>. | ||
| # | ||
| # | ||
| from twisted.internet.testing import MemoryReactor | ||
|
|
||
| from synapse.module_api import EventBase, StateMap, UserID | ||
| from synapse.rest import admin, login, room, room_upgrade_rest_servlet | ||
| from synapse.server import HomeServer | ||
| from synapse.types import JsonDict | ||
| from synapse.util.clock import Clock | ||
|
|
||
| from tests.server import FakeChannel | ||
| from tests.unittest import HomeserverTestCase | ||
|
|
||
|
|
||
| class ThirdPartyEventRulesTestCase(HomeserverTestCase): | ||
| servlets = [ | ||
| room.register_servlets, | ||
| admin.register_servlets, | ||
| login.register_servlets, | ||
| room_upgrade_rest_servlet.register_servlets, | ||
| ] | ||
|
|
||
| def prepare( | ||
| self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer | ||
| ) -> None: | ||
| self._module_api = homeserver.get_module_api() | ||
| self.user_id = self.register_user("user", "password") | ||
| self.token = self.login("user", "password") | ||
|
|
||
| def create_room(self, content: JsonDict) -> FakeChannel: | ||
| channel = self.make_request( | ||
| "POST", | ||
| "/_matrix/client/r0/createRoom", | ||
| content, | ||
| access_token=self.token, | ||
| ) | ||
|
|
||
| return channel | ||
|
|
||
| def test_check_event_allowed_with_mentions(self) -> None: | ||
| """Test that the check_event_allowed callback works with a message containing mentions.""" | ||
|
|
||
| async def check_event_allowed( | ||
| event: EventBase, | ||
| state_events: StateMap, | ||
| ) -> tuple[bool, dict | None]: | ||
| return True, None | ||
|
|
||
| self._module_api.register_third_party_rules_callbacks( | ||
| check_event_allowed=check_event_allowed | ||
| ) | ||
|
|
||
| channel = self.create_room({}) | ||
|
|
||
| self.assertEqual(channel.code, 200) | ||
|
|
||
| room_id = channel.json_body["room_id"] | ||
|
|
||
| event_id = self.create_and_send_event( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Tested that this test fails when we comment out the |
||
| room_id, | ||
| UserID.from_string(self.user_id), | ||
| content={ | ||
| "body": "test", | ||
| "msgtype": "m.text", | ||
| "m.mentions": {}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should call out that this is the secret sauce that tests xxx and is important because xxx |
||
| }, | ||
| ) | ||
|
|
||
| self.assertNotEquals(event_id, None) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -870,6 +870,7 @@ def create_and_send_event( | |
| user: UserID, | ||
| soft_failed: bool = False, | ||
| prev_event_ids: list[str] | None = None, | ||
| content: dict[str, Any] | None = None, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the new usage, it looks like this was done in order to be able to define |
||
| ) -> str: | ||
| """ | ||
| Create and send an event. | ||
|
|
@@ -878,6 +879,7 @@ def create_and_send_event( | |
| soft_failed: Whether to create a soft failed event or not | ||
| prev_event_ids: Explicitly set the prev events, | ||
| or if None just use the default | ||
| content: Event content to send, or if None use a default | ||
|
|
||
| Returns: | ||
| The new event's ID. | ||
|
|
@@ -892,7 +894,8 @@ def create_and_send_event( | |
| "type": EventTypes.Message, | ||
| "room_id": room_id, | ||
| "sender": user.to_string(), | ||
| "content": {"body": secrets.token_hex(), "msgtype": "m.text"}, | ||
| "content": content | ||
| or {"body": secrets.token_hex(), "msgtype": "m.text"}, | ||
| }, | ||
| prev_event_ids=prev_event_ids, | ||
| ) | ||
|
|
||
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.
Currently, the PR description just says "Related to #18117" but I think this fixes the issue. Is that the case?
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.
Well it depends of how you look at it: we are fixing the problem for the module callback here, but we are not fixing the fact that a frozen event can be problematic in synapse code globally.
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.
hmmm, I see. I was jumping the gun since this is the only place we call
event.freeze().But we still have the
USE_FROZEN_DICTSstuff which appears to do the same thing. At-least in this case, it's a self-inflicted configuration option and happens all the time, not just when a Synapse module is configured and somecheck_event_allowedcallbacks are configured.It's ironic that this config options says that it "prevents bugs" but introduces a whole new category of potential bugs.