Generated images for one file can be overwritten by another with the same name when mapping mime types for generated images

[ianmjones]

When two images with same base file name but different extensions are added to the Media Library within the same month and both have thumbnails changed to same format by the image_editor_output_format filter, their thumbnails are perpetually confused!

Steps to reproduce:

1. Add following filter to site that converts thumbnails to WebP for both jpeg and png source files.

add_filter( 'image_editor_output_format', function( $formats ) {
    $formats['image/jpeg'] = 'image/webp';
    $formats['image/png'] = 'image/webp';

    return $formats;
} );

2. Add "picture.jpg" and "picture.png" to Media Library within a month.
3. Notice that thumbnails for second upload clobber thumbnails of first, e.g. "picture-150x150.webp" shared by both images.
4. "Permanently delete" one of the images from the Media Library and all intersecting thumbnails used by the other Media Library item will disappear.

See attached screenshots for example.

I think wp_unique_filename needs to take into account all thumbnails that are to be generated for the new upload.

[ianmjones]

Original files.

[ianmjones]

On upload thumbnails confused already.

[ianmjones]

Confused further in Media Library.

[ianmjones]

Files on disk.

[desrosj]

Related #42437, #53667.

[desrosj]

This is closely related to #53667 in that this is an edge case with using the new image_editor_output_format filter. I've updated #53667 with a patch and status update, but copying here for visibility.

In the media chat, these were the steps agreed upon for 5.8 to address this issue and #53668:

• Leave the filters in as is.
• Update the inline docs for the filter to explain that it is a bit experimental, and there are two known potential issues when using the filter to change image formats for sub sizes.
• Post a developer note detailing these two edge cases.
• Follow up in 5.8.x releases to fix these edge cases.

53667.diff is an adjustment to the filter's inline documentation to make developer's aware of these two edge cases, and that the filter should generally be considered experimental.

[desrosj]

In 51442:

Media: Document edge cases with the new image_editor_output_format filter.

More testing has revealed that the image_editor_output_format filter has some interesting edge cases that developers should be aware of when electing to use this filter (see #53667 and #53668).

Because this is a new filter that was intended to be used for experimenting with different ways to handle generating image sizes and has not yet been adopted in the wild, expanding the inline documentation is an acceptable temporary solution while these edge cases are explored further and addressed.

Props mikeschroder, antpb, desrosj, adamsilverstein, ianmjones.
See #5366, #53668, #35725.

[desrosj]

In 51444:

Media: Document edge cases with the new image_editor_output_format filter.

More testing has revealed that the image_editor_output_format filter has some interesting edge cases that developers should be aware of when electing to use this filter (see #53667 and #53668).

Because this is a new filter that was intended to be used for experimenting with different ways to handle generating image sizes and has not yet been adopted in the wild, expanding the inline documentation is an acceptable temporary solution while these edge cases are explored further and addressed.

Props mikeschroder, antpb, desrosj, adamsilverstein, ianmjones.
Merges [51442] to the 5.8 branch.
See #53667, #53668, #35725.

[ianmjones]

Patch that allows wp_unique_filename to take into consideration alternate formats for the uploaded image, as well as other upload formats that may result in thumbnails for the current image format.

[ianmjones]

I've had a go at fixing this issue I raised, feels like the polite thing to do. ;-)

[antpb]

This was discussed in the recent Media component meeting and it was determined that due to the nature of this bug, the proposed patch needs to have a helper function due to the dependent logic running multiple times.

This helper function is necessary to fix the issue correctly and no other temporary fix is currently available without adding a new function in 5.8.X.

as @joedolson mentioned in the meeting

Given that the new function will only be used in a case that only exists due to the new filter, it seems like it shouldn’t have broad unintended consequences.

Some feedback also given in the meeting is that the doc block should include a @private tag to indicate that this is not encouraged to be used outside of Core functions. This gives the freedom to remove the function if a better solution is developed later.

[hellofromTonya]

Additional notes from Media component meeting, the patch also needs:

• Unit tests for the new functions and to show the problem before the patch and that's resolved after the patch
• Thorough code review

@azaozz can you take a look at 53668.diff​ for a technical and code review?

[SergeyBiryukov]

Looking at 53668.diff​, just a quick note that one of the function names has a typo, should be ..._mime_type instead of ...mine_type in wp_default_extension_for_mine_type().

[ianmjones]

Addresses typo in new function name, sets private tag on helper function, adds unit tests for new functions.

[azaozz]

Looking at 53668.2.diff, a bit unsure if the new _wp_check_alternate_output_format_uniqueness() is needed. Seems it repeats quite a bit of code/functionality from wp_unique_filename().

Agree that a good way to ensure the file name is unique is to check that file name with both possible extensions. To do that:

1. Find if an image file is set to be converted.
2. Increase the number that's appended to the file name until there are no matches for both extensions.

It seems enough to loop twice the portion of code that increases the number and changes the file name. Alternatively that code can be extracted in a new function, then called in the loop, but don't see any benefits. Alt patch coming up.

Alt patch for https://core.trac.wordpress.org/ticket/53668 partially based on https://core.trac.wordpress.org/attachment/ticket/53668/53668.2.diff.

Extends wp_unique_filename() to account for image files that will be converted after uploading. The file names with both the existing and expected extensions are checked against existing files.

Trac ticket: https://core.trac.wordpress.org/ticket/53668.

[ianmjones]

Added test for future clashing filenames from existing file when thumbnails regenerated, better name for utility function as suggested by @azaozz.

[ianmjones]

@azaozz Thanks for taking a look at my patch and suggesting a variation that does not use a helper function and a reduced iteration set.

However, I think your version misses out on a key reason that I used a helper function and recursion. Here's the scenario...

1. Upload picture.png before any implementation of the image_editor_output_format filter (or just remove the generated thumbnails).
2. Implement image_editor_output_format to generate jpg thumbnails for the image/png type.
3. Upload picture.jpg.
4. Regenerate thumbnails for picture.png.

With your patch I believe picture.jpg will not be versioned on upload, meaning that when the thumbnails are regenerated for picture.png they will overwrite the thumbnails for picture.jpg.

You can see me testing my working solution for this scenario in my original session for working on this bug, the following link should be direct to the timestamp of 1 hour 40 mins.

​https://youtu.be/sMDcIOnEvjU?t=6024

This particular scenario also happens to be important for a certain plugin that offloads media and optionally removes the files from the server. 😉️

I've updated my patch to include an assertion that covers this scenario, and also added the function name change that you made.

[azaozz]

Replying to ianmjones:

Yes the alt patch needs more work.

Looking at _wp_check_alternate_output_format_uniqueness() again, seems it tries to account for "circular" use of the image_editor_output_format filter. For example:

Array(
    'image/png' => 'image/gif',
    'image/gif' => 'image/jpeg',
    'image/jpeg' => 'image/png'
)

In that case uploaded PNG image will be converted to a GIF. There's no need to test for additional conversion settings. This is a weakness/lack of docs in the current filter implementation that needs fixing, the file naming shouldn't account for such cases.

Then _wp_check_alternate_output_format_uniqueness() runs wp_unique_filename() in a loop after the original filename was made unique. However seems that loop will run only once as there's static $checking_alternates; which prevents calling wp_unique_filename() second time. That's okay, a loop doesn't seem needed. There can only be one conversion per uploaded image as explained above. In addition running all of wp_unique_filename() again seems not that efficient. That's why I started on an alt patch, to see if things can be simplified and more efficient :)

[ianmjones]

Replying to azaozz:

Yes the alt patch needs more work.

Happy to test and review your PR when it's ready, I'm ianmjones on GitHub.

Looking at _wp_check_alternate_output_format_uniqueness() again, seems it tries to account for "circular" use of the image_editor_output_format filter. For example:

Array(
    'image/png' => 'image/gif',
    'image/gif' => 'image/jpeg',
    'image/jpeg' => 'image/png'
)

In that case uploaded PNG image will be converted to a GIF. There's no need to test for additional conversion settings. This is a weakness/lack of docs in the current filter implementation that needs fixing, the file naming shouldn't account for such cases.

It does cover that scenario, but only as a side-effect of the thumbnail regeneration protection scenario I was going for.

Then _wp_check_alternate_output_format_uniqueness() runs wp_unique_filename() in a loop after the original filename was made unique. However seems that loop will run only once as there's static $checking_alternates; which prevents calling wp_unique_filename() second time. That's okay, a loop doesn't seem needed. There can only be one conversion per uploaded image as explained above. In addition running all of wp_unique_filename() again seems not that efficient. That's why I started on an alt patch, to see if things can be simplified and more efficient :)

There are two loops in my new code, one checks the results of image_editor_output_format to find all the extensions that need to be analysed, and the second to check them via wp_unique_filename.

The first loop ensures that...

1. The alternate thumbnail output format for the just uploaded file is checked for clashes.
2. Any other upload file format that might also generate the same alternate thumbnail output format are checked for clashes.
3. Any other upload file format that might generate the same thumbnail output format as the uploaded file's native format are checked for clashes.

The static is there so that those loops are not run again when wp_unique_filename is called from within the second loop. I did it this way because wp_unique_filename works well for testing a single extension, and I did not want to complicate that function. The recursion into it is only one level as a result, the new function is effectively skipped on that second entry into wp_unique_filename, without needing to alter wp_unique_filename bar that one function call.

[azaozz]

Replying to ianmjones:

Happy to test and review your PR when it's ready

Sure, it's linked above.

It does cover that scenario, but only as a side-effect of the thumbnail regeneration protection scenario I was going for.

Ah, right. Added a check for that in the alt patch too. Think it's ready. It does the tests for the alt file extension in the same while loop as the tests for upper case and normal extensions. Another change is that wp_get_default_extension_for_mime_type() requires the mime-type arg. Also updated few of the test explanation strings for a bit more clarity.

Alt patch for https://core.trac.wordpress.org/ticket/53668 partially based on https://core.trac.wordpress.org/attachment/ticket/53668/53668.2.diff (take 2).

Extends wp_unique_filename() to account for image files that will be converted after uploading. The file names with the original and expected extensions are checked against existing files.

Trac ticket: https://core.trac.wordpress.org/ticket/53668.

[ianmjones]

New image added by @azaozz in alternate PR needed for tests.

[ianmjones]

New image added by @azaozz in alternate PR needed for tests.

[ianmjones]

More robust handling of versioned files, uppercase extensions including on pre-existing files, and re-analysis of of original upload type previously tested alternate types in response to filename versioning. Inspired by @azaozz's alternate PR, and includes his changes for tests and wp_get_default_mime_type_extension.

[azaozz]

53668.4.diff looks better but I still think that it's not good to run wp_unique_filename() recursively:

• It's slower.
• It would fire the filters several times for the same file which might cause problems.

Also, this won't work well imho:

// If filename already versioned, get version and un-versioned filename.
if ( preg_match( '/-(\d)$/', $fname, $matches ) ) {
     $fname  = preg_replace( '/' . $matches[0] . '$/', '', $fname );
     $number = (int) $matches[1];
}

It may match an uploaded file's name and we'll end up changing the original file name instead of appending a dash and a number. This can get pretty "messy" when somebody tries to upload files named something like: text-1.pdf, text-2.pdf, text-3.pdf, etc.

Ideally the increasing of $number and the renaming should happen all in the same loop. That would ensure simplicity and consistency. When checking alternate file names all of them, including the original name, have to be checked at the same time. Cannot be done in a loop one by one as there may be "gaps" in the numbering of existing files.

For example: existing files like img.jpg, img-1.png, img-2.webp, img-3.png and converting PNG and JPEG to WebP, and uploading img.jpg again. Then the sub-sizes of img-3.png will be overwritten if the alt name with .png extension is checked before the alt name with .webp (I'll try to make another test for this edge case).

[ianmjones]

Ensure original extension is supplied to exit filter, add test case that exercises the exit filter to ensure known but temporarily unavailable filenames are checked.

[ianmjones]

Ensure original extension passed to callable.

[ianmjones]

Replying to azaozz:

53668.4.diff looks better but I still think that it's not good to run wp_unique_filename() recursively:

• It's slower.

I suspect it will be a tiny bit slower if the filter is implemented and there's work to be done, but I doubt it's that much.

• It would fire the filters several times for the same file which might cause problems.

Each filename will be different, and this already happens, never seen a problem. What kind of problem do you envision?

Also, this won't work well imho:

// If filename already versioned, get version and un-versioned filename.
if ( preg_match( '/-(\d)$/', $fname, $matches ) ) {
     $fname  = preg_replace( '/' . $matches[0] . '$/', '', $fname );
     $number = (int) $matches[1];
}

It may match an uploaded file's name and we'll end up changing the original file name instead of appending a dash and a number. This can get pretty "messy" when somebody tries to upload files named something like: text-1.pdf, text-2.pdf, text-3.pdf, etc.

Ideally the increasing of $number and the renaming should happen all in the same loop. That would ensure simplicity and consistency. When checking alternate file names all of them, including the original name, have to be checked at the same time. Cannot be done in a loop one by one as there may be "gaps" in the numbering of existing files.

For example: existing files like img.jpg, img-1.png, img-2.webp, img-3.png and converting PNG and JPEG to WebP, and uploading img.jpg again. Then the sub-sizes of img-3.png will be overwritten if the alt name with .png extension is checked before the alt name with .webp (I'll try to make another test for this edge case).

As mentioned in your PR, I don't think my patch has the problem you mention here with regards to skipped png version, recursion saves the day! 😉️

I see your point about the code you mentioned though. If uploading a new series of photos (pic-1.png, pic-2.png, pic-3.png) and the second "pic-2.png" clashes with "pic-2.jpg", what do you do?

a) pic-1.png, pic-2-1.png, pic-3.png
b) pic-1.png, pic-3.png, pic-4.png

I take it (a) is expected?

[azaozz]

Replying to ianmjones:

Replying to azaozz:

For example: existing files like img.jpg, img-1.png, img-2.webp, img-3.png and converting PNG and JPEG to WebP, and uploading img.jpg again. Then the sub-sizes of img-3.png will be overwritten if the alt name with .png extension is checked before the alt name with .webp (I'll try to make another test for this edge case).

As mentioned in your PR, I don't think my patch has the problem you mention here with regards to skipped png version, recursion saves the day! 😉️

The logic there is that for each increase of $number all possible filenames have to be checked. Doing that with recursion means it has to "re-start" each time a possible name clash is detected.

Then if the conversion settings are like:

Array(
    'image/png' => 'image/webp',
    'image/jpeg' => 'image/webp',
    'image/gif' => 'image/webp',
)

it will have to check 4 name variations for each $number increase. This potentially can result in running wp_unique_filename() including the two filters and scandir() 10-15 times. It's definitely going to be quite slow :)

Also firing wp_unique_filename filter 10-15 times mostly for filenames that will never be used seems like a regression. Perhaps we can pass the alt filenames as another argument there, or maybe only the expected filename after the image conversion.

I see your point about the code you mentioned though. If uploading a new series of photos (pic-1.png, pic-2.png, pic-3.png) and the second "pic-2.png" clashes with "pic-2.jpg", what do you do?

a) pic-1.png, pic-2-1.png, pic-3.png
b) pic-1.png, pic-3.png, pic-4.png

I take it (a) is expected?

Yep, as pic-2 is the original name that WP shouldn't change.

[ianmjones]

Replying to azaozz:

it will have to check 4 name variations for each $number increase. This potentially can result in running wp_unique_filename() including the two filters and scandir() 10-15 times. It's definitely going to be quite slow :)

Yeah, but I honestly couldn't think of a more complete and correct way of doing it until ...

Also firing wp_unique_filename filter 10-15 times mostly for filenames that will never be used seems like a regression. Perhaps we can pass the alt filenames as another argument there, or maybe only the expected filename after the image conversion.

... bingo! Adding the alternative filenames that a filter implementer could check against would really help. :)

In the case of WP Offload Media, if there was an array of filenames with alternate extensions that have passed the check along with the proposed filename, we could quickly check our records of offloaded filenames for the same dir to see whether there's a clash and do the usual version bump we do.

[azaozz]

Looking at the new test in 53668.6.diff, not sure that's a good use of the wp_unique_filename filter. Perhaps having access to each filename that's checked for collision may be used in plugins. May be better to add another/specific filter there that should probably just filter $number and not allow changing the (currently tested) filename?

[azaozz]

Replying to ianmjones:

... bingo! Adding the alternative filenames that a filter implementer could check against would really help. :)

Okay, lets add that :)

we could quickly check our records of offloaded filenames for the same dir to see whether there's a clash and do the usual version bump we do.

That would need another run through wp_unique_filename(), right? Probably quite rarely.

Wondering what would be better:

1. A filter for $new_number inside the loop where the alt filenames are checked.
2. The array of the alt filenames (and perhaps $number?) in the closing wp_unique_filename filter.

[ianmjones]

Replying to azaozz:

Okay, lets add that :)

@ianmjones does a little dance. :)

we could quickly check our records of offloaded filenames for the same dir to see whether there's a clash and do the usual version bump we do.

That would need another run through wp_unique_filename(), right? Probably quite rarely.

Not at the moment, we do a quick check of the local filesystem and database ourselves. As we're generally at the end of the filter chain we avoid going round the horn ourselves too.

Wondering what would be better:

1. A filter for $new_number inside the loop where the alt filenames are checked.
2. The array of the alt filenames (and perhaps $number?) in the closing wp_unique_filename filter.

Definitely (2) as the alt filenames are fast to check, and the $number would be neat as we could use that as a starting point if non-zero rather than starting from 1 ourselves as we currently have to.

[azaozz]

Replying to ianmjones:

Definitely (2) as the alt filenames are fast to check, and the $number would be neat as we could use that as a starting point if non-zero rather than starting from 1 ourselves as we currently have to.

Sure, added to ​https://github.com/WordPress/wordpress-develop/pull/1591. In that case thinking the PR is ready for commit. Could you confirm it works well for your cases.

[ianmjones]

Replying to azaozz:

In that case thinking the PR is ready for commit. Could you confirm it works well for your cases.

Great, I'll give it a test and review as soon as I can, probably early next week.

[ianmjones]

Apart from the one minor code comment regarding undefined variables, looking great @azaozz. 🎉

I ran the tests, manually tested a bunch of scenarios, and reviewed the code, passed with flying colours!

[azaozz]

Replying to ianmjones:

Great, thanks!

[azaozz]

In 51653:

Media: Fix wp_unique_filename() to check for name collisions with all alternate file names when an image may be converted after uploading. This includes possible collinions with pre-existing images whose sub-sizes/thumbnails are regenerated.

Props ianmjones, azaozz.
Fixes #53668.

[azaozz]

Reopen for 5.8.1.

Committed in https://core.trac.wordpress.org/changeset/51653.

[peterwilsoncc]

Removing fixed-major keyword as the earlier commit has already been merged to the 5.8 branch.

[desrosj]

In 51693:

Coding Standards: Apply some minor alignment fixes.

These are updates caused by running composer format.

Follow up to [51501], [51599], [51618], [51653].
See #53359, #50542, #53238, #53668, #53690.

[azaozz]

Replying to peterwilsoncc:

Removing fixed-major keyword as the earlier commit has already been merged to the 5.8 branch.

Right but it was for WP 5.8. The actual fix, [51653] seems ready and still needs merging to the 5.8 branch for 5.8.1.

[desrosj]

I'll be looking at testing [51653] today to decide about including it in 5.8.1. The RC is due out tomorrow.

[desrosj]

[51653] and [51693] are looking good to backport. I haven't been able to reproduce the bahavior after those commits are made.

At first glance, I thought that 10,000 for the loops's upper limit was unnecessarily high, but the odds that a site will get over 100 or 1,000 is most likely a super edge case.

I say let's ship it.

[desrosj]

In 51706:

Media: Fix wp_unique_filename() to check for name collisions with all alternate file names when an image may be converted after uploading. This includes possible collinions with pre-existing images whose sub-sizes/thumbnails are regenerated.

Props ianmjones, azaozz.
Merges [51653] to the 5.8 branch.
Fixes #53668.

[desrosj]

In 51707:

Coding Standards: Apply some minor alignment fixes.

These are updates caused by running composer format.

Follow up to [51653].

Partially merges [51693] to the 5.8 branch.
See #53668.

[johnbillion]

In 52034:

Docs: Fix some docblock syntax errors and add a missing canonical reference.

See #53399, #52867, #38942, #53668