-
Notifications
You must be signed in to change notification settings - Fork 798
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
IP and WAF: Add CIDR support #39425
IP and WAF: Add CIDR support #39425
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
492f8a0
to
53c39c6
Compare
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.
Other than that, as you know, I am not a big fan of having static calls everywhere due to the non-mockability, but it sure works.
// Validate prefix length. | ||
if ( $ip_version === 'ipv4' ) { | ||
// For IPv4, the prefix length must be an integer between 0 and 32. | ||
if ( ! ctype_digit( $prefix ) || $prefix < 0 || $prefix > 32 ) { | ||
return false; | ||
} | ||
} elseif ( $ip_version === 'ipv6' ) { | ||
// For IPv6, the prefix length must be an integer between 0 and 128. | ||
if ( ! ctype_digit( $prefix ) || $prefix < 0 || $prefix > 128 ) { | ||
return false; | ||
} |
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.
I think this could be replaced with a call to self::validate_netmask()
and then return false if it comes back as false? This would also enable us to put the ctype_digit()
check into that function and not have to check it individually beforehand (if we like to)
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.
I've gone ahead and used self::validate_netmask()
here to avoid the code duplication! 👍
I do kind of like enforcing the numeric netmask up front in the parsing step, so that the parsing function deals with extracting it from the string and ensuring it is cast to the correct type right away, and the validation function only needs to validate the value of the netmask (no passing in strings to validate, that we will always want to use as integers anyways, if the validation passes).
No opinions strongly held though!
e718dae
to
ebbe743
Compare
Thank you @ArSn for the time reviewing and testing this PR!
I don't disagree, although I like the convenience of providing static methods from a utility-type library like this, so that consumers can essentially just opt-in to whatever methods they want to use like quickly getting or validating an IP address. Though it would be cool to interface with the package via classes that represent the IP lists, or individual IP addresses, i.e. |
ebbe743
to
1829a61
Compare
Proposed changes:
ip
package for handling CIDR ranges in all utility functions.waf
package for handling CIDR ranges in the block and allow lists.Other information:
Jetpack product discussion
https://github.com/Automattic/jetpack-scan-team/issues/1419
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Easily testing CIDR notation: If your IP address is
127.0.0.1
, use127.0.0.0/24
as a range that translates to127.0.0.*
.