WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11401 closed defect (bug) (fixed)

checks against array('draft', 'pending') should be changed

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.9
Component: Trash Keywords: has-patch
Focuses: Cc:

Description

With the introduction of the trash status, checks for a post_status in array('draft', 'pending') should probably be changed into either of = 'publish' or != 'publish'.

As things currently stand, a function like wp_link_pages() with a paginated trashed post probably ends up returning a fancy permalink.

Attachments (2)

11401-admin-ajax-reply-trash-option-1.diff (505 bytes) - added by nacin 4 years ago.
Adds a new string
11401-admin-ajax-reply-trash-option-2.diff (457 bytes) - added by nacin 4 years ago.
No new strings, but an inaccurate response

Download all attachments as: .zip

Change History (14)

comment:1 nacin4 years ago

  • Component changed from General to Trash

I looked through those instances, and it looks like wp_list_pages() is the only function that may be affected. Best I can tell, all other instances already filter out trashed posts or have their own trash handlers.

But the post itself can't be viewed -- you'll get a 404 in the frontend, and a wp_die in the backend, so is it really that bad that wp_link_pages() doesn't check for it?

That said, there is an instance that does need to be patched -- while wp-comments-posts.php checks for trash status and bails, admin-ajax.php doesn't check when replying to comments in wp-admin. I was able to reply to a comment on a post that was trashed.

Two patches forthcoming. One tramples over the string freeze, one doesn't but then returns an error message that isn't 100% correct.

nacin4 years ago

Adds a new string

nacin4 years ago

No new strings, but an inaccurate response

comment:2 Denis-de-Bernardy4 years ago

scanning further at the same time. there also are various occurrences of:

array('pending', 'draft', 'future')
array('pending', 'draft')
array('publish', 'future', 'private')
array('publish', 'future')
and probably
statements that aren't immediately obvious when doing a search in project.

comment:3 Denis-de-Bernardy4 years ago

continuing scans... post.dev.js has:

			if ( $('option:selected', postStatus).val() == 'private' || $('option:selected', postStatus).val() == 'publish' ) {
				$('#save-post').hide();
			} else {
				$('#save-post').show();
				if ( $('option:selected', postStatus).val() == 'pending' ) {
					$('#save-post').show().val( postL10n.savePending );
				} else {
					$('#save-post').show().val( postL10n.saveDraft );
				}
			}

and:

				if ( $('#original_post_status').val() == 'future' || $('#original_post_status').val() == 'draft' ) {
					if ( optPublish.length ) {
						optPublish.remove();
						postStatus.val($('#hidden_post_status').val());
					}

I'm supposing that most of these are covered indirectly, but it would be good to ultimately harden these checks as needed.

comment:4 nacin4 years ago

Yeah, my initial search string was in_array.*(pending|draft|future), which found 23 matches in 10 files. I also found one instance of an or/and check, in _get_page_link().

I looked through those matches quickly and saw nothing glaring other than the two I've pointed out, though a second look couldn't hurt.

Good catch in the JS.

This all said, while I agree with hardening, the indirect coverage is largely this: many of these instances are inaccessible as a trashed post/page returns 404 in the front and wp_die in the admin area.

comment:5 follow-up: westi4 years ago

  • Keywords reporter-feedback added

_get_page_link() looks fine to me - this check is whether or not the post has a public permalink yet or should just be referred by id - Trashed posts should be fine returning the real permalink.

Can you rephrase this ticket as a list of actual bugs - it is hard to investigate/fix without a list of actual issues.

comment:6 nacin4 years ago

All I found is that while wp-comments-posts.php checks for trash status and bails, admin-ajax.php doesn't check when replying to comments in wp-admin. I was able to reply to a comment on a post that was trashed.

Two patches correct this -- one adheres to string freeze, the other doesn't.

comment:7 in reply to: ↑ 5 Denis-de-Bernardy4 years ago

  • Keywords has-patch added; reporter-feedback removed

Replying to westi:

Can you rephrase this ticket as a list of actual bugs - it is hard to investigate/fix without a list of actual issues.

I'm afraid I cannot: I merely noted the issue while working on my caching plugin, I thought it looked very fishy, and so I opened a ticket in case anyone felt looking into it.

nacin did, checked that most potential issues were already covered indirectly, except the one that he mentions in his previous comment.

comment:8 azaozz4 years ago

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

(In [12394]) Discard replies to comments on a trashed post, props nacin, fixes #11401

comment:9 nacin4 years ago

  • Milestone changed from 2.9 to 3.0
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening and moving to 3.0 to get a proper string in there specific to a trashed post, like there is in wp-comments-post.php. Feel free to re-close if you think that's unnecessary.

comment:10 azaozz4 years ago

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

Not sure that's needed as this is just a double check. Comments on trashed posts are not displayed in the admin so the user cannot reply in the first place.

comment:11 nacin4 years ago

  • Milestone changed from 3.0 to 2.9

Right, I had to trash it in another tab to reproduce.

Note: See TracTickets for help on using tickets.