`FilterUsingXForwardedHeaders` should correctly deal with `<host>:<port>` pair in `X-FORWARDED-HOST` header about laminas-diactoros HOT 9 CLOSED

So it's a kind of little BC Break too :)

Looks like a regular bug/regression to me.

I get wierd URI from ServerRequestFactory::fromGlobals with port duplicated like https://host:port:port/path.

@3DFace would you be able to provide a failing test case? See tests introduced in e08e52e for reference.

from laminas-diactoros.

It depends if the upstream webserver is something like nginx, Caddy or apache2: all of those are very popular, and de-facto standards (regardless of what Mozilla says)

from laminas-diactoros.

Hm, I am not 100% sure if the port is expected to be passed via X-Forwarded-Host.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host

Did we properly support that edge-case before?

from laminas-diactoros.

I just got this exact same problem when using local-ssl-proxy in my dev env (it sends the port as part of XFH header).

This should work as a temporary fix:

if (($uri = $request->getUri()) && ($port = $uri->getPort()) && (str_ends_with($uri->getHost(), ":$port"))) {
    $request = $request->withUri($uri->withHost(substr($uri->getHost(), 0, -strlen((string)$port) - 1)));
}

from laminas-diactoros.

The question is, what port has the higher priority?
X-Forwarded-Port or X-Forwarded-Host?
Thats why a unit test is needed since I have no clue what the expected behavior is.

I stick with it that at least the non-browser-specific mozilla documentation says that X-Forwarded-Host does NOT contain a port.

It depends if the upstream webserver is something like nginx, Caddy or apache2: all of those are very popular, and de-facto standards (regardless of what Mozilla says)

Got that, but what if all behave different? Pretty sure we are not providing per-server features.

FWIW

NGINX does document X-Forwarded-Host header without port:
https://www.nginx.com/resources/wiki/start/topics/examples/forwarded/

Caddy refers to Mozilla doc when it comes to X-Forwarded-Host:
https://caddyserver.com/docs/caddyfile/directives/reverse_proxy#defaults

Apache2 states that X-Forwarded-Host contains the value of the original Host header (which usually does not include port):
https://httpd.apache.org/docs/2.4/en/mod/mod_proxy.html#x-headers

So apache2 seems to be the one who also adds :port to X-Forwarded-Host since Host header (as per mozilla can contain the port.

The only thing I wonder is: what if these ports do differ. What if the Host header contains port 80 while the originating request was sent to 443. Thats why I'd say we should probably just ignore that there can be a port within X-Forwarded-Host and just strip it.

I just got this exact same problem when using local-ssl-proxy in my dev env (it sends the port as part of XFH header).

This should work as a temporary fix:

if (($uri = $request->getUri()) && ($port = $uri->getPort()) && (str_ends_with($uri->getHost(), ":$port"))) {
    $request = $request->withUri($uri->withHost(substr($uri->getHost(), 0, -strlen((string)$port) - 1)));
}

Could you probably provide your stack? I've quickly jumped into the source of local-ssl-proxy and it uses http-proxy which only passes X-Forwarded-For, X-Forwarded-Port, X-Forwarded-Proto from incoming requests:

https://github.com/http-party/node-http-proxy/blob/9b96cd725127a024dabebec6c7ea8c807272223d/lib/http-proxy/passes/ws-incoming.js#L52-L67

https://github.com/http-party/node-http-proxy/blob/9b96cd725127a024dabebec6c7ea8c807272223d/lib/http-proxy/passes/web-incoming.js#L68-L86

So I can only guess that there has to be another HTTP Server in front of the proxy which already passes invalid X-Forwarded-Host, maybe even the same webserver as the author of this issue uses.

from laminas-diactoros.

In my case, chrome -> local-ssl-proxy -> php built-in server.

Looking at the code of http-proxy, I guess the problem relates to this line:
https://github.com/http-party/node-http-proxy/blob/9b96cd725127a024dabebec6c7ea8c807272223d/lib/http-proxy/passes/web-incoming.js#L85

Note that in the end it defaults to the host header, which at some point also included the port.

If I remember my original problem, it was about the port being duplicated in the url (causing a broken url).

from laminas-diactoros.

Yah, thanks for feedback @uuf6429! I've discovered the same from the apache documentation. They do pass Host header as well which (at least due to the Mozilla documentation) can contain a port.
I have no clue why it is allowed to have a port via Host since the connection can be initiated against port 443 (SSL) while the Host header can still contain Port 80 🤷🏼‍♂️

Not 100% sure but I guess that NGINX and Caddy are actually removing any port from Host as it has nothing to do with what was actually connected to.

i.e.:

$ curl -H "Host: localhost:80" https://localhost:443/

So what should the X-Forwarded-Port or the port returned from UriInterface#getPort represent?

from laminas-diactoros.

My thoughts on this are are:

1. $uri->getHost() should definitely be fixed - it shouldn't contain a port (the fix itself shouldn't break anything)
2. the original headers should stay as they were, even if contradictory (forwarded_host=hosXt:portX, forwarded_port=portY)
3. if there is a clear case of portX preferred over portY because "it's always so", maybe consider following that logic..
4. ..otherwise, keep the current logic and discard the extra port when it is set twice
5. ..and maybe trigger a warning/log message when both are set to different values
6. or if we want to be extremely strict about it, reject the request with status 503 or similar.

I have a strong opinion for point 1 & 2. In case of 3 & 4, I think we should strongly consider point 5.

from laminas-diactoros.

Should be fixed as of 2.24.0

from laminas-diactoros.