bpo-46756: Fix authorization check in urllib.request (GH-31353) (GH-31573)

miss-islington · serhiy-storchaka · web-flow · commit 31fef7edf951 · 2022-02-25T12:49:43.000-05:00
Fix a bug in urllib.request.HTTPPasswordMgr.find_user_password() and urllib.request.HTTPPasswordMgrWithPriorAuth.is_authenticated() which allowed to bypass authorization. For example, access to URI "example.org/foobar" was allowed if the user was authorized for URI "example.org/foo". (cherry picked from commit e2e7256) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
diff --git a/Lib/test/test_urllib2.py b/Lib/test/test_urllib2.py
@@ -160,7 +160,6 @@ def test_password_manager(self):
         self.assertEqual(find_user_pass("Some Realm",
                                         "http://example.com/spam"),
                          ('joe', 'password'))
-
         self.assertEqual(find_user_pass("Some Realm",
                                         "http://example.com/spam/spam"),
                          ('joe', 'password'))
@@ -169,12 +168,29 @@ def test_password_manager(self):
 
         add("c", "http://example.com/foo", "foo", "ni")
         add("c", "http://example.com/bar", "bar", "nini")
+        add("c", "http://example.com/foo/bar", "foobar", "nibar")
 
         self.assertEqual(find_user_pass("c", "http://example.com/foo"),
                          ('foo', 'ni'))
-
         self.assertEqual(find_user_pass("c", "http://example.com/bar"),
                          ('bar', 'nini'))
+        self.assertEqual(find_user_pass("c", "http://example.com/foo/"),
+                         ('foo', 'ni'))
+        self.assertEqual(find_user_pass("c", "http://example.com/foo/bar"),
+                         ('foo', 'ni'))
+        self.assertEqual(find_user_pass("c", "http://example.com/foo/baz"),
+                         ('foo', 'ni'))
+        self.assertEqual(find_user_pass("c", "http://example.com/foobar"),
+                         (None, None))
+
+        add("c", "http://example.com/baz/", "baz", "ninini")
+
+        self.assertEqual(find_user_pass("c", "http://example.com/baz"),
+                         (None, None))
+        self.assertEqual(find_user_pass("c", "http://example.com/baz/"),
+                         ('baz', 'ninini'))
+        self.assertEqual(find_user_pass("c", "http://example.com/baz/bar"),
+                         ('baz', 'ninini'))
 
         # For the same path, newer password should be considered.
 
@@ -1642,8 +1658,9 @@ def test_basic_prior_auth_auto_send(self):
         auth_prior_handler.add_password(
             None, request_url, user, password, is_authenticated=True)
 
-        is_auth = pwd_manager.is_authenticated(request_url)
-        self.assertTrue(is_auth)
+        self.assertTrue(pwd_manager.is_authenticated(request_url))
+        self.assertTrue(pwd_manager.is_authenticated(request_url + '/nested'))
+        self.assertFalse(pwd_manager.is_authenticated(request_url + 'plain'))
 
         opener = OpenerDirector()
         opener.add_handler(auth_prior_handler)
diff --git a/Lib/urllib/request.py b/Lib/urllib/request.py
@@ -890,10 +890,10 @@ def is_suburi(self, base, test):
             return True
         if base[0] != test[0]:
             return False
-        common = posixpath.commonprefix((base[1], test[1]))
-        if len(common) == len(base[1]):
-            return True
-        return False
+        prefix = base[1]
+        if prefix[-1:] != '/':
+            prefix += '/'
+        return test[1].startswith(prefix)
 
 
 class HTTPPasswordMgrWithDefaultRealm(HTTPPasswordMgr):
diff --git a/Misc/NEWS.d/next/Library/2022-02-15-11-57-53.bpo-46756.AigSPi.rst b/Misc/NEWS.d/next/Library/2022-02-15-11-57-53.bpo-46756.AigSPi.rst
@@ -0,0 +1,5 @@
+Fix a bug in :meth:`urllib.request.HTTPPasswordMgr.find_user_password` and
+:meth:`urllib.request.HTTPPasswordMgrWithPriorAuth.is_authenticated` which
+allowed to bypass authorization. For example, access to URI
+``example.org/foobar`` was allowed if the user was authorized for URI
+``example.org/foo``.
