Skip to content

Add test for PDU limit on transactions API#555

Merged
hawkowl merged 3 commits into
developfrom
anoa/transactions_limit_edu_pdus
Jan 31, 2019
Merged

Add test for PDU limit on transactions API#555
hawkowl merged 3 commits into
developfrom
anoa/transactions_limit_edu_pdus

Conversation

@anoadragon453
Copy link
Copy Markdown
Member

Check that transaction events with > 50 PDUs are rejected as a possible DOS vector.

Synapse PR: matrix-org/synapse#4513

Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

good start :)

Comment thread tests/50federation/51transactions.pl Outdated
@@ -0,0 +1,46 @@
test "Server correctly handles transactions that break edu limits",
requires => [ $main::OUTBOUND_CLIENT, $main::INBOUND_SERVER, $main::HOMESERVER_INFO[0],
local_user_and_room_fixtures( user_opts => { with_events => 1 } ),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need the user_opts thing

Comment thread tests/50federation/51transactions.pl Outdated
test "Server correctly handles transactions that break edu limits",
requires => [ $main::OUTBOUND_CLIENT, $main::INBOUND_SERVER, $main::HOMESERVER_INFO[0],
local_user_and_room_fixtures( user_opts => { with_events => 1 } ),
federation_user_id_fixture(), room_alias_name_fixture() ],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't think we need these either

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bump

Comment thread tests/50federation/51transactions.pl Outdated
my $local_server_name = $info->server_name;

my $remote_server_name = $inbound_server->server_name;
my $datastore = $inbound_server->datastore;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is unused

Comment thread tests/50federation/51transactions.pl Outdated

my $room_alias = "#$room_alias_name:$remote_server_name";

my $device_id = "random_device_id";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is unused

Comment thread tests/50federation/51transactions.pl Outdated
},
);

# Generate a messge with 51 PDUs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/messge/transaction/

Comment thread tests/50federation/51transactions.pl Outdated

# Generate a messge with 51 PDUs
my @pdus = ();
for my $i ( 0 .. 50 ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can probably put my @pdus = ( $new_event ) x 50; here

Comment thread tests/50federation/51transactions.pl
@anoadragon453 anoadragon453 requested a review from a team January 30, 2019 17:07
Comment thread tests/50federation/51transactions.pl Outdated
$outbound_client->join_room(
server_name => $local_server_name,
room_id => $room_id,
user_id => $creator_id,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can use $creator here (and below) I think

Comment thread tests/50federation/51transactions.pl Outdated
test "Server correctly handles transactions that break edu limits",
requires => [ $main::OUTBOUND_CLIENT, $main::INBOUND_SERVER, $main::HOMESERVER_INFO[0],
local_user_and_room_fixtures( user_opts => { with_events => 1 } ),
federation_user_id_fixture(), room_alias_name_fixture() ],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bump

@anoadragon453
Copy link
Copy Markdown
Member Author

Huh, TIL git does not like $ in commit titles.

@anoadragon453 anoadragon453 requested a review from a team January 30, 2019 17:54
@hawkowl hawkowl merged commit 33607b6 into develop Jan 31, 2019
@hawkowl hawkowl deleted the anoa/transactions_limit_edu_pdus branch January 31, 2019 11:43
anoadragon453 added a commit that referenced this pull request Mar 13, 2019
…server_specific_tests

* 'develop' of github.com:matrix-org/sytest: (44 commits)
  test endpoint for updating backup versions (#559)
  Add test for transferring bans on a room upgrade (#563)
  fix tests for matrix-org/synapse#2090 (#350)
  Fix typo
  Fixup
  Incorporate review
  Remove prev_state from federation API
  Make the userdir synced not rely on being able to search for yourself (#567)
  Add basic soft fail test
  Check that event_id is given over state fetching over federation (#566)
  Fix registration rate limiting settings
  Fix comments
  Make sytest support auth rate limiting
  Fixup to not send 100 messages
  Test history visibility works for backfill
  Check that Server ACLs are preserved on room upgrade
  Better diagnostics from synapse startup (#561)
  Don't set PYTHONPATH when running synapse (#560)
  Regression test for redactions in room v3 (#558)
  Add test for PDU limit on transactions API (#555)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants