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

morguldir · 2024-10-31T18:44:40Z

Complement test: matrix-org/complement#743

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Code style is correct

synapse/federation/federation_server.py

MadLittleMods · 2024-11-06T00:03:22Z

changelog.d/17893.doc

@@ -0,0 +1 @@
+Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir


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

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

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?

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

Does this exist in Synapse somewhere already?

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? 🤔

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

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

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.

@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 🤞

I see, thanks for the reviews so far 🙏

synapse/federation/federation_server.py

MadLittleMods · 2024-11-06T00:20:46Z

synapse/federation/federation_server.py

@@ -56,7 +56,11 @@
    SynapseError,


@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.

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

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

Co-authored-by: Eric Eastwood <[email protected]>

synapse/federation/federation_server.py

tests/federation/test_federation_server.py

changelog.d/17893.doc

synapse/federation/federation_server.py

MadLittleMods · 2024-11-07T14:49:26Z

changelog.d/17893.doc

@@ -0,0 +1 @@
+Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir


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?

Co-authored-by: Eric Eastwood <[email protected]>

synapse/events/__init__.py

synapse/federation/federation_server.py

tests/federation/test_federation_server.py

Comment updates, docstring, remove extra logs Co-authored-by: Eric Eastwood <[email protected]>

anoadragon453

I think this is good to merge in the meantime while matrix-org/matrix-spec#2027 is figured out.

It only improves today's situation, which is for Synapse to fail with a 500 upon receiving such transactions.

If the above spec issue ends up deciding that such events should be valid, then a future PR can modify the behaviour to accept these events. But we should still fix the 500 now.

Seeing that matrix-org/complement#743 only adds a new test, it's fine to merge this PR now.

anoadragon453 · 2024-12-09T11:37:40Z

changelog.d/17893.bugfix

I've created a spec issue to clarify whether events for v3+ rooms containing an event_id field should be considered valid: matrix-org/matrix-spec#2027

synapse/federation/federation_server.py

anoadragon453 · 2024-12-09T12:04:46Z

changelog.d/17893.bugfix

@morguldir it seems that this sytest test is currently failing with this PR. That is due to the test expecting the homeserver to return a 400 when a bad event is included in a transaction, whereas we're now just silently dropping them (and returning a 200).

Could you create a PR to update that test please? I consider that a blocking change to merging this PR.

Hmm, perl didn't live up to the bad rumors so far :D, added in matrix-org/sytest#1391

Although according to halfie it sounds like you need to push the branch to sytest if you want CI to pass matrix-org/synapse#12754 👀

morguldir · 2024-12-12T12:00:33Z

synapse/events/__init__.py

+        # If we see an `event_id` in a newer room version, then it's an invalid event
+        # and we should reject it.
+        assert (
+            "event_id" not in event_dict


The assert has always been there, this just adds an error message and a comment

I'm retracting my approval at the moment as discussion has continued on matrix-org/matrix-spec#2027 (comment) and is trending away from this PR's approach.

anoadragon453

~~Err sorry, I obviously need more coffee. The exact opposite is true of what I said in #17893 (comment)~~

Actually, it's partially true. This PR does two things, the latter of which the title fails to mention:

Silently drop PDUs we fail to parse from federation
Report an error back to the homeserver if an event_id was provided in an incoming v3+ room event.

Discussion in #17893 (comment) is advocating to also silently drop events which contain an event_id field. So dropping these lines and removing the second change from this PR.

@morguldir curious for your thoughts on that change?

morguldir · 2024-12-17T18:53:54Z

My reasoning for the error was that synapse would yell at you loudly before with a 500 for the whole transaction before, but there's not really anything another server can do with that information (a loud error)

Returning an error for that specific pdu at least lets the sending server output a warning or similar

From the discussion in #7543, i think the main problem is that most of the time it's impossible to calculate the event id, but here it was already provided. I'm not sure if there's a drawback to informing about the event not being processed

In the end i'm mostly worried about returning a 200 response every time though, regardless if it includes the error or not

morguldir added 3 commits October 31, 2024 18:52

Tolerate transaction pdus that contain superfluous event ids

95cbc99

Try to always process transactions even if a PDU can't be parsed

730822c

add changelog

e89add8

morguldir requested a review from a team as a code owner October 31, 2024 18:44

minor changes to strings

07578bd

MadLittleMods requested changes Nov 6, 2024

View reviewed changes

morguldir and others added 4 commits November 6, 2024 23:08

make constant for magic value used for unknown event ids

9953a36

Co-authored-by: Eric Eastwood <[email protected]>

add explanatory comments and clarifications

f55e645

ruff

464e9cf

Add test in test_federation_server, ensuring that good events are kept

b6b5d81

MadLittleMods requested changes Nov 7, 2024

View reviewed changes

MadLittleMods reviewed Nov 7, 2024

View reviewed changes

changelog.d/17893.doc Outdated Show resolved Hide resolved

MadLittleMods reviewed Nov 7, 2024

View reviewed changes

morguldir and others added 6 commits November 7, 2024 21:06

Remove explicit event_id that's already caught by the catch-all

89f91aa

Add additional event, identify events more clearly, and adjust logs

3ca943a

Rename changelog file

b8ce471

Add comments around event format assert and make it return a message

b2916f1

Remove unnecessary assert

7b3a695

Co-authored-by: Eric Eastwood <[email protected]>

Run lint

78e04dd

MadLittleMods requested changes Nov 8, 2024

View reviewed changes

Apply suggestions from code review

df07930

Comment updates, docstring, remove extra logs Co-authored-by: Eric Eastwood <[email protected]>

MadLittleMods requested a review from anoadragon453 December 4, 2024 23:18

anoadragon453 changed the title ~~Be more tolerant towards weird data in transaction pdus~~ Reject room v3+ events over federation if they contain an event_id field Dec 9, 2024

anoadragon453 mentioned this pull request Dec 9, 2024

Should v3+ room version events containing an event_id field be rejected? matrix-org/matrix-spec#2027

Open

anoadragon453 changed the title ~~Reject room v3+ events over federation if they contain an event_id field~~ Reject, instead of 500, room v3+ events over federation if they contain an event_id field Dec 9, 2024

anoadragon453 previously approved these changes Dec 9, 2024

View reviewed changes

Add comment

f61de20

anoadragon453 changed the title ~~Reject, instead of 500, room v3+ events over federation if they contain an event_id field~~ Reject room v3+ events over federation if they contain an event_id field, instead of entire transaction Dec 9, 2024

anoadragon453 changed the title ~~Reject room v3+ events over federation if they contain an event_id field, instead of entire transaction~~ Silently drop PDUs we fail to parse from federation, instead of failing entire transaction Dec 9, 2024

anoadragon453 reviewed Dec 9, 2024

View reviewed changes

morguldir commented Dec 12, 2024

View reviewed changes

anoadragon453 reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

morguldir commented Oct 31, 2024 •

edited by MadLittleMods

Loading

MadLittleMods Nov 6, 2024

morguldir Nov 6, 2024

MadLittleMods Nov 7, 2024 •

edited

Loading

MadLittleMods Nov 7, 2024

morguldir Nov 8, 2024 •

edited by MadLittleMods

Loading

morguldir Nov 8, 2024

morguldir Nov 8, 2024

MadLittleMods Nov 8, 2024 •

edited

Loading

MadLittleMods Nov 11, 2024

morguldir Nov 11, 2024

MadLittleMods Nov 6, 2024

morguldir Nov 7, 2024

MadLittleMods Nov 7, 2024

MadLittleMods Nov 7, 2024 •

edited

Loading

anoadragon453 left a comment •

edited

Loading

anoadragon453 Dec 9, 2024

anoadragon453 Dec 9, 2024

morguldir Dec 12, 2024

morguldir Dec 12, 2024

anoadragon453 left a comment •

edited

Loading

morguldir commented Dec 17, 2024

		@@ -0,0 +1 @@
		Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir

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

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

Are you sure you want to change the base?

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

Conversation

morguldir commented Oct 31, 2024 • edited by MadLittleMods Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morguldir Nov 8, 2024 • edited by MadLittleMods Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

anoadragon453 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoadragon453 left a comment • edited Loading

Choose a reason for hiding this comment

morguldir commented Dec 17, 2024

morguldir commented Oct 31, 2024 •

edited by MadLittleMods

Loading

MadLittleMods Nov 7, 2024 •

edited

Loading

morguldir Nov 8, 2024 •

edited by MadLittleMods

Loading

MadLittleMods Nov 8, 2024 •

edited

Loading

MadLittleMods Nov 7, 2024 •

edited

Loading

anoadragon453 left a comment •

edited

Loading

anoadragon453 left a comment •

edited

Loading