Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Reject large transactions on federation #4513

Merged
merged 4 commits into from
Jan 31, 2019

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jan 29, 2019

Rejects a transaction if it has more than 50 PDUs and/or 100 EDUs as per https://github.com/matrix-org/matrix-doc/pull/1581/files

Sytest PR: matrix-org/sytest#555

@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #4513 into develop will decrease coverage by 0.05%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4513      +/-   ##
===========================================
- Coverage    74.75%   74.69%   -0.06%     
===========================================
  Files          336      337       +1     
  Lines        34219    34452     +233     
  Branches      5570     5616      +46     
===========================================
+ Hits         25580    25734     +154     
- Misses        7060     7130      +70     
- Partials      1579     1588       +9

@richvdh richvdh requested a review from a team January 29, 2019 21:33
Copy link
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.

can haz sytest?

400, response
)
defer.returnValue((400, response))
return
Copy link
Member

Choose a reason for hiding this comment

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

nit: this return is redundant fwiw

"error": "Processing failed. More than 50 PDUs or 100 EDUs sent."
}

logger.debug(
Copy link
Member

Choose a reason for hiding this comment

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

this probably wants to be at least an info (and not contain a mahoosive list of events)

"pdus": {}
}

for pdu_key in transaction["pdus"].keys():
Copy link
Member

@richvdh richvdh Jan 29, 2019

Choose a reason for hiding this comment

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

I'm really not sure I'd bother with this. The spec doesn't say that this will be present for a non-200 response, and synapse will ignore it.

(in any case Transaction doesn't have a __getattr__, so transaction["pdus"] will fail, and the pdus are a list not a dict.)

@anoadragon453
Copy link
Member Author

Tried making a sytest. It broke me.

@anoadragon453
Copy link
Member Author

Sytest PR added: matrix-org/sytest#555

@anoadragon453 anoadragon453 requested a review from a team January 30, 2019 15:08
Copy link
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.

lgtm

@hawkowl hawkowl merged commit 563f6a8 into develop Jan 31, 2019
@hawkowl hawkowl deleted the anoa/transactions_limit_edu_pdus branch January 31, 2019 11:44
richvdh added a commit that referenced this pull request Feb 14, 2019
Synapse 0.99.1 (2019-02-14)
===========================

Features
--------

- Include m.room.encryption on invites by default ([\#3902](#3902))
- Federation OpenID listener resource can now be activated even if federation is disabled ([\#4420](#4420))
- Synapse's ACME support will now correctly reprovision a certificate that approaches its expiry while Synapse is running. ([\#4522](#4522))
- Add ability to update backup versions ([\#4580](#4580))
- Allow the "unavailable" presence status for /sync.
  This change makes Synapse compliant with r0.4.0 of the Client-Server specification. ([\#4592](#4592))
- There is no longer any need to specify `no_tls`: it is inferred from the absence of TLS listeners ([\#4613](#4613), [\#4615](#4615), [\#4617](#4617), [\#4636](#4636))
- The default configuration no longer requires TLS certificates. ([\#4614](#4614))

Bugfixes
--------

- Copy over room federation ability on room upgrade. ([\#4530](#4530))
- Fix noisy "twisted.internet.task.TaskStopped" errors in logs ([\#4546](#4546))
- Synapse is now tolerant of the `tls_fingerprints` option being None or not specified. ([\#4589](#4589))
- Fix 'no unique or exclusion constraint' error ([\#4591](#4591))
- Transfer Server ACLs on room upgrade. ([\#4608](#4608))
- Fix failure to start when not TLS certificate was given even if TLS was disabled. ([\#4618](#4618))
- Fix self-signed cert notice from generate-config. ([\#4625](#4625))
- Fix performance of `user_ips` table deduplication background update ([\#4626](#4626), [\#4627](#4627))

Internal Changes
----------------

- Change the user directory state query to use a filtered call to the db instead of a generic one. ([\#4462](#4462))
- Reject federation transactions if they include more than 50 PDUs or 100 EDUs. ([\#4513](#4513))
- Reduce duplication of ``synapse.app`` code. ([\#4567](#4567))
- Fix docker upload job to push -py2 images. ([\#4576](#4576))
- Add port configuration information to ACME instructions. ([\#4578](#4578))
- Update MSC1711 FAQ to calrify .well-known usage ([\#4584](#4584))
- Clean up default listener configuration ([\#4586](#4586))
- Clarifications for reverse proxy docs ([\#4607](#4607))
- Move ClientTLSOptionsFactory init out of `refresh_certificates` ([\#4611](#4611))
- Fail cleanly if listener config lacks a 'port' ([\#4616](#4616))
- Remove redundant entries from docker config ([\#4619](#4619))
- README updates ([\#4621](#4621))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants