Repository: matrix-org/synapse

Issue:
Doc: Make clear that sharing registration_shared_secret is dangerous
In the config file it reads about `registration_shared_secret`:
> If set, allows registration by anyone who also has the shared secret, even if registration is otherwise disabled.

Especially the last part implies that this is just a secret one can share to users, who want to create an account. It then enables the registration, but only for the user, who has this secret. Also sounds very convenient.

What you do not mention there is one big fact: With that secret, you can **register admin accounts**!
That means that this secret is much more dangerous than one expects from reading the config description! (I also thought it is harmless.)

And, BTW, I am not the only one, who had this idea: https://github.com/vector-im/riot-web/issues/2090

So better adjust the text there, to specifically point that out! (and make sure that no admin shares it)

Discussion:

Patch:
diff --git a/INSTALL.md b/INSTALL.md
index 2993f3a9e..5c67f14ed 100644
--- a/INSTALL.md
+++ b/INSTALL.md
@@ -402,8 +402,8 @@ This process uses a setting `registration_shared_secret` in
 `homeserver.yaml`, which is shared between Synapse itself and the
 `register_new_matrix_user` script. It doesn't matter what it is (a random
 value is generated by `--generate-config`), but it should be kept secret, as
-anyone with knowledge of it can register users on your server even if
-`enable_registration` is `false`.
+anyone with knowledge of it can register users, including admin accounts,
+on your server even if `enable_registration` is `false`.
 
 ## Setting up a TURN server
 
diff --git a/changelog.d/2090.bugfix b/changelog.d/2090.bugfix
new file mode 100644
index 000000000..de2d22fcb
--- /dev/null
+++ b/changelog.d/2090.bugfix
@@ -0,0 +1,1 @@
+Fix a bug where media with spaces in the name would get a corrupted name.
diff --git a/changelog.d/4793.feature b/changelog.d/4793.feature
new file mode 100644
index 000000000..90dba7d12
--- /dev/null
+++ b/changelog.d/4793.feature
@@ -0,0 +1,1 @@
+Synapse is now permissive about trailing slashes on some of its federation endpoints, allowing zero or more to be present.
\ No newline at end of file
diff --git a/changelog.d/4837.bugfix b/changelog.d/4837.bugfix
new file mode 100644
index 000000000..989aeb82b
--- /dev/null
+++ b/changelog.d/4837.bugfix
@@ -0,0 +1,1 @@
+Fix bug where synapse expected an un-specced `prev_state` field on state events.
diff --git a/changelog.d/4839.misc b/changelog.d/4839.misc
new file mode 100644
index 000000000..7c6868051
--- /dev/null
+++ b/changelog.d/4839.misc
@@ -0,0 +1,1 @@
+Disable captcha registration by default in unit tests.
\ No newline at end of file
diff --git a/changelog.d/4844.misc b/changelog.d/4844.misc
new file mode 100644
index 000000000..eff6f1c43
--- /dev/null
+++ b/changelog.d/4844.misc
@@ -0,0 +1,1 @@
+Clarify what registration_shared_secret allows for.
diff --git a/changelog.d/4847.misc b/changelog.d/4847.misc
new file mode 100644
index 000000000..a001238e0
--- /dev/null
+++ b/changelog.d/4847.misc
@@ -0,0 +1,1 @@
+Correctly log expected errors when fetching server keys.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index b62745dd6..22d5e6b1d 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -624,8 +624,8 @@ enable_registration: False
 #  - medium: msisdn
 #    pattern: '\+44'
 
-# If set, allows registration by anyone who also has the shared
-# secret, even if registration is otherwise disabled.
+# If set, allows registration of standard or admin accounts by anyone who
+# has the shared secret, even if registration is otherwise disabled.
 #
 # registration_shared_secret: <PRIVATE STRING>
 
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index d34dc9e45..a123f25a6 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -92,8 +92,8 @@ class RegistrationConfig(Config):
         #  - medium: msisdn
         #    pattern: '\\+44'
 
-        # If set, allows registration by anyone who also has the shared
-        # secret, even if registration is otherwise disabled.
+        # If set, allows registration of standard or admin accounts by anyone who
+        # has the shared secret, even if registration is otherwise disabled.
         #
         %(registration_shared_secret)s
 
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index 7474fd515..0207cd989 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -686,9 +686,9 @@ def _handle_key_deferred(verify_request):
     try:
         with PreserveLoggingContext():
             _, key_id, verify_key = yield verify_request.deferred
-    except (IOError, RequestSendFailed) as e:
+    except KeyLookupError as e:
         logger.warn(
-            "Got IOError when downloading keys for %s: %s %s",
+            "Failed to download keys for %s: %s %s",
             server_name, type(e).__name__, str(e),
         )
         raise SynapseError(
diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py
index bd130f881..fafa13518 100644
--- a/synapse/events/__init__.py
+++ b/synapse/events/__init__.py
@@ -141,7 +141,6 @@ class EventBase(object):
     origin = _event_dict_property("origin")
     origin_server_ts = _event_dict_property("origin_server_ts")
     prev_events = _event_dict_property("prev_events")
-    prev_state = _event_dict_property("prev_state")
     redacts = _event_dict_property("redacts")
     room_id = _event_dict_property("room_id")
     sender = _event_dict_property("sender")
diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py
index 8e2be218e..4e8919d65 100644
--- a/synapse/federation/transport/client.py
+++ b/synapse/federation/transport/client.py
@@ -167,7 +167,7 @@ class TransportLayerClient(object):
         # generated by the json_data_callback.
         json_data = transaction.get_dict()
 
-        path = _create_v1_path("/send/%s/", transaction.transaction_id)
+        path = _create_v1_path("/send/%s", transaction.transaction_id)
 
         response = yield self.client.put_json(
             transaction.destination,
diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py
index 96d680a5a..efb6bdca4 100644
--- a/synapse/federation/transport/server.py
+++ b/synapse/federation/transport/server.py
@@ -312,7 +312,7 @@ class BaseFederationServlet(object):
 
 
 class FederationSendServlet(BaseFederationServlet):
-    PATH = "/send/(?P<transaction_id>[^/]*)/"
+    PATH = "/send/(?P<transaction_id>[^/]*)/?"
 
     def __init__(self, handler, server_name, **kwargs):
         super(FederationSendServlet, self).__init__(
@@ -378,7 +378,7 @@ class FederationSendServlet(BaseFederationServlet):
 
 
 class FederationEventServlet(BaseFederationServlet):
-    PATH = "/event/(?P<event_id>[^/]*)/"
+    PATH = "/event/(?P<event_id>[^/]*)/?"
 
     # This is when someone asks for a data item for a given server data_id pair.
     def on_GET(self, origin, content, query, event_id):
@@ -386,7 +386,7 @@ class FederationEventServlet(BaseFederationServlet):
 
 
 class FederationStateServlet(BaseFederationServlet):
-    PATH = "/state/(?P<context>[^/]*)/"
+    PATH = "/state/(?P<context>[^/]*)/?"
 
     # This is when someone asks for all data for a given context.
     def on_GET(self, origin, content, query, context):
@@ -398,7 +398,7 @@ class FederationStateServlet(BaseFederationServlet):
 
 
 class FederationStateIdsServlet(BaseFederationServlet):
-    PATH = "/state_ids/(?P<room_id>[^/]*)/"
+    PATH = "/state_ids/(?P<room_id>[^/]*)/?"
 
     def on_GET(self, origin, content, query, room_id):
         return self.handler.on_state_ids_request(
@@ -409,7 +409,7 @@ class FederationStateIdsServlet(BaseFederationServlet):
 
 
 class FederationBackfillServlet(BaseFederationServlet):
-    PATH = "/backfill/(?P<context>[^/]*)/"
+    PATH = "/backfill/(?P<context>[^/]*)/?"
 
     def on_GET(self, origin, content, query, context):
         versions = [x.decode('ascii') for x in query[b"v"]]
@@ -1080,7 +1080,7 @@ class FederationGroupsCategoriesServlet(BaseFederationServlet):
     """Get all categories for a group
     """
     PATH = (
-        "/groups/(?P<group_id>[^/]*)/categories/"
+        "/groups/(?P<group_id>[^/]*)/categories/?"
     )
 
     @defer.inlineCallbacks
@@ -1150,7 +1150,7 @@ class FederationGroupsRolesServlet(BaseFederationServlet):
     """Get roles in a group
     """
     PATH = (
-        "/groups/(?P<group_id>[^/]*)/roles/"
+        "/groups/(?P<group_id>[^/]*)/roles/?"
     )
 
     @defer.inlineCallbacks
diff --git a/synapse/rest/media/v1/_base.py b/synapse/rest/media/v1/_base.py
index fece1ef0b..953d89bd8 100644
--- a/synapse/rest/media/v1/_base.py
+++ b/synapse/rest/media/v1/_base.py
@@ -100,10 +100,29 @@ def add_file_headers(request, media_type, file_size, upload_name):
 
     request.setHeader(b"Content-Type", media_type.encode("UTF-8"))
     if upload_name:
-        if is_ascii(upload_name):
-            disposition = "inline; filename=%s" % (_quote(upload_name),)
+        # RFC6266 section 4.1 [1] defines both `filename` and `filename*`.
+        #
+        # `filename` is defined to be a `value`, which is defined by RFC2616
+        # section 3.6 [2] to be a `token` or a `quoted-string`, where a `token`
+        # is (essentially) a single US-ASCII word, and a `quoted-string` is a
+        # US-ASCII string surrounded by double-quotes, using backslash as an
+        # escape charater. Note that %-encoding is *not* permitted.
+        #
+        # `filename*` is defined to be an `ext-value`, which is defined in
+        # RFC5987 section 3.2.1 [3] to be `charset "'" [ language ] "'" value-chars`,
+        # where `value-chars` is essentially a %-encoded string in the given charset.
+        #
+        # [1]: https://tools.ietf.org/html/rfc6266#section-4.1
+        # [2]: https://tools.ietf.org/html/rfc2616#section-3.6
+        # [3]: https://tools.ietf.org/html/rfc5987#section-3.2.1
+
+        # We avoid the quoted-string version of `filename`, because (a) synapse didn't
+        # correctly interpret those as of 0.99.2 and (b) they are a bit of a pain and we
+        # may as well just do the filename* version.
+        if _can_encode_filename_as_token(upload_name):
+            disposition = 'inline; filename=%s' % (upload_name, )
         else:
-            disposition = "inline; filename*=utf-8''%s" % (_quote(upload_name),)
+            disposition = "inline; filename*=utf-8''%s" % (_quote(upload_name), )
 
         request.setHeader(b"Content-Disposition", disposition.encode('ascii'))
 
@@ -116,6 +135,35 @@ def add_file_headers(request, media_type, file_size, upload_name):
     request.setHeader(b"Content-Length", b"%d" % (file_size,))
 
 
+# separators as defined in RFC2616. SP and HT are handled separately.
+# see _can_encode_filename_as_token.
+_FILENAME_SEPARATOR_CHARS = set((
+    "(", ")", "<", ">", "@", ",", ";", ":", "\\", '"',
+    "/", "[", "]", "?", "=", "{", "}",
+))
+
+
+def _can_encode_filename_as_token(x):
+    for c in x:
+        # from RFC2616:
+        #
+        #        token          = 1*<any CHAR except CTLs or separators>
+        #
+        #        separators     = "(" | ")" | "<" | ">" | "@"
+        #                       | "," | ";" | ":" | "\" | <">
+        #                       | "/" | "[" | "]" | "?" | "="
+        #                       | "{" | "}" | SP | HT
+        #
+        #        CHAR           = <any US-ASCII character (octets 0 - 127)>
+        #
+        #        CTL            = <any US-ASCII control character
+        #                         (octets 0 - 31) and DEL (127)>
+        #
+        if ord(c) >= 127 or ord(c) <= 32 or c in _FILENAME_SEPARATOR_CHARS:
+            return False
+    return True
+
+
 @defer.inlineCallbacks
 def respond_with_responder(request, responder, media_type, file_size, upload_name=None):
     """Responds to the request with given responder. If responder is None then
diff --git a/synapse/storage/events.py b/synapse/storage/events.py
index 990a5eaaa..428300ea0 100644
--- a/synapse/storage/events.py
+++ b/synapse/storage/events.py
@@ -1407,21 +1407,6 @@ class EventsStore(StateGroupWorkerStore, EventFederationStore, EventsWorkerStore
             values=state_values,
         )
 
-        self._simple_insert_many_txn(
-            txn,
-            table="event_edges",
-            values=[
-                {
-                    "event_id": event.event_id,
-                    "prev_event_id": prev_id,
-                    "room_id": event.room_id,
-                    "is_state": True,
-                }
-                for event, _ in state_events_and_contexts
-                for prev_id, _ in event.prev_state
-            ],
-        )
-
         # Prefill the event cache
         self._add_to_cache(txn, events_and_contexts)
 
diff --git a/synapse/storage/schema/full_schemas/11/event_edges.sql b/synapse/storage/schema/full_schemas/11/event_edges.sql
index 52eec8835..bccd1c6f7 100644
--- a/synapse/storage/schema/full_schemas/11/event_edges.sql
+++ b/synapse/storage/schema/full_schemas/11/event_edges.sql
@@ -37,6 +37,8 @@ CREATE TABLE IF NOT EXISTS event_edges(
     event_id TEXT NOT NULL,
     prev_event_id TEXT NOT NULL,
     room_id TEXT NOT NULL,
+    -- We no longer insert prev_state into this table, so all new rows will have
+    -- is_state as false.
     is_state BOOL NOT NULL,
     UNIQUE (event_id, prev_event_id, room_id, is_state)
 );


Test patch:
diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py
index 13486930f..b8e97390d 100644
--- a/tests/handlers/test_typing.py
+++ b/tests/handlers/test_typing.py
@@ -180,7 +180,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
         put_json = self.hs.get_http_client().put_json
         put_json.assert_called_once_with(
             "farm",
-            path="/_matrix/federation/v1/send/1000000/",
+            path="/_matrix/federation/v1/send/1000000",
             data=_expect_edu_transaction(
                 "m.typing",
                 content={
@@ -201,7 +201,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
 
         (request, channel) = self.make_request(
             "PUT",
-            "/_matrix/federation/v1/send/1000000/",
+            "/_matrix/federation/v1/send/1000000",
             _make_edu_transaction_json(
                 "m.typing",
                 content={
@@ -257,7 +257,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
         put_json = self.hs.get_http_client().put_json
         put_json.assert_called_once_with(
             "farm",
-            path="/_matrix/federation/v1/send/1000000/",
+            path="/_matrix/federation/v1/send/1000000",
             data=_expect_edu_transaction(
                 "m.typing",
                 content={
diff --git a/tests/utils.py b/tests/utils.py
index e4c42f9fa..9c8dc9dbc 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -115,6 +115,7 @@ def default_config(name):
     config.signing_key = [MockKey()]
     config.event_cache_size = 1
     config.enable_registration = True
+    config.enable_registration_captcha = False
     config.macaroon_secret_key = "not even a little secret"
     config.expire_access_token = False
     config.server_name = name