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

Require Host request header for HTTP/1.1 requests #404

Merged
merged 1 commit into from
Apr 10, 2021

Conversation

clue
Copy link
Member

@clue clue commented Apr 9, 2021

All HTTP/1.1 requests require a Host request header as per RFC 7230. The
proxy CONNECT method is an exception to this rule because we do not want
to break compatibility with common HTTP proxy clients that do not
strictly follow the RFCs.

This does not affect valid HTTP/1.1 requests and has no effect on
HTTP/1.0 requests.

Additionally, make sure we do not include a default Host request header
in the parsed request object if the incoming request does not make use
of the Host request header.

I've stumbled upon this when writing integration tests for https://twitter.com/x_framework that test common HTTP server behavior. With this changeset applied, this project now behaves just like nginx, Apache and PHP's built-in web server:

$ curl -v http://localhost:8080/headers -H User-Agent: -H Accept: -H Host: -10

I've also checked benchmark results and I'm seeing a consistent, but minor improvement from ~11k rps to ~12k rps on my machine (#162).

Builds on top of #201
Refs #208, golang/go#18215 and others

All HTTP/1.1 requests require a Host request header as per RFC 7230. The
proxy CONNECT method is an exception to this rule because we do not want
to break compatibility with common HTTP proxy clients that do not
strictly follow the RFCs.

This does not affect valid HTTP/1.1 requests and has no effect on
HTTP/1.0 requests.

Additionally, make sure we do not include a default Host request header
in the parsed request object if the incoming request does not make use
of the Host request header.
@WyriHaximus WyriHaximus merged commit c22fde6 into reactphp:master Apr 10, 2021
@clue clue deleted the require-host branch April 10, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants