Backout changes for automatically calculating the public baseurl. (#9313)

clokep · web-flow · commit e40d88cff3cc · 2021-02-11T11:16:54.000-05:00
This breaks some people's configurations (if their Client-Server API
is not accessed via port 443).
diff --git a/changelog.d/9313.bugfix b/changelog.d/9313.bugfix
@@ -0,0 +1 @@
+Do not automatically calculate `public_baseurl` since it can be wrong in some situations.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
@@ -74,10 +74,6 @@ pid_file: DATADIR/homeserver.pid
 # Otherwise, it should be the URL to reach Synapse's client HTTP listener (see
 # 'listeners' below).
 #
-# If this is left unset, it defaults to 'https://<server_name>/'. (Note that
-# that will not work unless you configure Synapse or a reverse-proxy to listen
-# on port 443.)
-#
 #public_baseurl: https://example.com/
 
 # Set the soft limit on the number of file descriptors synapse can use
@@ -1169,9 +1165,8 @@ account_validity:
   # send an email to the account's email address with a renewal link. By
   # default, no such emails are sent.
   #
-  # If you enable this setting, you will also need to fill out the 'email'
-  # configuration section. You should also check that 'public_baseurl' is set
-  # correctly.
+  # If you enable this setting, you will also need to fill out the 'email' and
+  # 'public_baseurl' configuration sections.
   #
   #renew_at: 1w
 
@@ -1262,7 +1257,8 @@ account_validity:
 # The identity server which we suggest that clients should use when users log
 # in on this server.
 #
-# (By default, no suggestion is made, so it is left up to the client.)
+# (By default, no suggestion is made, so it is left up to the client.
+# This setting is ignored unless public_baseurl is also set.)
 #
 #default_identity_server: https://matrix.org
 
@@ -1287,6 +1283,8 @@ account_validity:
 # by the Matrix Identity Service API specification:
 # https://matrix.org/docs/spec/identity_service/latest
 #
+# If a delegate is specified, the config option public_baseurl must also be filled out.
+#
 account_threepid_delegates:
     #email: https://example.com     # Delegate email sending to example.com
     #msisdn: http://localhost:8090  # Delegate SMS sending to this local process
@@ -1938,9 +1936,9 @@ sso:
     # phishing attacks from evil.site. To avoid this, include a slash after the
     # hostname: "https://my.client/".
     #
-    # The login fallback page (used by clients that don't natively support the
-    # required login flows) is automatically whitelisted in addition to any URLs
-    # in this list.
+    # If public_baseurl is set, then the login fallback page (used by clients
+    # that don't natively support the required login flows) is whitelisted in
+    # addition to any URLs in this list.
     #
     # By default, this list is empty.
     #
diff --git a/synapse/api/urls.py b/synapse/api/urls.py
@@ -42,6 +42,8 @@ def __init__(self, hs_config):
         """
         if hs_config.form_secret is None:
             raise ConfigError("form_secret not set in config")
+        if hs_config.public_baseurl is None:
+            raise ConfigError("public_baseurl not set in config")
 
         self._hmac_secret = hs_config.form_secret.encode("utf-8")
         self._public_baseurl = hs_config.public_baseurl
diff --git a/synapse/config/cas.py b/synapse/config/cas.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from ._base import Config
+from ._base import Config, ConfigError
 
 
 class CasConfig(Config):
@@ -30,13 +30,15 @@ def read_config(self, config, **kwargs):
 
         if self.cas_enabled:
             self.cas_server_url = cas_config["server_url"]
-            public_base_url = cas_config.get("service_url") or self.public_baseurl
-            if public_base_url[-1] != "/":
-                public_base_url += "/"
+
+            # The public baseurl is required because it is used by the redirect
+            # template.
+            public_baseurl = self.public_baseurl
+            if not public_baseurl:
+                raise ConfigError("cas_config requires a public_baseurl to be set")
+
             # TODO Update this to a _synapse URL.
-            self.cas_service_url = (
-                public_base_url + "_matrix/client/r0/login/cas/ticket"
-            )
+            self.cas_service_url = public_baseurl + "_matrix/client/r0/login/cas/ticket"
             self.cas_displayname_attribute = cas_config.get("displayname_attribute")
             self.cas_required_attributes = cas_config.get("required_attributes") or {}
         else:
diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py
@@ -166,6 +166,11 @@ def read_config(self, config, **kwargs):
             if not self.email_notif_from:
                 missing.append("email.notif_from")
 
+            # public_baseurl is required to build password reset and validation links that
+            # will be emailed to users
+            if config.get("public_baseurl") is None:
+                missing.append("public_baseurl")
+
             if missing:
                 raise ConfigError(
                     MISSING_PASSWORD_RESET_CONFIG_ERROR % (", ".join(missing),)
@@ -264,6 +269,9 @@ def read_config(self, config, **kwargs):
             if not self.email_notif_from:
                 missing.append("email.notif_from")
 
+            if config.get("public_baseurl") is None:
+                missing.append("public_baseurl")
+
             if missing:
                 raise ConfigError(
                     "email.enable_notifs is True but required keys are missing: %s"
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
@@ -53,7 +53,10 @@ def read_config(self, config, **kwargs):
                     "Multiple OIDC providers have the idp_id %r." % idp_id
                 )
 
-        self.oidc_callback_url = self.public_baseurl + "_synapse/client/oidc/callback"
+        public_baseurl = self.public_baseurl
+        if public_baseurl is None:
+            raise ConfigError("oidc_config requires a public_baseurl to be set")
+        self.oidc_callback_url = public_baseurl + "_synapse/client/oidc/callback"
 
     @property
     def oidc_enabled(self) -> bool:
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
@@ -49,6 +49,10 @@ def __init__(self, config, synapse_config):
 
             self.startup_job_max_delta = self.period * 10.0 / 100.0
 
+        if self.renew_by_email_enabled:
+            if "public_baseurl" not in synapse_config:
+                raise ConfigError("Can't send renewal emails without 'public_baseurl'")
+
         template_dir = config.get("template_dir")
 
         if not template_dir:
@@ -105,6 +109,13 @@ def read_config(self, config, **kwargs):
         account_threepid_delegates = config.get("account_threepid_delegates") or {}
         self.account_threepid_delegate_email = account_threepid_delegates.get("email")
         self.account_threepid_delegate_msisdn = account_threepid_delegates.get("msisdn")
+        if self.account_threepid_delegate_msisdn and not self.public_baseurl:
+            raise ConfigError(
+                "The configuration option `public_baseurl` is required if "
+                "`account_threepid_delegate.msisdn` is set, such that "
+                "clients know where to submit validation tokens to. Please "
+                "configure `public_baseurl`."
+            )
 
         self.default_identity_server = config.get("default_identity_server")
         self.allow_guest_access = config.get("allow_guest_access", False)
@@ -227,9 +238,8 @@ def generate_config_section(self, generate_secrets=False, **kwargs):
           # send an email to the account's email address with a renewal link. By
           # default, no such emails are sent.
           #
-          # If you enable this setting, you will also need to fill out the 'email'
-          # configuration section. You should also check that 'public_baseurl' is set
-          # correctly.
+          # If you enable this setting, you will also need to fill out the 'email' and
+          # 'public_baseurl' configuration sections.
           #
           #renew_at: 1w
 
@@ -320,7 +330,8 @@ def generate_config_section(self, generate_secrets=False, **kwargs):
         # The identity server which we suggest that clients should use when users log
         # in on this server.
         #
-        # (By default, no suggestion is made, so it is left up to the client.)
+        # (By default, no suggestion is made, so it is left up to the client.
+        # This setting is ignored unless public_baseurl is also set.)
         #
         #default_identity_server: https://matrix.org
 
@@ -345,6 +356,8 @@ def generate_config_section(self, generate_secrets=False, **kwargs):
         # by the Matrix Identity Service API specification:
         # https://matrix.org/docs/spec/identity_service/latest
         #
+        # If a delegate is specified, the config option public_baseurl must also be filled out.
+        #
         account_threepid_delegates:
             #email: https://example.com     # Delegate email sending to example.com
             #msisdn: http://localhost:8090  # Delegate SMS sending to this local process
diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py
@@ -189,6 +189,8 @@ def _default_saml_config_dict(
         import saml2
 
         public_baseurl = self.public_baseurl
+        if public_baseurl is None:
+            raise ConfigError("saml2_config requires a public_baseurl to be set")
 
         if self.saml2_grandfathered_mxid_source_attribute:
             optional_attributes.add(self.saml2_grandfathered_mxid_source_attribute)
diff --git a/synapse/config/server.py b/synapse/config/server.py
@@ -161,11 +161,7 @@ def read_config(self, config, **kwargs):
         self.print_pidfile = config.get("print_pidfile")
         self.user_agent_suffix = config.get("user_agent_suffix")
         self.use_frozen_dicts = config.get("use_frozen_dicts", False)
-        self.public_baseurl = config.get("public_baseurl") or "https://%s/" % (
-            self.server_name,
-        )
-        if self.public_baseurl[-1] != "/":
-            self.public_baseurl += "/"
+        self.public_baseurl = config.get("public_baseurl")
 
         # Whether to enable user presence.
         self.use_presence = config.get("use_presence", True)
@@ -321,6 +317,9 @@ def read_config(self, config, **kwargs):
         # Always blacklist 0.0.0.0, ::
         self.federation_ip_range_blacklist.update(["0.0.0.0", "::"])
 
+        if self.public_baseurl is not None:
+            if self.public_baseurl[-1] != "/":
+                self.public_baseurl += "/"
         self.start_pushers = config.get("start_pushers", True)
 
         # (undocumented) option for torturing the worker-mode replication a bit,
@@ -748,10 +747,6 @@ def generate_config_section(
         # Otherwise, it should be the URL to reach Synapse's client HTTP listener (see
         # 'listeners' below).
         #
-        # If this is left unset, it defaults to 'https://<server_name>/'. (Note that
-        # that will not work unless you configure Synapse or a reverse-proxy to listen
-        # on port 443.)
-        #
         #public_baseurl: https://example.com/
 
         # Set the soft limit on the number of file descriptors synapse can use
diff --git a/synapse/config/sso.py b/synapse/config/sso.py
@@ -64,8 +64,11 @@ def read_config(self, config, **kwargs):
         # gracefully to the client). This would make it pointless to ask the user for
         # confirmation, since the URL the confirmation page would be showing wouldn't be
         # the client's.
-        login_fallback_url = self.public_baseurl + "_matrix/static/client/login"
-        self.sso_client_whitelist.append(login_fallback_url)
+        # public_baseurl is an optional setting, so we only add the fallback's URL to the
+        # list if it's provided (because we can't figure out what that URL is otherwise).
+        if self.public_baseurl:
+            login_fallback_url = self.public_baseurl + "_matrix/static/client/login"
+            self.sso_client_whitelist.append(login_fallback_url)
 
     def generate_config_section(self, **kwargs):
         return """\
