-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add missing support for Host-header with port-number #26
Conversation
….ietf.org/html/rfc2616#section-14.23), may contain a port-number. The current implementation doesn't handle this, and effectively ends up producing an `UriInterface` instance that returns a host-name string with the port-number inside it. PSR-7 actually isn't explicit on this point, but I'd have to assume that, since there's a `getPort()` method for obtaining the port-number, then `getHost()` isn't supposed to also return the port-number, just the host-name. This PR includes a regression-test (which did fail) and a fix.
@Nyholm any chance you could take a look at this soon? This bug generates a failing test and is currently blocking a release for me. Let me know if there's anything else I can do to help. |
Is it safe to always prefer a user-supplied (as the Host header is) port over the server provided one? Not saying I doubt it, I simply have no information on the matter at al. |
I've been searching and didn't find anything useful on this subject to suggest that this is a concern. As far as I can figure, if the request made it to your app, the port-number is not a liability, unless you somehow manage to make it into one. In other words, you shouldn't trust Bottom line, if you trust headers, you better know what you're doing. |
If it isn't clear, this is a bugfix - Is @Nyholm on vacation or something? |
I don’t know. I only know I am getting used to a new job (and the commute that comes with it) meaning I spent less time on GitHub than I might want to.
I guess I was thinking that PHP somehow magically knew on what port the web server would’ve been running. Makes sense that this isn’t true (outside of I just know of tales where misuse of
I think the problem with abstractions like PSR-7’s The answer to that is probably: no we can’t. And as psr7-server already relies on
Agreed, we definitely shouldn’t be dropping the entire thing just in there as is done now. One thing that wasn’t fully clear to me from the cited RFC 2616: is the If we are fixing Host header parser, I’d love to get it right! (And I am sorry I haven’t been able to put more time into it!) |
It can be, yes - I've adjusted the regex and added a regression-test, plus some additional tests for IP-addresses in the host-header. |
For now, we've published a replacement package |
Looks like you get this issue if you run the script with |
Finally found time to test this locally, and I think it all looks good. I pushed a style fix, and if CI turns up green I am planning to merge this 👍 |
Awesome. Thank you @Zegnat for the review and @mindplay-dk for the patch. |
@Nyholm you seem to have a specific way you would like merges to happen? With mentions of the PR numbers in the git message? Could you merge this and/or point me to a guide on how you would like merges to happen on this repo? |
Not at all.
That happens automatically. Just do a "squash and merge" and you will be fine. |
Ah, so that’s where it comes from! I am trying to merge from the command line more often these days, so all the default GitHub is applying aren’t some thing I was thinking about. You could consider disabling the other merge options in settings to make Squash and Merge the default. |
Sure. Thanks |
The
Host
header, according to RFC2616 section 14.23, may contain a port-number.The current implementation doesn't handle this, and effectively ends up producing an
UriInterface
instance that returns a host-name string with the port-number inside it.PSR-7 actually isn't explicit on this point, but I'd have to assume that, since there's a
getPort()
method for obtaining the port-number, thengetHost()
isn't supposed to also return the port-number, just the host-name.This PR includes a regression-test (which did fail) and a fix.