Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop invalid PDUs instead of erroring #7543

Open
matrixbot opened this issue Dec 17, 2023 · 1 comment
Open

Drop invalid PDUs instead of erroring #7543

matrixbot opened this issue Dec 17, 2023 · 1 comment

Comments

@matrixbot
Copy link
Collaborator

matrixbot commented Dec 17, 2023

This issue has been migrated from #7543.


This is based on a conversation at matrix-org/matrix-spec-proposals#2540 (comment), 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 (Dummy issue #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.

@richvdh
Copy link
Member

richvdh commented Dec 9, 2024

Currently when an incoming federation event is "bad" for some reason it is rejected by returning a 400 error.

It's worth emphasising that this is only correct for events that cannot be parsed as an EventBase. There are plenty of other things that can make an event "bad" which do not cause a 400 error; instead the event is put onto the queue for asynchronous processing, and dropped if deemed "invalid".

Related: matrix-org/matrix-spec#365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants