laminas / laminas-diactoros

PSR HTTP Message implementations
https://docs.laminas.dev/laminas-diactoros/
BSD 3-Clause "New" or "Revised" License
483 stars 63 forks source link

Examine `X-Forwarded-Host` for additional port information #135

Closed boesing closed 1 year ago

boesing commented 1 year ago
Q A
Bugfix yes

Description

Fixes #111

boesing commented 1 year ago

TBH: Not even sure if it makes any sense to pass a port via Host header. When I connect via TCP to port 443 and add port 80 to the Host header, it will still connect to 443 tho... I wonder if any webserver out there then tries to internally connect to port 80. I hope they won't as this could be a serious security problem when some1 can use a webserver to actually connect to another port internally.

I guess most if not any webserver (besides apache2) do not pass any port via X-Forwarded-Host header but since there are already 2 reports regarding the same bug, I'd say lets fix this.

TimWolla commented 1 year ago

though what it looks like in here is it is properly considering X-Forwarded-Port as the "source of truth" if both that and X-Forwarded-Host are present.

@weierophinney From my understanding, the priority is implicitly decided based on the ordering of the headers within the $trustedHeaders list. I find this reasonable, but it should probably be spelled out explicitly that the order of headers matters (if this is the behavior we want to guarantee here).


The current version of the PR looks reasonable to me as well, the port is taken from the xfh header (if available), if both xfh and xfp match, it does the right thing by definition and if they differ, then the user is able to select the priority based on the ordering in the list. (If xfh and xfp differ, the reverse proxy is broken beyond repair, so … who cares).

Please just give me the time to also check the Traefik source code / behavior (see the existing comment thread with boesing), so that the decision can be backed up with some hard evidence in case someone complains in the future.