Make WordPress Core

Opened 6 years ago

Last modified 6 years ago

#45075 new defect (bug)

wp_delete_attachment() has unreliable return and wrong process order

Reported by: javeweb's profile jave.web Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords:
Focuses: Cc:

Description

When doing permanent deletion, this function should also remove the files of the target attachment. However successful return of this function does not guarantee this even though it still can delete the attachment object and its relations.

Problems when permanent deletion is in process:

  1. wp_delete_attachment() must check for wp_delete_attachment_files()'s return and should fail too when wp_delete_attachment_files() fails
  1. wp_delete_attachment_files() should be called first - what is the point of deleting object and it's relations when physical data could not be removed...

Change History (3)

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Media
  • Keywords attachment delete attachments thumbnails files removed

#2 in reply to: ↑ description @jave.web
6 years ago

  • Severity changed from major to critical

Replying to jave.web:

When doing permanent deletion, this function should also remove the files of the target attachment. However successful return of this function does not guarantee this even though it still can delete the attachment object and its relations.

Problems when permanent deletion is in process:

  1. wp_delete_attachment() must check for wp_delete_attachment_files()'s return and should fail too when wp_delete_attachment_files() fails
  1. wp_delete_attachment_files() should be called first - what is the point of deleting object and it's relations when physical data could not be removed...

I've just investigated more and I find a horrifying thing:

WP does not check if the files were really deleted or not at all!

wp_delete_attachment_files() - checks for wp_delete_file_from_directory() return

however: wp_delete_file_from_directory() is guessing its return value from filepath comparsion while just calling wp_delete_file() to the wind (agaín, not checking for its return)

This whole shocker ends in wp_delete_file() which does not bother to check if the file was really deleted, altough it is aware of possible errors since it silences them with @ - which is a good practice - but only if you check if it really happened.

wp_delete_file() - Shockingly, it does not return anything at all and this is the place, I think, it all started.

If you consider user's right to his personal data and the right to delete them, leaving the files over the internet can cause some legal issues - therefore I am raising the severity to critical. (One should check H-self in this case, but still WP core should handle this correctly)

#3 @joemcgill
6 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from critical to normal

Hi @javeweb, thanks for the detailed report and investigation into the logic chain here.

wp_delete_attachment_files() should be called first - what is the point of deleting object and it's relations when physical data could not be removed...

Deleting files as the last stage in the process seems to be intentional, since the first time this is called, the attachment post is moved to the trash instead of deleted. We might consider returning an error if there was an error with deleting the file, but at that point the post will already be gone from the database, so the file will need to be manually deleted.

In terms of severity, this is not a major breakage from previous behavior, so I don't see this being a critical bug. What's more important would be to see cases where deletion fails and fix those issues specifically.

Note: See TracTickets for help on using tickets.