Check that invalid PDUs are ignored, instead of erroring out #1399
Conversation
reivilibre
left a comment
reivilibre
left a comment
There was a problem hiding this comment.
looks good overall, just checking some things
tests/50federation/51transactions.pl
Outdated
| # 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. |
| method => "GET", | ||
| uri => "/v3/rooms/$room_id/event/$event_id", | ||
| )->main::expect_m_not_found | ||
| }); |
There was a problem hiding this comment.
should we expect an error in the PDU processing result for the bad PDU?: https://spec.matrix.org/v1.14/server-server-api/#put_matrixfederationv1sendtxnid_response-200_pdu-processing-result
There was a problem hiding this comment.
That's an open question; the sending homeserver technically can't do anything with that information.
There was a problem hiding this comment.
This is why we don't yet assert on the /send response body
tests/50federation/51transactions.pl
Outdated
| # 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. |
There was a problem hiding this comment.
Could you update this comment as well? Perhaps say that the transaction should succeed, but it's unclear whether the response body should contain the invalid event.
| # 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" | ||
| }) |
There was a problem hiding this comment.
Can we also check that we don't receive the bad event down sync?
There was a problem hiding this comment.
I couldn't find utilities to make sure that the sync timeline doesn't contain something :(
There was a problem hiding this comment.
Wouldn't it be just adding && $event->{content}{body} neq "Bad event"?
There was a problem hiding this comment.
It only checks if one event in the timeline checks that constraint
Dendrite indeed fails. I've made implementation specific tests in a10eebc |
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
This LGTM other than potentially fixing the nit in #1399 (comment).
Dendrite appears to still be failing, as it's failing to find this test? https://github.com/matrix-org/sytest/actions/runs/14194392227/job/39766068369?pr=1399#step:6:9
|
@anoadragon453 fixed the test, that was a typo 7e2a47e |
This is similar to #1391, but it doesn't assert on the /send body response, and rather looks at events persisted through /sync and /events