Delete attachment AJAX fails when EMPTY_TRASH_DAYS is set to 0 days

[simonwheatley]

To replicate:

1. Set EMPTY_TRASH_DAYS to 0 (zero) in wp-config.php
2. Edit a post
3. Upload a file (e.g. image) through via the edit post screen
4. Attempt to delete the file
5. Get error “apple” has failed to upload. You do not have permission. Has your session expired?

The problem is that the JS is binding an AJAX call to the a.delete element in handlers.dev.js, but incorrectly specifying the action as trash-post rather than delete-attachment which is what the nonce action was set as.

Possible solutions:

• Either the nonce needs to be created with the trash-post action (seems wrong)
• The class of the element needs to change so it doesn't attract the JS binding
• The class of all current delete links should be renamed to trash, which seems more accurate but it kind of a big thing which will probably introduce other bugs
• Something I've not thought of…

[SergeyBiryukov]

Confirmed.

[c3mdigital]

It also fails on move to trash when MEDIA_TRASH is set to true.

Do we want to do a check for EMPTY_TRASH_DAYS on attachments since the only difference currently is "delete immediately" vs the "confirm deletion" routine?

The attached patch removes the check for EMPTY_TRASH_DAYS and removes class="delete" which fixes the fail on moving to trash. Not sure if it would be better to remove the AJAX binding on a.delete.

[c3mdigital]

2nd patch fixes bug and allows for the _ajax_nonce to work by changing the action to trash-post.

3rd patch which seems like the best way to do it adds a check for attachment post_type in ajax-actions.php.

[nacin]

Rather than juggling trash-post_$id versus trash-attachment_$id, we can always just convert it all to trash-post_$id.

[nacin]

It looks like we do trash-{$post_type}_$id in a number of places. This is more work for us, considering we use the $id to glean the post type anyway and already couple these with delete_post cap checks (so no added security benefit), and just causes breakage like this.

I wouldn't mind converting all trash/untrash/delete-{$post_type} nonces (including 'attachment') to be converted to a simple 'post'. Seems like that would fix our problems.

[kurtpayne]

Convert trash|untrash|update|delete nonces to just use "post" post type

[kurtpayne]

Replying to nacin:

I wouldn't mind converting all trash/untrash/delete-{$post_type} nonces (including 'attachment') to be converted to a simple 'post'. Seems like that would fix our problems.

This does fix the original EMPTY_TRASH_DAYS issue.

[c3mdigital]

It doesn't make sense to do a check for EMPTY_TRASH_DAYS in media.php since media won't have trash unless MEDIA_TRASH is set to true.

If you have EMPTY_TRASH_DAYS set to 0 and MEDIA_TRASH set to true you can't add media to the trash only delete it. The modified patch removes the check for EMPTY_TRASH_DAYS.

[nacin]

Per IRC discussion, the constant logic is good.

kurtpayne's latest patch looks good for commit. Want to do another sweep to confirm we didn't miss any nonces.

[c3mdigital]

Replying to nacin:

Per IRC discussion, the constant logic is good.

kurtpayne's latest patch looks good for commit. Want to do another sweep to confirm we didn't miss any nonces.

After further testing on kurtpayne's patch the nonce check still failed when ! EMPTY_TRASH_DAYS for media iframe upload as well as page deletes from the list table and get_delete_post_link() . The updated v4 patch fixes all three.

[c3mdigital]

Fixes failed nonce on delete page

[SergeyBiryukov]

21194v4.patch​ changes the class of the "Delete Permanently" button. This would affect its styling (remove the red background on hover, see ticket:21196:9). Let's skip this UI change.

[SergeyBiryukov]

Didn't mean to change the milestone.

[c3mdigital]

Replying to SergeyBiryukov:

21194v4.patch​ changes the class of the "Delete Permanently" button. This would affect its styling (remove the red background on hover, see ticket:21196:9). Let's skip this UI change.

The reason I changed the class in the 21194v4.patch is because class="delete" binds it to the "trash-post" ajax action in handlers.js.

jQuery('a.delete', item).click(function(){
		// Tell the server to delete it. TODO: handle exceptions
		jQuery.ajax({
			url: ajaxurl,
			type: 'post',
			success: deleteSuccess,
			error: deleteError,
			id: fileObj.id,
			data: {
				id : this.id.replace(/[^0-9]/g, ''),
				action : 'trash-post',
				_ajax_nonce : this.href.replace(/^.*wpnonce=/,'')
			}
		});
		return false;
	});

This causes the nonce check to fail and never runs the delete.

function wp_ajax_trash_post( $action ) {
	if ( empty( $action ) )
		$action = 'trash-post';
	$id = isset( $_POST['id'] ) ? (int) $_POST['id'] : 0;

	check_ajax_referer( "{$action}_$id" );
	if ( !current_user_can( 'delete_post', $id ) )
		wp_die( -1 );

	if ( !get_post( $id ) )
		wp_die( 1 );

	if ( 'trash-post' == $action )
		$done = wp_trash_post( $id );
	else
		$done = wp_untrash_post( $id );

	if ( $done )
		wp_die( 1 );

	wp_die( 0 );
}

[SergeyBiryukov]

Replying to c3mdigital:

The reason I changed the class in the 21194v4.patch is because class="delete" binds it to the "trash-post" ajax action in handlers.js.

I see, thanks for the clarification. 21194v5.patch​ reflects the change in the CSS files.

[ryan]

In [21504]:

Remove post type from post nonces. Fixes attachment deletion when EMPTY_TRASH_DAYS is 0. Props c3mdigital, kurtpayne, SergeyBiryukov. fixes #21194