This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Improved validation for received requests #9817
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richvdh
force-pushed
the
rav/request_limit
branch
from
746eebd
to
c583f96
Compare
richvdh
commented
Apr 14, 2021
| @@ -528,3 +530,25 @@ def sdnotify(state): | |||
| # this is a bit surprising, since we don't expect to have a NOTIFY_SOCKET | |||
| # unless systemd is expecting us to notify it. | |||
| logger.warning("Unable to send notification to systemd: %s", e) | |||
|
|
|||
|
|
|||
| def max_request_body_size(config: HomeServerConfig) -> int: | |||
There was a problem hiding this comment.
I'm not terribly happy with this, but couldn't think of anywhere better to do it.
richvdh
commented
Apr 14, 2021
Comment on lines
+320
to
+321
| from synapse.app import _base as appbase | ||
|
|
There was a problem hiding this comment.
this is needed to avoid a circular import. config should not be depending on app.
richvdh
force-pushed
the
rav/request_limit
branch
from
c583f96
to
02b270c
Compare
clokep
reviewed
Apr 15, 2021
richvdh
force-pushed
the
rav/request_limit
branch
from
be63bae
to
d837408
Compare
| @@ -59,8 +61,9 @@ class SynapseRequest(Request): | |||
| logcontext: the log context for this request | |||
| """ | |||
|
|
|||
| def __init__(self, channel, *args, **kw): | |||
| def __init__(self, channel, *args, max_request_body_size=1024, **kw): | |||
There was a problem hiding this comment.
This seems like a surprisingly low default? Can we not make it a required field?
There was a problem hiding this comment.
the default was mostly to avoid messing around with the tests more than I had to. Happy to make it required if you prefer.
| @@ -286,6 +286,10 @@ def setup(self) -> None: | |||
| if self.config.run_background_tasks: | |||
| self.setup_background_tasks() | |||
|
|
|||
| def start_listening(self) -> None: | |||
| """Start the HTTP, manhole, metrics, etc listeners""" | |||
| pass | |||
There was a problem hiding this comment.
I find it a bit odd that the default implementation is a no-op, given the doc comment. What do you think about making this an abstract method and keeping the no-op version in admin cmd?
There was a problem hiding this comment.
personally, it seems fairly reasonable that the default be a no-op - I basically see it as a hook that the derived classes can override. I could make it clearer in the docstring?
don't feel that strongly though, so let me know what you think.
There was a problem hiding this comment.
I guess it just feels slightly odd that start_listening() doesn't do the right thing by default (which it feels like it could if we deduplicate the code between master and worker apps), the fact that the admin cmd server is no-oping it is a special thing that its doing that I'd expect to be specifically called out in its definition.
It doesn't really matter though, given we're not really adding new server objects.
erikjohnston
approved these changes
Apr 19, 2021
|
I suspect the test failures here are real. |
richvdh
force-pushed
the
rav/request_limit
branch
from
17e6ff5
to
b173324
Compare
|
I've rebased this on top of #9878, both to simplify the patches, and in the hope of making the tests pass. |
There's really no point in having `hs.start` pull out the listener config, so that it can be passed down into `_base.start`, and then back into `hs.start_listening`.
... because it turns out some replication requests are bigger than 1234 bytes.
richvdh
force-pushed
the
rav/request_limit
branch
from
68908d5
to
9849d60
Compare
|
FINALLY. The only change since erik's ✔️ are 9849d60 and a minor docstring tweak, so I'm going to take his approval and run with it. |
anoadragon453
added a commit
that referenced
this pull request
Apr 28, 2021
Synapse 1.33.0rc1 (2021-04-28) ============================== Features -------- - Update experimental support for [MSC3083](matrix-org/matrix-spec-proposals#3083): restricting room access via group membership. ([\#9800](#9800), [\#9814](#9814)) - Add experimental support for handling presence on a worker. ([\#9819](#9819), [\#9820](#9820), [\#9828](#9828), [\#9850](#9850)) - Return a new template when an user attempts to renew their account multiple times with the same token, stating that their account is set to expire. This replaces the invalid token template that would previously be shown in this case. This change concerns the optional account validity feature. ([\#9832](#9832)) Bugfixes -------- - Fixes the OIDC SSO flow when using a `public_baseurl` value including a non-root URL path. ([\#9726](#9726)) - Fix thumbnail generation for some sites with non-standard content types. Contributed by @rkfg. ([\#9788](#9788)) - Add some sanity checks to identity server passed to 3PID bind/unbind endpoints. ([\#9802](#9802)) - Limit the size of HTTP responses read over federation. ([\#9833](#9833)) - Fix a bug which could cause Synapse to get stuck in a loop of resyncing device lists. ([\#9867](#9867)) - Fix a long-standing bug where errors from federation did not propagate to the client. ([\#9868](#9868)) Improved Documentation ---------------------- - Add a note to the docker docs mentioning that we mirror upstream's supported Docker platforms. ([\#9801](#9801)) Internal Changes ---------------- - Add a dockerfile for running Synapse in worker-mode under Complement. ([\#9162](#9162)) - Apply `pyupgrade` across the codebase. ([\#9786](#9786)) - Move some replication processing out of `generic_worker`. ([\#9796](#9796)) - Replace `HomeServer.get_config()` with inline references. ([\#9815](#9815)) - Rename some handlers and config modules to not duplicate the top-level module. ([\#9816](#9816)) - Fix a long-standing bug which caused `max_upload_size` to not be correctly enforced. ([\#9817](#9817)) - Reduce CPU usage of the user directory by reusing existing calculated room membership. ([\#9821](#9821)) - Small speed up for joining large remote rooms. ([\#9825](#9825)) - Introduce flake8-bugbear to the test suite and fix some of its lint violations. ([\#9838](#9838)) - Only store the raw data in the in-memory caches, rather than objects that include references to e.g. the data stores. ([\#9845](#9845)) - Limit length of accepted email addresses. ([\#9855](#9855)) - Remove redundant `synapse.types.Collection` type definition. ([\#9856](#9856)) - Handle recently added rate limits correctly when using `--no-rate-limit` with the demo scripts. ([\#9858](#9858)) - Disable invite rate-limiting by default when running the unit tests. ([\#9871](#9871)) - Pass a reactor into `SynapseSite` to make testing easier. ([\#9874](#9874)) - Make `DomainSpecificString` an `attrs` class. ([\#9875](#9875)) - Add type hints to `synapse.api.auth` and `synapse.api.auth_blocking` modules. ([\#9876](#9876)) - Remove redundant `_PushHTTPChannel` test class. ([\#9878](#9878)) - Remove backwards-compatibility code for Python versions < 3.6. ([\#9879](#9879)) - Small performance improvement around handling new local presence updates. ([\#9887](#9887))
babolivier
added a commit
to matrix-org/synapse-dinsic
that referenced
this pull request
Sep 1, 2021
Synapse 1.33.0 (2021-05-05) =========================== Features -------- - Build Debian packages for Ubuntu 21.04 (Hirsute Hippo). ([\#9909](matrix-org/synapse#9909)) Synapse 1.33.0rc2 (2021-04-29) ============================== Bugfixes -------- - Fix tight loop when handling presence replication when using workers. Introduced in v1.33.0rc1. ([\#9900](matrix-org/synapse#9900)) Synapse 1.33.0rc1 (2021-04-28) ============================== Features -------- - Update experimental support for [MSC3083](matrix-org/matrix-spec-proposals#3083): restricting room access via group membership. ([\#9800](matrix-org/synapse#9800), [\#9814](matrix-org/synapse#9814)) - Add experimental support for handling presence on a worker. ([\#9819](matrix-org/synapse#9819), [\#9820](matrix-org/synapse#9820), [\#9828](matrix-org/synapse#9828), [\#9850](matrix-org/synapse#9850)) - Return a new template when an user attempts to renew their account multiple times with the same token, stating that their account is set to expire. This replaces the invalid token template that would previously be shown in this case. This change concerns the optional account validity feature. ([\#9832](matrix-org/synapse#9832)) Bugfixes -------- - Fixes the OIDC SSO flow when using a `public_baseurl` value including a non-root URL path. ([\#9726](matrix-org/synapse#9726)) - Fix thumbnail generation for some sites with non-standard content types. Contributed by @rkfg. ([\#9788](matrix-org/synapse#9788)) - Add some sanity checks to identity server passed to 3PID bind/unbind endpoints. ([\#9802](matrix-org/synapse#9802)) - Limit the size of HTTP responses read over federation. ([\#9833](matrix-org/synapse#9833)) - Fix a bug which could cause Synapse to get stuck in a loop of resyncing device lists. ([\#9867](matrix-org/synapse#9867)) - Fix a long-standing bug where errors from federation did not propagate to the client. ([\#9868](matrix-org/synapse#9868)) Improved Documentation ---------------------- - Add a note to the docker docs mentioning that we mirror upstream's supported Docker platforms. ([\#9801](matrix-org/synapse#9801)) Internal Changes ---------------- - Add a dockerfile for running Synapse in worker-mode under Complement. ([\#9162](matrix-org/synapse#9162)) - Apply `pyupgrade` across the codebase. ([\#9786](matrix-org/synapse#9786)) - Move some replication processing out of `generic_worker`. ([\#9796](matrix-org/synapse#9796)) - Replace `HomeServer.get_config()` with inline references. ([\#9815](matrix-org/synapse#9815)) - Rename some handlers and config modules to not duplicate the top-level module. ([\#9816](matrix-org/synapse#9816)) - Fix a long-standing bug which caused `max_upload_size` to not be correctly enforced. ([\#9817](matrix-org/synapse#9817)) - Reduce CPU usage of the user directory by reusing existing calculated room membership. ([\#9821](matrix-org/synapse#9821)) - Small speed up for joining large remote rooms. ([\#9825](matrix-org/synapse#9825)) - Introduce flake8-bugbear to the test suite and fix some of its lint violations. ([\#9838](matrix-org/synapse#9838)) - Only store the raw data in the in-memory caches, rather than objects that include references to e.g. the data stores. ([\#9845](matrix-org/synapse#9845)) - Limit length of accepted email addresses. ([\#9855](matrix-org/synapse#9855)) - Remove redundant `synapse.types.Collection` type definition. ([\#9856](matrix-org/synapse#9856)) - Handle recently added rate limits correctly when using `--no-rate-limit` with the demo scripts. ([\#9858](matrix-org/synapse#9858)) - Disable invite rate-limiting by default when running the unit tests. ([\#9871](matrix-org/synapse#9871)) - Pass a reactor into `SynapseSite` to make testing easier. ([\#9874](matrix-org/synapse#9874)) - Make `DomainSpecificString` an `attrs` class. ([\#9875](matrix-org/synapse#9875)) - Add type hints to `synapse.api.auth` and `synapse.api.auth_blocking` modules. ([\#9876](matrix-org/synapse#9876)) - Remove redundant `_PushHTTPChannel` test class. ([\#9878](matrix-org/synapse#9878)) - Remove backwards-compatibility code for Python versions < 3.6. ([\#9879](matrix-org/synapse#9879)) - Small performance improvement around handling new local presence updates. ([\#9887](matrix-org/synapse#9887))
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a couple of commits here. Hopefully each one is clear enough.
Fixes: #1610