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

Make Host header be always the first in requests #723

Merged
merged 3 commits into from Oct 16, 2020

Conversation

@andrepiske
Copy link
Contributor

@andrepiske andrepiske commented Sep 25, 2020

I observed that some servers (Cloudflare, for instance) will fail the request or will respond with redirects to the same URL if the Host header comes too late in the request.

I think this actually make sense from a security perspective, as having Host late in the request would mean a server with virtual hosts would have to buffer everything before the Host header before deciding where the request will land and whether the virtual host is even valid.

RFC 7230 section 5.4 supports this, stating that:

Since the Host field-value is critical information for handling a
request, a user agent SHOULD generate Host as the first header field
following the request-line.

I propose here this changes that always lifts the Host header to be the first one sent. By looking at the previous lines, the if condition shouldn't alter semantics as I don't see how datum[:headers]['Host'] could be nil.

What do you think? Happy to discuss solutions.

Would be also nice if someone could point me into which file I could add tests/specs :)

RFC 7230 5.4 tells that Host SHOULD be the first
header sent.

Some servers will refuse the request or redirect
it indefinitely if the total HTTP content before
the Host header is too large.
@geemus
Copy link
Contributor

@geemus geemus commented Oct 2, 2020

Thanks for the detailed report. Looks good.

Testing is trickier here, I think it might require a custom server to check things. Did you have any particular ideas/thoughts?

@andrepiske
Copy link
Contributor Author

@andrepiske andrepiske commented Oct 8, 2020

Hey sorry for the delay.
It seems the request_headers.ru file already had enough to test this :)

Let me know if this seems good to you!

@geemus
Copy link
Contributor

@geemus geemus commented Oct 9, 2020

Good catch, I forgot we already had that (it's been a while since I needed to dig deeper).

lib/excon/connection.rb Show resolved Hide resolved
@geemus
geemus approved these changes Oct 16, 2020
@geemus geemus merged commit 6b20581 into excon:master Oct 16, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geemus
Copy link
Contributor

@geemus geemus commented Oct 16, 2020

@andrepiske Thanks!

Released as part of 0.77.0.

@andrepiske andrepiske deleted the andrepiske:host-always-first-header branch Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.