bpo-46756: Fix authorization check in urllib.request by serhiy-storchaka · Pull Request #31353 · python/cpython

serhiy-storchaka · 2022-02-15T10:02:25Z

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

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 · 2022-02-15T13:30:01Z

Lib/test/test_urllib2.py


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


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'))

If we wanted to highlight that only /foo path's password will be used. A comment above line 175 will be helpful.

The behavior is a bit confusing too. Cannot /foo/bar have it's own username:password?

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?

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 · 2022-02-15T19:39:37Z

Lib/test/test_urllib2.py

+        self.assertEqual(find_user_pass("c", "http://example.com/baz"),
+                         (None, None))


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?

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.

miss-islington · 2022-02-25T11:31:06Z

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

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 <[email protected]>

bedevere-bot · 2022-02-25T11:31:15Z

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

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 <[email protected]>

bedevere-bot · 2022-02-25T11:31:19Z

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

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 <[email protected]>

bedevere-bot · 2022-02-25T11:31:24Z

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

bedevere-bot · 2022-02-25T11:31:28Z

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

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 <[email protected]>

…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 <[email protected]>

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

…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 <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

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)

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 <[email protected]>

serhiy-storchaka added type-bug An unexpected behavior, bug, or error type-security A security issue needs backport to 3.7 needs backport to 3.10 only security fixes labels Feb 15, 2022

bedevere-bot added the awaiting core review label Feb 15, 2022

the-knights-who-say-ni added the CLA signed label Feb 15, 2022

orsenthil reviewed Feb 15, 2022

View reviewed changes

serhiy-storchaka commented Feb 15, 2022

View reviewed changes

akulakov mentioned this pull request Feb 21, 2022

bpo-42766 Fix 2 bugs in urllib.request.HTTPPasswordMgr.is_suburi #24181

Closed

serhiy-storchaka merged commit e2e7256 into python:main Feb 25, 2022

bedevere-bot removed the awaiting core review label Feb 25, 2022

serhiy-storchaka deleted the urllib-is_suburi branch February 25, 2022 11:31

bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 25, 2022

bedevere-bot removed the needs backport to 3.9 label Feb 25, 2022

bedevere-bot removed the needs backport to 3.8 label Feb 25, 2022

bedevere-bot removed the needs backport to 3.7 label Feb 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

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

bpo-46756: Fix authorization check in urllib.request#31353
serhiy-storchaka merged 1 commit intopython:mainfrom
serhiy-storchaka:urllib-is_suburi

serhiy-storchaka commented Feb 15, 2022 •

edited by bedevere-bot

Loading

Uh oh!

orsenthil Feb 15, 2022

Uh oh!

serhiy-storchaka Feb 15, 2022

Uh oh!

orsenthil Feb 17, 2022

Uh oh!

serhiy-storchaka Feb 15, 2022

Uh oh!

orsenthil Feb 17, 2022

Uh oh!

miss-islington commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		self.assertEqual(find_user_pass("c", "http://example.com/baz"),
		(None, None))

Uh oh!

Comments

Conversation

serhiy-storchaka commented Feb 15, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orsenthil Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

orsenthil Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

orsenthil Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

bedevere-bot commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Feb 15, 2022 •

edited by bedevere-bot

Loading