WAF: Improve cross-compatibility of IP list options

[nateweller]

Proposed changes:

• Update the WAF REST API endpoint to compute the legacy "jetpack_waf_ip_lists" value based on the new block and allow list option values, and similarly set the new block and allow list values directly when the legacy "jetpack_waf_ip_lists" parameter is included in a POST request.
• Use Jetpack_Options::get_raw_option() in compatibility hooks, to use the unfiltered value from the database.
• Use '1' or '' as option values to ensure related hooks fire (source).
• Update/fix tests to correctly mock option values.

Other information:

• Have you written new tests for your changes, if applicable?
• Have you checked the E2E test CI results, and verified that your changes do not break them?
• Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

No

Testing instructions:

• Set up a site with running one of the following environments:
  • Jetpack with branch applied, Protect from production
  • Jetpack from production, Protect with branch applied
  • Both Jetpack AND Protect with branch applied
  • Jetpack OR Protect with branch applied
• Test toggling between the Jetpack and Protect WAF UI, and updating the new block and allow list toggles in Protect, and updating the legacy IP lists toggle in Jetpack.

[github-actions]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the add/waf/ip-list-cross-compat branch.

• To test on Simple, run the following command on your sandbox:

  bin/jetpack-downloader test jetpack add/waf/ip-list-cross-compat
  

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

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.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

[dkmyta]

Code looks good, one non-blocking suggestion to maintain consistency.

Had some initial issues testing - installed the branch via the beta tester on a JN site that had pre-existing legacy options set (manual rules enabled or disabled). Regardless of the existing settings, after installing the branch the manual rules show as on and toggling from either Jetpack or Protect does not work. The request succeeds and the payload looks correct jetpack_waf_ip_list: false, but the response continues to include jetpack_waf_ip_list: true, the toggle flips momentarily but returns to the on position.

Works great on a fresh JT install, even after downgrading and/or using an outdated version of the other plugin. Odd thing is, if I install the new version, modify settings, downgrade, modify settings, then upgrade again, it works as expected. I was under the impression that doing this would present the same issues as starting from and old version and upgrading, but perhaps because in this particular case the new options are already set from the new version being installed initially, vs in the other case, they did not exist when upgrading?

UPDATE: Although still worth a deeper look, I was unable to reproduce the issue on another installation with a similar setup, so its potentially just a one-off - I've provided you login creds privately so you can take a look at the problematic site.

Another thing I did notice though was on the first time enabling the manual rules setting using the branch, after hitting the toggle it was disabled for a noticeably longer time then any subsequent toggles - perhaps irrelevant or might have always been the case but thought it worthy of a note.

[dkmyta]

To maintain consistency with other related operations, should we use Jetpack_Options::get_raw_option() here also?

[nateweller]

Interesting question! After double checking the logic, I don't think these should be changed.

I'm using get_option here so that it uses the default value hooks for the block and allow list. This ensures that if the deprecated option was set, and the new options are not, they will base their value on the old one and still return an accurate value 👍

In the compatibility class, I used get_raw_option to bypass those filters, in order to make those default value decisions based on the "true" value of the options in the database.

But at this point, in the runner class, we want those default values to be used 👍

[nateweller]

Had some initial issues testing - installed the branch via the beta tester on a JN site that had pre-existing legacy options set (manual rules enabled or disabled). Regardless of the existing settings, after installing the branch the manual rules show as on and toggling from either Jetpack or Protect does not work. The request succeeds and the payload looks correct jetpack_waf_ip_list: false, but the response continues to include jetpack_waf_ip_list: true, the toggle flips momentarily but returns to the on position.

Some notes from debugging on this particular test site:

• The deprecated jetpack_waf_ip_list option was set to "" (disabled).
• The jetpack_waf_ip_allow_list_enabled and jetpack_waf_ip_block_list_enabled options were both set to 1 (enabled).
• Since the REST API uses the new values to compute the value for jetpack_waf_ip_list, it was correctly returning true for the value.
• Setting the value to false also correctly sent a request with a payload where jetpack_waf_ip_list as false.
• The update endpoint correctly attempted to update the individual block and allow list settings to "" (disabled). The update_option call got all the way to the $wpdb->update call, where it failed and returned false. This is where the error is happening.
• Strangely, turning automatic rules on from the Jetpack settings fixed the issue and it stopped being reproducible on the test site.

[nateweller]

Works great on a fresh JT install, even after downgrading and/or using an outdated version of the other plugin. Odd thing is, if I install the new version, modify settings, downgrade, modify settings, then upgrade again, it works as expected. I was under the impression that doing this would present the same issues as starting from and old version and upgrading, but perhaps because in this particular case the new options are already set from the new version being installed initially, vs in the other case, they did not exist when upgrading?

If you upgrade, make changes, and then downgrade - it would go back to using the deprecated option value, which is never modified or used directly by the upgraded version.

Likewise, if you go back to the upgraded version, where you already had set values for the new options, it will always use those directly.

So I am not sure if it would always keep the changes you made in the middle of that process, as when the latest values are available when running the updated code, it will always use those. I'm not too worried about the case where the updated version is installed, used, and then downgraded though.

[nateweller]

Made some minor adjustments, this is good for re-review @dkmyta 🙇

[dkmyta]

I think we're solid here! I was not able to replicate the original issue - I think we can conclude that it was definitely an odd one-off that we do have handling for.

As we discussed, there is not much we can do about the edge case where one or the other plugin is using the legacy version and have a single IP list toggled in the upgraded version, but I agree that the following is a logical solution:

• Disabling manual rules in the legacy UI disables both lists in the upgraded UI
• Disabling both lists in the upgraded UI disables manual rules in the legacy UI
• Enabling any one list in the upgraded UI enables manual rules and access to both lists in the legacy UI (and updates to a disabled list re-enable it in the upgraded UI)