From 495b214f4f8f45d16ffee851c8ab7a380dd0e2b2 Mon Sep 17 00:00:00 2001
From: Jonathan de Jong <jonathan@automatia.nl>
Date: Tue, 20 Apr 2021 12:50:49 +0200
Subject: Fix (final) Bugbear violations (#9838)

---
 tests/unittest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'tests/unittest.py')

diff --git a/tests/unittest.py b/tests/unittest.py
index d890ad981f..ee22a53849 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -133,7 +133,7 @@ class TestCase(unittest.TestCase):
     def assertObjectHasAttributes(self, attrs, obj):
         """Asserts that the given object has each of the attributes given, and
         that the value of each matches according to assertEquals."""
-        for (key, value) in attrs.items():
+        for key in attrs.keys():
             if not hasattr(obj, key):
                 raise AssertionError("Expected obj to have a '.%s'" % key)
             try:
-- 
cgit 1.5.1


From 59d24c5bef4e05fa7be0cad1f7e63f0a0097374b Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Fri, 23 Apr 2021 17:06:47 +0100
Subject: pass a reactor into SynapseSite (#9874)

---
 changelog.d/9874.misc         |  1 +
 synapse/app/generic_worker.py |  1 +
 synapse/app/homeserver.py     | 25 ++++++++++---------------
 synapse/http/site.py          | 37 ++++++++++++++++++++++++++++---------
 tests/replication/_base.py    |  1 +
 tests/test_server.py          |  1 +
 tests/unittest.py             |  1 +
 7 files changed, 43 insertions(+), 24 deletions(-)
 create mode 100644 changelog.d/9874.misc

(limited to 'tests/unittest.py')

diff --git a/changelog.d/9874.misc b/changelog.d/9874.misc
new file mode 100644
index 0000000000..ba1097e65e
--- /dev/null
+++ b/changelog.d/9874.misc
@@ -0,0 +1 @@
+Pass a reactor into `SynapseSite` to make testing easier.
diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py
index 7b2ac3ca64..70e07d0574 100644
--- a/synapse/app/generic_worker.py
+++ b/synapse/app/generic_worker.py
@@ -367,6 +367,7 @@ class GenericWorkerServer(HomeServer):
                 listener_config,
                 root_resource,
                 self.version_string,
+                reactor=self.get_reactor(),
             ),
             reactor=self.get_reactor(),
         )
diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py
index 8be8b520eb..140f6bcdee 100644
--- a/synapse/app/homeserver.py
+++ b/synapse/app/homeserver.py
@@ -126,19 +126,20 @@ class SynapseHomeServer(HomeServer):
         else:
             root_resource = OptionsResource()
 
-        root_resource = create_resource_tree(resources, root_resource)
+        site = SynapseSite(
+            "synapse.access.%s.%s" % ("https" if tls else "http", site_tag),
+            site_tag,
+            listener_config,
+            create_resource_tree(resources, root_resource),
+            self.version_string,
+            reactor=self.get_reactor(),
+        )
 
         if tls:
             ports = listen_ssl(
                 bind_addresses,
                 port,
-                SynapseSite(
-                    "synapse.access.https.%s" % (site_tag,),
-                    site_tag,
-                    listener_config,
-                    root_resource,
-                    self.version_string,
-                ),
+                site,
                 self.tls_server_context_factory,
                 reactor=self.get_reactor(),
             )
@@ -148,13 +149,7 @@ class SynapseHomeServer(HomeServer):
             ports = listen_tcp(
                 bind_addresses,
                 port,
-                SynapseSite(
-                    "synapse.access.http.%s" % (site_tag,),
-                    site_tag,
-                    listener_config,
-                    root_resource,
-                    self.version_string,
-                ),
+                site,
                 reactor=self.get_reactor(),
             )
             logger.info("Synapse now listening on TCP port %d", port)
diff --git a/synapse/http/site.py b/synapse/http/site.py
index 32b5e19c09..e911ee4809 100644
--- a/synapse/http/site.py
+++ b/synapse/http/site.py
@@ -19,8 +19,9 @@ from typing import Optional, Tuple, Type, Union
 import attr
 from zope.interface import implementer
 
-from twisted.internet.interfaces import IAddress
+from twisted.internet.interfaces import IAddress, IReactorTime
 from twisted.python.failure import Failure
+from twisted.web.resource import IResource
 from twisted.web.server import Request, Site
 
 from synapse.config.server import ListenerConfig
@@ -485,21 +486,39 @@ class _XForwardedForAddress:
 
 class SynapseSite(Site):
     """
-    Subclass of a twisted http Site that does access logging with python's
-    standard logging
+    Synapse-specific twisted http Site
+
+    This does two main things.
+
+    First, it replaces the requestFactory in use so that we build SynapseRequests
+    instead of regular t.w.server.Requests. All of the  constructor params are really
+    just parameters for SynapseRequest.
+
+    Second, it inhibits the log() method called by Request.finish, since SynapseRequest
+    does its own logging.
     """
 
     def __init__(
         self,
-        logger_name,
-        site_tag,
+        logger_name: str,
+        site_tag: str,
         config: ListenerConfig,
-        resource,
+        resource: IResource,
         server_version_string,
-        *args,
-        **kwargs,
+        reactor: IReactorTime,
     ):
-        Site.__init__(self, resource, *args, **kwargs)
+        """
+
+        Args:
+            logger_name:  The name of the logger to use for access logs.
+            site_tag:  A tag to use for this site - mostly in access logs.
+            config:  Configuration for the HTTP listener corresponding to this site
+            resource:  The base of the resource tree to be used for serving requests on
+                this site
+            server_version_string: A string to present for the Server header
+            reactor: reactor to be used to manage connection timeouts
+        """
+        Site.__init__(self, resource, reactor=reactor)
 
         self.site_tag = site_tag
 
diff --git a/tests/replication/_base.py b/tests/replication/_base.py
index c9d04aef29..5cf58d8b60 100644
--- a/tests/replication/_base.py
+++ b/tests/replication/_base.py
@@ -349,6 +349,7 @@ class BaseMultiWorkerStreamTestCase(unittest.HomeserverTestCase):
             config=worker_hs.config.server.listeners[0],
             resource=resource,
             server_version_string="1",
+            reactor=self.reactor,
         )
 
         if worker_hs.config.redis.redis_enabled:
diff --git a/tests/test_server.py b/tests/test_server.py
index 55cde7f62f..45400be367 100644
--- a/tests/test_server.py
+++ b/tests/test_server.py
@@ -202,6 +202,7 @@ class OptionsResourceTests(unittest.TestCase):
             parse_listener_def({"type": "http", "port": 0}),
             self.resource,
             "1.0",
+            reactor=self.reactor,
         )
 
         # render the request and return the channel
diff --git a/tests/unittest.py b/tests/unittest.py
index ee22a53849..5353e75c7c 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -247,6 +247,7 @@ class HomeserverTestCase(TestCase):
             config=self.hs.config.server.listeners[0],
             resource=self.resource,
             server_version_string="1",
+            reactor=self.reactor,
         )
 
         from tests.rest.client.v1.utils import RestHelper
-- 
cgit 1.5.1


