8000 [3.5] bpo-30458: Disallow control chars in http URLs. (GH-12755) by hroncok · Pull Request #13207 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

[3.5] bpo-30458: Disallow control chars in http URLs. (GH-12755) #13207

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

Jump to bottom
Merged
merged 4 commits into from
Jul 14, 2019
Merged

[3.5] bpo-30458: Disallow control chars in http URLs. (GH-12755) #13207

merged 4 commits into from
Jul 14, 2019

Conversation

hroncok
Copy link
Contributor
@hroncok hroncok commented May 8, 2019 •
edited by bedevere-bot
Loading

Disallow control chars in http URLs in urllib.urlopen. This addresses a potential security problem for applications that do not sanity check their URLs where http request headers could be injected.

Disable https related urllib tests on a build without ssl (GH-13032)
These tests require an SSL enabled build. Skip these tests when python is built without SSL to fix test failures.

Use http.client.InvalidURL instead of ValueError as the new error case's exception. (GH-13044)

Co-Authored-By: Miro Hrončok miro@hroncok.cz

https://bugs.python.org/issue30458

@gpshead @hroncok
bpo-30458: Disallow control chars in http URLs. (pythonGH-12755)
d46584b 
Disallow control chars in http URLs in urllib.urlopen.  This addresses a potential security problem for applications that do not sanity check their URLs where http request headers could be injected.

Disable https related urllib tests on a build without ssl (pythonGH-13032)
These tests require an SSL enabled build. Skip these tests when python is built without SSL to fix test failures.

Use http.client.InvalidURL instead of ValueError as the new error case's exception. (pythonGH-13044)

Co-Authored-By: Miro Hrončok <miro@hroncok.cz>
@bedevere-bot bedevere-bot added type-bug An unexpected behavior, bug, or error type-security A security issue labels May 8, 2019
@vstinner
Copy link
Member
vstinner commented May 21, 2019 •
edited
Loading

Multiple tests failed on AppVeyor:

ERROR: test_networked_good_cert (test.test_httplib.HTTPSTest)
ERROR: test_connect (test.test_ssl.NetworkedTests)
ERROR: test_connect_cadata (test.test_ssl.NetworkedTests)
ERROR: test_handshake (test.test_ssl.NetworkedBIOTests)
...

With TLS certification validation error:

[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:728)

It seems like these failures are related to https://bugs.python.org/issue36816: PR #13200 isn't merged yet.

@vstinner
Copy link
Member

It seems like these failures are related to https://bugs.python.org/issue36816: PR #13200 isn't merged yet.

Travis CI basically has the same failures and so it's likely the same issue.

vstinner
vstinner approved these changes May 21, 2019
View reviewed changes
Copy link
Member
@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. Straightforward backport (cherry-pick). The patch went fine in all other branches.

@hroncok
Copy link
Contributor Author
hroncok commented May 21, 2019

Compared to 3.6, I've removed the f-strings.

@tirkarthi tirkarthi mentioned this pull request Jun 3, 2019
Closed
@larryhastings larryhastings merged commit afe3a49 into python:3.5 Jul 14, 2019
@bedevere-bot
Copy link

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings
Copy link
Contributor

Thanks for the 3.5 love!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error type-security A security issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hroncok @vstinner @bedevere-bot @larryhastings @the-knights-who-say-ni @gpshead
0