diff --git a/changelog.d/19723.bugfix b/changelog.d/19723.bugfix new file mode 100644 index 00000000000..ed635a9f4f8 --- /dev/null +++ b/changelog.d/19723.bugfix @@ -0,0 +1 @@ +Remove flawed [MSC4311](https://github.com/matrix-org/matrix-spec-proposals/pull/4311) partial implementation: Client-side API's like `/sync` should still use stripped events. diff --git a/rust/src/room_versions.rs b/rust/src/room_versions.rs index 2860bfdbd73..94252ecca99 100644 --- a/rust/src/room_versions.rs +++ b/rust/src/room_versions.rs @@ -161,6 +161,23 @@ pub struct RoomVersion { /// This is similar to how doubly-linked lists can potentially not refer to previous items correctly /// without verifying the list's integrity, but doing it on every insert is too expensive. pub msc4242_state_dags: bool, + /// Determines whether a room version *SHOULD* rather than *MAY* reject invites/knocks + /// with invalid stripped state events. + /// + /// According to MSC4311: + /// > If any of the [stripped state] events are not a PDU, not for the room ID specified, or fail + /// > signature checks, or the `m.room.create` event is missing, the receiving + /// > server MAY respond to invites with a `400 M_MISSING_PARAM` standard Matrix + /// > error (new to the endpoint). For invites to room version 12+ rooms, servers + /// > SHOULD rather than MAY respond to such requests with `400 M_MISSING_PARAM`. + /// + /// Regardless of room version (we should always do these things): + /// 1. The `m.room.create` event *MUST* be included in + /// `invite_room_state`/`knock_room_state` when sending invites/knocks over the + /// federation API's. + /// 2. Use full PDU's in the `invite_room_state`/`knock_room_state` in the federation + /// API. The client API still uses stripped state. + msc4311_stripped_state: bool, } impl RoomVersion { @@ -187,6 +204,7 @@ impl RoomVersion { msc4291_room_ids_as_hashes: false, strict_event_byte_limits_room_versions: false, msc4242_state_dags: false, + msc4311_stripped_state: false, }; pub const V2: RoomVersion = RoomVersion { @@ -304,6 +322,7 @@ impl RoomVersion { state_res: StateResolutionVersions::V2_1, msc4289_creator_power_enabled: true, msc4291_room_ids_as_hashes: true, + msc4311_stripped_state: true, ..Self::V11 }; diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index b01e786550a..cab2fe1a46b 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -199,6 +199,46 @@ class StrippedStateEvent: sender: str content: dict[str, Any] + def as_json_dict(self) -> JsonDict: + """ + Serialize to the JSON representation of 'Stripped State' according to the Matrix + spec + """ + + return { + "type": self.type, + "state_key": self.state_key, + "sender": self.sender, + "content": self.content, + } + + @staticmethod + def from_json_dict(raw_stripped_event: JsonDict) -> "StrippedStateEvent | None": + """ + Given a raw value from an event's `unsigned` field, attempt to parse it into a + `StrippedStateEvent`. + """ + if isinstance(raw_stripped_event, dict): + # All of these fields are required + type = raw_stripped_event.get("type") + state_key = raw_stripped_event.get("state_key") + sender = raw_stripped_event.get("sender") + content = raw_stripped_event.get("content") + if ( + isinstance(type, str) + and isinstance(state_key, str) + and isinstance(sender, str) + and isinstance(content, dict) + ): + return StrippedStateEvent( + type=type, + state_key=state_key, + sender=sender, + content=content, + ) + + return None + @attr.s(slots=True, frozen=True, auto_attribs=True) class EventMetadata: diff --git a/synapse/events/utils.py b/synapse/events/utils.py index 55891f9b15c..c3bc6fa9da1 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -37,7 +37,6 @@ CANONICALJSON_MAX_INT, CANONICALJSON_MIN_INT, MAX_PDU_SIZE, - EventTypes, ) from synapse.api.errors import Codes, SynapseError from synapse.logging.opentracing import SynapseTags, set_tag, trace @@ -55,7 +54,7 @@ from synapse.synapse_rust.types import Requester from synapse.types import JsonDict -from . import EventBase, StrippedStateEvent +from . import EventBase # These are imported only to re-export them (callers import them from this # module); listing them in __all__ stops the unused-import lint flagging them @@ -549,45 +548,9 @@ def strip_event(event: EventBase) -> JsonDict: Stripped state events can only have the `sender`, `type`, `state_key` and `content` properties present. """ - # MSC4311: Ensure the create event is available on invites and knocks. - # TODO: Implement the rest of MSC4311 - if ( - event.room_version.msc4291_room_ids_as_hashes - and event.type == EventTypes.Create - and event.get_state_key() == "" - ): - return event.get_pdu_json() - return { "type": event.type, "state_key": event.state_key, "content": dict(event.content), "sender": event.sender, } - - -def parse_stripped_state_event(raw_stripped_event: Any) -> StrippedStateEvent | None: - """ - Given a raw value from an event's `unsigned` field, attempt to parse it into a - `StrippedStateEvent`. - """ - if isinstance(raw_stripped_event, dict): - # All of these fields are required - type = raw_stripped_event.get("type") - state_key = raw_stripped_event.get("state_key") - sender = raw_stripped_event.get("sender") - content = raw_stripped_event.get("content") - if ( - isinstance(type, str) - and isinstance(state_key, str) - and isinstance(sender, str) - and isinstance(content, dict) - ): - return StrippedStateEvent( - type=type, - state_key=state_key, - sender=sender, - content=content, - ) - - return None diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 5b334543256..de0ae6cb240 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -23,6 +23,7 @@ import itertools import logging +from http import HTTPStatus from typing import ( TYPE_CHECKING, AbstractSet, @@ -59,6 +60,7 @@ RoomVersions, ) from synapse.events import EventBase, builder, make_event_from_dict +from synapse.events.snapshot import EventContext from synapse.federation.federation_base import ( FederationBase, InvalidEventSignatureError, @@ -70,7 +72,12 @@ from synapse.http.types import QueryParams from synapse.logging.opentracing import SynapseTags, log_kv, set_tag, tag_args, trace from synapse.metrics import SERVER_NAME_LABEL -from synapse.types import JsonDict, StrCollection, UserID, get_domain_from_id +from synapse.types import ( + JsonDict, + StrCollection, + UserID, + get_domain_from_id, +) from synapse.util.async_helpers import concurrently_execute from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.duration import Duration @@ -1303,12 +1310,12 @@ async def send_invite( self, destination: str, room_id: str, - event_id: str, pdu: EventBase, + context: EventContext, ) -> EventBase: room_version = await self.store.get_room_version(room_id) - content = await self._do_send_invite(destination, pdu, room_version) + content = await self._do_send_invite(destination, pdu, context, room_version) pdu_dict = content["event"] @@ -1329,11 +1336,21 @@ async def send_invite( return pdu async def _do_send_invite( - self, destination: str, pdu: EventBase, room_version: RoomVersion + self, + destination: str, + pdu: EventBase, + context: EventContext, + room_version: RoomVersion, ) -> JsonDict: """Actually sends the invite, first trying v2 API and falling back to v1 API if necessary. + Args: + destination: + pdu: Invite event + context: + room_version: + Returns: The event as a dict as returned by the remote server @@ -1344,6 +1361,19 @@ async def _do_send_invite( """ time_now = self._clock.time_msec() + # MSC4311: For the federation API, format events in `invite_room_state` as full + # PDU's + # + # Find the full events based on the state at the time of the invite + state_ids = await self.store.get_stripped_room_state_ids_from_event_context( + pdu, + context, + ) + 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." + ) + try: return await self.transport_layer.send_invite_v2( destination=destination, @@ -1352,7 +1382,11 @@ async def _do_send_invite( content={ "event": pdu.get_pdu_json(time_now), "room_version": room_version.identifier, - "invite_room_state": pdu.unsigned.get("invite_room_state", []), + "invite_room_state": [ + # Use full PDU's according to MSC4311 + state_event.get_pdu_json(time_now) + for state_event in state_events.values() + ], }, ) except HttpResponseException as e: @@ -1367,18 +1401,74 @@ async def _do_send_invite( "User's homeserver does not support this room version", Codes.UNSUPPORTED_ROOM_VERSION, ) + # MSC4311: The 400 `M_MISSING_PARAM` error SHOULD be translated to a 5xx + # error by the sending server over the Client-Server API. This is done + # because there's nothing the client can materially do differently to make + # the request succeed. + elif ( + err.code == HTTPStatus.BAD_REQUEST + and err.errcode == Codes.MISSING_PARAM + ): + raise SynapseError( + 500, + 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).", + Codes.UNKNOWN, + additional_fields={ + "cause": err.msg, + "destination_server": destination, + }, + ) else: raise err # Didn't work, try v1 API. # Note the v1 API returns a tuple of `(200, content)` - _, content = await self.transport_layer.send_invite_v1( - destination=destination, - room_id=pdu.room_id, - event_id=pdu.event_id, - content=pdu.get_pdu_json(time_now), - ) + try: + # Use full PDU's for `invite_room_state` according to MSC4311 + # + # With the v1 invite API, `invite_room_state` is carried inside the event + # instead of a separate field like in v2 so we must munge it in ourselves + event_json = pdu.get_pdu_json(time_now) + event_json.setdefault("unsigned", {})["invite_room_state"] = [ + # Use full PDU's according to MSC4311 + state_event.get_pdu_json(time_now) + for state_event in state_events.values() + ] + + _, content = await self.transport_layer.send_invite_v1( + destination=destination, + room_id=pdu.room_id, + event_id=pdu.event_id, + content=event_json, + ) + except HttpResponseException as e: + # MSC4311: The 400 `M_MISSING_PARAM` error SHOULD be translated to a 5xx + # error by the sending server over the Client-Server API. This is done + # because there's nothing the client can materially do differently to make + # the request succeed. + err = e.to_synapse_error() + if ( + err.code == HTTPStatus.BAD_REQUEST + and err.errcode == Codes.MISSING_PARAM + ): + raise SynapseError( + 500, + 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).", + Codes.UNKNOWN, + additional_fields={ + "cause": err.msg, + "destination_server": destination, + }, + ) + else: + raise err return content async def send_leave(self, destinations: Iterable[str], pdu: EventBase) -> None: diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 2379b2ed2ce..7062ea91031 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -196,8 +196,6 @@ def __init__(self, hs: "HomeServer"): hs.config.federation.federation_metrics_domains ) - self._room_prejoin_state_types = hs.config.api.room_prejoin_state - # Whether we have started handling old events in the staging area. self._started_handling_of_staged_events = False @@ -815,8 +813,27 @@ async def on_make_join_request( return {"event": pdu.get_templated_pdu_json(), "room_version": room_version} async def on_invite_request( - self, origin: str, content: JsonDict, room_version_id: str + self, + *, + origin: str, + expected_room_id: str, + expected_event_id: str, + event_json: JsonDict, + room_version_id: str, ) -> dict[str, Any]: + """ + Args: + origin: + expected_room_id: The room ID specified in the + `/_matrix/federation/v1/invite/{roomId}/{eventId}` request that we expect to + match in the actual event itself. + expected_event_id: The event ID specified in the + `/_matrix/federation/v1/invite/{roomId}/{eventId}` request that we expect to + match in the actual event itself. + event_json: + room_version_id: + """ + room_version = KNOWN_ROOM_VERSIONS.get(room_version_id) if not room_version: raise SynapseError( @@ -825,9 +842,21 @@ async def on_invite_request( Codes.UNSUPPORTED_ROOM_VERSION, ) - pdu = event_from_pdu_json(content, room_version) + pdu = event_from_pdu_json(event_json, room_version) origin_host, _ = parse_server_name(origin) await self.check_server_matches_acl(origin_host, pdu.room_id) + if pdu.event_id != expected_event_id: + raise SynapseError( + 400, + "Invite event ID must match event ID specified in the federation `/invite` request", + Codes.INVALID_PARAM, + ) + if pdu.room_id != expected_room_id: + raise SynapseError( + 400, + "The room_id specified in the invite event must match room ID specified in the federation `/invite` request", + Codes.INVALID_PARAM, + ) if await self._spam_checker_module_callbacks.should_drop_federated_event(pdu): logger.info( "Federated event contains spam, dropping %s", @@ -840,7 +869,11 @@ async def on_invite_request( errmsg = f"event id {pdu.event_id}: {e}" logger.warning("%s", errmsg) raise SynapseError(403, errmsg, Codes.FORBIDDEN) - ret_pdu = await self.handler.on_invite_request(origin, pdu, room_version) + ret_pdu = await self.handler.on_invite_request( + origin=origin, + event=pdu, + room_version=room_version, + ) time_now = self._clock.time_msec() return {"event": ret_pdu.get_pdu_json(time_now)} @@ -1039,19 +1072,31 @@ async def on_send_knock_request( Returns: The stripped room state. """ - _, context = await self._on_send_membership_event( + time_now = self._clock.time_msec() + + event, context = await self._on_send_membership_event( origin, content, Membership.KNOCK, room_id ) - # Retrieve stripped state events from the room and send them back to the remote - # server. This will allow the remote server's clients to display information - # related to the room while the knock request is pending. - stripped_room_state = ( - await self.store.get_stripped_room_state_from_event_context( - context, self._room_prejoin_state_types - ) + # MSC4311: For the federation API, format events in `knock_room_state` as full + # PDU's + # + # Find the full events based on the state at the time of the knock + state_ids = await self.store.get_stripped_room_state_ids_from_event_context( + event, context + ) + 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." ) - return {"knock_room_state": stripped_room_state} + + return { + "knock_room_state": [ + # Use full PDU's according to MSC4311 + state_event.get_pdu_json(time_now) + for state_event in state_events.values() + ] + } async def _on_send_membership_event( self, origin: str, content: JsonDict, membership_type: str, room_id: str diff --git a/synapse/federation/transport/server/federation.py b/synapse/federation/transport/server/federation.py index 8a8914bf4f7..d524dc12662 100644 --- a/synapse/federation/transport/server/federation.py +++ b/synapse/federation/transport/server/federation.py @@ -491,7 +491,11 @@ async def on_PUT( # state resolution algorithm, and we don't use that for processing # invites result = await self.handler.on_invite_request( - origin, content, room_version_id=RoomVersions.V1.identifier + origin=origin, + expected_room_id=room_id, + expected_event_id=event_id, + event_json=content, + room_version_id=RoomVersions.V1.identifier, ) # V1 federation API is defined to return a content of `[200, {...}]` @@ -513,9 +517,6 @@ async def on_PUT( room_id: str, event_id: str, ) -> tuple[int, JsonDict]: - # TODO(paul): assert that room_id/event_id parsed from path actually - # match those given in content - room_version = content["room_version"] event = content["event"] invite_room_state = content.get("invite_room_state", []) @@ -524,12 +525,15 @@ async def on_PUT( invite_room_state = [] # Synapse expects invite_room_state to be in unsigned, as it is in v1 - # API - + # API. We will sanitize this inside `on_invite_request(...)` event.setdefault("unsigned", {})["invite_room_state"] = invite_room_state result = await self.handler.on_invite_request( - origin, event, room_version_id=room_version + origin=origin, + expected_room_id=room_id, + expected_event_id=event_id, + event_json=event, + room_version_id=room_version, ) # We only store invite_room_state for internal use, so remove it before diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 4816c79c7b7..b30f2ad9234 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -30,6 +30,7 @@ from typing import ( TYPE_CHECKING, AbstractSet, + Any, Iterable, ) @@ -57,9 +58,13 @@ from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion from synapse.crypto.event_signing import compute_event_signature from synapse.event_auth import validate_event_for_room_version -from synapse.events import EventBase +from synapse.events import EventBase, StrippedStateEvent from synapse.events.snapshot import EventContext, UnpersistedEventContextBase from synapse.events.validator import EventValidator +from synapse.federation.federation_base import ( + InvalidEventSignatureError, + event_from_pdu_json, +) from synapse.federation.federation_client import InvalidResponseError from synapse.handlers.pagination import PURGE_PAGINATION_LOCK_NAME from synapse.http.servlet import assert_params_in_dict @@ -565,7 +570,9 @@ async def try_backfill(domains: StrCollection) -> bool: return False - async def send_invite(self, target_host: str, event: EventBase) -> EventBase: + async def send_invite( + self, target_host: str, event: EventBase, context: EventContext + ) -> EventBase: """Sends the invite to the remote server for signing. Invites must be signed by the invitee's server before distribution. @@ -574,8 +581,8 @@ async def send_invite(self, target_host: str, event: EventBase) -> EventBase: pdu = await self.federation_client.send_invite( destination=target_host, room_id=event.room_id, - event_id=event.event_id, pdu=event, + context=context, ) except RequestSendFailed: raise SynapseError(502, f"Can't connect to server {target_host}") @@ -937,15 +944,56 @@ async def do_knock( # This is a bit of a hack and is cribbing off of invites. Basically we # store the room state here and retrieve it again when this event appears # in the invitee's sync stream. It is stripped out for all other local users. - stripped_room_state = knock_response.get("knock_room_state") - - if stripped_room_state is None: - raise KeyError("Missing 'knock_room_state' field in send_knock response") - - if not isinstance(stripped_room_state, list): - raise TypeError("'knock_room_state' has wrong type") + # + # Parse/validate `knock_room_state` + try: + stripped_room_state = await self._parse_stripped_room_state( + stripped_room_state=knock_response.get("knock_room_state"), + room_id=event.room_id, + room_version=event_format_version, + ) + # Replace with our sanitized `knock_room_state` + event.unsigned["knock_room_state"] = [ + stripped_state_event.as_json_dict() + for stripped_state_event in stripped_room_state + ] + except Exception as exc: + # FIXME(MSC4311): Apply this validation for all room versions after + # 2027-06-01 (to allow some time for the ecosystem to adapt and support + # MSC4311), see https://github.com/element-hq/synapse/issues/19943 + # + # The Matrix spec says that for "version 12+ rooms, servers SHOULD rather than + # MAY respond to such requests with 400 M_MISSING_PARAM". Given we have the + # lee-way to enforce this in all room versions, we might as well. + # + # For now, we'll only log in room versions 12+ where this SHOULD be the case + # already. + if event_format_version.msc4311_stripped_state: + # FIXME(MSC4311): Instead of logging, reject with 400 `M_MISSING_PARAM` + # after 2027-06-01. Given Synapse claimed to support room version 12 but + # didn't adhere to this behavior until 2026-07, we will only warn for + # now. Don't forget to unskip the + # `TestMSC4311RejectInvalidStrippedStateFederation` Complement tests as + # well. + logger.warning( + "Continuing anyway but failed to validate `knock_room_state` on knock %s (room_version=%s): %s", + event, + event_format_version, + exc, + ) - event.unsigned["knock_room_state"] = stripped_room_state + # FIXME(MSC4311): Remove this whole block after we always enforce the + # validation above. The only reason this is here is because the validation + # can fail for non-compliant servers but we should still use stripped state. + stripped_room_state_for_client = self._minimal_parse_stripped_room_state( + stripped_room_state=knock_response.get("knock_room_state"), + ) + if stripped_room_state_for_client is not None: + # Replace with our sanitized `knock_room_state` + event.unsigned["knock_room_state"] = [ + stripped_state_event.as_json_dict() + for stripped_state_event in stripped_room_state_for_client + ] context = EventContext.for_outlier(self._storage_controllers) stream_id = await self._federation_event_handler.persist_events_and_notify( @@ -1106,8 +1154,136 @@ async def on_make_join_request( await self._event_auth_handler.check_auth_rules_from_context(event) return event + def _minimal_parse_stripped_room_state( + self, + *, + stripped_room_state: Any, + ) -> list[StrippedStateEvent] | None: + """ + The goal of this function is to sanitize whatever we got from federation and + make it presentable to the client. The minimum amount of parsing necessary to + ensure `invite_room_state`/`knock_room_state` is at-least a list of stripped + state events (compared to `_parse_stripped_room_state`). + """ + + # Scrutinize JSON values + # + # In previous versions of the Matrix spec, + # `invite_room_state`/`knock_room_state` was an optional list of stripped state + # events which means we can't strictly enforce that this is always present. + if stripped_room_state is None: + return None + # We're going to strictly enforce that they at-least gave us a list. + elif not isinstance(stripped_room_state, list): + raise TypeError("Stripped state must be a list of PDU's") + + parsed_stripped_room_state = [] + for raw_stripped_event in stripped_room_state: + # Parse each stripped event + parsed_stripped_event = StrippedStateEvent.from_json_dict( + raw_stripped_event + ) + if parsed_stripped_event is None: + # Drop any invalid stripped state events as this is spec'ed and we + # might as well save the client from dealing with anything crazy. + continue + parsed_stripped_room_state.append(parsed_stripped_event) + + return parsed_stripped_room_state + + async def _parse_stripped_room_state( + self, + *, + stripped_room_state: Any, + room_id: str, + room_version: RoomVersion, + ) -> list[StrippedStateEvent]: + """ + Parse and validate `invite_room_state`/`knock_room_state` according to the + Matrix spec (c.f. MSC4311). + + > If any of the events are not a PDU, not for the room ID specified, or fail + > signature checks, or the `m.room.create` event is missing, the receiving + > server MAY respond to invites with a `400 M_MISSING_PARAM` standard Matrix + > error (new to the endpoint). For invites to room version 12+ rooms, servers + > SHOULD rather than MAY respond to such requests with `400 M_MISSING_PARAM`. + + We refer to `invite_room_state`/`knock_room_state` as `stripped_room_state` but + the events contained within can be full PDU's or stripped state events (older + version of the Matrix spec). + + Args: + stripped_room_state: The raw `invite_room_state`/`knock_room_state` JSON + room_id: The room ID the invite/knock is happening in + room_version: The version of the room the invite/knock is happening in + + Returns: + A list of parsed `StrippedStateEvent` + + Raises: + `TypeError`/`ValueError` when the stripped room state is invalid + """ + # Scrutinize JSON values + if not isinstance(stripped_room_state, list): + raise TypeError( + "Stripped state must be a list of PDU's that includes the `m.room.create` event" + ) + + parsed_stripped_room_state = [] + includes_create_event = False + for raw_stripped_event in stripped_room_state: + # Validate PDU + try: + pdu = event_from_pdu_json(raw_stripped_event, room_version) + except Exception as exc: + raise ValueError( + "Unable to parse one of the stripped state events as a PDU" + ) from exc + + # Validate that it's from the same room + if pdu.room_id != room_id: + raise ValueError( + "PDU from stripped state must be from the room ID specified in the request" + ) + # Validate signature/hashes + try: + pdu = await self.federation_client._check_sigs_and_hash( + room_version, pdu + ) + except InvalidEventSignatureError as exc: + raise ValueError( + "PDU from stripped state must pass signature/hash checks" + ) from exc + + # Mark down whether we saw the create event which we will validate just below + # + # We do this after the above checks to make sure it's a valid event + # from this room. + if pdu.type == EventTypes.Create: + includes_create_event = True + + # Parse the stripped events to ensure it has all of the fields necessary + parsed_stripped_event = StrippedStateEvent.from_json_dict( + raw_stripped_event + ) + if parsed_stripped_event is None: + raise ValueError("Unable to parse as stripped event") + parsed_stripped_room_state.append(parsed_stripped_event) + + # Validate `m.room.create` event is included + if not includes_create_event: + raise ValueError( + "Stripped state must include `m.room.create` event (MSC4311)" + ) + + return parsed_stripped_room_state + async def on_invite_request( - self, origin: str, event: EventBase, room_version: RoomVersion + self, + *, + origin: str, + event: EventBase, + room_version: RoomVersion, ) -> EventBase: """We've got an invite event. Process and persist it. Sign it. @@ -1180,6 +1356,54 @@ async def on_invite_request( room_id=event.room_id, room_version=room_version ) + # Parse/validate `invite_room_state` + try: + stripped_room_state = await self._parse_stripped_room_state( + stripped_room_state=event.unsigned.get("invite_room_state"), + room_id=event.room_id, + room_version=room_version, + ) + # Replace with our sanitized `invite_room_state` + event.unsigned["invite_room_state"] = [ + stripped_state_event.as_json_dict() + for stripped_state_event in stripped_room_state + ] + except Exception as exc: + # FIXME(MSC4311): Apply this validation for all room versions after + # 2027-06-01 (to allow some time for the ecosystem to adapt and support + # MSC4311), see https://github.com/element-hq/synapse/issues/19943 + # + # The Matrix spec says that for "version 12+ rooms, servers SHOULD rather than + # MAY respond to such requests with 400 M_MISSING_PARAM". Given we have the + # lee-way to enforce this in all room versions, we might as well. + # + # For now, we'll only log in room versions 12+ where this SHOULD be the case + # already. + if room_version.msc4311_stripped_state: + # FIXME(MSC4311): Instead of logging, reject with 400 `M_MISSING_PARAM` + # after 2027-06-01. Given Synapse claimed to support room version 12 but + # didn't adhere to this behavior until 2026-07, we will only warn for + # now. + logger.warning( + "Continuing anyway but failed to validate `invite_room_state` on invite %s (room_version=%s): %s", + event, + room_version, + exc, + ) + + # FIXME(MSC4311): Remove this whole block after we always enforce the + # validation above. The only reason this is here is because the validation + # can fail for non-compliant servers but we should still use stripped state. + stripped_room_state_for_client = self._minimal_parse_stripped_room_state( + stripped_room_state=event.unsigned.get("invite_room_state"), + ) + if stripped_room_state_for_client is not None: + # Replace with our sanitized `invite_room_state` + event.unsigned["invite_room_state"] = [ + stripped_state_event.as_json_dict() + for stripped_state_event in stripped_room_state_for_client + ] + event.internal_metadata.outlier = True event.internal_metadata.out_of_band_membership = True diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index b34ee9d50f7..e631b738d97 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -509,8 +509,6 @@ def __init__(self, hs: "HomeServer"): self._worker_lock_handler = hs.get_worker_locks_handler() self._policy_handler = hs.get_room_policy_handler() - self.room_prejoin_state_types = self.hs.config.api.room_prejoin_state - self.send_events = ReplicationSendEventsRestServlet.make_client(hs) self.request_ratelimiter = hs.get_request_ratelimiter() @@ -2078,9 +2076,8 @@ async def persist_and_notify_client_events( event.unsigned, "invite_room_state", await self.store.get_stripped_room_state_from_event_context( + event, context, - self.room_prejoin_state_types, - membership_user_id=event.sender, ), ) @@ -2091,7 +2088,7 @@ async def persist_and_notify_client_events( # to get them to sign the event. returned_invite = await federation_handler.send_invite( - invitee.domain, event + invitee.domain, event, context ) # TODO: Make sure the signatures actually are correct. @@ -2103,8 +2100,8 @@ async def persist_and_notify_client_events( event.unsigned, "knock_room_state", await self.store.get_stripped_room_state_from_event_context( + event, context, - self.room_prejoin_state_types, ), ) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 836cee6c20f..f4d1f484e78 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -37,7 +37,6 @@ from synapse.api.errors import SlidingSyncUnknownPosition from synapse.api.room_versions import KNOWN_ROOM_VERSIONS from synapse.events import StrippedStateEvent -from synapse.events.utils import parse_stripped_state_event from synapse.logging.opentracing import start_active_span, trace from synapse.storage.databases.main.sliding_sync import UPDATE_INTERVAL_LAST_USED_TS from synapse.storage.databases.main.state import ( @@ -1618,7 +1617,7 @@ async def _bulk_get_stripped_state_for_rooms_from_sync_room_map( stripped_state_map = {} if isinstance(raw_stripped_state_events, list): for raw_stripped_event in raw_stripped_state_events: - stripped_state_event = parse_stripped_state_event( + stripped_state_event = StrippedStateEvent.from_json_dict( raw_stripped_event ) if stripped_state_event is not None: diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 962317dedbe..9b8b4dd98dc 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -33,6 +33,7 @@ EventFormat, FilteredEvent, SerializeEventConfig, + strip_event, ) from synapse.handlers.presence import format_user_presence_state from synapse.handlers.sliding_sync import SlidingSyncConfig, SlidingSyncResult @@ -459,7 +460,8 @@ async def encode_invited( invited_state = [] invited_state = list(invited_state) - invited_state.append(invite) + # MSC4319: Add the invite itself + invited_state.append(strip_event(room.invite)) invited[room.room_id] = {"invite_state": {"events": invited_state}} return invited @@ -505,12 +507,11 @@ async def encode_knocked( knocked_state = [] knocked_state = list(knocked_state) - # Append the actual knock membership event itself as well. This provides - # the client with: + # MSC4319: Append the actual knock membership event itself as well. This + # provides the client with: # # * A knock state event that they can use for easier internal tracking - # * The rough timestamp of when the knock occurred contained within the event - knocked_state.append(knock) + knocked_state.append(strip_event(room.knock)) # Build the `knock_state` dictionary, which will contain the state of the # room that the client has knocked on diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index d92bbeeae31..f7d84eab68a 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -55,7 +55,6 @@ ) from synapse.events.py_protocol import MSC4242Event, supports_msc4242_state_dag from synapse.events.snapshot import EventPersistencePair -from synapse.events.utils import parse_stripped_state_event from synapse.logging.opentracing import trace from synapse.metrics import SERVER_NAME_LABEL from synapse.storage._base import db_to_json, make_in_list_sql_clause @@ -2252,7 +2251,7 @@ def _get_sliding_sync_insert_values_from_stripped_state( stripped_state_map: MutableStateMap[StrippedStateEvent] = {} if isinstance(unsigned_stripped_state_events, list): for raw_stripped_event in unsigned_stripped_state_events: - stripped_state_event = parse_stripped_state_event( + stripped_state_event = StrippedStateEvent.from_json_dict( raw_stripped_event ) if stripped_state_event is not None: diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index 27dab290b39..d3f16e6b7ab 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -41,7 +41,7 @@ from twisted.internet import defer -from synapse.api.constants import Direction, EventTypes +from synapse.api.constants import Direction, EventTypes, Membership from synapse.api.errors import NotFoundError, SynapseError from synapse.api.room_versions import ( KNOWN_ROOM_VERSIONS, @@ -385,6 +385,8 @@ def get_chain_id_txn(txn: Cursor) -> int: finished (so we don't have to keep querying it every time) """ + self._room_prejoin_state_types = hs.config.api.room_prejoin_state + def get_un_partial_stated_events_token(self, instance_name: str) -> int: return ( self._un_partial_stated_events_stream_id_gen.get_current_token_for_writer( @@ -1127,14 +1129,12 @@ async def _get_events_from_local_cache( async def get_stripped_room_state_from_event_context( self, + event: EventBase, context: EventContext, - state_keys_to_include: StateFilter, - membership_user_id: str | None = None, ) -> list[JsonDict]: """ Retrieve the stripped state from a room, given an event context to retrieve state - from as well as the state types to include. Optionally, include the membership - events from a specific user. + from as well as the state types to include. "Stripped" state means that only the `type`, `state_key`, `content` and `sender` keys are included from each state event. @@ -1142,35 +1142,63 @@ async def get_stripped_room_state_from_event_context( Args: context: The event context to retrieve state of the room from. state_keys_to_include: The state events to include, for each event type. - membership_user_id: An optional user ID to include the stripped membership state - events of. This is useful when generating the stripped state of a room for - invites. We want to send membership events of the inviter, so that the - invitee can display the inviter's profile information if the room lacks any. Returns: A list of dictionaries, each representing a stripped state event from the room. """ - if membership_user_id: + selected_state_ids = await self.get_stripped_room_state_ids_from_event_context( + event, context + ) + + state_to_include = await self.get_events(selected_state_ids) + + return [strip_event(e) for e in state_to_include.values()] + + async def get_stripped_room_state_ids_from_event_context( + self, + event: EventBase, + context: EventContext, + ) -> list[str]: + """ + Retrieve the stripped state IDs for an event, given an event context to retrieve state + from as well as the state types to include. + + "Stripped" state means that only the `type`, `state_key`, `content` and `sender` keys + are included from each state event. + + Args: + context: The event context to retrieve state of the room from. + + Returns: + A list of event_ids, each representing the stripped state event to include for this event + """ + # Start with the configured default set of stripped state to include + state_filter = self._room_prejoin_state_types + + # MSC4319: We want to send membership events of the inviter, so that the invitee + # can display the inviter's profile information if the room lacks any. + is_invite_event = ( + event.type == EventTypes.Member and event.membership == Membership.INVITE + ) + if is_invite_event: types = chain( - state_keys_to_include.to_types(), - [(EventTypes.Member, membership_user_id)], + self._room_prejoin_state_types.to_types(), + [(EventTypes.Member, event.sender)], ) - filter = StateFilter.from_types(types) - else: - filter = state_keys_to_include - selected_state_ids = await context.get_current_state_ids(filter) + state_filter = StateFilter.from_types(types) + + # Get the relevant state + selected_state_ids = await context.get_current_state_ids(state_filter) # We know this event is not an outlier, so this must be # non-None. assert selected_state_ids is not None - # Confusingly, get_current_state_events may return events that are discarded by - # the filter, if they're in context._state_delta_due_to_event. Strip these away. - selected_state_ids = filter.filter_state(selected_state_ids) - - state_to_include = await self.get_events(selected_state_ids.values()) + # Confusingly, `get_current_state_ids` may return events that are discarded by + # the filter, if they're in `context._state_delta_due_to_event`. Strip these away. + selected_state_ids = state_filter.filter_state(selected_state_ids) - return [strip_event(e) for e in state_to_include.values()] + return list(selected_state_ids.values()) def _maybe_start_fetch_thread(self) -> None: """Starts an event fetch thread if we are not yet at the maximum number.""" diff --git a/synapse/synapse_rust/room_versions.pyi b/synapse/synapse_rust/room_versions.pyi index 9bbb538f185..c69fd77a224 100644 --- a/synapse/synapse_rust/room_versions.pyi +++ b/synapse/synapse_rust/room_versions.pyi @@ -123,6 +123,25 @@ class RoomVersion: to the create event every time we insert an event would be prohibitively expensive. This is similar to how doubly-linked lists can potentially not refer to previous items correctly without verifying the list's integrity, but doing it on every insert is too expensive.""" + msc4311_stripped_state: bool + """ + Determines whether a room version *SHOULD* rather than *MAY* reject invites/knocks + with invalid stripped state events. + + According to MSC4311: + > If any of the [stripped state] events are not a PDU, not for the room ID specified, or fail + > signature checks, or the `m.room.create` event is missing, the receiving + > server MAY respond to invites with a `400 M_MISSING_PARAM` standard Matrix + > error (new to the endpoint). For invites to room version 12+ rooms, servers + > SHOULD rather than MAY respond to such requests with `400 M_MISSING_PARAM`. + + Regardless of room version (we should always do these things): + 1. The `m.room.create` event *MUST* be included in + `invite_room_state`/`knock_room_state` when sending invites/knocks over the + federation API's. + 2. Use full PDU's in the `invite_room_state`/`knock_room_state` in the federation + API. The client API still uses stripped state. + """ class RoomVersions: V1: RoomVersion diff --git a/tests/events/test_auto_accept_invites.py b/tests/events/test_auto_accept_invites.py index 632d1dc4f64..c44b4e011ed 100644 --- a/tests/events/test_auto_accept_invites.py +++ b/tests/events/test_auto_accept_invites.py @@ -198,9 +198,9 @@ def test_invite_from_remote_user(self) -> None: ) self.get_success( self.handler.on_invite_request( - remote_server, - invite_event, - invite_event.room_version, + origin=remote_server, + event=invite_event, + room_version=invite_event.room_version, ) ) @@ -324,9 +324,9 @@ def test_accept_invite_local_user( ) self.get_success( self.handler.on_invite_request( - remote_server, - invite_event, - invite_event.room_version, + origin=remote_server, + event=invite_event, + room_version=invite_event.room_version, ) ) else: diff --git a/tests/federation/transport/test_knocking.py b/tests/federation/transport/test_knocking.py index ec705676cce..cdaae5cfd75 100644 --- a/tests/federation/transport/test_knocking.py +++ b/tests/federation/transport/test_knocking.py @@ -184,14 +184,11 @@ def check_knock_room_state_against_room_state( expected_room_state[event_type]["content"], event["content"] ) - # Check the state key is correct + # Check the state_key is correct self.assertEqual( expected_room_state[event_type]["state_key"], event["state_key"] ) - # Ensure the event has been stripped - self.assertNotIn("signatures", event) - # Pop once we've found and processed a state event expected_room_state.pop(event_type) diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index 1f42490d96a..0138c8e6fd0 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -383,18 +383,18 @@ def create_invite() -> EventBase: event = create_invite() self.get_success( self.handler.on_invite_request( - other_server, - event, - event.room_version, + origin=other_server, + event=event, + room_version=event.room_version, ) ) event = create_invite() self.get_failure( self.handler.on_invite_request( - other_server, - event, - event.room_version, + origin=other_server, + event=event, + room_version=event.room_version, ), exc=LimitExceededError, ) diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 0a7475856a8..1c3b146ba7a 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -560,9 +560,9 @@ def test_msc4155_block_invite_remote(self) -> None: f = self.get_failure( self.fed_handler.on_invite_request( - remote_server, - invite_event, - invite_event.room_version, + origin=remote_server, + event=invite_event, + room_version=invite_event.room_version, ), SynapseError, ).value @@ -606,9 +606,9 @@ def test_msc4155_block_invite_remote_server(self) -> None: f = self.get_failure( self.fed_handler.on_invite_request( - remote_server, - invite_event, - invite_event.room_version, + origin=remote_server, + event=invite_event, + room_version=invite_event.room_version, ), SynapseError, ).value @@ -721,9 +721,9 @@ def test_msc4380_block_invite_remote(self) -> None: f = self.get_failure( self.fed_handler.on_invite_request( - remote_server, - invite_event, - invite_event.room_version, + origin=remote_server, + event=invite_event, + room_version=invite_event.room_version, ), SynapseError, ).value diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py index 0f8de6e7b92..ba9749d02c4 100644 --- a/tests/handlers/test_room_summary.py +++ b/tests/handlers/test_room_summary.py @@ -232,7 +232,9 @@ def _poke_fed_invite(self, room_id: str, from_user: str) -> None: } ) self.get_success( - fed_handler.on_invite_request(fed_hostname, event, RoomVersions.V6) + fed_handler.on_invite_request( + origin=fed_hostname, event=event, room_version=RoomVersions.V6 + ) ) def test_simple_space(self) -> None: diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 9a5efbdd399..0654f84351e 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -608,9 +608,11 @@ def test_out_of_band_invite_rejection(self) -> None: self.get_success( self.hs.get_federation_server().on_invite_request( - self.OTHER_SERVER_NAME, - invite_pdu, - "9", + origin=self.OTHER_SERVER_NAME, + expected_event_id=invite_event_id, + expected_room_id="!room:id", + event_json=invite_pdu, + room_version_id="9", ) )