Make WordPress Core

Opened 13 years ago

Closed 11 years ago

#17180 closed defect (bug) (fixed)

Invalid published date insertion in posts

Reported by: hew's profile hew Owned by: westi's profile westi
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Date/Time Keywords: has-patch
Focuses: Cc:

Description

To reproduce:

  1. Set date on a post to Feb 29th in a non-leap year.
  2. Click "Update".

The UI JS will prevent you from hitting OK, but you can bypass it and insert the bad datestamp as described above. I get other inconsistant oddness using the same procedure with other bad numerals (54, -2, etc).

This can also be done via Quick Edits using the same procedure.

I didn't explore pages as thoroughly, but that's probably worth investigation too.

Attachments (10)

17180.diff (2.2 KB) - added by solarissmoke 13 years ago.
17180.2.diff (2.1 KB) - added by solarissmoke 13 years ago.
There was some unnecesasry JS in the previous patch
17180-3.diff (1.3 KB) - added by jkudish 12 years ago.
17180-4.diff (2.3 KB) - added by jkudish 12 years ago.
17180.unit-test.patch (989 bytes) - added by jkudish 12 years ago.
Unit test that tests the proposed patch (17180-4.diff) and confirms that a WP_Error is returned for invalid dates
17180.improvements.diff (2.3 KB) - added by westi 12 years ago.
Improvements based on nacin's feedback
17180.5.diff (737 bytes) - added by jkudish 11 years ago.
Adjust the js originally introduced [21921] and reverted in [21947] to detect wether or not the date fields are present on the page or not, and only provide validation when the fields are present
17180.6.diff (1.1 KB) - added by jkudish 11 years ago.
as suggested by nacin, make updateText() bail when there is no date/time field in the current page
17180.7.diff (1.0 KB) - added by jkudish 11 years ago.
refreshed patch which properly hides the spinner when a date error occurs
17180.8.diff (1.0 KB) - added by jkudish 11 years ago.
just 17180.7.diff but now using jQuery's 'on' method instead of 'submit' as suggested by 'kovshenin'

Download all attachments as: .zip

Change History (45)

#1 @hew
13 years ago

  • Summary changed from Published date invalid data to Invalid published date insertion in posts

#2 follow-up: @solarissmoke
13 years ago

  • Keywords close added

Even if you bypass the JS, the date fields are validated server side and invalid values are ignored (so Feb 29, 2011 will not be saved as a future publish date). So I don't see what the problem is?

#3 in reply to: ↑ 2 @westi
13 years ago

  • Keywords needs-patch added; close removed
  • Milestone changed from Awaiting Review to 3.2

Replying to solarissmoke:

Even if you bypass the JS, the date fields are validated server side and invalid values are ignored (so Feb 29, 2011 will not be saved as a future publish date). So I don't see what the problem is?

We end up with a valid date in post_date_gmt and an invalid one in post_date.

I think with newer mysql the insertion may fail

#4 @westi
13 years ago

Example:

#5 @westi
13 years ago

  • Owner set to westi
  • Priority changed from normal to high
  • Severity changed from normal to major
  • Status changed from new to assigned

@solarissmoke
13 years ago

#6 @solarissmoke
13 years ago

  • Keywords has-patch added; needs-patch removed

Ah, problem indeed. Here's a tentative patch - it disables the submit button until you click OK on the timestamp editor and the date is valid (this may need UI feedback), and issues an error server-side if an invalid date is supplied (for when JS is disabled).

@solarissmoke
13 years ago

There was some unnecesasry JS in the previous patch

#7 @hew
13 years ago

Don't forget the edit.php using Quick Edit avenue.

#8 @solarissmoke
13 years ago

  • Keywords dev-feedback added

That one is tricky... edit_post() calls wp_die() when there is an error. In AJAX mode, wp_die() kills the error message and returns -1 (which list table JS doesn't handle too well right now, it just prints it). I don't think the behaviour of edit_post() can be changed because it is used in many places. We could:

  • Just print a generic error response when the response is -1 (not very helpful to the user IMO)
  • Add JS validation to the inline-edit form
  • Change the inline-save procedure to call _wp_translate_post_data() manually and return any errors before calling edit_post()

I don't know which (if any) is a good idea.

#9 follow-up: @ryan
13 years ago

We'll need to play nice with plugins that implement non-Gregorian calendars.

http://wordpress.org/extend/plugins/wp-jalali/

#10 @ryan
13 years ago

  • Milestone changed from 3.2 to Future Release

No movement, possible plugin incompatibilities, and date/time always rouses dragons, so punting.

@jkudish
12 years ago

#11 in reply to: ↑ 9 ; follow-up: @jkudish
12 years ago

In 17180-3.diff:

  • Check the dates via js whenever the post publishing form #post is submitted. If the dates are invalid, prevent publish, hide the ajax spinner, remove the disabled state from the publish button and slide down the #timestampdiv revealing the date error (highlited in red), giving the user a visual indication of the problem and an opportunity to fix it.
  • Check the dates with php's native chekdate() function before saving a post and return a WP_Error if the date is wrong

Tested the patch with posts, pages and quick edit and it all works as expected.

In regards to:

Replying to ryan:

We'll need to play nice with plugins that implement non-Gregorian calendars.
http://wordpress.org/extend/plugins/wp-jalali/

I tested with this plugin active, but it was hard to tell whether things we're behaving properly or not. Partially because I have no knowledge of how this calendar works or of arabic (which the plugin's text is in) and also because the plugin spits out a bunch of php notices and warnings. Nonetheless, I seemed to be able to save posts properly with the plugin active.

Last edited 12 years ago by jkudish (previous) (diff)

#12 @jkudish
12 years ago

  • Cc joachim.kudish@… added

#13 @jkudish
12 years ago

  • Keywords 3.5-early added

#14 in reply to: ↑ 11 ; follow-up: @westi
12 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Future Release to 3.5

Replying to jkudish:

In 17180-3.diff:

  • Check the dates via js whenever the post publishing form #post is submitted. If the dates are invalid, prevent publish, hide the ajax spinner, remove the disabled state from the publish button and slide down the #timestampdiv revealing the date error (highlited in red), giving the user a visual indication of the problem and an opportunity to fix it.
  • Check the dates with php's native chekdate() function before saving a post and return a WP_Error if the date is wrong

Tested the patch with posts, pages and quick edit and it all works as expected.

This looks like a good first pass but I think it doesn't cover all possible post editing/insertion routes and so we could still have invalid dates coming in from other places because this is only really in the code flow for the admin ui.

We should protect against invalid dates from XML-RPC as well by adding a similar check lower down in the API inside wp_insert_post as this is the function that finally does the db writes.

In regards to:

Replying to ryan:

We'll need to play nice with plugins that implement non-Gregorian calendars.
http://wordpress.org/extend/plugins/wp-jalali/

I tested with this plugin active, but it was hard to tell whether things we're behaving properly or not. Partially because I have no knowledge of how this calendar works or of arabic (which the plugin's text is in) and also because the plugin spits out a bunch of php notices and warnings. Nonetheless, I seemed to be able to save posts properly with the plugin active.

I think that as checkdate is specifically a Gregorian function we should either have a wrapper function with a filter in it or we should filter the response from it so that plugins that want to implement different calendar systems can check the date in there own way.

@jkudish
12 years ago

#15 in reply to: ↑ 14 @jkudish
12 years ago

  • Keywords has-patch added; needs-patch removed

Replying to westi:

We should protect against invalid dates from XML-RPC as well by adding a similar check lower down in the API inside wp_insert_post as this is the function that finally does the db writes.

I think that as checkdate is specifically a Gregorian function we should either have a wrapper function with a filter in it or we should filter the response from it so that plugins that want to implement different calendar systems can check the date in there own way.

Both these good points have been addressed in 17180-4.diff

Last edited 12 years ago by jkudish (previous) (diff)

@jkudish
12 years ago

Unit test that tests the proposed patch (17180-4.diff) and confirms that a WP_Error is returned for invalid dates

#16 @westi
12 years ago

Unit Tests added in [UT1026]

#17 follow-ups: @westi
12 years ago

I gave this a quick test in trunk.

Apply using curl "http://core.trac.wordpress.org/raw-attachment/ticket/17180/17180-4.diff" | sed "s/.dev//" | patch -p0 to get around the script renaming.

The User Experience when I try and Save a Draft or Publish a post with a bad date it a little in-elegant.

I just end up on a wp_die page and loose my hard work, feels like we should be able to do something better than that as this is probably worse that the current experience of a whacky date in the db.

#18 in reply to: ↑ 17 @jkudish
12 years ago

Replying to westi:

The User Experience when I try and Save a Draft or Publish a post with a bad date it a little in-elegant.

I just end up on a wp_die page and loose my hard work, feels like we should be able to do something better than that as this is probably worse that the current experience of a whacky date in the db.

I think the patch might need an update because this is not the intended behaviour. Unless you had js turned off?

I'll check it again soon.

#19 in reply to: ↑ 17 @westi
12 years ago

Replying to westi:

I gave this a quick test in trunk.

Apply using curl "http://core.trac.wordpress.org/raw-attachment/ticket/17180/17180-4.diff" | sed "s/.dev//" | patch -p0 to get around the script renaming.

The User Experience when I try and Save a Draft or Publish a post with a bad date it a little in-elegant.

I just end up on a wp_die page and loose my hard work, feels like we should be able to do something better than that as this is probably worse that the current experience of a whacky date in the db.

Ignore me - I fail at late night testing. It works fine.

#20 @westi
12 years ago

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

In [21921]:

Posting: Make it much harder to create posts with invalid dates by enforcing the post date tests in the UI and the backend code.

Previously you could quite easily send a new post into the back of beyond by specifying an invalid date like the 30th Feb and this was very confusing.
Sometimes it would seem to work and sometimes the post would end up very far in the past - depending on the mysql version and other factors.

Fixes #17180 props jkudish.

#21 follow-ups: @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

These filters are two different names. They should probably be the same. I usually like filters over wrapper functions, but wp_checkdate() probably isn't a bad idea, here. I don't think the context (whether you came from wp_insert_post() or edit_post(), or what the post array is) is necessary, and if anything it is spurious. If you want to modify a date based on some context, there are other filters for that. I usually don't fight for less context, but the only context that should matter here is what calendar system is being used.

Also, wp_insert_post() can return 0 depending on $wp_error, so we need to avoid returning WP_Error in that case. And whoops has an "h". :-)

#22 in reply to: ↑ 21 @jkudish
12 years ago

Replying to nacin:

Also, wp_insert_post() can return 0 depending on $wp_error, so we need to avoid returning WP_Error in that case.

What would be a better return? false? 0?

@westi
12 years ago

Improvements based on nacin's feedback

#23 in reply to: ↑ 21 ; follow-up: @westi
12 years ago

Replying to nacin:

These filters are two different names. They should probably be the same. I usually like filters over wrapper functions, but wp_checkdate() probably isn't a bad idea, here. I don't think the context (whether you came from wp_insert_post() or edit_post(), or what the post array is) is necessary, and if anything it is spurious. If you want to modify a date based on some context, there are other filters for that. I usually don't fight for less context, but the only context that should matter here is what calendar system is being used.

Also, wp_insert_post() can return 0 depending on $wp_error, so we need to avoid returning WP_Error in that case. And whoops has an "h". :-)

Thanks :)

I think http://core.trac.wordpress.org/attachment/ticket/17180/17180.improvements.diff should address all your points - all further feedback?

#24 in reply to: ↑ 23 @jkudish
12 years ago

Replying to westi:

I think http://core.trac.wordpress.org/attachment/ticket/17180/17180.improvements.diff should address all your points - all further feedback?

Tested, looks good to me.

#25 @westi
12 years ago

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

In [21922]:

Posting: Improve the invalid date protection code based on feedback from nacin.

  • Introduce a wp_checkdate() function with a single filter to centralise the code that validates dates.
  • Improve the error message
  • Correctly handle the return value of wp_insert_post which is not always a WP_Error on failure

Fixes #17180

#26 @nacin
11 years ago

In [21947]:

Revert JS from [21921] as it breaks saving when no post date fields are present. see #17180.

#27 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@jkudish
11 years ago

Adjust the js originally introduced [21921] and reverted in [21947] to detect wether or not the date fields are present on the page or not, and only provide validation when the fields are present

@jkudish
11 years ago

as suggested by nacin, make updateText() bail when there is no date/time field in the current page

#28 @westi
11 years ago

@jkudish: I tested the latest patch and it looks like it works but has confusing behaviour in current trunk because it kicks in after the spinner has been enabled and so you have a spinner still spinning.

#29 @kovshenin
11 years ago

@jkudish: unless I'm missing something, this might be a good time to change that to $.on instead of $.submit: http://api.jquery.com/on/

#30 @wonderboymusic
11 years ago

  • Keywords needs-refresh added; 3.5-early removed

Tagging refresh to get an update on where this is

@jkudish
11 years ago

refreshed patch which properly hides the spinner when a date error occurs

@jkudish
11 years ago

just 17180.7.diff but now using jQuery's 'on' method instead of 'submit' as suggested by 'kovshenin'

#31 @jkudish
11 years ago

  • Keywords needs-refresh removed

Newest patch should be good to go

#32 @nacin
11 years ago

  • Status changed from reopened to assigned

westi: punt candidate in my eyes.

#33 @jkudish
11 years ago

westi: punt candidate in my eyes.

Curious why? The php part of this patch has been in for a while and seemingly works fine. The js part was working fine but broke due to some other js changes in core. It's now back to working fine and should be ok to commit. I think that if we are to punt this then we should revert the php changes. Because the experience of attempting a bad date and ending up on a wp_die screen (something the js otherwise prevents) is a poor experience. But of course, I would vote for not punting :)

#34 @nacin
11 years ago

  • Priority changed from high to normal
  • Severity changed from major to normal

#35 @westi
11 years ago

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

In 22442:

Post UI: Provide visual feedback to the user if they try to set an invalid date for a post. Fixes #17180 props jkudish.

Note: See TracTickets for help on using tickets.