@@ -83,9 +86,9 @@ def generate_config_section(self, **kwargs):
             # phishing attacks from evil.site. To avoid this, include a slash after the
             # hostname: "https://my.client/".
             #
-            # The login fallback page (used by clients that don't natively support the
-            # required login flows) is automatically whitelisted in addition to any URLs
-            # in this list.
+            # If public_baseurl is set, then the login fallback page (used by clients
+            # that don't natively support the required login flows) is whitelisted in
+            # addition to any URLs in this list.
             #
             # By default, this list is empty.
             #
diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
@@ -504,6 +504,10 @@ async def requestMsisdnToken(
         except RequestTimedOutError:
             raise SynapseError(500, "Timed out contacting identity server")
 
+        # It is already checked that public_baseurl is configured since this code
+        # should only be used if account_threepid_delegate_msisdn is true.
+        assert self.hs.config.public_baseurl
+
         # we need to tell the client to send the token back to us, since it doesn't
         # otherwise know where to send it, so add submit_url response parameter
         # (see also MSC2078)
diff --git a/synapse/rest/well_known.py b/synapse/rest/well_known.py
@@ -34,6 +34,10 @@ def __init__(self, hs):
         self._config = hs.config
 
     def get_well_known(self):
+        # if we don't have a public_baseurl, we can't help much here.
+        if self._config.public_baseurl is None:
+            return None
+
         result = {"m.homeserver": {"base_url": self._config.public_baseurl}}
 
         if self._config.default_identity_server:
diff --git a/synapse/util/templates.py b/synapse/util/templates.py
@@ -17,7 +17,7 @@
 
 import time
 import urllib.parse
-from typing import TYPE_CHECKING, Callable, Iterable, Union
+from typing import TYPE_CHECKING, Callable, Iterable, Optional, Union
 
 import jinja2
 
@@ -74,14 +74,23 @@ def build_jinja_env(
     return env
 
 
-def _create_mxc_to_http_filter(public_baseurl: str) -> Callable:
+def _create_mxc_to_http_filter(
+    public_baseurl: Optional[str],
+) -> Callable[[str, int, int, str], str]:
     """Create and return a jinja2 filter that converts MXC urls to HTTP
 
     Args:
         public_baseurl: The public, accessible base URL of the homeserver
     """
 
-    def mxc_to_http_filter(value, width, height, resize_method="crop"):
+    def mxc_to_http_filter(
+        value: str, width: int, height: int, resize_method: str = "crop"
+    ) -> str:
+        if not public_baseurl:
+            raise RuntimeError(
+                "public_baseurl must be set in the homeserver config to convert MXC URLs to HTTP URLs."
+            )
+
         if value[0:6] != "mxc://":
             return ""
 
diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
@@ -672,10 +672,12 @@ def make_homeserver(self, reactor, clock):
         self.redirect_path = "_synapse/client/login/sso/redirect/confirm"
 
         config = self.default_config()
+        config["public_baseurl"] = (
+            config.get("public_baseurl") or "https://matrix.goodserver.com:8448"
+        )
         config["cas_config"] = {
             "enabled": True,
             "server_url": CAS_SERVER,
-            "service_url": "https://matrix.goodserver.com:8448",
         }
 
         cas_user_id = "username"
diff --git a/tests/rest/test_well_known.py b/tests/rest/test_well_known.py
@@ -40,3 +40,12 @@ def test_well_known(self):
                 "m.identity_server": {"base_url": "https://testis"},
             },
         )
