Return local IP when proxy IP detection fails #4
Conversation
shadowhand
force-pushed
the
fix/proxy-ip-detect
branch
from
09fac8c to
edf1435
Compare
src/ClientIp.php
Outdated
| if ($ip = $this->getLocalIp($request)) { | ||
| if (!in_array($ip, $this->proxyIps)) { | ||
| return $ip; | ||
| } |
There was a problem hiding this comment.
I refactored this because it was really hard for me to follow as written. Functionality remains the same, with better separation of concerns.
There was a problem hiding this comment.
I think the functionality does not remain the same. The logic should be:
- Get the ip using the default way (
REMOTE_ADDR). Let's call it$localIp - If proxies hasn't been configured returns
$localIp - If there's an array of allowed proxy ips, check if
$localIpis in this array. If it's not, returns$localIp. - Get the new ip using the proxy headers and return it if it's found, or
$localIpif doesn't.
In this refactoring $this->proxyIps is used to check the ip returned by $this->getProxyIp(), it should used to check the ip returned by $this->getLocalIp()
There was a problem hiding this comment.
Ah, it makes sense now. The purpose of proxyIps was a bit confusing. Will update.
There was a problem hiding this comment.
Feel free to improve readme for a better understanding of proxyIps functionality.
There was a problem hiding this comment.
Okay, still a bit confused. Shouldn't the choices be:
- If remote IP detection is enabled, attempt to return remote IP address.
- If proxy headers are enabled, attempt to return proxied IP address.
- If local IP address is found:
- If proxy IPs are defined, and the local IP is in the proxy list, void the local IP.
- Otherwise return the local IP.
There was a problem hiding this comment.
You may want to configure the proxies with or without ips. No ips means you're trusting in all proxies, so just try to get the ip from headers and use the remote_addr as fallback. Obviously, this less secure. Setting an array of ips of well known proxies, get the ip only from any of these proxies, and use remote_addr as fallback too.
There was a problem hiding this comment.
I think the current changes follow the correct order.
There was a problem hiding this comment.
Ah, there's a subtle difference that I missed that the tests are not accounting for:
The local IP has to be checked against proxy IPs if the proxy IP list is defined. Otherwise something weird is happening. Got it.
|
Scrutinizer liked my changes. 😎 |
src/ClientIp.php
Outdated
| if ($ip = $this->getLocalIp($request)) { | ||
| if (!in_array($ip, $this->proxyIps)) { | ||
| return $ip; | ||
| } |
There was a problem hiding this comment.
I think the functionality does not remain the same. The logic should be:
- Get the ip using the default way (
REMOTE_ADDR). Let's call it$localIp - If proxies hasn't been configured returns
$localIp - If there's an array of allowed proxy ips, check if
$localIpis in this array. If it's not, returns$localIp. - Get the new ip using the proxy headers and return it if it's found, or
$localIpif doesn't.
In this refactoring $this->proxyIps is used to check the ip returned by $this->getProxyIp(), it should used to check the ip returned by $this->getLocalIp()
shadowhand
force-pushed
the
fix/proxy-ip-detect
branch
from
edf1435 to
17fc737
Compare
shadowhand
force-pushed
the
fix/proxy-ip-detect
branch
from
17fc737 to
013777f
Compare
| } | ||
|
|
||
| foreach ($this->proxyHeaders as $name) { | ||
| if ($request->hasHeader($name) && ($ip = self::getHeaderIp($request->getHeaderLine($name))) !== null) { | ||
| return $localIp; |
There was a problem hiding this comment.
@oscarotero okay, functionality should be a match with existing code. Verify for me?
| } | ||
|
|
||
| foreach ($this->proxyHeaders as $name) { | ||
| if ($request->hasHeader($name) && ($ip = self::getHeaderIp($request->getHeaderLine($name))) !== null) { | ||
| return $localIp; |
|
Released in v0.5.0. |
|
Well, this is a bugfix, so it could be released as 0.4.1. And the link to compare this version with the previous in changelog is missing. |
|
@oscarotero 🤦♂️ good point on the version. I updated the changelog to include the compare link. |
Fixes #3