element-hq / element-hq/synapse

Drop invalid PDUs instead of erroring

Open
#7,543 2 comments 0 reactions 0 assignees View on GitHub
A-Federation A-Validation P4 S-Minor T-Defect z-bug
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#7543](https://github.com/matrix-org/synapse/issues/7543).

---

This is based on a conversation at https://github.com/matrix-org/matrix-doc/pull/2540#discussion_r427973050, summarized below:

Currently when an incoming federation event is "bad" for some reason it is rejected by returning a 400 error. This is particularly troublesome in endpoints where multiple events are handled at once, as the entire transaction gets rejected.

Reasons an event might be rejected include:

* events with no `type` or `depth`
* events with bad `depth` values
* events in v{1,2} rooms with no event_id/v3+ rooms with an `event_id`
* events in v6 rooms that have integers out of range or floats
* events with too many prev_events (#3124)
* etc.

There are three proposed options for this situation:
> 1. silently drop the event. I would argue that we should not be blackholing events, ever: it feels like we'll end up dropping events unexpectedly.
> 2. send an error back in the response, against the event's event_id. This, of course, requires the recipient to parse the json, remove a couple of properties, then re-encode it using canonicaljson (which, you recall, is theoretically impossible) to calculate the event id. That sounds like a tautology to me. It also requires servers which don't even support older room versions to still accept transactions with floats in them. So that leaves us with:
> 3. reject the whole lot.

It is potentially difficult to return a sensible error since (theoretically) you might not even be able to parse the event data and thus it is proposed to silently drop these events for now.

Contributor guide

Open the contributing guide

Research direction

Start by reading the linked matrix-doc PR discussion, then trace Synapse's incoming federation transaction and event-validation entry points for the listed malformed-event cases. Done means invalid events are dropped without rejecting the whole transaction, with coverage for the listed cases; the issue names no specific files or tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend, distributed-systems
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.