-
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
WAF: Remove unnecessary mode check before updating rule files #38326
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. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. |
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.
Tests pass, and functionality appears to remain the same when testing manually. I think I am not grasping the full context though, we continue to run this exact process (define_mode
and is_allowed_mode
) in a number of other WAF class methods:
Waf_Initializer::check_for_updates
Waf_Rules_Manager::update_rules_cron
Waf_Runner::activate
I understand that the change is specific to the updating of rules but why is it that only this particular method can remove the check when these others seem to be processing similar actions?
Hmm... I think these "allowed mode" checks could also be updated or removed. I recall these checks being a part of the initial version of the WAF, and I may have ended up copying it into a few methods where it isn't doing anything super useful. Or perhaps, updates to how we generate rules and the bootstrap file have made these unnecessary? Or... it may have always been this way 🤷 Anyways, the only time the WAF actually uses the When running in not-standalone-mode, there is a check against the WAF mode in the initialization, which makes sense as it will be used in the runtime. When running in standalone-mode, the constant is defined by the bootstrap scripts generated without actually checking if it is an "allowed" mode. IMO we don't need to consider the mode when updating the rule files or activating the WAF, we just need to ensure the mode is valid when attempting to set the value, or use the value (i.e. run the WAF). I'll make an update to this PR to reflect this... |
3904d7d
to
bfdfd73
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.
LGTM
Working towards the ability to update the IP allow list via
update_waf
regardless of the WAF module's status, I noticed that this check doesn't appear to be useful in several cases. We only use theJETPACK_WAF_MODE
constant when actually running the firewall, it is not referenced when updating rules or regenerating the bootstrap file for example. See this comment below for more details.This also helps unblock us from using these methods when the WAF is not enabled (no mode option set).
Proposed changes:
Waf_Constants::define_mode()
and subsequentWaf_Runner::is_allowed_mode()
check from WAF methods that do not use the mode option.Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
Testing instructions:
jetpack test packages/waf php
update_rules_if_changed
method, and validate that the mode is never used.jetpack_waf_mode
option to "silent", ensure requests are not blocked.jetpack_waf_mode
option to "invalid", ensure the firewall does not run.