WordPress.org

Make WordPress Core

#21194 closed defect (bug) (fixed)

Delete attachment AJAX fails when EMPTY_TRASH_DAYS is set to 0 days

Reported by: simonwheatley Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 2.0
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

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…

Attachments (7)

21194.patch (2.0 KB) - added by c3mdigital 22 months ago.
21194v2.patch (2.2 KB) - added by c3mdigital 22 months ago.
21194v3.patch (2.1 KB) - added by c3mdigital 22 months ago.
21194_just_use_post.patch (9.8 KB) - added by kurtpayne 22 months ago.
Convert trash|untrash|update|delete nonces to just use "post" post type
21194_just_use_postv2.patch (9.6 KB) - added by c3mdigital 22 months ago.
21194v4.patch (10.4 KB) - added by c3mdigital 21 months ago.
Fixes failed nonce on delete page
21194v5.patch (11.7 KB) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 SergeyBiryukov22 months ago

Confirmed.

comment:2 c3mdigital22 months ago

  • Version set to 2.0

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.

c3mdigital22 months ago

c3mdigital22 months ago

comment:3 c3mdigital22 months ago

2nd patch fixes bug and allows for the _ajax_nonce to work.

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

c3mdigital22 months ago

comment:4 nacin22 months ago

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

comment:5 follow-up: nacin22 months ago

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.

comment:6 kurtpayne22 months ago

  • Cc kpayne@… added
  • Owner set to kurtpayne
  • Status changed from new to accepted

comment:7 kurtpayne22 months ago

  • Owner kurtpayne deleted
  • Status changed from accepted to assigned

kurtpayne22 months ago

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

comment:8 in reply to: ↑ 5 kurtpayne22 months ago

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.

Last edited 22 months ago by kurtpayne (previous) (diff)

comment:9 c3mdigital22 months ago

It doesn't make sense to do a check for EMPTY_TRASH_DAYS in media upload iframe 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 in the upload iframe.

Last edited 22 months ago by c3mdigital (previous) (diff)

comment:10 follow-up: nacin22 months ago

  • Keywords has-patch commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.5

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.

comment:11 in reply to: ↑ 10 c3mdigital22 months ago

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.

Last edited 21 months ago by c3mdigital (previous) (diff)

c3mdigital21 months ago

Fixes failed nonce on delete page

comment:12 follow-up: SergeyBiryukov21 months ago

  • Milestone changed from 3.5 to Awaiting Review

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.

comment:13 SergeyBiryukov21 months ago

  • Milestone changed from Awaiting Review to 3.5

Didn't mean to change the milestone.

comment:14 in reply to: ↑ 12 ; follow-up: c3mdigital21 months ago

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 );
}

SergeyBiryukov21 months ago

comment:15 in reply to: ↑ 14 SergeyBiryukov21 months ago

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.

comment:16 ryan20 months ago

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

In [21504]:

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

Note: See TracTickets for help on using tickets.