From 91ba7b12878a81d9be80eab72ab1f4a128470266 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 1 Apr 2025 12:40:46 +0200 Subject: [PATCH 1/5] Check that invalid PDUs are ignored, instead of erroring out --- tests/50federation/51transactions.pl | 39 +++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/tests/50federation/51transactions.pl b/tests/50federation/51transactions.pl index 4723f25ea..54ea63537 100644 --- a/tests/50federation/51transactions.pl +++ b/tests/50federation/51transactions.pl @@ -55,7 +55,7 @@ # sent. It is unclear if this is the correct behavior or not. # # See https://github.com/matrix-org/synapse/issues/7543 -test "Server rejects invalid JSON in a version 6 room", +test "Server discards events with invalid JSON in a version 6 room", requires => [ $main::OUTBOUND_CLIENT, federated_rooms_fixture( room_opts => { room_version => "6" } ) ], @@ -63,23 +63,54 @@ my ( $outbound_client, $creator, $user_id, @rooms ) = @_; my $room = $rooms[0]; + my $room_id = $room->room_id; + + my $good_event = $room->create_and_insert_event( + type => "m.room.message", + + sender => $user_id, + content => { + body => "Good event", + }, + ); my $bad_event = $room->create_and_insert_event( type => "m.room.message", sender => $user_id, content => { - body => "Message 1", + body => "Bad event", # Insert a "bad" value into the PDU, in this case a float. bad_val => 1.1, }, ); - my @pdus = ( $bad_event ); + my @pdus = ( $good_event, $bad_event ); # Send the transaction to the client and expect a fail $outbound_client->send_transaction( pdus => \@pdus, destination => $creator->server_name, - )->main::expect_m_bad_json; + )->then(sub { + # Wait for the good event to be sent down through sync + await_sync_timeline_contains( $creator, $room_id, check => sub { + my ( $event ) = @_; + $event->{type} eq "m.room.message" && + $event->{content}{body} eq "Good event" + }) + })->then(sub { + # Check that we can fetch the good event + my $event_id = $room->id_for_event( $good_event ); + do_request_json_for( $creator, + method => "GET", + uri => "/v3/rooms/$room_id/event/$event_id", + ) + })->then(sub { + # Check that we have ignored the bad event PDU + my $event_id = $room->id_for_event( $bad_event ); + do_request_json_for( $creator, + method => "GET", + uri => "/v3/rooms/$room_id/event/$event_id", + )->main::expect_m_not_found + }); }; From 5359ab1401bbc114419068512f05b5c25d82a499 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 1 Apr 2025 12:51:13 +0200 Subject: [PATCH 2/5] Update the comments to better reflect the test --- tests/50federation/51transactions.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/50federation/51transactions.pl b/tests/50federation/51transactions.pl index 54ea63537..c9919e15f 100644 --- a/tests/50federation/51transactions.pl +++ b/tests/50federation/51transactions.pl @@ -51,8 +51,8 @@ # 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. +# This enforces that invalid PDUs are discarded rather than failing the entire +# transaction. # # See https://github.com/matrix-org/synapse/issues/7543 test "Server discards events with invalid JSON in a version 6 room", @@ -87,7 +87,7 @@ my @pdus = ( $good_event, $bad_event ); - # Send the transaction to the client and expect a fail + # Send the transaction to the client and expect to succeed $outbound_client->send_transaction( pdus => \@pdus, destination => $creator->server_name, From a10eebc5b794ecb66719b0570479fbbea6ca3ca9 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 1 Apr 2025 13:23:25 +0200 Subject: [PATCH 3/5] Have an implementation-specific test --- tests/50federation/51transactions.pl | 45 ++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/50federation/51transactions.pl b/tests/50federation/51transactions.pl index c9919e15f..cbad028e2 100644 --- a/tests/50federation/51transactions.pl +++ b/tests/50federation/51transactions.pl @@ -58,6 +58,8 @@ test "Server discards events with invalid JSON in a version 6 room", requires => [ $main::OUTBOUND_CLIENT, federated_rooms_fixture( room_opts => { room_version => "6" } ) ], + # This behaviour has only been changed in Synapse, not Dendrite + implementation_specific => ['synapse'], do => sub { my ( $outbound_client, $creator, $user_id, @rooms ) = @_; @@ -114,3 +116,46 @@ )->main::expect_m_not_found }); }; + +# This is an alternative behaviour that isn't spec compliant, where the server +# rejects the whole transaction if any PDU is invalid. +# This is the behaviour that Dendrite currently implements. +test "Server rejects invalid JSON in a version 6 room", + requires => [ $main::OUTBOUND_CLIENT, + federated_rooms_fixture( room_opts => { room_version => "6" } ) ], + implementation_specific => ['dendrite'], + + do => sub { + my ( $outbound_client, $creator, $user_id, @rooms ) = @_; + + my $room = $rooms[0]; + my $room_id = $room->room_id; + + my $good_event = $room->create_and_insert_event( + type => "m.room.message", + + sender => $user_id, + content => { + body => "Good event", + }, + ); + + my $bad_event = $room->create_and_insert_event( + type => "m.room.message", + + sender => $user_id, + content => { + body => "Bad event", + # Insert a "bad" value into the PDU, in this case a float. + bad_val => 1.1, + }, + ); + + my @pdus = ( $good_event, $bad_event ); + + # Send the transaction to the client and expect to fail + $outbound_client->send_transaction( + pdus => \@pdus, + destination => $creator->server_name, + )->main::expect_bad_json + }; From 842e5b7139fb70221c4999631653f773f66a84c6 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 1 Apr 2025 13:25:42 +0200 Subject: [PATCH 4/5] Explain why we don't check the /send response --- tests/50federation/51transactions.pl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/50federation/51transactions.pl b/tests/50federation/51transactions.pl index cbad028e2..4c540a1a8 100644 --- a/tests/50federation/51transactions.pl +++ b/tests/50federation/51transactions.pl @@ -54,6 +54,9 @@ # This enforces that invalid PDUs are discarded rather than failing the entire # transaction. # +# It is unclear whether the invalid PDU should be included in the /send response +# with an error, which is why there is no assertion on the response. +# # See https://github.com/matrix-org/synapse/issues/7543 test "Server discards events with invalid JSON in a version 6 room", requires => [ $main::OUTBOUND_CLIENT, From 7e2a47ee63487745438a5d222555c83568d54ca8 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 1 Apr 2025 13:44:11 +0200 Subject: [PATCH 5/5] Fix typo in test --- tests/50federation/51transactions.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/50federation/51transactions.pl b/tests/50federation/51transactions.pl index 4c540a1a8..9a7baaed7 100644 --- a/tests/50federation/51transactions.pl +++ b/tests/50federation/51transactions.pl @@ -160,5 +160,5 @@ $outbound_client->send_transaction( pdus => \@pdus, destination => $creator->server_name, - )->main::expect_bad_json + )->main::expect_m_bad_json };