Skip to content

bpo-46756: Fix authorization check in urllib.request #31353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 25, 2022
Merged

bpo-46756: Fix authorization check in urllib.request #31353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions Lib/test/test_urllib2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,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'))
Expand All @@ -173,12 +172,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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line

add("c", "http://example.com/foo/bar", "foobar", "nibar")

along with this test a line 184 confuses me.

    self.assertEqual(find_user_pass("c", "http://example.com/foo/bar"), ('foo', 'ni'))
  1. If we wanted to highlight that only /foo path's password will be used. A comment above line 175 will be helpful.
  2. The behavior is a bit confusing too. Cannot /foo/bar have it's own username:password?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior seems intentional. See a comment at line 155: "For the same realm, password set the highest path is the winner." Tests at lines 158, 164 and 167 ensure this, but only for the root path. I added new tests for non-root path.

Should I repeat this comment above line 175?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. I missed seeing the context in the diff. I notice an example illustrating the behavior for the comment already present in line 149 and line 150.

So, I don't think we need another comment. Thank you.


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))
Comment on lines +192 to +193
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this test. Should http://example.com/baz be considered a sub-URI of http://example.com/baz/ or not? In other words, should a trailing slash be ignored?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. I think, this test, which doesn't consider /baz to be sub-URI or /baz/ for HTTPPasswordMgr behavior seems safer.

If there is any reference to this in any rfc or any other software behavior that will be helpful as a validation.

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.

Expand Down Expand Up @@ -1658,8 +1674,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)
Expand Down
8 changes: 4 additions & 4 deletions Lib/urllib/request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,10 +889,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):
Expand Down
5 changes: 5 additions & 0 deletions Misc/NEWS.d/next/Library/2022-02-15-11-57-53.bpo-46756.AigSPi.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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``.