Merge ~pelpsi/launchpad:avoid-open-redirect-attack-on-logout into launchpad:master

[Simone Pelosi]

Restricted user control on next_to redirect

A penetration test found that lougot redirect is vulnerable to open redirect
attack. "next_to" url is now validated: if it belongs to our domains, the
user is redirected to that url, otherwise the user is redirected to
a default url (homepage).

[Colin Watson (cjwatson)]

review: Needs Fixing

[Colin Watson (cjwatson)]

Thanks for the correction. Could you also add a test that a request to `config.codehosting.secure_codebrowse_root + "+logout?" + urlencode({"next_to": config.launchpad.openid_provider_root + "+logout"})` works, since that's roughly what `CookieLogoutPage` does? (No need for re-review after adding that.)

review: Approve

[Simone Pelosi (pelpsi)]

> Thanks for the correction. Could you also add a test that a request to
> `config.codehosting.secure_codebrowse_root + "+logout?" +
> urlencode({"next_to": config.launchpad.openid_provider_root + "+logout"})`
> works, since that's roughly what `CookieLogoutPage` does? (No need for re-
> review after adding that.)
Sure!

diff --git a/lib/launchpad_loggerhead/app.py b/lib/launchpad_loggerhead/app.py
index 8326a12..20549b4 100644
--- a/lib/launchpad_loggerhead/app.py
+++ b/lib/launchpad_loggerhead/app.py
@@ -5,7 +5,7 @@ import logging
 import os
 import threading
 import xmlrpc.client
-from urllib.parse import urlencode, urljoin
+from urllib.parse import urlencode, urljoin, urlparse
 
 import oops_wsgi
 from breezy import errors, lru_cache, urlutils
@@ -163,7 +163,13 @@ class RootApp:
         environ[self.session_var].clear()
         query = dict(parse_querystring(environ))
         next_url = query.get("next_to")
-        if next_url is None:
+        next_url_domain = urlparse(next_url).netloc
+        open_id_url = config.launchpad.openid_provider_root
+        open_id_domain = urlparse(open_id_url).netloc
+        if next_url is None or (
+            next_url_domain not in allvhosts.hostnames
+            and next_url_domain != open_id_domain
+        ):
             next_url = allvhosts.configs["mainsite"].rooturl
         raise HTTPMovedPermanently(next_url)
 
diff --git a/lib/launchpad_loggerhead/tests.py b/lib/launchpad_loggerhead/tests.py
index 560042c..1bc5fae 100644
--- a/lib/launchpad_loggerhead/tests.py
+++ b/lib/launchpad_loggerhead/tests.py
@@ -106,7 +106,7 @@ class TestLogout(TestCase):
         # TestLoginAndLogout.test_CookieLogoutPage).
 
         # Here, we will have a more useless example of the basic machinery.
-        dummy_root = "http://dummy.test/"
+        dummy_root = "http://launchpad.test/"
         self.browser.open(
             config.codehosting.secure_codebrowse_root
             + "+logout?"
@@ -122,6 +122,53 @@ class TestLogout(TestCase):
             self.browser.contents, b"This is a dummy destination.\n"
         )
 
+    def testLoggerheadLogoutRedirectCookieLogoutMimic(self):
+        # When we visit +logout with a 'next_to' value in the query string,
+        # the logout page will redirect to the given URI.  As of this
+        # writing, this is used by Launchpad to redirect to our OpenId
+        # provider (see lp.testing.tests.test_login.
+        # TestLoginAndLogout.test_CookieLogoutPage).
+
+        # CookieLogout behaviour mimic
+        self.browser.open(
+            config.codehosting.secure_codebrowse_root
+            + "+logout?"
+            + urlencode(
+                dict(next_to=config.launchpad.openid_provider_root + "+logout")
+            )
+        )
+
+        # We are logged out, as before.
+        self.assertEqual(self.session, {})
+
+        # Now, though, we are redirected to the ``next_to`` destination.
+        self.assertEqual(
+            self.browser.url, config.launchpad.openid_provider_root + "+logout"
+        )
+        self.assertEqual(
+            self.browser.contents, b"This is a dummy destination.\n"
+        )
+
+    def testLoggerheadLogoutRedirectFailure(self):
+        # When we visit +logout with a 'next_to' value in the query string,
+        # the logout page will redirect to the given URI only if
+        # the url belongs to well known domains
+
+        # Here, we will have an example of open redirect attack
+        dummy_root = "http://launchpad.phishing.test/"
+        self.browser.open(
+            config.codehosting.secure_codebrowse_root
+            + "+logout?"
+            + urlencode(dict(next_to=dummy_root + "+logout"))
+        )
+
+        # We are logged out, as before.
+        self.assertEqual(self.session, {})
+
+        # We are redirected to the default homepage because
+        # the next_to is unknown
+        self.assertEqual(self.browser.url, "http://launchpad.test/")
+
 
 class TestWSGI(TestCaseWithFactory):
     """Smoke tests for Launchpad's loggerhead WSGI server."""