[wp-trac] [WordPress Trac] #42437: Thumbnails can overwrite other uploads if filename matches

[WordPress Trac]

#42437: Thumbnails can overwrite other uploads if filename matches
--------------------------+-----------------------
 Reporter:  Viper007Bond  |       Owner:  pbiron
     Type:  defect (bug)  |      Status:  assigned
 Priority:  normal        |   Milestone:  5.3.1
Component:  Upload        |     Version:  4.8.3
 Severity:  normal        |  Resolution:
 Keywords:  needs-patch   |     Focuses:
--------------------------+-----------------------

Comment (by pbiron):

 [https://core.trac.wordpress.org/attachment/ticket/42437/42437.2.diff
 42437.2.diff] updates my old POC patch.

 1. all the new collision detection is in `wp_unique_filename()` (instead
 of in `_wp_handle_upload()` as in the POC)
 2. replaces the `glob()` from the POC with an `array_filter()` on the
 results of `scandir()`.  This is for 2 reasons: 1) glob isn't expressive
 enough (not full regex) and 2) as
 [https://core.trac.wordpress.org/ticket/42437#comment:31 @azaozz] pointed
 out, glob doesn't doesn't do remote files, e.g., CDNs, but `scandir()`
 does)
 3. refactors `wp_unique_filename()` so that there is only 1 return point
 (and one `apply_filters( 'wp_unique_filename')`).  This made it easier to
 do the final collision check (number 3 below)
 4. adds a simple unit test (luckily, there is already an image in the test
 data with a "dimension-like" filename)

 A few notes:

 The revised patch:

 1. adds a number to any file that could cause a collision with sub-sizes
 2. Then, it does the same collision detection that has always existed.
 3.  An finally, it checks collisions with existing files (e.g., sub-size
 files uploaded before the patch was applied).  That last check addresses
 [https://core.trac.wordpress.org/ticket/42437#comment:18 @blobfolio]'s
 comment.

 My thinking is that it is best to bracket the existing collision detection
 checks with the new ones.  Curious what others think...

 I locally ran all of the existing unit tests that directly call
 `wp_unique_filename()` and they all still pass.  I have not run **all** of
 the existing unit tests, but I figure that if the ones that directly call
 `wp_unique_filename()` aren't enough to catch problems then we should
 probably add some more direct-call tests :-)

 To test (without running the unit tests), be sure you upload at 1 file
 that has a "dimension-like" filename **before** applying the patch.  For
 example, upload ''image-150x150.jpg''.  Then apply the patch.  Then upload
 ''image.jpg'', which should get renamed to ''image-1.jpg'' before sub-
 sizes are created.  The upload ''another-image-150x150.jpg'', which should
 get renamed to ''another-image-150x150-1.jpg'' before sub-sizes are
 created.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/42437#comment:35>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform