Missmatch between actual check and warning message for Background Updates

[kraftner]

The site health check currently might sometimes have a mismatch between wording and the actual checks performed. It is reporting disabled background updates while they actually work.

Let me elaborate the setup:

1. DISALLOW_FILE_MODS is set to true to disable plugin/theme editor, manual updates, installation,...
2. But since the auto-update functionality is still desired the file_mod_allowed filter is used to re-enable automatic updates:

   <?php
   add_filter('file_mod_allowed', function($file_mod_allowed, $context){
   
       if($context === 'automatic_updater'){
           return true;
       }
   
       return $file_mod_allowed;
   },10,2);
   

3. The actual check being performed in WP_Site_Health::check_wp_version_check_exists though is current_user_can( 'update_core' ) which is way broader than actually checking for a working background update.
4. Even if we'd get past that check having DISALLOW_FILE_MODS set to true also reports as an error, while actually the check should probably be for wp_is_file_mod_allowed( 'automatic_updater' ) just like inside WP_Automatic_Updater::is_disabled.

---

I see two possible courses of action:

• This is a bug in the check and the check needs to be fixed.
• This is a wording issue for the site health page.

Looking for feedback on how to proceed.

[Clorith]

For the first few states you mentioned, if your filter is added in a reasonable manner, it should be picked up just like in core, how are you implementing the filter you describe in point 2?

The reason for the current_user_can( 'update_core' ) in the check_wp_version_check_exists() call is to prevent disclosing a sites update state to those who have no reason to see it, just checking if a user is logged in isn't enough since you could be a subscriber. It might be viable to change this ot the new view_site_health_checks capability, but it also sort of makes sense to check that the user that's looking for the state of updates is one that can update core in the first place, so I'm a bit torn on this.

I do agree with you on point 4, we should absolutely be checking the same things that core are checking here.

So for your two courses of action, I think it's a little of both, and we can (and should)= definitely sort both :)

[kraftner]

I am not sure what you mean by "first few states", but the list in the initial bug report just describes my setup and where in WP things go off track, sorry if I was unclear. The filter works for the auto-update, it's just that the health check doesn't actually check for a working auto-update.

So, the check assumes that the site either allows or disallows manual updating and auto-updating both or none, while the way my filter is set up manual updates are disabled while auto-updating is enabled. So if you say

sort of makes sense to check that the user that's looking for the state of updates is one that can update core in the first place

this isn't a good/valid assumption here.

[Clorith]

Hiya,

I've had a chance to look at this today and I think there were some misunderstandings going on, which I believe I found the root of.

47869.2.patch​ removes the check for the constants DISALLOW_FILE_MODS and AUTOMATIC_UPDATES_DISABLED, as they're both covered by the new WP_Site_Health_Auto_Updates::test_wp_automatic_updates_is_disabled checker (which looks up both of those in one go), and should reduce the false positive possibilities here.

[afragen]

@Clorith a quick look at the patch and I’m not really understanding the variable name $auto_updatesa. Is it a typo?

[Clorith]

Hah, why yes, yes it is. At least I'm consistent with them so nothing breaks. Fixed in 47869.3.patch​

[SergeyBiryukov]

In 46276:

Site Health: Use WP_Automatic_Updater::is_disabled() to check whether automatic updates are disabled.

The previous check for DISALLOW_FILE_MODS and AUTOMATIC_UPDATER_DISABLED constants didn't always provide accurate results.

Props Clorith, kraftner, afragen.
Fixes #47869.