Skip to content

/cpython

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

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

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 commented May 8, 2019 •
edited by bedevere-bot

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. (GH-12755)
Loading status checks…
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 (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>
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented May 21, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented May 21, 2019

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 changes
Copy link
Member

vstinner left a comment

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

This comment has been minimized.

Sign in to view
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
Merge branch '3.5' into 3.5-issue30458
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
ed1e2f7
@larryhastings
Merge branch '3.5' into 3.5-issue30458
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
3f340e4
@larryhastings
Merge branch '3.5' into 3.5-issue30458
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d69fa78
@larryhastings larryhastings merged commit afe3a49 into python:3.5 Jul 14, 2019
5 checks passed
5 checks passed
bedevere/issue-number Issue number 30458 found
Details
bedevere/maintenance-branch-pr Valid maintenance branch PR title.
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jul 14, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

larryhastings commented Jul 14, 2019

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

Assignees
No one assigned
Labels
CLA signed type-bugfix type-security
Projects
None yet
Milestone
No milestone
6 participants
@hroncok @vstinner @bedevere-bot @larryhastings @the-knights-who-say-ni @gpshead
You can’t perform that action at this time.