WordPress.org

Make WordPress Core

#47869 closed defect (bug) (fixed)

Missmatch between actual check and warning message for Background Updates

Reported by: kraftner Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: has-patch commit
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.

Attachments (3)

47869.patch (697 bytes) - added by Clorith 12 months ago.
47869.2.patch (1.4 KB) - added by Clorith 12 months ago.
47869.3.patch (1.4 KB) - added by Clorith 12 months ago.

Download all attachments as: .zip

Change History (9)

#1 @Clorith
13 months 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
13 months 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.

@Clorith
12 months ago

@Clorith
12 months ago

#3 @Clorith
12 months ago

  • Keywords has-patch commit added

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.

#4 @afragen
12 months ago

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

@Clorith
12 months ago

#5 @Clorith
12 months ago

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

#6 @SergeyBiryukov
12 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.