◐ Shell
reader mode source ↗
Skip to content

gh-66897: Upgrade HTTP CONNECT to protocol HTTP/1.1#8305

Merged
orsenthil merged 17 commits into
python:mainfrom
handlerbot:fix-bpo-22708
Apr 5, 2023
Merged

gh-66897: Upgrade HTTP CONNECT to protocol HTTP/1.1#8305
orsenthil merged 17 commits into
python:mainfrom
handlerbot:fix-bpo-22708

Conversation

@handlerbot

@handlerbot handlerbot commented Jul 16, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Use protocol HTTP/1.1 when sending HTTP CONNECT tunnelling requests; generate Host: headers if one is not already provided (required by HTTP/1.1), convert IDN domains to punycode in HTTP CONNECT requests.

This builds upon and obsoletes #742. cc @berkerpeksag who reviewed that PR.

(I have signed the CLA, but it was less than a day ago, so the human review may not yet be completed.)

https://bugs.python.org/issue22708

@handlerbot
bpo-22708: Upgrade HTTP CONNECT to protocol HTTP/1.1 (GH-NNNN) …
9f5ce09 
Use protocol HTTP/1.1 when sending HTTP CONNECT tunnelling requests;
generate Host: headers if one is not already provided (required by
HTTP/1.1), convert IDN domains to punycode in HTTP CONNECT requests.
@handlerbot

handlerbot commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

Working on fix for BytesWarning now.

@berkerpeksag berkerpeksag self-requested a review July 17, 2018 14:07
@berkerpeksag

berkerpeksag commented Jul 17, 2018

Copy link
Copy Markdown
Member

Thanks for updating that PR, I've just added this to my TODO list, but it may take a while.

@berkerpeksag berkerpeksag mentioned this pull request Jul 17, 2018
Closed
@handlerbot

handlerbot commented Jan 27, 2019

Copy link
Copy Markdown
Contributor Author

Hi @berkerpeksag, circling back to this one, do you still have time and interest in the topic, or should we find another reviewer?

@akhayyat

akhayyat commented Jul 2, 2019

Copy link
Copy Markdown

Is this going to be merged any time soon?

@csabella csabella requested review from berkerpeksag and removed request for berkerpeksag February 6, 2020 22:42
@berkerpeksag

berkerpeksag commented Feb 7, 2020

Copy link
Copy Markdown
Member

Sorry, I will try to review this PR this weekend.

@0weight 0weight mentioned this pull request Feb 22, 2020
Closed
@keelson

keelson commented Sep 2, 2020

Copy link
Copy Markdown

any update?

this bug currently blocks any integretion with the linkerd2 proxy as this throws a 400 bad request for connections inititaded by python apps

https://github.com/linkerd/linkerd2/pull/1200/files

@mirkokg mirkokg mentioned this pull request Sep 23, 2020
Open
@merwok

merwok commented Sep 23, 2020

Copy link
Copy Markdown
Member

It crashes in _tunnel on response line:

Can you show the exact error for this?

«crash» usually describes a segfault of the interpreter, is that what happended or was it a regular Python exception?

@merwok

merwok commented Sep 23, 2020

Copy link
Copy Markdown
Member

Please answer: what exactly do you see when you say «it crashes»?

@handlerbot

handlerbot commented Mar 5, 2021

Copy link
Copy Markdown
Contributor Author

Hi folks -- would love to try again to get this merged. :-) cc @berkerpeksag who I solicited the original review from, and tagging in @orsenthil and @SethMichaelLarson (I think this is the same GitHub identity for the person who approved #20959, that's who I am looking for, sorry if I tagged the wrong person) who have also reviewed PRs for Lib/http/client.py in the past...

@handlerbot

handlerbot commented Mar 5, 2021

Copy link
Copy Markdown
Contributor Author

Sigh, I did mean @sethmlarson not @SethMichaelLarson, sorry about that, the GitHub autocompleter was determined to mislead me!

@orsenthil orsenthil self-assigned this Mar 5, 2021
@orsenthil

orsenthil commented Mar 5, 2021

Copy link
Copy Markdown
Member

Hi Michael, Thanks for following up. I will be able to review and bring it to a closure.

@GtTmy GtTmy mentioned this pull request Apr 19, 2021
Merged
@yilinjuang

yilinjuang commented Nov 7, 2021

Copy link
Copy Markdown

any updates on this? would love to see this merge as it prevents our services from using certain proxies. thanks

26 hidden items Load more…
@jalopezsilva jalopezsilva mentioned this pull request Oct 11, 2022
Open
@vo-va vo-va mannequin mentioned this pull request Dec 26, 2022
Closed
@lukaszgalezewski

lukaszgalezewski commented Jan 12, 2023

Copy link
Copy Markdown

why this PR is still open and not merged? and when can be merged? :)

@merwok merwok requested a review from orsenthil January 12, 2023 15:25
@orsenthil

orsenthil commented Jan 13, 2023

Copy link
Copy Markdown
Member

@handlerbot , @lukaszgalezewski - apologies. None of us go to it. I will review this in coming week and target for upcoming Python release (3.12, if features are allowed) otherwise we can go with 3.13.

@l8huang

l8huang commented Mar 15, 2023

Copy link
Copy Markdown

@orsenthil do you have any update on this? Hope this can be merged in python 3.12. I recently run into this issue, even latest python can't set protocol version for HTTP CONNECT is kind of embarrassment. Thanks.

@roman-vynar

roman-vynar commented Mar 22, 2023

Copy link
Copy Markdown

3.12, if features are allowed

It's not a feature, it's a shame. HTTP 1.0 is horribly obsolete, host header was added to HTTP 1.1 in 1997, this is 3000 years ago.
This ticket was open in 2018. Today we are in 2023. If this is merged into 3.12 for October it will be great.

@arhadthedev arhadthedev changed the title bpo-22708: Upgrade HTTP CONNECT to protocol HTTP/1.1 Mar 22, 2023
@arhadthedev

arhadthedev commented Mar 22, 2023

Copy link
Copy Markdown
Member

@vstinner @gpshead could you review and possible merge this PR please? (as committers into Lib/http/client.py)

orsenthil
orsenthil approved these changes Apr 5, 2023
View reviewed changes

@orsenthil orsenthil 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.

Hide comment

This looks good to me.

I reviewed for the choices made for handling unicode, and sending Host with HTTP 1.1 for CONNECT, when not specified. And verified if it is backwards compatible.

The feedback in the comments seem to say this helps for folks who had a need too.

This change looks good to me.

Apologize for taking long time.

@orsenthil orsenthil merged commit 1a8f862 into python:main Apr 5, 2023
gaogaotiantian pushed a commit to gaogaotiantian/cpython that referenced this pull request Apr 8, 2023
@handlerbot @merwok
@bgehman @arhadthedev @gaogaotiantian
pythongh-66897: Upgrade HTTP CONNECT to protocol HTTP/1.1 (python#8305) …
4867e9d 
* bpo-22708: Upgrade HTTP CONNECT to protocol HTTP/1.1 (GH-NNNN)

Use protocol HTTP/1.1 when sending HTTP CONNECT tunnelling requests;
generate Host: headers if one is not already provided (required by
HTTP/1.1), convert IDN domains to punycode in HTTP CONNECT requests.

* Refactor tests to pass under -bb (fix ByteWarnings); missed some lines >80.

* Use consistent 'tunnelling' spelling in Lib/http/client.py

* Lib/test/test_httplib: Remove remnant of obsoleted test.

* Use dict.copy() not copy.copy()

* fix version changed

* Update Lib/http/client.py

Co-authored-by: bgehman <bgehman@users.noreply.github.com>

* Switch to for/else: syntax, as suggested

* Don't use for: else:

* Sure, fine, w/e

* Oops

* 1nm to the left

---------

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: bgehman <bgehman@users.noreply.github.com>
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@handlerbot @merwok
@bgehman @arhadthedev @warsaw
pythongh-66897: Upgrade HTTP CONNECT to protocol HTTP/1.1 (python#8305) …
3eb10b2 
* bpo-22708: Upgrade HTTP CONNECT to protocol HTTP/1.1 (GH-NNNN)

Use protocol HTTP/1.1 when sending HTTP CONNECT tunnelling requests;
generate Host: headers if one is not already provided (required by
HTTP/1.1), convert IDN domains to punycode in HTTP CONNECT requests.

* Refactor tests to pass under -bb (fix ByteWarnings); missed some lines >80.

* Use consistent 'tunnelling' spelling in Lib/http/client.py

* Lib/test/test_httplib: Remove remnant of obsoleted test.

* Use dict.copy() not copy.copy()

* fix version changed

* Update Lib/http/client.py

Co-authored-by: bgehman <bgehman@users.noreply.github.com>

* Switch to for/else: syntax, as suggested

* Don't use for: else:

* Sure, fine, w/e

* Oops

* 1nm to the left

---------

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: bgehman <bgehman@users.noreply.github.com>
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
@nametkin nametkin mentioned this pull request Apr 23, 2023
Merged
@hugovk

hugovk commented May 20, 2023

Copy link
Copy Markdown
Member

@orsenthil Shall we also backport this to 3.11?

@gpshead

gpshead commented May 20, 2023

Copy link
Copy Markdown
Member

@orsenthil Shall we also backport this to 3.11?

No, this is more of a feature. it's a notable behavior change, not a mere unintended behavior fix.

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

Reviewers

@merwok merwok merwok left review comments

@ammaraskar ammaraskar ammaraskar left review comments

@orsenthil orsenthil orsenthil approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

+2 more reviewers

@bgehman bgehman bgehman left review comments

@nametkin nametkin nametkin left review comments

Assignees

@orsenthil orsenthil

Labels

stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.