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.

Sign up for GitHub

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

Silently drop PDUs we fail to parse from federation, instead of failing entire transaction #17893

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Silently drop PDUs we fail to parse from federation, instead of failing entire transaction #17893

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
95cbc99
Tolerate transaction pdus that contain superfluous event ids
morguldir Oct 31, 2024
730822c
Try to always process transactions even if a PDU can't be parsed
morguldir Oct 31, 2024
e89add8
add changelog
morguldir Oct 31, 2024
07578bd
minor changes to strings
morguldir Oct 31, 2024
9953a36
make constant for magic value used for unknown event ids
morguldir Nov 6, 2024
f55e645
add explanatory comments and clarifications
MadLittleMods Nov 6, 2024
464e9cf
ruff
morguldir Nov 7, 2024
b6b5d81
Add test in test_federation_server, ensuring that good events are kept
morguldir Nov 7, 2024
89f91aa
Remove explicit event_id that's already caught by the catch-all
morguldir Nov 7, 2024
3ca943a
Add additional event, identify events more clearly, and adjust logs
morguldir Nov 7, 2024
b8ce471
Rename changelog file
morguldir Nov 8, 2024
b2916f1
Add comments around event format assert and make it return a message
morguldir Nov 8, 2024
7b3a695
Remove unnecessary assert
morguldir Nov 8, 2024
78e04dd
Run lint
morguldir Nov 8, 2024
df07930
Apply suggestions from code review
morguldir Nov 11, 2024
f61de20
Add comment
anoadragon453 Dec 9, 2024
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
1 change: 1 addition & 0 deletions changelog.d/17893.doc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there more context for the issue you ran into specifically? Some issue that should be linked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think there's an issue for it, but basically unless the room is v11, the version key is actually unprotected. Unfortunately there was no cs api protection for redacting the create event in conduwuit, so a silly bot creator accidentally redacted everything in the room, making that particular room version fall back to "1" like the spec says

Once i finally returned 200 OK, all the pdus and edus that had been failing started to trickle in again at last

I think matrix.org was also in the room at the time, so it's similarly affected

Copy link
Contributor

@MadLittleMods MadLittleMods Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I assume you're talking about the behavior described here:

Room Version 11

[New in this version] [...] The m.room.create event now keeps the entire content property.

-- https://spec.matrix.org/v1.12/rooms/v11/#redactions

And the m.room.create event defines the content.room_version.

For my own reference, this was added in MSC2176 and included in room version 11 via MSC3820.

I see how this PR allows valid events from a transaction to be received while ignoring invalid ones.

In the scenario where a room falls back to a v1 room, does the room_version we have stored in Synapse get updated?

If the room_version doesn't get updated, this just makes sure that the server can receive the final "valid" events in the room before it broke (people started sending v1 events). Is there an issue tracking whether Synapse should update the room_version after the m.room.create redaction?

If the room_version does get updated to v1, then events are only accepted according to the order received (before/after receiving the redaction forcing the fallback).

Perhaps we should have a test for this scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately there was no cs api protection for redacting the create event

Does this exist in Synapse somewhere already?

Copy link
Contributor Author

@morguldir morguldir Nov 8, 2024 •
edited by MadLittleMods
Loading

Choose a reason for hiding this comment

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

cs api errors with

synapse/synapse/handlers/message.py

Lines 1855 to 1856 in 4be3bd4

if original_event.type == EventTypes.Create:
raise AuthError(403, "Redacting create events is not permitted")

i tried to send a redaction from construct (conduwuit would update the room version and fail to send the redaction as well), when it was a partial state room it seemed to error somewhat, but for a normal room it didn't seem like anything happened, the event didn't show in my timeline, but it was persisted on disk without errors.

I did an initial sync too, and the create event was still there, so i assume it just gets ignored? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the scenario where a room falls back to a v1 room, does the room_version we have stored in Synapse get updated?

