[HttpFoundation] IpUtils::checkIp4() should allow /0 networks
#14690
Conversation
….0.0.0/0" as a valid mask (that includes every IPv4 address) See http://tools.ietf.org/html/rfc4632#section-3.1
|
This looks wrong, |
…base IP address is a valid IPv4
|
@dosten that's a good catch, fixed |
| return false; | ||
| } | ||
|
|
||
| if ($netmask === '0') { |
There was a problem hiding this comment.
0 === $netmask to keep consistency with other places.
There was a problem hiding this comment.
'0' === $netmask then? (it's a string)
…or consistency purposes
| return false; | ||
| } | ||
|
|
||
| if ('0' === $netmask) { |
There was a problem hiding this comment.
The only address allowed to have a /0 netmask is 0.0.0.0. So this check must be: 0 === $netmask && '0.0.0.0' === $address. Please don't cast 0 as string, do 0, don't '0'.
There was a problem hiding this comment.
@dosten I do not cast anything - it's a string initially, please look carefully at the code
There was a problem hiding this comment.
Sorry, cast is not the word :) i say: 0 === $netmask instead of '0' === $netmask
There was a problem hiding this comment.
@dosten you cannot do that, $netmask is a string
|
Doesn't this should be merged in 2.3? Regardless the technical BC break, it's a bugfix. |
…icitly, since it's the only allowed network of a `/0` size
|
@phansys yep, I think it should be, but it's up to the core dev team to decide. |
|
👍 and it should indeed go in 2.3 |
|
Thank you @zerkms. |
…works (zerkms) This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #14690). Discussion ---------- [HttpFoundation] IpUtils::checkIp4() should allow `/0` networks | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14674 | License | MIT Technically it's a breaking change, since the result of the IpUtils::checkIp4('1.2.3.4', '0.0.0.0/0') call was `false` now `true`. Practically - no one should ever relied on this since it's simply wrong Commits ------- 921ecff [HttpFoundation] IpUtils::checkIp4() should allow networks
|
Guys, I see it's merged to 2.3, but will it eventually be included into 2.7, 2.8 and 3.0? |
|
@zerkms exactly |
|
@dosten how does it (porting) usually happen in symfony? |
|
@zerkms merge between maintained versions. Ex: 2.3 -> 2.6 -> 2.7 -> 2.8 -> master |
…ed proxy (zerkms) This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #15706). Discussion ---------- [framework-bundle] Added support for the `0.0.0.0/0` trusted proxy | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT It is relevant to my other PR: #14690 The original intention was to start accepting `0.0.0.0/0` as a trusted proxy (which is a valid CIDR notation). The prupose is to allow all requests to be treated as trusted and to eliminate using dirty `['0.0.0.0/1', '128.0.0.0/1']` workaround. Commits ------- 3188e1b Added support for the `0.0.0.0/0` trusted proxy
Technically it's a breaking change, since the result of the
call was
falsenowtrue.Practically - no one should ever relied on this since it's simply wrong