WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#50436 closed defect (bug) (fixed)

Unnecessary DISABLE_WP_CRON check in plugin and theme auto updates

Reported by: johnbillion Owned by: audrasjb
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: Security Keywords: has-patch
Focuses: Cc:

Description

The wp_get_auto_update_message() function added in [47835] checks the value of the DISABLE_WP_CRON constant and returns an error if it's set to true.

This check is unnecessary. The DISABLE_WP_CRON constant disables the cron spawner, not cron altogether, and is used when an external cron spawner is in use (eg. a system-level cron runner). The use of this constant doesn't indicate a problem that needs to be shown here.

The check for this constant can be removed altogether.

Attachments (2)

50436.diff (2.3 KB) - added by pbiron 3 months ago.
50436.1.diff (2.2 KB) - added by pbiron 3 months ago.

Download all attachments as: .zip

Change History (13)

This ticket was mentioned in PR #343 on WordPress/wordpress-develop by helloarpitgshah.


3 months ago

  • Keywords has-patch added; needs-patch removed

#2 @johnbillion
3 months ago

  • Keywords dev-feedback added

CC @bookdude13, it looks like you introduced this check into the feature plugin. Any concerns here?

#3 @audrasjb
3 months ago

  • Keywords dev-feedback removed
  • Owner set to audrasjb
  • Status changed from new to accepted

Thanks for spotting this @johnbillion and for the complete explanation.

This was introduced a long time ago in the feature plugin, but indeed it is not needed.
The provided pull request looks good to me, I think we can move forward.

I will update the feature plugin accordingly.

#4 @bookdude13
3 months ago

I made that check without fully understanding the constant, so if removing it seems good then do it :)

#5 @pbiron
3 months ago

As John says, that constant doesn't prevent a direct request of /wp-cron.php from triggering WP-Cron jobs (e.g., from a UNIX cron job; nor them being manually triggered by something like WP Crontrol).

But I thought the reason for that check was to provide "context" for why the job may be overdue, but since it does an early when the constant is true the message could be misleading.

Let me work up a patch that moves the check within the block that executes only if the job is overdue...

@pbiron
3 months ago

#6 @pbiron
3 months ago

On 2nd thought, I now agree with @johnbillion. The check for DISABLE_WP_CRON being true is unnecessary.

50436.diff refactors wp_get_auto_update_message() so that:

  1. there is no check for DISABLE_WP_CRON
  2. all returned strings now begin with Automatic update... (Automatic update not scheduled., Automatic update overdue by... and Automatic update scheduled in...)
  3. there is a single return
Last edited 3 months ago by pbiron (previous) (diff)

#7 @audrasjb
3 months ago

  • Keywords good-first-bug removed

Just noting we should replace all Automatic updates instances with Auto-updates for better consistency :)
Otherwise, patch looks good to me 👍

@pbiron
3 months ago

#8 @pbiron
3 months ago

50436.1.diff is the same as 50436.diff, but uses Auto-update... to begin each string.

#9 @audrasjb
3 months ago

Thanks for the update!

#10 @whyisjake
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 48147:

Security: Remove unnecessary DISABLE_WP_CRON check in plugin and theme automatic updates.

Fixes #50436.

Props johnbillion, audrasjb, bookdude13, pbiron, arpitgshah.

#11 @SergeyBiryukov
3 months ago

In 48148:

I18N: Include placeholder in translator comments in wp_get_auto_update_message().

See #50436, #50052.

Note: See TracTickets for help on using tickets.