+
+    def test_well_known_no_public_baseurl(self):
+        self.hs.config.public_baseurl = None
+
+        channel = self.make_request(
+            "GET", "/.well-known/matrix/client", shorthand=False
+        )
+
+        self.assertEqual(channel.code, 404)
diff --git a/tests/utils.py b/tests/utils.py
@@ -159,6 +159,7 @@ def default_config(name, parse=False):
         },
         "rc_3pid_validation": {"per_second": 10000, "burst_count": 10000},
         "saml2_enabled": False,
+        "public_baseurl": None,
         "default_identity_server": None,
         "key_refresh_interval": 24 * 60 * 60 * 1000,
         "old_signing_keys": {},

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Do not automatically calculate `public_baseurl` since it can be wrong in some situations.
Original file line number	Diff line number	Diff line change
`@@ -74,10 +74,6 @@ pid_file: DATADIR/homeserver.pid`
`74`	`74`	`# Otherwise, it should be the URL to reach Synapse's client HTTP listener (see`
`75`	`75`	`# 'listeners' below).`
`76`	`76`	`#`
`77`		`-# If this is left unset, it defaults to 'https://<server_name>/'. (Note that`
`78`		`-# that will not work unless you configure Synapse or a reverse-proxy to listen`
`79`		`-# on port 443.)`
`80`		`-#`
`81`	`77`	`#public_baseurl: https://example.com/`
`82`	`78`
`83`	`79`	`# Set the soft limit on the number of file descriptors synapse can use`
`@@ -1169,9 +1165,8 @@ account_validity:`
`1169`	`1165`	`# send an email to the account's email address with a renewal link. By`
`1170`	`1166`	`# default, no such emails are sent.`
`1171`	`1167`	`#`
`1172`		`- # If you enable this setting, you will also need to fill out the 'email'`
`1173`		`- # configuration section. You should also check that 'public_baseurl' is set`
`1174`		`- # correctly.`
	`1168`	`+ # If you enable this setting, you will also need to fill out the 'email' and`
	`1169`	`+ # 'public_baseurl' configuration sections.`
`1175`	`1170`	`#`
`1176`	`1171`	`#renew_at: 1w`
`1177`	`1172`
`@@ -1262,7 +1257,8 @@ account_validity:`
`1262`	`1257`	`# The identity server which we suggest that clients should use when users log`
`1263`	`1258`	`# in on this server.`
`1264`	`1259`	`#`
`1265`		`-# (By default, no suggestion is made, so it is left up to the client.)`
	`1260`	`+# (By default, no suggestion is made, so it is left up to the client.`
	`1261`	`+# This setting is ignored unless public_baseurl is also set.)`
`1266`	`1262`	`#`
`1267`	`1263`	`#default_identity_server: https://matrix.org`
`1268`	`1264`
`@@ -1287,6 +1283,8 @@ account_validity:`
`1287`	`1283`	`# by the Matrix Identity Service API specification:`
`1288`	`1284`	`# https://matrix.org/docs/spec/identity_service/latest`
`1289`	`1285`	`#`
	`1286`	`+# If a delegate is specified, the config option public_baseurl must also be filled out.`
	`1287`	`+#`
`1290`	`1288`	`account_threepid_delegates:`
`1291`	`1289`	`#email: https://example.com # Delegate email sending to example.com`
`1292`	`1290`	`#msisdn: http://localhost:8090 # Delegate SMS sending to this local process`
`@@ -1938,9 +1936,9 @@ sso:`
`1938`	`1936`	`# phishing attacks from evil.site. To avoid this, include a slash after the`
`1939`	`1937`	`# hostname: "https://my.client/".`
`1940`	`1938`	`#`
`1941`		`- # The login fallback page (used by clients that don't natively support the`
`1942`		`- # required login flows) is automatically whitelisted in addition to any URLs`
`1943`		`- # in this list.`
	`1939`	`+ # If public_baseurl is set, then the login fallback page (used by clients`
	`1940`	`+ # that don't natively support the required login flows) is whitelisted in`
	`1941`	`+ # addition to any URLs in this list.`
`1944`	`1942`	`#`
`1945`	`1943`	`# By default, this list is empty.`
`1946`	`1944`	`#`