search

laminas / laminas-diactoros

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

Add test to detect regression described in #111 #112

Closed 3DFace closed 1 year ago

3DFace commented 2 years ago

Signed-off-by: Denis Ponomarev ponomarev@gmail.com

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

FilterUsingXForwardedHeaders should accept host:port pair in X-FORWARDED-HOST header.

Test added to detect it parsed incorrectly.

Ocramius commented 2 years ago

Ok, now the question is just what to do with it inside the marshalling logic :D

boesing commented 2 years ago

Ok, now the question is just what to do with it inside the marshalling logic :D

I'd say we should provide the pre-fix solution. I can not have a look into it right now but lets see what the outcome of my question is

3DFace commented 2 years ago

BTW this is my first ever pull-request. I will be grateful for explicit instructions if any actions are expected from me.

Ocramius commented 2 years ago

@3DFace so far, all is correct, now we just need to fix this :D

If you have a clear idea of what is needed, you can just push new commits to your branch (with the proposed changes), otherwise somebody else will take your commits and propose a patch.

If you have time and capacity, then just give it a shot: change the sources that affect this test outcome, until the tests are all green again.

Ocramius commented 2 years ago

As for the other 5 unrelated commits: I will get rid of them before merging, or they can be rebased out: that's because this is a bugfix, so it should go to 2.13.x (next patch release branch), and your patch started from the default branch (2.14.x).

That's not a big problem though, and can be addressed later.

3DFace commented 2 years ago

As for the other 5 unrelated commits

Thank you for clarifications! I was a bit confused about it.

boesing commented 1 year ago

I've created a fix in #111

Due to the fact that at least apache is blindly passing Host header via X-Forwarded-Host and the Host header allows containing a port (even tho it won't do anything), I'd say the correct behavior would be to simply ignore the port from X-Forwarded-Host.

If the behavior should be that we are able to determine the initial request / port, we shouldn't use X-Forwarded-Host or X-Forwarded-Port as thats always those values from the latest proxy (at least that is what I would assume, not sure if proxies do actually pass these values in case they're existing). Instead, we might want to use Forwarded which contains the whole history (in correct order) of all proxies which interacted with the request containing the hostnames they received. But that would be a whole another topic.

In the OP scenario, that would be traefik. We are only able to determine those headers from NGINX which are most probably not those we might want to use here.

weierophinney commented 1 year ago

@boesing Should we close this in favor of the solution in #135?

boesing commented 1 year ago

@weierophinney would be fine for me, I guess the issue is covered?