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

Fix cookie scoping for HTTPS urls. #343

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

dreid
Copy link
Contributor

@dreid dreid commented Feb 8, 2022

If Cookie.port is specified not None then CookieJar will attempt to
compare it to the port for the Request object by first parsing it out of
Request.host and if there is no port specified there falling back to
the DEFAULT_HTTP_PORT value of 80.

This caused cookies to never be sent for HTTPS domains because the
Cookie.port was set to 443, and the _FakeUrllib2Request.host did not
contain the default port value.

I've also added a test to make sure non-default port values work properly.

dreid and others added 2 commits February 8, 2022 08:55
If Cookie.port is specified not None then CookieJar will attempt to
compare it to the port for the Request object by first parsing it out of
`Request.host` and if there is no port specified there falling back to
the `DEFAULT_HTTP_PORT` value of 80.

This caused cookies to never be sent for HTTPS domains because the
Cookie.port was set to 443, and the _FakeUrllib2Request.host did not
contain the default port value.

I've also added a test to make sure non-default port values work properly.
Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Thanks. Obviously an important fix, and I love the improvements to the test coverage too. Let's land it.

@glyph glyph enabled auto-merge February 8, 2022 19:29
@glyph
Copy link
Member

glyph commented Feb 8, 2022

@twm if you have a minute to do another fast-following release once this lands that would be great, this is a pretty bad bug in our security fix :-|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants