Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Do not allow cross-room relations, per MSC2674. #11516

Merged
merged 7 commits into from
Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion synapse/events/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ async def _injected_bundled_aggregations(
return

event_id = event.event_id
room_id = event.room_id

# The bundled aggregations to include.
aggregations = {}
Expand All @@ -463,7 +464,7 @@ async def _injected_bundled_aggregations(
aggregations[RelationTypes.ANNOTATION] = annotations.to_dict()

references = await self.store.get_relations_for_event(
event_id, RelationTypes.REFERENCE, direction="f"
event_id, room_id, RelationTypes.REFERENCE, direction="f"
)
if references.chunk:
aggregations[RelationTypes.REFERENCE] = references.to_dict()
Expand Down
6 changes: 5 additions & 1 deletion synapse/rest/client/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ async def on_GET(

pagination_chunk = await self.store.get_relations_for_event(
event_id=parent_id,
room_id=room_id,
relation_type=relation_type,
event_type=event_type,
limit=limit,
Expand Down Expand Up @@ -383,7 +384,9 @@ async def on_GET(

# This checks that a) the event exists and b) the user is allowed to
# view it.
await self.event_handler.get_event(requester.user, room_id, parent_id)
event = await self.event_handler.get_event(requester.user, room_id, parent_id)
if event is None:
raise SynapseError(404, "Unknown parent event.")

if relation_type != RelationTypes.ANNOTATION:
raise SynapseError(400, "Relation type must be 'annotation'")
Expand All @@ -402,6 +405,7 @@ async def on_GET(

result = await self.store.get_relations_for_event(
event_id=parent_id,
room_id=room_id,
relation_type=relation_type,
event_type=event_type,
aggregation_key=key,
Expand Down
6 changes: 4 additions & 2 deletions synapse/storage/databases/main/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class RelationsWorkerStore(SQLBaseStore):
async def get_relations_for_event(
self,
event_id: str,
room_id: str,
relation_type: Optional[str] = None,
event_type: Optional[str] = None,
aggregation_key: Optional[str] = None,
Expand All @@ -49,6 +50,7 @@ async def get_relations_for_event(

Args:
event_id: Fetch events that relate to this event ID.
room_id: The room the event belongs to.
relation_type: Only fetch events with this relation type, if given.
event_type: Only fetch events with this event type, if given.
aggregation_key: Only fetch events with this aggregation key, if given.
Expand All @@ -63,8 +65,8 @@ async def get_relations_for_event(
the form `{"event_id": "..."}`.
"""

where_clause = ["relates_to_id = ?"]
where_args: List[Union[str, int]] = [event_id]
where_clause = ["relates_to_id = ?", "room_id = ?"]
where_args: List[Union[str, int]] = [event_id, room_id]

if relation_type is not None:
where_clause.append("relation_type = ?")
Expand Down
56 changes: 56 additions & 0 deletions tests/rest/client/test_relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
import itertools
import urllib.parse
from typing import Dict, List, Optional, Tuple
from unittest.mock import patch

from synapse.api.constants import EventTypes, RelationTypes
from synapse.rest import admin
from synapse.rest.client import login, register, relations, room, sync

from tests import unittest
from tests.server import FakeChannel
from tests.test_utils.event_injection import inject_event


class RelationsTestCase(unittest.HomeserverTestCase):
Expand Down Expand Up @@ -651,6 +653,60 @@ def test_aggregation_get_event_for_thread(self):
},
)

def test_ignore_invalid_room(self):
"""Test that we ignore invalid relations over federation."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why single out federation here, sorry? Because the CS API should reject this kind of event if a local client tries to send it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why single out federation here, sorry? Because the CS API should reject this kind of event if a local client tries to send it?

Yes, exactly. We're somewhat assuming our server isn't generating "bad" events, but they could have before C-S had validation, but it is more likely to be from a buggy / malicious server over federation.

# Create another room and send a message in it.
room2 = self.helper.create_room_as(self.user_id, tok=self.user_token)
res = self.helper.send(room2, body="Hi!", tok=self.user_token)
parent_id = res["event_id"]

# Disable the validation to pretend this came over federation.
with patch(
"synapse.handlers.message.EventCreationHandler._validate_event_relation"
):
self.get_success(
inject_event(
self.hs,
room_id=self.room,
type="m.reaction",
sender=self.user_id,
content={
"m.relates_to": {
"rel_type": RelationTypes.ANNOTATION,
"event_id": parent_id,
"key": "A",
}
},
)
)

# They should be ignored when fetching relations.
channel = self.make_request(
"GET",
f"/_matrix/client/unstable/rooms/{room2}/relations/{parent_id}",
access_token=self.user_token,
)
self.assertEquals(200, channel.code, channel.json_body)
self.assertEqual(channel.json_body["chunk"], [])

# And when fetching aggregations.
channel = self.make_request(
"GET",
f"/_matrix/client/unstable/rooms/{room2}/aggregations/{parent_id}",
access_token=self.user_token,
)
self.assertEquals(200, channel.code, channel.json_body)
self.assertEqual(channel.json_body["chunk"], [])

# And for bundled aggregations.
channel = self.make_request(
"GET",
f"/rooms/{room2}/event/{parent_id}",
access_token=self.user_token,
)
self.assertEquals(200, channel.code, channel.json_body)
self.assertNotIn("m.relations", channel.json_body["unsigned"])

def test_edit(self):
"""Test that a simple edit works."""

Expand Down