From 3ff225175462dde8376aa584e3a47c43b1f0e790 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Fri, 23 Apr 2021 19:20:44 +0100
Subject: Improved validation for received requests (#9817)

* Simplify `start_listening` callpath

* Correctly check the size of uploaded files
---
 changelog.d/9817.misc                    |  1 +
 synapse/api/constants.py                 |  3 ++
 synapse/app/_base.py                     | 30 ++++++++++--
 synapse/app/admin_cmd.py                 |  8 +--
 synapse/app/generic_worker.py            | 11 +++--
 synapse/app/homeserver.py                | 17 +++++--
 synapse/config/logger.py                 |  3 +-
 synapse/event_auth.py                    |  4 +-
 synapse/http/site.py                     | 32 ++++++++++--
 synapse/rest/media/v1/upload_resource.py |  2 -
 synapse/server.py                        |  8 +++
 tests/http/test_site.py                  | 83 ++++++++++++++++++++++++++++++++
 tests/replication/_base.py               |  1 +
 tests/test_server.py                     |  1 +
 tests/unittest.py                        |  1 +
 15 files changed, 174 insertions(+), 31 deletions(-)
 create mode 100644 changelog.d/9817.misc
 create mode 100644 tests/http/test_site.py

(limited to 'tests/unittest.py')

diff --git a/changelog.d/9817.misc b/changelog.d/9817.misc
new file mode 100644
index 0000000000..8aa8895f05
--- /dev/null
+++ b/changelog.d/9817.misc
@@ -0,0 +1 @@
+Fix a long-standing bug which caused `max_upload_size` to not be correctly enforced.
diff --git a/synapse/api/constants.py b/synapse/api/constants.py
index 31a59bceec..936b6534b4 100644
--- a/synapse/api/constants.py
+++ b/synapse/api/constants.py
@@ -17,6 +17,9 @@
 
 """Contains constants from the specification."""
 
+# the max size of a (canonical-json-encoded) event
+MAX_PDU_SIZE = 65536
+
 # the "depth" field on events is limited to 2**63 - 1
 MAX_DEPTH = 2 ** 63 - 1
 
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index 2113c4f370..638e01c1b2 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -30,9 +30,10 @@ from twisted.internet import defer, error, reactor
 from twisted.protocols.tls import TLSMemoryBIOFactory
 
 import synapse
+from synapse.api.constants import MAX_PDU_SIZE
 from synapse.app import check_bind_error
 from synapse.app.phone_stats_home import start_phone_stats_home
-from synapse.config.server import ListenerConfig
+from synapse.config.homeserver import HomeServerConfig
 from synapse.crypto import context_factory
 from synapse.logging.context import PreserveLoggingContext
 from synapse.metrics.background_process_metrics import wrap_as_background_process
@@ -288,7 +289,7 @@ def refresh_certificate(hs):
         logger.info("Context factories updated.")
 
 
-async def start(hs: "synapse.server.HomeServer", listeners: Iterable[ListenerConfig]):
+async def start(hs: "synapse.server.HomeServer"):
     """
     Start a Synapse server or worker.
 
@@ -300,7 +301,6 @@ async def start(hs: "synapse.server.HomeServer", listeners: Iterable[ListenerCon
 
     Args:
         hs: homeserver instance
-        listeners: Listener configuration ('listeners' in homeserver.yaml)
     """
     # Set up the SIGHUP machinery.
     if hasattr(signal, "SIGHUP"):
@@ -336,7 +336,7 @@ async def start(hs: "synapse.server.HomeServer", listeners: Iterable[ListenerCon
     synapse.logging.opentracing.init_tracer(hs)  # type: ignore[attr-defined] # noqa
 
     # It is now safe to start your Synapse.
-    hs.start_listening(listeners)
+    hs.start_listening()
     hs.get_datastore().db_pool.start_profiling()
     hs.get_pusherpool().start()
 
@@ -530,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:
+    """Get a suitable maximum size for incoming HTTP requests"""
+
+    # Other than media uploads, the biggest request we expect to see is a fully-loaded
+    # /federation/v1/send request.
+    #
+    # The main thing in such a request is up to 50 PDUs, and up to 100 EDUs. PDUs are
+    # limited to 65536 bytes (possibly slightly more if the sender didn't use canonical
+    # json encoding); there is no specced limit to EDUs (see
+    # https://github.com/matrix-org/matrix-doc/issues/3121).
+    #
+    # in short, we somewhat arbitrarily limit requests to 200 * 64K (about 12.5M)
+    #
+    max_request_size = 200 * MAX_PDU_SIZE
+
+    # if we have a media repo enabled, we may need to allow larger uploads than that
+    if config.media.can_load_media_repo:
+        max_request_size = max(max_request_size, config.media.max_upload_size)
+
+    return max_request_size
diff --git a/synapse/app/admin_cmd.py b/synapse/app/admin_cmd.py
index eb256db749..68ae19c977 100644
--- a/synapse/app/admin_cmd.py
+++ b/synapse/app/admin_cmd.py
@@ -70,12 +70,6 @@ class AdminCmdSlavedStore(
 class AdminCmdServer(HomeServer):
     DATASTORE_CLASS = AdminCmdSlavedStore
 
-    def _listen_http(self, listener_config):
-        pass
-
-    def start_listening(self, listeners):
-        pass
-
 
 async def export_data_command(hs, args):
     """Export data for a user.
@@ -232,7 +226,7 @@ def start(config_options):
 
     async def run():
         with LoggingContext("command"):
-            _base.start(ss, [])
+            _base.start(ss)
             await args.func(ss, args)
 
     _base.start_worker_reactor(
diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py
index 70e07d0574..1a15ceee81 100644
--- a/synapse/app/generic_worker.py
+++ b/synapse/app/generic_worker.py
@@ -15,7 +15,7 @@
 # limitations under the License.
 import logging
 import sys
-from typing import Dict, Iterable, Optional
+from typing import Dict, Optional
 
 from twisted.internet import address
 from twisted.web.resource import IResource
@@ -32,7 +32,7 @@ from synapse.api.urls import (
     SERVER_KEY_V2_PREFIX,
 )
 from synapse.app import _base
-from synapse.app._base import register_start
+from synapse.app._base import max_request_body_size, register_start
 from synapse.config._base import ConfigError
 from synapse.config.homeserver import HomeServerConfig
 from synapse.config.logger import setup_logging
@@ -367,6 +367,7 @@ class GenericWorkerServer(HomeServer):
                 listener_config,
                 root_resource,
                 self.version_string,
+                max_request_body_size=max_request_body_size(self.config),
                 reactor=self.get_reactor(),
             ),
             reactor=self.get_reactor(),
@@ -374,8 +375,8 @@ class GenericWorkerServer(HomeServer):
 
         logger.info("Synapse worker now listening on port %d", port)
 
-    def start_listening(self, listeners: Iterable[ListenerConfig]):
-        for listener in listeners:
+    def start_listening(self):
+        for listener in self.config.worker_listeners:
             if listener.type == "http":
                 self._listen_http(listener)
             elif listener.type == "manhole":
@@ -468,7 +469,7 @@ def start(config_options):
     # streams. Will no-op if no streams can be written to by this worker.
     hs.get_replication_streamer()
 
-    register_start(_base.start, hs, config.worker_listeners)
+    register_start(_base.start, hs)
 
     _base.start_worker_reactor("synapse-generic-worker", config)
 
diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py
index 140f6bcdee..8e78134bbe 100644
--- a/synapse/app/homeserver.py
+++ b/synapse/app/homeserver.py
@@ -17,7 +17,7 @@
 import logging
 import os
 import sys
-from typing import Iterable, Iterator
+from typing import Iterator
 
 from twisted.internet import reactor
 from twisted.web.resource import EncodingResourceWrapper, IResource
@@ -36,7 +36,13 @@ from synapse.api.urls import (
     WEB_CLIENT_PREFIX,
 )
 from synapse.app import _base
-from synapse.app._base import listen_ssl, listen_tcp, quit_with_error, register_start
+from synapse.app._base import (
+    listen_ssl,
+    listen_tcp,
+    max_request_body_size,
+    quit_with_error,
+    register_start,
+)
 from synapse.config._base import ConfigError
 from synapse.config.emailconfig import ThreepidBehaviour
 from synapse.config.homeserver import HomeServerConfig
@@ -132,6 +138,7 @@ class SynapseHomeServer(HomeServer):
             listener_config,
             create_resource_tree(resources, root_resource),
             self.version_string,
+            max_request_body_size=max_request_body_size(self.config),
             reactor=self.get_reactor(),
         )
 
@@ -268,14 +275,14 @@ class SynapseHomeServer(HomeServer):
 
         return resources
 
-    def start_listening(self, listeners: Iterable[ListenerConfig]):
+    def start_listening(self):
         if self.config.redis_enabled:
             # If redis is enabled we connect via the replication command handler
             # in the same way as the workers (since we're effectively a client
             # rather than a server).
             self.get_tcp_replication().start_replication(self)
 
-        for listener in listeners:
+        for listener in self.config.server.listeners:
             if listener.type == "http":
                 self._listening_services.extend(
                     self._listener_http(self.config, listener)
@@ -407,7 +414,7 @@ def setup(config_options):
             # Loading the provider metadata also ensures the provider config is valid.
             await oidc.load_metadata()
 
-        await _base.start(hs, config.listeners)
+        await _base.start(hs)
 
         hs.get_datastore().db_pool.updates.start_doing_background_updates()
 
diff --git a/synapse/config/logger.py b/synapse/config/logger.py
index b174e0df6d..813076dfe2 100644
--- a/synapse/config/logger.py
+++ b/synapse/config/logger.py
@@ -31,7 +31,6 @@ from twisted.logger import (
 )
 
 import synapse
-from synapse.app import _base as appbase
 from synapse.logging._structured import setup_structured_logging
 from synapse.logging.context import LoggingContextFilter
 from synapse.logging.filter import MetadataFilter
@@ -318,6 +317,8 @@ def setup_logging(
     # Perform one-time logging configuration.
     _setup_stdlib_logging(config, log_config_path, logBeginner=logBeginner)
     # Add a SIGHUP handler to reload the logging configuration, if one is available.
+    from synapse.app import _base as appbase
+
     appbase.register_sighup(_reload_logging_config, log_config_path)
 
     # Log immediately so we can grep backwards.
diff --git a/synapse/event_auth.py b/synapse/event_auth.py
index afc2bc8267..70c556566e 100644
--- a/synapse/event_auth.py
+++ b/synapse/event_auth.py
@@ -21,7 +21,7 @@ from signedjson.key import decode_verify_key_bytes
 from signedjson.sign import SignatureVerifyException, verify_signed_json
 from unpaddedbase64 import decode_base64
 
-from synapse.api.constants import EventTypes, JoinRules, Membership
+from synapse.api.constants import MAX_PDU_SIZE, EventTypes, JoinRules, Membership
 from synapse.api.errors import AuthError, EventSizeError, SynapseError
 from synapse.api.room_versions import (
     KNOWN_ROOM_VERSIONS,
@@ -205,7 +205,7 @@ def _check_size_limits(event: EventBase) -> None:
         too_big("type")
     if len(event.event_id) > 255:
         too_big("event_id")
-    if len(encode_canonical_json(event.get_pdu_json())) > 65536:
+    if len(encode_canonical_json(event.get_pdu_json())) > MAX_PDU_SIZE:
         too_big("event")
 
 
diff --git a/synapse/http/site.py b/synapse/http/site.py
index e911ee4809..671fd3fbcc 100644
--- a/synapse/http/site.py
+++ b/synapse/http/site.py
@@ -14,7 +14,7 @@
 import contextlib
 import logging
 import time
-from typing import Optional, Tuple, Type, Union
+from typing import Optional, Tuple, Union
 
 import attr
 from zope.interface import implementer
@@ -50,6 +50,7 @@ class SynapseRequest(Request):
      * Redaction of access_token query-params in __repr__
      * Logging at start and end
      * Metrics to record CPU, wallclock and DB time by endpoint.
+     * A limit to the size of request which will be accepted
 
     It also provides a method `processing`, which returns a context manager. If this
     method is called, the request won't be logged until the context manager is closed;
@@ -60,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):
         Request.__init__(self, channel, *args, **kw)
+        self._max_request_body_size = max_request_body_size
         self.site = channel.site  # type: SynapseSite
         self._channel = channel  # this is used by the tests
         self.start_time = 0.0
@@ -98,6 +100,18 @@ class SynapseRequest(Request):
             self.site.site_tag,
         )
 
+    def handleContentChunk(self, data):
+        # we should have a `content` by now.
+        assert self.content, "handleContentChunk() called before gotLength()"
+        if self.content.tell() + len(data) > self._max_request_body_size:
+            logger.warning(
+                "Aborting connection from %s because the request exceeds maximum size",
+                self.client,
+            )
+            self.transport.abortConnection()
+            return
+        super().handleContentChunk(data)
+
     @property
     def requester(self) -> Optional[Union[Requester, str]]:
         return self._requester
@@ -505,6 +519,7 @@ class SynapseSite(Site):
         config: ListenerConfig,
         resource: IResource,
         server_version_string,
+        max_request_body_size: int,
         reactor: IReactorTime,
     ):
         """
@@ -516,6 +531,8 @@ class SynapseSite(Site):
             resource:  The base of the resource tree to be used for serving requests on
                 this site
             server_version_string: A string to present for the Server header
+            max_request_body_size: Maximum request body length to allow before
+                dropping the connection
             reactor: reactor to be used to manage connection timeouts
         """
         Site.__init__(self, resource, reactor=reactor)
@@ -524,9 +541,14 @@ class SynapseSite(Site):
 
         assert config.http_options is not None
         proxied = config.http_options.x_forwarded
-        self.requestFactory = (
-            XForwardedForRequest if proxied else SynapseRequest
-        )  # type: Type[Request]
+        request_class = XForwardedForRequest if proxied else SynapseRequest
+
+        def request_factory(channel, queued) -> Request:
+            return request_class(
+                channel, max_request_body_size=max_request_body_size, queued=queued
+            )
+
+        self.requestFactory = request_factory  # type: ignore
         self.access_logger = logging.getLogger(logger_name)
         self.server_version_string = server_version_string.encode("ascii")
 
diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py
index 80f017a4dd..024a105bf2 100644
--- a/synapse/rest/media/v1/upload_resource.py
+++ b/synapse/rest/media/v1/upload_resource.py
@@ -51,8 +51,6 @@ class UploadResource(DirectServeJsonResource):
 
     async def _async_render_POST(self, request: SynapseRequest) -> None:
         requester = await self.auth.get_user_by_req(request)
-        # TODO: The checks here are a bit late. The content will have
-        # already been uploaded to a tmp file at this point
         content_length = request.getHeader("Content-Length")
         if content_length is None:
             raise SynapseError(msg="Request must specify a Content-Length", code=400)
diff --git a/synapse/server.py b/synapse/server.py
index 8c147be2b3..06570bb1ce 100644
--- a/synapse/server.py
+++ b/synapse/server.py
@@ -287,6 +287,14 @@ class HomeServer(metaclass=abc.ABCMeta):
         if self.config.run_background_tasks:
             self.setup_background_tasks()
 
+    def start_listening(self) -> None:
+        """Start the HTTP, manhole, metrics, etc listeners
+
+        Does nothing in this base class; overridden in derived classes to start the
+        appropriate listeners.
+        """
+        pass
+
     def setup_background_tasks(self) -> None:
         """
         Some handlers have side effects on instantiation (like registering
diff --git a/tests/http/test_site.py b/tests/http/test_site.py
new file mode 100644
index 0000000000..8c13b4f693
--- /dev/null
+++ b/tests/http/test_site.py
@@ -0,0 +1,83 @@
+# Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from twisted.internet.address import IPv6Address
+from twisted.test.proto_helpers import StringTransport
+
+from synapse.app.homeserver import SynapseHomeServer
+
+from tests.unittest import HomeserverTestCase
+
+
+class SynapseRequestTestCase(HomeserverTestCase):
+    def make_homeserver(self, reactor, clock):
+        return self.setup_test_homeserver(homeserver_to_use=SynapseHomeServer)
+
+    def test_large_request(self):
+        """overlarge HTTP requests should be rejected"""
+        self.hs.start_listening()
+
+        # find the HTTP server which is configured to listen on port 0
+        (port, factory, _backlog, interface) = self.reactor.tcpServers[0]
+        self.assertEqual(interface, "::")
+        self.assertEqual(port, 0)
+
+        # as a control case, first send a regular request.
+
+        # complete the connection and wire it up to a fake transport
+        client_address = IPv6Address("TCP", "::1", "2345")
+        protocol = factory.buildProtocol(client_address)
+        transport = StringTransport()
+        protocol.makeConnection(transport)
+
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Transfer-Encoding: chunked\r\n"
+            b"\r\n"
+            b"0\r\n"
+            b"\r\n"
+        )
+
+        while not transport.disconnecting:
+            self.reactor.advance(1)
+
+        # we should get a 404
+        self.assertRegex(transport.value().decode(), r"^HTTP/1\.1 404 ")
+
+        # now send an oversized request
+        protocol = factory.buildProtocol(client_address)
+        transport = StringTransport()
+        protocol.makeConnection(transport)
+
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Transfer-Encoding: chunked\r\n"
+            b"\r\n"
+        )
+
+        # we deliberately send all the data in one big chunk, to ensure that
+        # twisted isn't buffering the data in the chunked transfer decoder.
+        # we start with the chunk size, in hex. (We won't actually send this much)
+        protocol.dataReceived(b"10000000\r\n")
+        sent = 0
+        while not transport.disconnected:
+            self.assertLess(sent, 0x10000000, "connection did not drop")
+            protocol.dataReceived(b"\0" * 1024)
+            sent += 1024
+
+        # default max upload size is 50M, so it should drop on the next buffer after
+        # that.
+        self.assertEqual(sent, 50 * 1024 * 1024 + 1024)
diff --git a/tests/replication/_base.py b/tests/replication/_base.py
index dc3519ea13..624bd1b927 100644
--- a/tests/replication/_base.py
+++ b/tests/replication/_base.py
@@ -359,6 +359,7 @@ class BaseMultiWorkerStreamTestCase(unittest.HomeserverTestCase):
             config=worker_hs.config.server.listeners[0],
             resource=resource,
             server_version_string="1",
+            max_request_body_size=4096,
             reactor=self.reactor,
         )
 
diff --git a/tests/test_server.py b/tests/test_server.py
index 45400be367..407e172e41 100644
--- a/tests/test_server.py
+++ b/tests/test_server.py
@@ -202,6 +202,7 @@ class OptionsResourceTests(unittest.TestCase):
             parse_listener_def({"type": "http", "port": 0}),
             self.resource,
             "1.0",
+            max_request_body_size=1234,
             reactor=self.reactor,
         )
 
diff --git a/tests/unittest.py b/tests/unittest.py
index 5353e75c7c..9bd02bd9c4 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -247,6 +247,7 @@ class HomeserverTestCase(TestCase):
             config=self.hs.config.server.listeners[0],
             resource=self.resource,
             server_version_string="1",
+            max_request_body_size=1234,
             reactor=self.reactor,
         )
 
