Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 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 10 years ago.
Patch #33475
33475_1.patch (578 bytes) - added by utkarshpatel 10 years ago.
Added comment
33475.diff (635 bytes) - added by wonderboymusic 10 years ago.
33475.2.diff (2.2 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (14)

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

Patch #33475

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

Added comment

#4 @utkarshpatel
10 years ago

Hey Hulse,

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

#5 @wonderboymusic
10 years ago

33475.diff refreshes from project root

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


10 years ago

#8 @SergeyBiryukov
10 years ago

  • Keywords commit added

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

In 34044:

Document @return value for wp_unschedule_event().

See #33475.

Note: See TracTickets for help on using tickets.