Merge pull request #1399 from matrix-org/quenting/room-v6-bad-json

Check that invalid PDUs are ignored, instead of erroring out

Author: sandhose

tests/50federation/51transactions.pl

@@ -51,35 +51,114 @@
 # 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.
+#
+# 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,
+                 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 ) = @_;
+
+      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 succeed
+      $outbound_client->send_transaction(
+          pdus => \@pdus,
+          destination => $creator->server_name,
+      )->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
+      });
+   };
+
+# 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    => "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
+      # Send the transaction to the client and expect to fail
       $outbound_client->send_transaction(
           pdus => \@pdus,
           destination => $creator->server_name,
-      )->main::expect_m_bad_json;
+      )->main::expect_m_bad_json
    };