-- 
cgit 1.5.1


From fe604a022a7142157da7e90a40330beb2a11af7a Mon Sep 17 00:00:00 2001
From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Date: Tue, 27 Apr 2021 13:13:07 +0100
Subject: Remove various bits of compatibility code for Python <3.6 (#9879)

I went through and removed a bunch of cruft that was lying around for compatibility with old Python versions. This PR also will now prevent Synapse from starting unless you're running Python 3.6+.
---
 changelog.d/9879.misc                    |  1 +
 mypy.ini                                 |  1 -
 synapse/__init__.py                      |  4 +--
 synapse/python_dependencies.py           |  9 ++-----
 synapse/rest/admin/users.py              |  3 ++-
 synapse/rest/consent/consent_resource.py | 10 +-------
 synapse/rest/media/v1/filepath.py        |  2 +-
 synapse/secrets.py                       | 44 --------------------------------
 synapse/server.py                        |  5 ----
 synapse/storage/_base.py                 |  2 +-
 synapse/storage/database.py              | 15 +++++------
 synapse/util/caches/response_cache.py    |  2 +-
 tests/rest/admin/test_user.py            | 15 +++++------
 tests/storage/test__base.py              |  3 ++-
 tests/unittest.py                        |  2 +-
 tox.ini                                  |  9 +++----
 16 files changed, 29 insertions(+), 98 deletions(-)
 create mode 100644 changelog.d/9879.misc
 delete mode 100644 synapse/secrets.py

(limited to 'tests/unittest.py')

diff --git a/changelog.d/9879.misc b/changelog.d/9879.misc
new file mode 100644
index 0000000000..c9ca37cf48
--- /dev/null
+++ b/changelog.d/9879.misc
@@ -0,0 +1 @@
+Remove backwards-compatibility code for Python versions < 3.6.
\ No newline at end of file
diff --git a/mypy.ini b/mypy.ini
index 32e6197409..a40f705b76 100644
--- a/mypy.ini
+++ b/mypy.ini
@@ -41,7 +41,6 @@ files =
   synapse/push,
   synapse/replication,
   synapse/rest,
-  synapse/secrets.py,
   synapse/server.py,
   synapse/server_notices,
   synapse/spam_checker_api,
diff --git a/synapse/__init__.py b/synapse/__init__.py
index 837e938f56..fbd49a93e1 100644
--- a/synapse/__init__.py
+++ b/synapse/__init__.py
@@ -21,8 +21,8 @@ import os
 import sys
 
 # Check that we're not running on an unsupported Python version.
-if sys.version_info < (3, 5):
-    print("Synapse requires Python 3.5 or above.")
+if sys.version_info < (3, 6):
+    print("Synapse requires Python 3.6 or above.")
     sys.exit(1)
 
 # Twisted and canonicaljson will fail to import when this file is executed to
diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py
index 2a1c925ee8..2de946f464 100644
--- a/synapse/python_dependencies.py
+++ b/synapse/python_dependencies.py
@@ -85,7 +85,7 @@ REQUIREMENTS = [
     "typing-extensions>=3.7.4",
     # We enforce that we have a `cryptography` version that bundles an `openssl`
     # with the latest security patches.
-    "cryptography>=3.4.7;python_version>='3.6'",
+    "cryptography>=3.4.7",
 ]
 
 CONDITIONAL_REQUIREMENTS = {
@@ -100,14 +100,9 @@ CONDITIONAL_REQUIREMENTS = {
     # that use the protocol, such as Let's Encrypt.
     "acme": [
         "txacme>=0.9.2",
-        # txacme depends on eliot. Eliot 1.8.0 is incompatible with
-        # python 3.5.2, as per https://github.com/itamarst/eliot/issues/418
-        "eliot<1.8.0;python_version<'3.5.3'",
     ],
     "saml2": [
-        # pysaml2 6.4.0 is incompatible with Python 3.5 (see https://github.com/IdentityPython/pysaml2/issues/749)
-        "pysaml2>=4.5.0,<6.4.0;python_version<'3.6'",
-        "pysaml2>=4.5.0;python_version>='3.6'",
+        "pysaml2>=4.5.0",
     ],
     "oidc": ["authlib>=0.14.0"],
     # systemd-python is necessary for logging to the systemd journal via
diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index edda7861fa..8c9d21d3ea 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -14,6 +14,7 @@
 import hashlib
 import hmac
 import logging
+import secrets
 from http import HTTPStatus
 from typing import TYPE_CHECKING, Dict, List, Optional, Tuple
 
@@ -375,7 +376,7 @@ class UserRegisterServlet(RestServlet):
         """
         self._clear_old_nonces()
 
-        nonce = self.hs.get_secrets().token_hex(64)
+        nonce = secrets.token_hex(64)
         self.nonces[nonce] = int(self.reactor.seconds())
         return 200, {"nonce": nonce}
 
diff --git a/synapse/rest/consent/consent_resource.py b/synapse/rest/consent/consent_resource.py
index c4550d3cf0..b19cd8afc5 100644
--- a/synapse/rest/consent/consent_resource.py
+++ b/synapse/rest/consent/consent_resource.py
@@ -32,14 +32,6 @@ TEMPLATE_LANGUAGE = "en"
 
 logger = logging.getLogger(__name__)
 
-# use hmac.compare_digest if we have it (python 2.7.7), else just use equality
-if hasattr(hmac, "compare_digest"):
-    compare_digest = hmac.compare_digest
-else:
-
-    def compare_digest(a, b):
-        return a == b
-
 
 class ConsentResource(DirectServeHtmlResource):
     """A twisted Resource to display a privacy policy and gather consent to it
@@ -209,5 +201,5 @@ class ConsentResource(DirectServeHtmlResource):
             .encode("ascii")
         )
 
-        if not compare_digest(want_mac, userhmac):
+        if not hmac.compare_digest(want_mac, userhmac):
             raise SynapseError(HTTPStatus.FORBIDDEN, "HMAC incorrect")
diff --git a/synapse/rest/media/v1/filepath.py b/synapse/rest/media/v1/filepath.py
index 4088e7a059..09531ebf54 100644
--- a/synapse/rest/media/v1/filepath.py
+++ b/synapse/rest/media/v1/filepath.py
@@ -21,7 +21,7 @@ from typing import Callable, List
 NEW_FORMAT_ID_RE = re.compile(r"^\d\d\d\d-\d\d-\d\d")
 
 
-def _wrap_in_base_path(func: "Callable[..., str]") -> "Callable[..., str]":
+def _wrap_in_base_path(func: Callable[..., str]) -> Callable[..., str]:
     """Takes a function that returns a relative path and turns it into an
     absolute path based on the location of the primary media store
     """
diff --git a/synapse/secrets.py b/synapse/secrets.py
deleted file mode 100644
index bf829251fd..0000000000
--- a/synapse/secrets.py
+++ /dev/null
@@ -1,44 +0,0 @@
-# Copyright 2018 New Vector Ltd
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-"""
-Injectable secrets module for Synapse.
-
-See https://docs.python.org/3/library/secrets.html#module-secrets for the API
-used in Python 3.6, and the API emulated in Python 2.7.
-"""
-import sys
-
-# secrets is available since python 3.6
-if sys.version_info[0:2] >= (3, 6):
-    import secrets
-
-    class Secrets:
-        def token_bytes(self, nbytes: int = 32) -> bytes:
-            return secrets.token_bytes(nbytes)
-
-        def token_hex(self, nbytes: int = 32) -> str:
-            return secrets.token_hex(nbytes)
-
-
-else:
-    import binascii
-    import os
-
-    class Secrets:
-        def token_bytes(self, nbytes: int = 32) -> bytes:
-            return os.urandom(nbytes)
-
-        def token_hex(self, nbytes: int = 32) -> str:
-            return binascii.hexlify(self.token_bytes(nbytes)).decode("ascii")
diff --git a/synapse/server.py b/synapse/server.py
index 06570bb1ce..2337d2d9b4 100644
--- a/synapse/server.py
+++ b/synapse/server.py
@@ -126,7 +126,6 @@ from synapse.rest.media.v1.media_repository import (
     MediaRepository,
     MediaRepositoryResource,
 )
-from synapse.secrets import Secrets
 from synapse.server_notices.server_notices_manager import ServerNoticesManager
 from synapse.server_notices.server_notices_sender import ServerNoticesSender
 from synapse.server_notices.worker_server_notices_sender import (
@@ -641,10 +640,6 @@ class HomeServer(metaclass=abc.ABCMeta):
     def get_groups_attestation_renewer(self) -> GroupAttestionRenewer:
         return GroupAttestionRenewer(self)
 
-    @cache_in_self
-    def get_secrets(self) -> Secrets:
-        return Secrets()
-
     @cache_in_self
     def get_stats_handler(self) -> StatsHandler:
         return StatsHandler(self)
diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index d472676acf..6b68d8720c 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -114,7 +114,7 @@ def db_to_json(db_content: Union[memoryview, bytes, bytearray, str]) -> Any:
         db_content = db_content.tobytes()
 
     # Decode it to a Unicode string before feeding it to the JSON decoder, since
-    # Python 3.5 does not support deserializing bytes.
+    # it only supports handling strings
     if isinstance(db_content, (bytes, bytearray)):
         db_content = db_content.decode("utf8")
 
diff --git a/synapse/storage/database.py b/synapse/storage/database.py
index 9452368bf0..bd39c095af 100644
--- a/synapse/storage/database.py
+++ b/synapse/storage/database.py
@@ -171,10 +171,7 @@ class LoggingDatabaseConnection:
 
 
 # The type of entry which goes on our after_callbacks and exception_callbacks lists.
-#
-# Python 3.5.2 doesn't support Callable with an ellipsis, so we wrap it in quotes so
-# that mypy sees the type but the runtime python doesn't.
-_CallbackListEntry = Tuple["Callable[..., None]", Iterable[Any], Dict[str, Any]]
+_CallbackListEntry = Tuple[Callable[..., None], Iterable[Any], Dict[str, Any]]
 
 
 R = TypeVar("R")
@@ -221,7 +218,7 @@ class LoggingTransaction:
         self.after_callbacks = after_callbacks
         self.exception_callbacks = exception_callbacks
 
-    def call_after(self, callback: "Callable[..., None]", *args: Any, **kwargs: Any):
+    def call_after(self, callback: Callable[..., None], *args: Any, **kwargs: Any):
         """Call the given callback on the main twisted thread after the
         transaction has finished. Used to invalidate the caches on the
         correct thread.
@@ -233,7 +230,7 @@ class LoggingTransaction:
         self.after_callbacks.append((callback, args, kwargs))
 
     def call_on_exception(
-        self, callback: "Callable[..., None]", *args: Any, **kwargs: Any
+        self, callback: Callable[..., None], *args: Any, **kwargs: Any
     ):
         # if self.exception_callbacks is None, that means that whatever constructed the
         # LoggingTransaction isn't expecting there to be any callbacks; assert that
@@ -485,7 +482,7 @@ class DatabasePool:
         desc: str,
         after_callbacks: List[_CallbackListEntry],
         exception_callbacks: List[_CallbackListEntry],
-        func: "Callable[..., R]",
+        func: Callable[..., R],
         *args: Any,
         **kwargs: Any,
     ) -> R:
@@ -618,7 +615,7 @@ class DatabasePool:
     async def runInteraction(
         self,
         desc: str,
-        func: "Callable[..., R]",
+        func: Callable[..., R],
         *args: Any,
         db_autocommit: bool = False,
         **kwargs: Any,
@@ -678,7 +675,7 @@ class DatabasePool:
 
     async def runWithConnection(
         self,
-        func: "Callable[..., R]",
+        func: Callable[..., R],
         *args: Any,
         db_autocommit: bool = False,
         **kwargs: Any,
diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py
index 2529845c9e..25ea1bcc91 100644
--- a/synapse/util/caches/response_cache.py
+++ b/synapse/util/caches/response_cache.py
@@ -110,7 +110,7 @@ class ResponseCache(Generic[T]):
         return result.observe()
 
     def wrap(
-        self, key: T, callback: "Callable[..., Any]", *args: Any, **kwargs: Any
+        self, key: T, callback: Callable[..., Any], *args: Any, **kwargs: Any
     ) -> defer.Deferred:
         """Wrap together a *get* and *set* call, taking care of logcontexts
 
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index b3afd51522..d599a4c984 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -18,7 +18,7 @@ import json
 import urllib.parse
 from binascii import unhexlify
 from typing import List, Optional
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
 
 import synapse.rest.admin
 from synapse.api.constants import UserTypes
@@ -54,8 +54,6 @@ class UserRegisterTestCase(unittest.HomeserverTestCase):
         self.datastore = Mock(return_value=Mock())
         self.datastore.get_current_state_deltas = Mock(return_value=(0, []))
 
-        self.secrets = Mock()
-
         self.hs = self.setup_test_homeserver()
 
         self.hs.config.registration_shared_secret = "shared"
@@ -84,14 +82,13 @@ class UserRegisterTestCase(unittest.HomeserverTestCase):
         Calling GET on the endpoint will return a randomised nonce, using the
         homeserver's secrets provider.
         """
-        secrets = Mock()
-        secrets.token_hex = Mock(return_value="abcd")
-
-        self.hs.get_secrets = Mock(return_value=secrets)
+        with patch("secrets.token_hex") as token_hex:
+            # Patch secrets.token_hex for the duration of this context
+            token_hex.return_value = "abcd"
 
-        channel = self.make_request("GET", self.url)
+            channel = self.make_request("GET", self.url)
 
-        self.assertEqual(channel.json_body, {"nonce": "abcd"})
+            self.assertEqual(channel.json_body, {"nonce": "abcd"})
 
     def test_expired_nonce(self):
         """
diff --git a/tests/storage/test__base.py b/tests/storage/test__base.py
index 6339a43f0c..200b9198f9 100644
--- a/tests/storage/test__base.py
+++ b/tests/storage/test__base.py
@@ -13,6 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import secrets
 
 from tests import unittest
 
@@ -21,7 +22,7 @@ class UpsertManyTests(unittest.HomeserverTestCase):
     def prepare(self, reactor, clock, hs):
         self.storage = hs.get_datastore()
 
-        self.table_name = "table_" + hs.get_secrets().token_hex(6)
+        self.table_name = "table_" + secrets.token_hex(6)
         self.get_success(
             self.storage.db_pool.runInteraction(
                 "create",
diff --git a/tests/unittest.py b/tests/unittest.py
index 9bd02bd9c4..74db7c08f1 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -18,6 +18,7 @@ import hashlib
 import hmac
 import inspect
 import logging
+import secrets
 import time
 from typing import Callable, Dict, Iterable, Optional, Tuple, Type, TypeVar, Union
 from unittest.mock import Mock, patch
@@ -626,7 +627,6 @@ class HomeserverTestCase(TestCase):
             str: The new event's ID.
         """
         event_creator = self.hs.get_event_creation_handler()
-        secrets = self.hs.get_secrets()
         requester = create_requester(user)
 
         event, context = self.get_success(
diff --git a/tox.ini b/tox.ini
index 998b04b224..ecd609271d 100644
--- a/tox.ini
+++ b/tox.ini
@@ -21,13 +21,11 @@ deps =
     # installed on that).
     #
     # anyway, make sure that we have a recent enough setuptools.
-    setuptools>=18.5 ; python_version >= '3.6'
-    setuptools>=18.5,<51.0.0 ; python_version < '3.6'
+    setuptools>=18.5
 
     # we also need a semi-recent version of pip, because old ones fail to
     # install the "enum34" dependency of cryptography.
-    pip>=10 ; python_version >= '3.6'
-    pip>=10,<21.0 ; python_version < '3.6'
+    pip>=10
 
 # directories/files we run the linters on.
 # if you update this list, make sure to do the same in scripts-dev/lint.sh
@@ -168,8 +166,7 @@ skip_install = true
 usedevelop = false
 deps =
     coverage
-    pip>=10 ; python_version >= '3.6'
-    pip>=10,<21.0 ; python_version < '3.6'
+    pip>=10
 commands=
     coverage combine
     coverage report
-- 
cgit 1.5.1