subdomains: Tighten search for EXTERNAL_HOST in get_subdomain.

If a Zulip install at example.org got a request at an HTTP `Host`
like foo.example.org.evil.com (or even foo.example.orgevil.com),
we would accept it as subdomain foo.  This isn't likely to happen
in practice because it shouldn't pass ALLOWED_HOSTS, and it's not
obvious to me that anything untoward could be done with it even
if ALLOWED_HOSTS were set wide open, but if nothing else it
multiplies the cases in analyzing this logic.

The reason we had a loose match like this, I assume, is to allow
the user to come from arbitrary ports -- especially in development.
So tighten the pattern to allow just that, and add some tests for
that behavior and a comment explaining why this complication is
needed.

zerver/lib/subdomains.py

@@ -2,20 +2,37 @@
 
 from django.conf import settings
 from django.http import HttpRequest
+import re
 from typing import Optional, Text
 
 from zerver.models import get_realm, Realm, UserProfile
 
 def get_subdomain(request):
     # type: (HttpRequest) -> Text
+
+    # The HTTP spec allows, but doesn't require, a client to omit the
+    # port in the `Host` header if it's "the default port for the
+    # service requested", i.e. typically either 443 or 80; and
+    # whatever Django gets there, or from proxies reporting that via
+    # X-Forwarded-Host, it passes right through the same way.  So our
+    # logic is a bit complicated to allow for that variation.
+    #
+    # For EXTERNAL_HOST, we take a missing port to mean that any port
+    # should be accepted in Host.  It's not totally clear that's the
+    # right behavior, but it keeps compatibility with older versions
+    # of Zulip, so that's a start.
+
     host = request.get_host().lower()
-    index = host.find("." + settings.EXTERNAL_HOST)
-    if index == -1:
-        return Realm.SUBDOMAIN_FOR_ROOT_DOMAIN
-    subdomain = host[0:index]
-    if subdomain in settings.ROOT_SUBDOMAIN_ALIASES:
-        return Realm.SUBDOMAIN_FOR_ROOT_DOMAIN
-    return subdomain
+
+    m = re.search('\.%s(:\d+)?$' % (settings.EXTERNAL_HOST,),
+                  host)
+    if m:
+        subdomain = host[:m.start()]
+        if subdomain in settings.ROOT_SUBDOMAIN_ALIASES:
+            return Realm.SUBDOMAIN_FOR_ROOT_DOMAIN
+        return subdomain
+
+    return Realm.SUBDOMAIN_FOR_ROOT_DOMAIN
 
 def is_subdomain_root_or_alias(request):
     # type: (HttpRequest) -> bool

zerver/tests/test_subdomains.py

@@ -17,15 +17,32 @@ class SubdomainsTest(TestCase):
             request.attach_mock(mock.Mock(return_value=host), 'get_host')
             return request
 
-        def test(expected, host, *, root_aliases=[]):
-            # type: (str, str, List[str]) -> None
-            with self.settings(EXTERNAL_HOST='example.org',
+        def test(expected, host, *, plusport=True,
+                 external_host='example.org', root_aliases=[]):
+            # type: (str, str, bool, str, List[str]) -> None
+            with self.settings(EXTERNAL_HOST=external_host,
                                ROOT_SUBDOMAIN_ALIASES=root_aliases):
                 self.assertEqual(get_subdomain(request_mock(host)), expected)
-                self.assertEqual(get_subdomain(request_mock(host + ':443')), expected)
+                if plusport and ':' not in host:
+                    self.assertEqual(get_subdomain(request_mock(host + ':443')),
+                                     expected)
 
         ROOT = Realm.SUBDOMAIN_FOR_ROOT_DOMAIN
+
+        # Basics
         test(ROOT, 'example.org')
         test('foo', 'foo.example.org')
         test(ROOT, 'www.example.org', root_aliases=['www'])
+
+        # Unrecognized patterns fall back to root
         test(ROOT, 'arbitrary.com')
+        test(ROOT, 'foo.example.org.evil.com')
+
+        # Any port is fine in Host if there's none in EXTERNAL_HOST
+        test('foo', 'foo.example.org:443', external_host='example.org')
+        test('foo', 'foo.example.org:12345', external_host='example.org')
+
+        # Explicit port in EXTERNAL_HOST must be explicitly matched in Host
+        test(ROOT, 'foo.example.org', external_host='example.org:12345')
+        test(ROOT, 'foo.example.org', external_host='example.org:443', plusport=False)
+        test('foo', 'foo.example.org:443', external_host='example.org:443')