Skip to content

[HttpFoundation] Fixes /0 subnet handling in IpUtils #16177

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

ultrafez
Copy link
Contributor

@ultrafez ultrafez commented Oct 8, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #16055
License MIT
Doc PR Not needed

Fixes bug #16055. For IP addresses with CIDR subnet length 0, the IP address must be valid - IPs with subnet masks greater than zero are implicitly validated due to the use of ip2long and substr_compare (although it's not particularly robust - there could be some future work to improve this here).

@ultrafez ultrafez changed the title bug #16055 [HttpFoundation] Fixes /0 subnet handling in IpUtils [HttpFoundation] Fixes /0 subnet handling in IpUtils Oct 8, 2015
array(true, '192.168.1.1', array('1.2.3.4/1', '192.168.1.0/24')),
array(true, '192.168.1.1', array('192.168.1.0/24', '1.2.3.4/1')),
array(false, '192.168.1.1', array('1.2.3.4/1', '4.3.2.1/1')),
array(true, '1.2.3.4', '0.0.0.0/0'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this case must be kept

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case verifying 1.2.3.4 is in subnet 192.168.1.0/0 covers this code path, but I'll add it back anyway.

@ultrafez
Copy link
Contributor Author

ultrafez commented Oct 9, 2015

The AppVeyor build seems to have failed for reasons unrelated to this pull request - the Travis build passed which includes the unit tests I updated. Do all of the checks need to pass before the pull request can be merged, even if unrelated to this PR?

@fabpot
Copy link
Member

fabpot commented Oct 11, 2015

Don't we have the same issue for the IPv6 check?

@fabpot
Copy link
Member

fabpot commented Oct 19, 2015

Thank you @ultrafez.

fabpot added a commit that referenced this pull request Oct 19, 2015
…afez)

This PR was squashed before being merged into the 2.3 branch (closes #16177).

Discussion
----------

[HttpFoundation] Fixes /0 subnet handling in IpUtils

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16055
| License       | MIT
| Doc PR        | Not needed

Fixes bug #16055. For IP addresses with CIDR subnet length 0, the IP address must be valid - IPs with subnet masks greater than zero are implicitly validated due to the use of `ip2long` and `substr_compare` (although it's not particularly robust - there could be some future work to improve this here).

Commits
-------

d9ac571 [HttpFoundation] Fixes /0 subnet handling in IpUtils
@fabpot fabpot closed this Oct 19, 2015
This was referenced Oct 27, 2015
@ultrafez
Copy link
Contributor Author

Yes, there is the same issue with IPv6 addresses - I'm not as familiar with them as I am with IPv4, but if I get a chance I'll take a look and submit it as a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants