search

Nyholm / psr7-server

Helper classes to use any PSR7 implementation as your main request and response
MIT License
90 stars 21 forks source link

Make sure the Host header only contains 1 host #49

Open paynl-wesley opened 3 years ago

paynl-wesley commented 3 years ago

Make sure the Host header always contains 1 value by always resetting the header with the first found value.

This fixes the issue of multiple hosts in the Host header field as reported: https://github.com/Nyholm/psr7-server/issues/48

Zegnat commented 2 years ago

This is a really nice catch! Could you also add a test for the behaviour?

I am also wondering which Host value would be the correct one to keep in the final object. My gut feeling is that we want to keep the value of the actual Host header that came with the request. The first value that you are defaulting to here, if I understand it correctly, is the value set by the ServerRequest constructor. According to PSR-7 a generated value should only be used if no other is provided:

During construction, implementations MUST attempt to set the Host header from a provided URI if no Host header is provided.

So maybe we should be defaulting to either the second value or the last value, if there are multiple values? There is also something to say for passing through multiple if the HTTP request contained multiple Host headers. Just as long as we know for sure that none of the headers are there because we put it there.

paynl-wesley commented 2 years ago

I agree that we should pass on multiple Host values if these were actually in the request. We want to reflect how the request was sent, even if it contains errors.

I have updated my fix to remove the Host header that was added from the URI if a Host header was present in the $headers array. In this case the Host header will not be added twice, it will contain all Host header values in the request and will fallback to the URI host if there is no Host header present.

Also, I have added a test to check that there is only 1 host header present when using the fromGlobals() function.

Please let me know if what you think of this solution :)

ajgarlag commented 2 years ago

@paynl-wesley Thanks for the PR. It fixed a problem in my app. I hope it can be merged and released soon.

paynl-wesley commented 2 years ago

@ajgarlag No problem! I'm glad someone found it useful!

@Zegnat Is there any chance this PR could be merged?