[HttpFoundation][FrameworkBundle] Add CIDR notation support in trusted proxy list #7312
Conversation
| } | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
Is breaking intended here ?
Maybe Continue should be return ?
There was a problem hiding this comment.
If checkIp() was able to match against array, I could simplify it to this:
do {
$clientIp = array_pop($clientIps);
if (!IpUtils::checkIp($clientIp, $trustedProxy)) {
return $clientIp;
}
} while ($clientIps);
But for now, since we need to check other trusted proxies in the loop, we can't return when false === checkIp(), and we can't return when true === checkIp() as well, since there may be another trusted proxy up the list. That's why I use continue within inner loop, and then break outer loop in case $clientIp does not match any of trusted ips.
There was a problem hiding this comment.
Or maybe that looks better?
$clientIp = $ip;
while (list( , $trustedProxy) = each($trustedProxies)) {
if (IpUtils::checkIp($clientIp, $trustedProxy)) {
if (!$clientIps) {
return $clientIp;
}
$clientIp = array_pop($clientIps);
reset($trustedProxies);
}
}
There was a problem hiding this comment.
Why not:
foreach (array_reverse($clientIps) as $clientIp) {
foreach ($trustedProxies as $trustedProxy) {
if (IpUtils::checkIp($clientIp, $trustedProxy)) {
continue;
}
}
return $clientIp;
}
There was a problem hiding this comment.
Yeah, that's much more readable! 👍
Adds the ability to use CIDR notation in the trusted proxy list
|
+1 we are stuck on the deprecated trust_proxy_headers because the Amazon ELB changes its IP's |
|
👍 for the amazon issue here too |
|
Closing in favor of #7735 |
This PR was merged into the master branch. Discussion ---------- [HttpFoundation][FrameworkBundle] Add CIDR notation support in trusted proxy list | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7312, #7262 | License | MIT | Doc PR | symfony/symfony-docs#2287 Should be rebased once #7734 is merged. Commits ------- 7b32794 [HttpFoundation] updated CHANGELOG e7c1696 [HttpFoundation] refactored code to avoid code duplication 1695067 [HttpFoundation] added some unit tests for ranges of trusted IP addresses 811434f Allow setting trusted_proxies using CIDR notation ddc9e38 Modify Request::getClientIp() to use IpUtils::checkIp()
Added CIDR notation support as requested in #7262.
Also, as long as we need to check against a list of IPs at least in 3 places:
does it make sense to modify IpUtils' methods to also accept array as second argument?