Skip to content

Conversation

@aaronbonneau
Copy link
Contributor

RFC 7230 requires "A client MUST send a Host header field in all HTTP/1.1 request messages."

}

if ($request->getProtocolVersion() === '1.1' && !$request->hasHeader('Host')){
thrownew \InvalidArgumentException('A client must send a host header field in all HTTP/1.1 request messages');

Choose a reason for hiding this comment

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

Why \InvalidArgumentException?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The parameter to parseRequest ($headers) is not a valid argument.

Also, all exceptions in this method are InvalidArgumentExceptions so I was just keeping the format.

@clue
Copy link
Member

clue commented Jul 27, 2017

Thanks for filing this PR! 👍 I see where you're coming from and would love to hear some more thoughts on this!

I think the (more) relevant quote from the RFC would be this:

A server MUST respond with a 400 (Bad Request) status code to any
HTTP/1.1 request message that lacks a Host header field and to any
request message that contains more than one Host header field or a
Host header field with an invalid field-value.

A similar patch was filed with #127 which was later reverted partly by #173 and #201 (all by me in fact). This was done both for consistency between HTTP/1.1 and HTTP/1.0 requests and because I've been running this project in production for a few months now and (despite violating RFC 7230) have repeatedly seen HTTP/1.1 requests without a Host header in production (leproxy/leproxy#16).

The question actually boils down to: Is a Host header really required here and/or do we miss anything if we disregard the specs here?

@clue
Copy link
Member

clue commented Aug 10, 2017

Ping @aaronbonneau, what's the status here? 👍

@clue
Copy link
Member

clue commented Sep 5, 2017

I'm closing this for now as it hasn't received any input in a while and I believe this has been answered. Please come back with more details if this problem persists and we can reopen this 👍

Sign up for freeto 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.

3 participants

@aaronbonneau@clue@kelunik