WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 months ago

#39476 closed defect (bug) (fixed)

wp_delete_file filter expects both absolute and relative paths

Reported by: rmccue Owned by: mikeschroder
Milestone: 4.9.7 Priority: normal
Severity: normal Version: 4.2
Component: Media Keywords: has-patch early
Focuses: Cc:

Description

The wp_delete_file filter is called in a number of places. In particular, it's called in wp_delete_file() and wp_delete_attachment(). Unfortunately, the filter acts inconsistently.

In wp_delete_file(), the result of the filter is passed to unlink directly (assuming it's not empty). In wp_delete_attachment() however, the file is combined with the upload base directory using path_join().

In most normal circumstances, this is not an issue, as path_join() no-ops when passed an absolute path. However, when using stream wrappers with a custom protocol (such as s3://), path_join() does not recognise these as absolute paths and redundantly joins the paths, resulting in a broken path. The error suppression asperand (@) hides this invalid path error.

There's two obvious solutions we could choose:

  1. Be consistent with passing paths in. Either always absolute or always relative.
  2. Just use wp_delete_file in wp_delete_attachment. This forces it to be consistent. This implies being consistently absolute.

As the filter is currently inconsistent, I don't think there's a huge amount of damage to changing it to be always absolute.

When wp_delete_file() was added in #17864, it was used in most places, but not in wp_delete_attachment() for Unknown Reasons. Every other invocation of the function (and hence filter) is absolute.

Patch attached.

Attachments (4)

39476.diff (1.9 KB) - added by rmccue 2 years ago.
Use wp_delete_file() in wp_delete_attachment
39476.2.diff (11.9 KB) - added by gitlost 2 years ago.
Use dirname(), with unit tests.
39476.3.diff (9.6 KB) - added by gitlost 2 years ago.
Remove unnecessary "cleanup" (save/restore of options).
39476.4.diff (9.6 KB) - added by gitlost 2 years ago.
Set $prev_ctype_locale properly in unit test.

Download all attachments as: .zip

Change History (23)

@rmccue
2 years ago

Use wp_delete_file() in wp_delete_attachment

#1 @joemcgill
2 years ago

Not sure about the deep history here, but this seems sane to me. We should probably do a cursory look to see if anyone is doing anything unusual with that filter, where relative paths are expected.

#2 follow-up: @dd32
2 years ago

Seems sane to me also, I have no idea of the history here either.

I'm running a scan on a plugins checkout to see if I can see anything relying upon the behaviour, but even if I do, I don't think that's not a good reason to change this.
I don't see a need to push this for 4.7.1 though (with the pending commit deadline, and testing time).

Last edited 2 years ago by dd32 (previous) (diff)

#3 in reply to: ↑ 2 @rmccue
2 years ago

  • Milestone changed from 4.7.1 to 4.7.2

Replying to dd32:

I don't see a need to push this for 4.7.1 though (with the pending commit deadline, and testing time).

4.7.2 is fine by me.

#4 @dd32
2 years ago

Seems most plugins are related to deleting caches and extra files after attachment deletion. A bunch of the plugins however appear to expect to receive relative paths on the filter.
A bunch of others pass what appears to probably be relative paths to the wp_delete_file filter too.

I don't really see it as a blocker to change here, but I question if it should be in a point release. Here's the list of plugins who include wp_delete_file and filter on the same line if someone would like to look closer.

polylang
post-thumbnail-editor
selectel-storage-upload
wp2cloud-wordpress-to-cloud
geodirectory
mailbase
evdbfiles
tantan-s3
upload-to-ftp
scissors-continued
profile-custom-content-type
editor-by-mintboard
hammy
detuyun-image-cloud-storage
qiniu-cloud
upyun
file-gallery
scissors-watermark
hm-portfolio
storageqloud-for-wordpress
mapify-lite-google-maps-plus
wp-thumb
rotating-header
scissors
cdn-tools
tantan-s3-cloudfront
xili-language
foogallery
conoha-object-sync

#5 @Eric3D
2 years ago

Can we simply look for :// in the string and treat it like absolute path if it's present and as relative path if it's not?

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 years ago

#8 @rmccue
2 years ago

@Eric3D I'd be happy with that, but we need to be careful making that change to ensure it's always applied. In any case, I still think it's a good idea to unify the behaviour here; whether that's to relative or to absolute, I don't mind.

#9 @joemcgill
2 years ago

  • Milestone changed from 4.7.3 to 4.8

I think the approach in 39476.diff makes sense, but this should probably have longer time in trunk so I'm going move this to the major.

#10 @gitlost
2 years ago

Of the 3 branches, only the last $backup_sizes gives a relative path to the wp_delete_file filter, as the other 2, $meta['thumb'] and $meta['sizes'], do a str_replace() on the absolute path $file, so the path_join() with $uploadpath['basedir'] is always a no-op (except when it messes up).

So the behavioural change would only be for the backup sizes.

I think it would make sense to make all 3 cases the same basic code, avoiding the non-semantic str_replace()/basename() combo by using dirname() instead, which would also fix the UTF-8 issue #33227.

The following patch does this, with unit tests for backup sizes/wp_delete_file filter and the #33227 UTF-8 case, and some cleaning up of other tests.

(I looked at the reasonably recently updated plugins in those listed above

polylang, post-thumbnail-editor, geodirectory, upload-to-ftp, editor-by-mintboard, file-gallery, mapify-lite-google-maps-plus, xili-language, foogallery, conoha-object-sync

and only mapify-lite-google-maps-plus and foogallery, which both use the no longer maintained https://github.com/humanmade/WPThumb, were expecting a relative path.)

@gitlost
2 years ago

Use dirname(), with unit tests.

@gitlost
2 years ago

Remove unnecessary "cleanup" (save/restore of options).

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 years ago

@gitlost
2 years ago

Set $prev_ctype_locale properly in unit test.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 years ago

#13 @mikeschroder
2 years ago

  • Owner set to mikeschroder
  • Status changed from new to assigned

#14 @mikeschroder
2 years ago

  • Keywords early added
  • Milestone changed from 4.8 to Future Release

After chatting with @azaozz and @stephdau about this, thinking that it needs more time to soak than we have before 4.8.

I'd like to see this land, but going to punt to Future Release for now, with the idea that it should land early in the next major.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 years ago

#16 @joemcgill
22 months ago

#38907 was marked as a duplicate.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


9 months ago

#18 @calin
2 months ago

@mikeschroder, @joemcgill is thare any resolution for this bug?

#19 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 4.9.7
  • Resolution set to fixed
  • Status changed from assigned to closed

It looks like this was fixed in 4.9.7 by [43392].

wp_delete_attachment() no longer applies the wp_delete_file filter, only wp_delete_file() does.

Note: See TracTickets for help on using tickets.