Skip to content
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.

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

Jump to bottom

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

Merged
merged 1 commit into from Feb 25, 2022
Merged

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

merged 1 commit into from Feb 25, 2022

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Feb 15, 2022 •
edited by bedevere-bot

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".

https://bugs.python.org/issue46756

@serhiy-storchaka
bpo-46756: Fix authorization check in urllib.request
4bde28f 
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".
orsenthil
orsenthil reviewed Feb 15, 2022
View changes
Lib/test/test_urllib2.py
@@ -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

@orsenthil orsenthil Feb 15, 2022

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

@serhiy-storchaka serhiy-storchaka Feb 15, 2022

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

@orsenthil orsenthil Feb 17, 2022

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.

serhiy-storchaka
serhiy-storchaka reviewed Feb 15, 2022
View changes
Lib/test/test_urllib2.py
self.assertEqual(find_user_pass("c", "http://example.com/baz"),
(None, None))
Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Feb 15, 2022

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

@orsenthil orsenthil Feb 17, 2022

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.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Feb 16, 2022
Open
@akulakov akulakov mentioned this pull request Feb 21, 2022
Closed
@serhiy-storchaka serhiy-storchaka merged commit e2e7256 into python:main Feb 25, 2022
12 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Feb 25, 2022

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8, 3.9, 3.10.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the urllib-is_suburi branch Feb 25, 2022
miss-islington added a commit to miss-islington/cpython that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (pythonGH-31353)
95b03ad 
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>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Feb 25, 2022

GH-31570 is a backport of this pull request to the 3.10 branch.

miss-islington added a commit to miss-islington/cpython that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (pythonGH-31353)
f904958 
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>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Feb 25, 2022

GH-31571 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit to miss-islington/cpython that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (pythonGH-31353)
2bb307b 
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>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Feb 25, 2022

GH-31572 is a backport of this pull request to the 3.8 branch.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Feb 25, 2022

GH-31573 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit to miss-islington/cpython that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (pythonGH-31353)
c3412a4 
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>
miss-islington added a commit that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (GH-31353)
4560c7e 
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>
miss-islington added a commit that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (GH-31353)
2b7e04d 
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>
ned-deily pushed a commit that referenced this issue Feb 25, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (GH-31353) (GH-3…
31fef7e 
…1573)

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>
asvetlov added a commit that referenced this issue Feb 26, 2022
@serhiy-storchaka @asvetlov
bpo-46756: Fix authorization check in urllib.request (GH-31353)
146079d 
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".
pablogsal pushed a commit that referenced this issue Mar 2, 2022
@serhiy-storchaka @miss-islington
bpo-46756: Fix authorization check in urllib.request (GH-31353) (GH-3…
1c9701a 
…1572)

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>

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
gentoo-bot pushed a commit to gentoo/cpython that referenced this issue Mar 18, 2022
@mgorny
bpo-46756: Fix authorization check in urllib.request (pythonGH-31353)
e972fd9 
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".

(rebased for 2.7 by Michał Górny)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orsenthil orsenthil

Assignees
No one assigned
Labels
CLA signed type-bug type-security
Projects
None yet
Milestone
No milestone
5 participants
@serhiy-storchaka @miss-islington @bedevere-bot @orsenthil @the-knights-who-say-ni