NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.8k

bpo-22708: Fix http protocol version in CONNECT method, support IDN#742

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

vo-va wants to merge1 commit intopython:masterfromvo-va:fix-issue-22708

Closed

bpo-22708: Fix http protocol version in CONNECT method, support IDN#742

vo-va wants to merge1 commit intopython:masterfromvo-va:fix-issue-22708

Conversation

Copy link

vo-va commentedMar 20, 2017

Original author of this patchdemianbrecht@gmail.com

HTTP/1.0 not supported CONNECT method, fix http version.
Support for IDN.

https://bugs.python.org/issue22708

Fix http protocol version in CONNECT method, support IDN

a9b201c

Original author of this patch demianbrecht@gmail.com

Mariatta added CLA not signed and removed CLA not signed labels

Mar 20, 2017

the-knights-who-say-ni added the CLA not signed label

Mar 20, 2017

Mariatta removed the CLA not signed label

Mar 21, 2017

the-knights-who-say-ni added the CLA signed label

Mar 21, 2017

Copy link

Member

brettcannon commentedMar 24, 2017

FYI for any other core dev, Demian Brecht has signed the CLA.

berkerpeksag requested changes

Apr 15, 2017

View reviewed changes

Copy link

Member

berkerpeksag left a comment

Choose a reason for hiding this comment

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

Please also include the NEWS entry in theoriginal patch and append "Patch by Demian Brecht.".

Lib/test/test_httplib.py


		deftest_connect_with_tunnel_idna(self):
		dest='\u03b4\u03c0\u03b8.gr'
		expected='CONNECT %s:%d HTTP/1.1\r\n'.encode('ascii')% (

Copy link

Member

Choose a reason for hiding this comment

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

Is there a reason to not useb'CONNECT %s:%d HTTP/1.1\r\n' instead? We already know that "HTTP/1.1",dest.encode('idna') andclient.HTTP_PORT don't contain any non-ASCII chars so there is no need to encode it ASCII.

Copy link

Member

berkerpeksag commentedApr 15, 2017

And please rebase your branch so we can run the tests on AppVeyor.

berkerpeksag reviewed

Apr 15, 2017

View reviewed changes

Lib/test/test_httplib.py

		self.assertNotIn(b'Host: destination.com:None',self.conn.sock.data)
		self.assertIn(b'Host: destination.com',self.conn.sock.data)

		# This test should be removed when CONNECT gets the HTTP/1.1 blessing

Copy link

Member

berkerpeksagApr 15, 2017

Choose a reason for hiding this comment

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

This comment is now redundant and can be removed, but I don't see a reason to remove the test itself.

berkerpeksag added needs backport to 3.5 labels

Apr 15, 2017

brettcannon removed the needs backport to 3.5 label

Aug 11, 2017

Copy link

Member

brettcannon commentedFeb 21, 2018

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says,I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.