-
Notifications
You must be signed in to change notification settings - Fork 9.4k
PageCache/AccessList: Add CIDR support #37809
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
base: 2.4-develop
Are you sure you want to change the base?
Conversation
Hi @tdgroot. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
24526c2
to
1c7183c
Compare
@magento run all tests |
@magento create issue |
Hello @tdgroot, Thanks for the contribution! But as this is feature enhancement, we need a PO confirmation to proceed with this PR. We have started the process for the same. Meanwhile we are moving this PR Thanks |
@magento run all tests |
Hello @tdgroot, We have received an approval for this PR for further evaluation. Hence moving it to appropriate bucket. Thanks |
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.
Hello @tdgroot,
Thank you for your contribution to add CIDR notation support to PageCache access lists. This is a valuable enhancement that would allow for more flexible IP-based access control.
However, the PR currently only modifies the validation pattern without implementing the actual CIDR functionality. For full CIDR support, we need changes in:
- The component that performs IP matching against the access list
- A utility method to properly compare an IP against a CIDR range
Thanks
@@ -24,7 +24,7 @@ public function beforeSave() | |||
parent::beforeSave(); | |||
|
|||
$value = $this->getValue(); | |||
if (!is_string($value) || !preg_match('/^[\w\s\.\-\,\:]+$/', $value)) { | |||
if (!is_string($value) || !preg_match('/^[\w\s\.\-\,\:(\/\d+)?]+$/', $value)) { |
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 pattern places
(\/\d+)?
inside a character class([...])
, which means these are treated as individual characters rather than a sequence. - The pattern remains overly permissive and would validate strings like "this is: a more / or less, valid string.... oh, lets add numbers 12345" which aren't valid IP addresses or CIDR notations
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 extra CIDR validation regex should indeed be moved outside of the character class, so should become something like this: ^[\w\s\.\-\,\:]+(\/\d+)?$
Good remark!
That's not really true, the only place where we use this configuration value is over here:
Which is used to output the access list into the VCL file that Magento generates for Varnish. Varnish already supports CIDR notation in ACL's, so there is no need for extra matching or parsing. One thing that I'm not sure about, but maybe @tdgroot can clarify, is that the Varnish docs say to put the IP address in double quotes and the CIDR part outside of those quotes: https://varnish-cache.org/docs/trunk/users-guide/vcl-example-acls.html#acls |
Description (*)
When accepting purge requests within a network, it's easier to just supply a CIDR range.
Related Pull Requests
None.
Fixed Issues (if relevant)
None.
Manual testing scenarios (*)
172.16.0.1/24
Questions or comments
Contribution checklist (*)
Resolved issues: