Ensure we use a copy of the event content dict before modifying it in serialize_event (#9585)

This bug was discovered by DINUM. We were modifying `serialized_event["content"]`, which - if you've got `USE_FROZEN_DICTS` turned on or are [using a third party rules module](17cd48fe51/synapse/events/third_party_rules.py (L73-L76)) - will raise a 500 if you try to a edit a reply to a message.

`serialized_event["content"]` could be set to the edit event's content, instead of a copy of it, which is bad as we attempt to modify it. Instead, we also end up modifying the original event's content. DINUM uses a third party rules module, which meant the event's content got frozen and thus an exception was raised.

To be clear, the problem is not that the event's content was frozen. In fact doing so helped us uncover the fact we weren't copying event content correctly.
This commit is contained in:
Andrew Morgan 2021-03-17 16:51:55 +00:00 committed by GitHub
parent cc324d53fe
commit 7b06f85c0e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 147 additions and 2 deletions

1
changelog.d/9585.bugfix Normal file
View File

@ -0,0 +1 @@
Fix a longstanding bug that could cause issues when editing a reply to a message.

View File

@ -22,6 +22,7 @@ from synapse.api.constants import EventTypes, RelationTypes
from synapse.api.errors import Codes, SynapseError
from synapse.api.room_versions import RoomVersion
from synapse.util.async_helpers import yieldable_gather_results
from synapse.util.frozenutils import unfreeze
from . import EventBase
@ -402,10 +403,19 @@ class EventClientSerializer:
# If there is an edit replace the content, preserving existing
# relations.
# Ensure we take copies of the edit content, otherwise we risk modifying
# the original event.
edit_content = edit.content.copy()
# Unfreeze the event content if necessary, so that we may modify it below
edit_content = unfreeze(edit_content)
serialized_event["content"] = edit_content.get("m.new_content", {})
# Check for existing relations
relations = event.content.get("m.relates_to")
serialized_event["content"] = edit.content.get("m.new_content", {})
if relations:
serialized_event["content"]["m.relates_to"] = relations
# Keep the relations, ensuring we use a dict copy of the original
serialized_event["content"]["m.relates_to"] = relations.copy()
else:
serialized_event["content"].pop("m.relates_to", None)

View File

@ -161,6 +161,68 @@ class ThirdPartyRulesTestCase(unittest.HomeserverTestCase):
ev = channel.json_body
self.assertEqual(ev["content"]["x"], "y")
def test_message_edit(self):
"""Ensure that the module doesn't cause issues with edited messages."""
# first patch the event checker so that it will modify the event
async def check(ev: EventBase, state):
d = ev.get_dict()
d["content"] = {
"msgtype": "m.text",
"body": d["content"]["body"].upper(),
}
return d
current_rules_module().check_event_allowed = check
# Send an event, then edit it.
channel = self.make_request(
"PUT",
"/_matrix/client/r0/rooms/%s/send/modifyme/1" % self.room_id,
{
"msgtype": "m.text",
"body": "Original body",
},
access_token=self.tok,
)
self.assertEqual(channel.result["code"], b"200", channel.result)
orig_event_id = channel.json_body["event_id"]
channel = self.make_request(
"PUT",
"/_matrix/client/r0/rooms/%s/send/m.room.message/2" % self.room_id,
{
"m.new_content": {"msgtype": "m.text", "body": "Edited body"},
"m.relates_to": {
"rel_type": "m.replace",
"event_id": orig_event_id,
},
"msgtype": "m.text",
"body": "Edited body",
},
access_token=self.tok,
)
self.assertEqual(channel.result["code"], b"200", channel.result)
edited_event_id = channel.json_body["event_id"]
# ... and check that they both got modified
channel = self.make_request(
"GET",
"/_matrix/client/r0/rooms/%s/event/%s" % (self.room_id, orig_event_id),
access_token=self.tok,
)
self.assertEqual(channel.result["code"], b"200", channel.result)
ev = channel.json_body
self.assertEqual(ev["content"]["body"], "ORIGINAL BODY")
channel = self.make_request(
"GET",
"/_matrix/client/r0/rooms/%s/event/%s" % (self.room_id, edited_event_id),
access_token=self.tok,
)
self.assertEqual(channel.result["code"], b"200", channel.result)
ev = channel.json_body
self.assertEqual(ev["content"]["body"], "EDITED BODY")
def test_send_event(self):
"""Tests that the module can send an event into a room via the module api"""
content = {

View File

@ -39,6 +39,11 @@ class RelationsTestCase(unittest.HomeserverTestCase):
# We need to enable msc1849 support for aggregations
config = self.default_config()
config["experimental_msc1849_support_enabled"] = True
# We enable frozen dicts as relations/edits change event contents, so we
# want to test that we don't modify the events in the caches.
config["use_frozen_dicts"] = True
return self.setup_test_homeserver(config=config)
def prepare(self, reactor, clock, hs):
@ -518,6 +523,63 @@ class RelationsTestCase(unittest.HomeserverTestCase):
{"event_id": edit_event_id, "sender": self.user_id}, m_replace_dict
)
def test_edit_reply(self):
"""Test that editing a reply works."""
# Create a reply to edit.
channel = self._send_relation(
RelationTypes.REFERENCE,
"m.room.message",
content={"msgtype": "m.text", "body": "A reply!"},
)
self.assertEquals(200, channel.code, channel.json_body)
reply = channel.json_body["event_id"]
new_body = {"msgtype": "m.text", "body": "I've been edited!"}
channel = self._send_relation(
RelationTypes.REPLACE,
"m.room.message",
content={"msgtype": "m.text", "body": "foo", "m.new_content": new_body},
parent_id=reply,
)
self.assertEquals(200, channel.code, channel.json_body)
edit_event_id = channel.json_body["event_id"]
channel = self.make_request(
"GET",
"/rooms/%s/event/%s" % (self.room, reply),
access_token=self.user_token,
)
self.assertEquals(200, channel.code, channel.json_body)
# We expect to see the new body in the dict, as well as the reference
# metadata sill intact.
self.assertDictContainsSubset(new_body, channel.json_body["content"])
self.assertDictContainsSubset(
{
"m.relates_to": {
"event_id": self.parent_id,
"key": None,
"rel_type": "m.reference",
}
},
channel.json_body["content"],
)
# We expect that the edit relation appears in the unsigned relations
# section.
relations_dict = channel.json_body["unsigned"].get("m.relations")
self.assertIn(RelationTypes.REPLACE, relations_dict)
m_replace_dict = relations_dict[RelationTypes.REPLACE]
for key in ["event_id", "sender", "origin_server_ts"]:
self.assertIn(key, m_replace_dict)
self.assert_dict(
{"event_id": edit_event_id, "sender": self.user_id}, m_replace_dict
)
def test_relations_redaction_redacts_edits(self):
"""Test that edits of an event are redacted when the original event
is redacted.

View File

@ -32,6 +32,7 @@ from twisted.python.threadpool import ThreadPool
from twisted.trial import unittest
from twisted.web.resource import Resource
from synapse import events
from synapse.api.constants import EventTypes, Membership
from synapse.config.homeserver import HomeServerConfig
from synapse.config.ratelimiting import FederationRateLimitConfig
@ -229,6 +230,11 @@ class HomeserverTestCase(TestCase):
self._hs_args = {"clock": self.clock, "reactor": self.reactor}
self.hs = self.make_homeserver(self.reactor, self.clock)
# Honour the `use_frozen_dicts` config option. We have to do this
# manually because this is taken care of in the app `start` code, which
# we don't run. Plus we want to reset it on tearDown.
events.USE_FROZEN_DICTS = self.hs.config.use_frozen_dicts
if self.hs is None:
raise Exception("No homeserver returned from make_homeserver.")
@ -292,6 +298,10 @@ class HomeserverTestCase(TestCase):
if hasattr(self, "prepare"):
self.prepare(self.reactor, self.clock, self.hs)
def tearDown(self):
# Reset to not use frozen dicts.
events.USE_FROZEN_DICTS = False
def wait_on_thread(self, deferred, timeout=10):
"""
Wait until a Deferred is done, where it's waiting on a real thread.