Addressed how wildcards are handled so it functions with multiple load balancers

[simonworkhouse]

This could either be an issue with the documentation and comments, or it could be an issue with the way that wildcards are being handled. Because of that, I have only created this as a draft pull request to prompt discussion on the matter and in the hope that either the code or the documentation are fixed.

From the documentation and code comments, I am concluding that the intent of using * as a wildcard is to trust all proxies, however; all it currently appears to do is trust the calling IP address and not every IP in the chain.

For example, if a request goes through the following flow CloudFlare > DigitalOcean > Kubernetes one may end up with the something akin to 123.456.0.1, 172.68.0.1, 104.16.0.1 for the value of $_SERVER['HTTP_X_FORWARDED_FOR'] where 123.456.0.1 is the actual source IP. When using * as a wildcard, this package erroneously returns 104.16.0.1 when calling request()->ip() instead of the expected result.

This pull request addresses this issue and corrects the order of IPs in tests so that they match what's received in real world scenarios. The reversed IP addresses are also an indicator that this is a code issue, that is, assuming that wasn't the intent, and if it was the intent, then that would ideally be handled in a separate code block or perhaps some comments added to indicate so.

Other open issues also mention this, for example, #115 and #107

[fideloper]

Thanks!

That's a good point, this may be better addressed in the documentation.

I think it comes down to whether we want the option to use one star to mean "only allow one proxy" and two stars to mean "allow any number of proxies" or if we want to just always allow multiple proxies.

I don't know the security ramifications off the top of my head there.

[LukeTowers]

@fideloper see wintercms/storm@411695b for background information on the origin of the ** value, I'm not sure why it was removed in v4, but probably the best option is to reinstate the original behaviour.

[kellerjmrtn]

@fideloper are you still accepting new issues/PRs here? Or should everything go to the laravel/framework repo now?

I'm having the same issue as OP, where my requests are travelling through more than one proxy (Cloudflare > AWS Load Balancer > Web Server), and the wildcard * does not trust both proxies, only the Load Balancer. I submitted laravel/framework#49168, but Taylor has (understandably) closed it, so for now, I plan on implementing a similar fix to that PR in my project code

I do think that reinstating the ** behavior or implementing a fix like in that PR would be a helpful addition here though, for people using multiple proxies. Just thought I'd bump this issue if you're still accepting changes

[fideloper]

If you want to make a PR for that, I'm happy to accept it for those using older Laravel! (Laravel 9+ doesn't use this repo)

[fideloper]

(Would that be this PR? I could see this one MAYBE being a breaking change for those relying on it getting the IP?)

[kellerjmrtn]

Good point, changing behavior for * could break stuff. If ** is deprecated though, we could reintroduce the functionality just for that, maybe? Something like:

if * => trust the first proxy
if ** => trust the whole proxy chain

I can open a new PR for that

(The PR I opened to laravel/framework allowed specifying n "layers" of the chain to trust, but that may be more complex behavior then anyone really needs)

[LukeTowers]

@kellerjmrtn I'm interested in the n layers approach, perhaps where the amount of * characters indicates the number of layers to trust?

With the security concern that you mentioned of trusting the entire chain, does that mean that the client can include the X-Forwarded-For header in their initial request and then have the proxies include that? I.e:

Client IP: client.ip
Client X-Forwarded-For: fake.ip
CloudFlare IP: cf.ip
AWS ELB IP: aws.ip

Does that result in a final value of X-Forwarded-For: client.ip, fake.ip, cf.ip, aws.ip? If so, wouldn't the logic to get the client's IP still just grab the client.ip or am I missing something here?