Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#33475 closed defect (bug) (fixed)

Ensure timestamp is integer when scheduling single events

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: dd32's profile 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 9 years ago.
Patch #33475
33475_1.patch (578 bytes) - added by utkarshpatel 9 years ago.
Added comment
33475.diff (635 bytes) - added by wonderboymusic 9 years ago.
33475.2.diff (2.2 KB) - added by SergeyBiryukov 9 years ago.

Download all attachments as: .zip

Change History (14)

#1 @dd32
9 years 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
9 years ago

Patch #33475

#2 @peterwilsoncc
9 years 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
9 years 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
9 years ago

Added comment

#4 @utkarshpatel
9 years ago

Hey Hulse,

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

@wonderboymusic
9 years ago

#5 @wonderboymusic
9 years ago

33475.diff refreshes from project root

#6 @SergeyBiryukov
9 years 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.


9 years ago

#8 @SergeyBiryukov
9 years ago

  • Keywords commit added

#9 @dd32
9 years 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
9 years ago

In 34044:

Document @return value for wp_unschedule_event().

See #33475.

Note: See TracTickets for help on using tickets.