WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 18 months ago

#17180 closed defect (bug) (fixed)

Invalid published date insertion in posts

Reported by: hew Owned by: 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 3 years ago.
17180.2.diff (2.1 KB) - added by solarissmoke 3 years ago.
There was some unnecesasry JS in the previous patch
17180-3.diff (1.3 KB) - added by jkudish 22 months ago.
17180-4.diff (2.3 KB) - added by jkudish 22 months ago.
17180.unit-test.patch (989 bytes) - added by jkudish 21 months 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 19 months ago.
Improvements based on nacin's feedback
17180.5.diff (737 bytes) - added by jkudish 19 months 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 19 months 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 18 months ago.
refreshed patch which properly hides the spinner when a date error occurs
17180.8.diff (1.0 KB) - added by jkudish 18 months 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)

comment:1 hew3 years ago

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

comment:2 follow-up: solarissmoke3 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?

comment:3 in reply to: ↑ 2 westi3 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

comment:4 westi3 years ago

Example:

comment:5 westi3 years ago

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

solarissmoke3 years ago

comment:6 solarissmoke3 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).

solarissmoke3 years ago

There was some unnecesasry JS in the previous patch

comment:7 hew3 years ago

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

comment:8 solarissmoke3 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.

comment:9 follow-up: ryan3 years ago

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

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

comment:10 ryan3 years ago

  • Milestone changed from 3.2 to Future Release

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

jkudish22 months ago

comment:11 in reply to: ↑ 9 ; follow-up: jkudish22 months 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 22 months ago by jkudish (previous) (diff)

comment:12 jkudish22 months ago

  • Cc joachim.kudish@… added

comment:13 jkudish22 months ago

  • Keywords 3.5-early added

comment:14 in reply to: ↑ 11 ; follow-up: westi22 months 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.

jkudish22 months ago

comment:15 in reply to: ↑ 14 jkudish22 months 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

Version 0, edited 22 months ago by jkudish (next)

jkudish21 months ago

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

comment:16 westi19 months ago

Unit Tests added in [UT1026]

comment:17 follow-ups: westi19 months 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.

comment:18 in reply to: ↑ 17 jkudish19 months 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.

comment:19 in reply to: ↑ 17 westi19 months 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.

comment:20 westi19 months 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.

comment:21 follow-ups: nacin19 months 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". :-)

comment:22 in reply to: ↑ 21 jkudish19 months 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?

westi19 months ago

Improvements based on nacin's feedback

comment:23 in reply to: ↑ 21 ; follow-up: westi19 months 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?

comment:24 in reply to: ↑ 23 jkudish19 months 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.

comment:25 westi19 months 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

comment:26 nacin19 months ago

In [21947]:

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

comment:27 nacin19 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

jkudish19 months 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

jkudish19 months ago

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

comment:28 westi18 months 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.

comment:29 kovshenin18 months 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/

comment:30 wonderboymusic18 months ago

  • Keywords needs-refresh added; 3.5-early removed

Tagging refresh to get an update on where this is

jkudish18 months ago

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

jkudish18 months ago

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

comment:31 jkudish18 months ago

  • Keywords needs-refresh removed

Newest patch should be good to go

comment:32 nacin18 months ago

  • Status changed from reopened to assigned

westi: punt candidate in my eyes.

comment:33 jkudish18 months 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 :)

comment:34 nacin18 months ago

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

comment:35 westi18 months 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.