Skip to content
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

Send only one Host header in chunked request #5391

Merged
merged 2 commits into from Nov 29, 2021
Merged

Conversation

hodbn
Copy link
Contributor

@hodbn hodbn commented Mar 17, 2020

Send only one Host header when specifying a custom Host header in a chunked request.

Before this fix, the default Host header was sent along with the custom Host header.

Closes #5274

@hodbn hodbn force-pushed the fix-5274 branch 2 times, most recently from e982e4c to 73f88ce Compare March 17, 2020 22:24
AaronRobson
AaronRobson previously approved these changes Jun 6, 2020
@hodbn hodbn force-pushed the fix-5274 branch 4 times, most recently from 4de7ad2 to bc3b46e Compare August 14, 2020 14:57
@hodbn hodbn changed the title fix issue #5274 Send only one Host header in chunked request Nov 14, 2020
@hodbn hodbn force-pushed the fix-5274 branch 2 times, most recently from 925dd80 to 24a3641 Compare November 14, 2020 21:00
@hodbn
Copy link
Contributor Author

hodbn commented Nov 14, 2020

Hi @AaronRobson :)
Can you take another look at this?

@@ -459,7 +459,8 @@ def send(self, request, stream=False, timeout=None, verify=True, cert=None, prox
try:
low_conn.putrequest(request.method,
url,
skip_accept_encoding=True)
skip_accept_encoding=True,
skip_host='Host' in request.headers)

Choose a reason for hiding this comment

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

That's good, this appears to be the same way that the same situation is handled in the urllib3 code itself: https://github.com/urllib3/urllib3/blob/74d6be1ab66cef44c0f479c24b0fc1756a8fe4e9/src/urllib3/connection.py#L243-L246

@nateprewitt nateprewitt changed the base branch from master to main September 2, 2021 08:40
@nateprewitt nateprewitt added this to the 2.27.0 milestone Sep 2, 2021
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Thanks for digging in a finding a simple solution here, @hodbn! This seems like a straight forward win that alleviates the risks of other patches we've had for #5274. Apologies this has been sitting so long.

I've added some minor test refactoring to make these easier to expand later and rebased your changes onto our main branch. I think this should be set!

@sethmlarson
Copy link
Member

@nateprewitt What's the state of this PR?

@nateprewitt
Copy link
Member

It's ready, it just needs to be rebased to rerun tests and we can merge. It's on my list for Monday now that 2.27.0 is unblocked.

@nateprewitt nateprewitt merged commit 7556ea4 into psf:main Nov 29, 2021
@nateprewitt
Copy link
Member

Thanks again @hodbn!

@nateprewitt nateprewitt mentioned this pull request Dec 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two Host header are being sent when doing chunk transfer with Session
4 participants