Add missing support for Host-header with port-number

[mindplay-dk]

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, 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.

[mindplay-dk]

@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.

[Zegnat]

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.

[mindplay-dk]

@Zegnat good question.

Maybe $_SERVER["SERVER_PORT"]?

Though, according to PHP docs:

It is not safe to rely on this value in security-dependent contexts.

Is there an alternative?

[mindplay-dk]

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 getPort() anymore than you trust $_SERVER["SERVER_PORT"] or the Host header. I think our responsibility ends with parsing the header according to the standard - we can't prevent anyone from shooting themselves in the foot, which they could just do by simply calling getHeader("Host") and parsing the port-number themselves anyway.

Bottom line, if you trust headers, you better know what you're doing.

[mindplay-dk]

If it isn't clear, this is a bugfix - getHost() is supposed to return the host-name, not the host-header.

Is @Nyholm on vacation or something?

[Zegnat]

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.

In other words, you shouldn't trust getPort() anymore than you trust $_SERVER["SERVER_PORT"] or the Host header.

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 php -S, perhaps), so of course all information about the port should be treated with some scepticism.

I just know of tales where misuse of Host headers have led to issues. Web servers will often respond with whatever their default “website” is for non-matched host values, and if those are then depending on the host value in any way it can lead to trouble. I was wondering if there was a similar issue with the port info going on.

Bottom line, if you trust headers, you better know what you're doing.

I think the problem with abstractions like PSR-7’s ServerRequestInterface is that people do not always realise getUri() isn’t by definition the same as “the URL entered into a browser”. It isn’t even always the canonical URL of the resource being loaded, because of things like the Host header being UA defined. So I was just wondering if there is a way for us to mitigate that somehow.

The answer to that is probably: no we can’t. And as psr7-server already relies on HTTP_HOST and SERVER_PORT here, this PR makes sense.

If it isn't clear, this is a bugfix - getHost() is supposed to return the host-name, not the host-header.

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 Host header sent for IP-based requests? Because if it is, your regular expression will fail as it doesn’t allow for : in the hostname, which would happen in the case of something like https://[2606:2800:220:1:248:1893:25c8:1946]:443/

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!)

[mindplay-dk]

One thing that wasn’t fully clear to me from the cited RFC 2616: is the Host header sent for IP-based requests?

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.

[mindplay-dk]

For now, we've published a replacement package kodus/psr7-server, since we have several projects/libraries that depend on this package.

[mahagr]

Looks like you get this issue if you run the script with php -S localhost:8001 index.php

[Zegnat]

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 👍

[Nyholm]

Awesome. Thank you @Zegnat for the review and @mindplay-dk for the patch.

[Zegnat]

@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?

[Nyholm]

@Nyholm you seem to have a specific way you would like merges to happen?

Not at all.

With mentions of the PR numbers in the git message?

That happens automatically. Just do a "squash and merge" and you will be fine.

[Zegnat]

With mentions of the PR numbers in the git message?

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.

[Nyholm]

Sure. Thanks