Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] IpUtils::checkIp4() should allow/0 networks#14690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
….0.0.0/0" as a valid mask (that includes every IPv4 address)Seehttp://tools.ietf.org/html/rfc4632#section-3.1
dosten commentedMay 19, 2015
This looks wrong, |
…base IP address is a valid IPv4
zerkms commentedMay 19, 2015
@dosten that's a good catch, fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
0 === $netmask to keep consistency with other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'0' === $netmask then? (it's a string)
…or consistency purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, do0, don't'0'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dosten I do not cast anything - it's a string initially, please look carefully at the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, cast is not the word :) i say:0 === $netmask instead of'0' === $netmask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dosten you cannot do that,$netmask is a string
phansys commentedMay 19, 2015
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
zerkms commentedMay 19, 2015
@phansys yep, I think it should be, but it's up to the core dev team to decide. |
stof commentedMay 20, 2015
👍 and it should indeed go in 2.3 |
fabpot commentedMay 20, 2015
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 | MITTechnically 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 wrongCommits-------921ecff [HttpFoundation] IpUtils::checkIp4() should allow networks
zerkms commentedMay 21, 2015
Guys, I see it's merged to 2.3, but will it eventually be included into 2.7, 2.8 and 3.0? |
dosten commentedMay 21, 2015
@zerkms exactly |
zerkms commentedMay 21, 2015
@dosten how does it (porting) usually happen in symfony? |
dosten commentedMay 21, 2015
@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 | MITIt is relevant to my other PR:#14690The 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