launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+439730@xxxxxxxxxxxxxxxxxx>]

Simone Pelosi has proposed merging ~pelpsi/launchpad:avoid-open-redirect-attack-on-logout into launchpad:master.

Commit message:
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).

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/439730
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:avoid-open-redirect-attack-on-logout into launchpad:master.

diff --git a/lib/launchpad_loggerhead/app.py b/lib/launchpad_loggerhead/app.py
index 8326a12..abd30e4 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,8 @@ 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
+        if next_url is None or next_url_domain not in allvhosts.hostnames:
             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..b39420f 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,26 @@ class TestLogout(TestCase):
             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."""