WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 8 days ago

#47869 new defect (bug)

Missmatch between actual check and warning message for Background Updates

Reported by: kraftner Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Site Health Keywords:
Focuses: Cc:

Description

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.

Change History (2)

#1 @Clorith
3 weeks ago

  • Milestone changed from Awaiting Review to 5.3

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 :)

#2 @kraftner
8 days ago

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.

Note: See TracTickets for help on using tickets.