WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#33475 closed defect (bug) (fixed)

Ensure timestamp is integer when scheduling single events

Reported by: peterwilsoncc Owned by: dd32
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.1
Component: Cron API Keywords: has-patch commit
Focuses: Cc:

Description

Validate the timestamp passed to wp_schedule_single_event to ensure it is an integer and occurs in the future.

Related: #33423

Attachments (4)

33475.patch (584 bytes) - added by utkarshpatel 20 months ago.
Patch #33475
33475_1.patch (578 bytes) - added by utkarshpatel 20 months ago.
Added comment
33475.diff (635 bytes) - added by wonderboymusic 20 months ago.
33475.2.diff (2.2 KB) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (14)

#1 @dd32
20 months ago

  • Milestone changed from Awaiting Review to 4.4

Existing in the past is also valid I believe, cron simply processes any events <= time().

I think the rule should be is_numeric( $timestamp ) && $timestamp > 0

@utkarshpatel
20 months ago

Patch #33475

#2 @peterwilsoncc
20 months ago

  • Keywords has-patch added; needs-patch removed
  • Version set to 2.1

Thanks for the patch @utkarshpatel.

I am in two minds as to whether it is worth a doing_it_wrong call if the timestamp is not numeric. Reviewers, any thoughts?

#3 @dd32
20 months ago

@utkarshpatel for 33475.patch I'd probably have kept the two checks distinct from each other (valid timestamps; and already scheduled) for the simple desire of readability (Also, please always add a comment after you upload a patch, Trac doesn't send out notifications when you only upload a patch)

I don't really see a huge benefit for a _doing_it_wrong() here, developers will find out that their events are not running pretty quickly while testing (I would hope), this just prevents the cron array filling with things that won't ever be executed.

@utkarshpatel
20 months ago

Added comment

#4 @utkarshpatel
20 months ago

Hey Hulse,

As you pointed out comment and new conditionI have added new patch. Please check and let me know.

#5 @wonderboymusic
20 months ago

33475.diff refreshes from project root

#6 @SergeyBiryukov
20 months ago

Seems like other functions would benefit from the same check for consistency, see 33475.2.diff.

This ticket was mentioned in Slack in #core by sergey. View the logs.


20 months ago

#8 @SergeyBiryukov
20 months ago

  • Keywords commit added

#9 @dd32
20 months ago

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

In 33936:

Cron: Reject events when the provided $timestamp is not a valid timestamp.

Invalid timestamps were previously accepted by the scheduling functions but would never be run due to our implementation which caused the cron option to forever contain the events.
This rejects such events which most likely only occur due to developer error.

Props utkarshpatel, wonderboymusic, SergeyBiryukov.
See #33423, Fixes #33475

#10 @SergeyBiryukov
20 months ago

In 34044:

Document @return value for wp_unschedule_event().

See #33475.

Note: See TracTickets for help on using tickets.