Fix unhandled upper/lower case change in wp_unique_filename()

[azaozz]

Introduced in #42437, [46822].

On non case-sensitive file systems it may fail to match a file name when the uploaded file is with upper case extension. Also if a file with upper case extension is uploaded, and the file name matches exactly an existing file name that has dimensions-like ending, it may fail to rename the file causing a fatal error.

Both cases seem very rare but worth fixing asap as uploading a file with upper case (or mixed case) extension may result in a fatal error.

[azaozz]

In 48975.diff: fix wp_unique_filename() when uploading a file with upper case extension and the file name matches an existing file with "sub-size like" file name.

[pbiron]

Good catch!!!

I can confirm the problem does exist in 5.3.1 and that 48975.diff fixes it and the unit tests pass locally.

[karl94]

@azaozz it's unclear to me how this could trigger a fatal error, could you be more specific?
To my knowledge in PHP a rename() call should only trigger warnings, not fatal errors. Do you have an example of fatal error message caused by this?

[azaozz]

In 48975.2.diff: same as 48975.diff plus a simple limit for the while loop (just in case) as it uses different methods for checking and changes.

@karl94 it can when the extension is upper case as the replacement (the change inside the while loop) will fail.

[azaozz]

In 46966:

Upload: Fix the final file name collision test in wp_unique_filename() when uploading a file with upper case extension. Add a unit test to catch that in the future.

Fixes #48975 for trunk.

[azaozz]

Reopen for 5.3.2.

[pbiron]

I just had a look at the latest patch and have a couple of questions (sorry I didn't get to this until after you committed it):

1. checking that it hasn't gone through the last while() loop more times than there are files in the directory seems like a sensible safe guard. However, if it does hit that limit then the filename is returned as it is at that point...which means that there would (could?) still be a conflict. I don't have any suggestions off the top of my my head for how to address that, but wanted to mention it.

2. should we add add unit tests that specifically test for that case (and the one #48960)?

3. similarly, the last 2 assertions in test_unique_filename_with_dimension_like_filename() have a comment that certain files should exist is DIR_TESTDATA . '/images/' for them to actually test what they are intended to test. I think it would be good if there were assertions to test that (instead of just the comment)...just in case (e.g., $this->assertTrue( file_exists( "{$testdir}one-blue-pixel-100x100.png"); $this->assertTrue( file_exists( "{$testdir}one-blue-pixel-1-100x100.png");.

[azaozz]

Replying to pbiron:

...which means that there would (could?) still be a conflict.

Right, think it needs to run one more time, i.e. count + 1. To do that need to set $i = 0; initially.

We check one string (file name) against an array of possible matches. If there are 5 files in the dir (array has 5 elements) and all have names that match, the 6th run will add -6 to the file name making it different.

Should we add unit tests that specifically test for that case (and the one #48960)?

Sure, the more tests -- the better :)

Similarly, the last 2 assertions in test_unique_filename_with_dimension_like_filename() have a comment that certain files should exist is DIR_TESTDATA . '/images/'...

Yeah, makes sense to confirm the files actually exist. The tests will fail if they don't so it would be pretty easy to spot if any of them is deleted, but having a test will point to the right reason :)

[azaozz]

In 46976:

Upload: Run the final file name collision test in wp_unique_filename() for each existing file + 1.

Props pbiron.
See #48975.

[pbiron]

@azaozz thanx for all your work on this!

Unfortunately, I'm tied up in other things and won't be able to get to the other unit tests until after 5.3.2, but I think that's OK.

[azaozz]

Replying to pbiron:

Yep, we can update the tests later, during 5.4 :)

[azaozz]

In 46980:

Upload: Fix the final file name collision test in wp_unique_filename() when uploading a file with upper case extension and limit it to run for each file in the directory + 1. Add a unit test to catch that in the future.

Props pbiron, azaozz.
Merges [46966] and [46976] to the 5.3 branch.
Fixes #48975.