Implement MSC2730: verifiable forwarded events#8078
Conversation
|
@tulir is this PR ready for review? |
|
I think it is |
|
|
||
|
|
||
| class FederationHandler(BaseHandler): | ||
| class FederationHandler(BaseHandler, FederationBase): |
There was a problem hiding this comment.
ftr, FederationBase is something I wish would go away. Those utility functions need to be brought in by composition, not inheritance.
|
|
||
| return fetched_events | ||
|
|
||
| _forwarded_key = "net.maunium.msc2730.forwarded" |
There was a problem hiding this comment.
constants go in UPPER_CASE at the top of the file.
|
|
||
| async def _validate_forwarded_event( | ||
| self, event: EventBase | ||
| ) -> Tuple[bool, Optional[str]]: |
There was a problem hiding this comment.
why not just return the event id if it's valid, and None if it's not?
There was a problem hiding this comment.
(alternatively: what does it mean if valid is False, but an event id is returned? Some docstring would help here).
| except SynapseError: | ||
| return False, None |
There was a problem hiding this comment.
as a general rule, eating exceptions like this without giving any clue about what the exception was leads to hard-to-debug failures. I'd recommend logging something before returning.
| return False, None | ||
|
|
||
| try: | ||
| checked_evt = await self._check_sigs_and_hash(room_version, source_evt) |
There was a problem hiding this comment.
I wonder if there is some horrible attack where you can claim that an event is for a different room version than it is, and hence get it to pass the hash checks when it shouldn't...
| # Pass through the old event ID to the new unsigned data | ||
| event_id = unsigned[self._data_key]["event_id"] | ||
| elif not has_forward_meta: | ||
| content[self._data_key] = event_dict |
There was a problem hiding this comment.
careful: I think this modifies the original event, stored in the cache. You need to copy content before modifying it.
|
Hi @tulir, thanks for this! It looks generally like a sensible idea, and from the point of view of the MSC process I certainly think there is enough here to demonstrate a workable implementation, but I'm afraid I don't think we can accept this into mainline synapse until the MSC gets wider acceptance, so I'm going to close it for now. We'll be very happy to reopen once the MSC progresses! |
3 participants
This adds an implementation of matrix-org/matrix-spec-proposals#2730 to Synapse, i.e. it adds a new
PUT /_matrix/client/unstable/net.maunium.msc2730/rooms/{roomId}/event/{eventId}/forward/{targetRoomId}/{txnId}endpoint and implements validation of incoming events that have thenet.maunium.msc2730key.Element web implementation: matrix-org/matrix-js-sdk#1439 / matrix-org/matrix-react-sdk#5117
Signed-off-by: Tulir Asokan <tulir@maunium.net>
Pull Request Checklist