Also to be clear, the scenario is mostly on the conduwuit side in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there more context for the issue you ran into specifically? Some issue that should be linked?

Seeing the sytest failure now there actually is an for it already, matrix-org/synapse#7543 / #7543

https://github.com/matrix-org/sytest/blob/9bff8eba1bdbdcdfcdbf8277c1515e07e356e37b/tests/50federation/51transactions.pl#L50-L58

Copy link
Contributor

@MadLittleMods MadLittleMods Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good find! I've linked the issues in the PR description at the top

The Sytest says this:

# Room version 6 states that homeservers should strictly enforce canonical JSON
# on PDUs. Test that a transaction to `send` with a PDU that has bad data will
# be handled properly.
#
# This enforces that the entire transaction is rejected if a single bad PDU is
# sent. It is unclear if this is the correct behavior or not.
#
# See https://github.com/matrix-org/synapse/issues/7543

I feel like given the nature of how PUT /_matrix/federation/v1/send/{txnId} is spec'ed, it makes sense that some PDU's can fail to process while others succeed. The response is literally a map of events to any potential errors when processing that specific event.

Since the Sytest test also says "It is unclear if this is the correct behavior or not.", my interpretation could be correct. Probably should get a few more eyeballs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@morguldir I had this on the To-Discuss for the weekly meeting today but we ran out of time to discuss. This is the first thing on the board to discuss next week though 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for the reviews so far 🙏

MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
27 changes: 23 additions & 4 deletions synapse/federation/federation_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@
SynapseError,
Copy link
Contributor

Choose a reason for hiding this comment

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

@morguldir Are you up for adding a test for this? Probably something in tests/federation/test_federation_server.py, make a request like this and then assert that the other PDU's in the transaction besides the corrupted one were persisted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is b6b5d81 what you had in mind? See also matrix-org/complement#743

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I completely missed your Complement test in my initial review. The Synapse test is looking good 👍

UnsupportedRoomVersionError,
)
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
from synapse.api.room_versions import (
KNOWN_ROOM_VERSIONS,
EventFormatVersions,
RoomVersion,
)
from synapse.crypto.event_signing import compute_event_signature
from synapse.events import EventBase
from synapse.events.snapshot import EventContext
Expand Down Expand Up @@ -432,6 +436,8 @@ async def _handle_pdus_in_txn(

newest_pdu_ts = 0

pdu_results = {}

for p in transaction.pdus:
# FIXME (richardv): I don't think this works:
# https://github.com/matrix-org/synapse/issues/8429
Expand Down Expand Up @@ -469,14 +475,27 @@ async def _handle_pdus_in_txn(
logger.info("Ignoring PDU: %s", e)
continue

event = event_from_pdu_json(p, room_version)
if possible_event_id != "<Unknown>":
morguldir marked this conversation as resolved.
Show resolved Hide resolved
morguldir marked this conversation as resolved.
Show resolved Hide resolved
if room_version.event_format != EventFormatVersions.ROOM_V1_V2:
logger.info(f"Rejecting event {possible_event_id} from {origin} "
f"because the event was made for a v1 room, "
f"while {room_id} is a v{room_version.identifier} room")
pdu_results[possible_event_id] = {"error": "Event ID incorrectly supplied in non-v1/v2 room"}
morguldir marked this conversation as resolved.
Show resolved Hide resolved
continue

try:
event = event_from_pdu_json(p, room_version)
except Exception as e:
if possible_event_id != "<Unknown>":
pdu_results[possible_event_id] = {"error": f"Failed to convert json into event, {e}"}
morguldir marked this conversation as resolved.
Show resolved Hide resolved
logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}")
continue

pdus_by_room.setdefault(room_id, []).append(event)

if event.origin_server_ts > newest_pdu_ts:
newest_pdu_ts = event.origin_server_ts

pdu_results = {}

# we can process different rooms in parallel (which is useful if they
# require callouts to other servers to fetch missing events), but
# impose a limit to avoid going too crazy with ram/cpu.
Expand Down