Make WordPress Core

Opened 8 years ago

Closed 10 months ago

Last modified 8 months ago

#35755 closed defect (bug) (fixed)

wp_tempnam produces filenames longer than 255 characters

Reported by: doems's profile doems Owned by: azaozz's profile azaozz
Milestone: 6.3 Priority: normal
Severity: normal Version: 4.4
Component: Filesystem API Keywords: has-patch has-unit-tests dev-feedback has-testing-info
Focuses: Cc:

Description

Summary
Since the maximal filename length of the most filesystems out there is 255, the function should try to avoid filenames longer than 255 characters.

Reproduce
If the passed-in $filename parameter is longer than 245 characters (WordPress appends a string which has 10 characters), PHP will fail on the following line: https://github.com/WordPress/WordPress/blob/6995ec613e5f113cedea4f0481b9030f79fabc0a/wp-admin/includes/file.php#L183.

Fix

<?php
if ( strlen( $filename ) > 245 ) {
    $filename = substr( $filename, 0, 244);
}

Attachments (1)

test_35755.zip (829 bytes) - added by costdev 10 months ago.
Test plugin.

Download all attachments as: .zip

Change History (18)

#1 @obenland
8 years ago

  • Version changed from trunk to 4.4

Introduced in [35644] for #34562.

This ticket was mentioned in PR #4647 on WordPress/wordpress-develop by @costdev.


10 months ago
#2

  • Keywords has-patch has-unit-tests added

Most filesystems have a maximum filename length of 255 characters.
wp_tempnam() adds characters to the $filename to make it unique, which can push it beyond this limit.

fopen() will fail silently due to the @ error suppression operator, and the file won't be created.
The suppressed warning is:

Warning: failed to open stream: File name too long

Existing checks inside wp_tempnam() fail to capture this failure, and even if they did, simply recalling wp_tempnam() will produce the same issue.

This checks the size of the generated unique filename before calling fopen().
If it's over 255, the additional characters are subtracted from the initial $filename and it's passed to wp_tempnam() again.

Includes PHPUnit tests.

#3 @costdev
10 months ago

  • Keywords dev-feedback added
  • Milestone set to 6.3

@azaozz @mikeschroder @audrasjb PR 4647 addresses the issue directly in wp_tempnam().

In the future, this fix could possibly be extended/moved to wp_unique_filename().

However, as wp_unique_filename() is used within the Media component and has other considerations/potential impacts, this would need investigation by Media component maintainers in a separate ticket. As we don't know how long this may take, we can fix wp_tempnam() in the meantime so its users are protected from this issue.

It would be ideal to get this fix considered for inclusion in Beta 1 as it touches a Filesystem API function

Last edited 10 months ago by costdev (previous) (diff)

#4 @kirasong
10 months ago

I have not tested this manually, but I think the approach is solid, and am +1 on this.

I wish we could do it at the same time for wp_unique_filename(), but fixing it in one place doesn't keep us from doing future changes!

#5 @oglekler
10 months ago

wp_unique_filename() should not be responsible for file name length, it has one clear purpose. It is possible to introduce a new function, something like file_name_sanity_check() but it should be called before the wp_unique_filename().

#6 @costdev
10 months ago

@oglekler What would the new function do before wp_unique_filename() is run?

#7 @oglekler
10 months ago

@costdev you wrote on the PR that wp_unique_filename() is called a second time after cutting the filename, but calling a function a second time doesn't look like an optimal scenario. So, I believe that a better approach will be to check name length and cut it if needed, and only then call wp_unique_filename() once. If check of file length and cutting will be moved to another function, it will be accessible to use in other places.

#8 @costdev
10 months ago

If we cut the filename, then call wp_unique_filename(), it could add characters that make the filename go over the 255 character limit.

The return value of wp_unique_filename() is also filterable, so we can't anticipate the length accurately. To ensure the unique filename isn't over the limit, we need to check the difference between the result of wp_unique_filename() and 255, and cut the difference off the original filename before re-running.

#9 @costdev
10 months ago

@azaozz @oglekler I've added some more test cases to PR 4647 which protect the new behaviour. You can test ideas for alternative implementation locally with vendor/bin/phpunit --group 35755 (or npm run test:php -- --group 35755 if you're using wp-env).

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


10 months ago

#11 @mrinal013
10 months ago

  • Keywords needs-testing-info added

This ticket is discussed in yesterday's bug-scrub meeting.

Seems like it is complex to test from the user's perspective. So, what are the steps to test?

#12 @mrinal013
10 months ago

PHPUnit Test result

npm run test:php -- --group 35755
......
OK (9 tests, 20 assertions)

wp --info

OS: Linux 5.19.0-46-generic #47~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Jun 21 15:35:31 UTC 2 x86_64
......
PHP version: 8.2.7
.....
MySQL version: mysql Ver 8.0.33-0ubuntu0.22.04.2 for Linux on x86_64 ((Ubuntu))

Last edited 10 months ago by mrinal013 (previous) (diff)

@costdev
10 months ago

Test plugin.

#13 @costdev
10 months ago

  • Keywords has-testing-info added; needs-testing-info removed

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce

  1. Install and activate the test plugin.
  2. 🐞 Check the results of the admin notice.

Expected Results

When reproducing a bug:

  • ❌ The notice should display the following:

Length of filename to be created: 311 ❌
Was the file created? No ❌

When testing a patch to validate it works as expected:

  • ✅ The notice should display the following:

Length of filename to be created: 252 ✅
Was the file created? Yes ✅

#14 @costdev
10 months ago

@azaozz @mikeschroder What do you think, is this ready for commit consideration now ahead of 6.3 Beta 4 tomorrow?

#15 @azaozz
10 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 56186:

Filesystem API: Ensure wp_tempnam() does not produce file names longer than 255 characters as this is the limit on most filesystems.

Props: costdev, doems, mikeschroder, oglekler, mrinal013.
Fixes: #35755.

#17 @costdev
8 months ago

#52844 was marked as a duplicate.

Note: See TracTickets for help on using tickets.