WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 5 weeks ago

#48975 closed defect (bug) (fixed)

Fix unhandled upper/lower case change in wp_unique_filename()

Reported by: azaozz Owned by: azaozz
Milestone: 5.3.2 Priority: high
Severity: major Version: 5.3.1
Component: Upload Keywords: has-patch 2nd-opinion has-unit-tests fixed-major
Focuses: Cc:
PR Number:

Description (last modified by 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.

Attachments (2)

48975.diff (2.1 KB) - added by azaozz 6 weeks ago.
48975.2.diff (2.4 KB) - added by azaozz 5 weeks ago.

Download all attachments as: .zip

Change History (19)

@azaozz
6 weeks ago

#1 @azaozz
6 weeks ago

  • Keywords has-patch 2nd-opinion needs-testing has-unit-tests added
  • Priority changed from normal to high
  • Severity changed from normal to major

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.

#2 @pbiron
6 weeks ago

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.

#3 @azaozz
6 weeks ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


6 weeks ago

#5 @audrasjb
6 weeks ago

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

#6 @audrasjb
6 weeks ago

  • Keywords needs-testing removed

#7 @karl94
6 weeks ago

@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
5 weeks ago

#8 @azaozz
5 weeks ago

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.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


5 weeks ago

#10 @azaozz
5 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#11 @azaozz
5 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 5.3.2.

#12 follow-up: @pbiron
5 weeks ago

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.
  1. should we add add unit tests that specifically test for that case (and the one #48960)?
  1. 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");.

#13 in reply to: ↑ 12 @azaozz
5 weeks ago

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 :)

Last edited 5 weeks ago by azaozz (previous) (diff)

#14 @azaozz
5 weeks ago

In 46976:

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

Props pbiron.
See #48975.

#15 follow-up: @pbiron
5 weeks ago

@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.

#16 in reply to: ↑ 15 @azaozz
5 weeks ago

Replying to pbiron:

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

#17 @azaozz
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.