Skip to content

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

Merged
larryhastings merged 4 commits into
python:3.5from
hroncok:3.5-issue30458
Jul 14, 2019
Merged

[3.5] bpo-30458: Disallow control chars in http URLs. (GH-12755)#13207
larryhastings merged 4 commits into
python:3.5from
hroncok:3.5-issue30458

Conversation

@hroncok

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

Copy link
Copy Markdown
Contributor

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

vstinner commented May 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

vstinner commented May 21, 2019

Copy link
Copy Markdown
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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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

hroncok commented May 21, 2019

Copy link
Copy Markdown
Contributor Author

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

bedevere-bot commented Jul 14, 2019

Copy link
Copy Markdown

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

@larryhastings

larryhastings commented Jul 14, 2019

Copy link
Copy Markdown
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