Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#50436 closed defect (bug) (fixed)

Unnecessary DISABLE_WP_CRON check in plugin and theme auto updates

Reported by: johnbillion's profile johnbillion Owned by: audrasjb's profile 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 5 years ago.
50436.1.diff (2.2 KB) - added by pbiron 5 years ago.

Download all attachments as: .zip

Change History (13)

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


5 years ago
#1

  • Keywords has-patch added; needs-patch removed

#2 @johnbillion
5 years ago

  • Keywords dev-feedback added

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

#3 @audrasjb
5 years 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
5 years ago

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

#5 @pbiron
5 years 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
5 years ago

#6 @pbiron
5 years 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 5 years ago by pbiron (previous) (diff)

#7 @audrasjb
5 years 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
5 years ago

#8 @pbiron
5 years ago

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

#9 @audrasjb
5 years ago

Thanks for the update!

#10 @whyisjake
5 years 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
5 years 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.