WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 8 months ago

Last modified 8 months ago

#22813 closed defect (bug) (fixed)

Media Uploader doesn't escape "+" in filenames and doesn't upload file

Reported by: devinreams Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.4.2
Component: Media Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

I downloaded a photo this morning with filename Screenshot+2012-12-07+at+08.55.33.png — when I upload it in the WordPress media uploader it acts like its uploading (progress bar, crunching) but then shows a broken image as the result.

  1. Upload media with "+" in filename
  2. Observe progress bar uploading and complete the upload

Expected: file is uploaded A-OK to server, attachment created, image loads as expected.

Actual: no file is stored in uploads (or blogs.dir/files) directory, attachment is still created, no error, file does not exist.

FWIW the filename that WordPress expects to have uploaded is /2012/12/Screenshot+2012-12-07+at+08.55.33-150x150.png

I'm on Media Temple, running trunk as of this morning. Not sure if a regression, specific to my server configuration, etc. so more testing here would be appreciated.

Attachments (2)

22813.diff (833 bytes) - added by jamescollins 2 years ago.
22813-ms-files.diff (550 bytes) - added by jamescollins 22 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 @SergeyBiryukov2 years ago

Sounds like a duplicate of #16330.

comment:2 @devinreams2 years ago

I think it's similar but likely not quite a duplicate, since my file was not actually getting uploaded to the uploads directory...

comment:3 @jeremyfelt2 years ago

I'm able to reproduce on 3.5-RC4-23121, though with slightly different results.

Uploaded a file "screen+shot+1+2+3.png" in WordPress 3.4.2 (Multisite), everything appears to work as expected. An immediate thumbnail appears showing uploaded image, accessible via http://domain.dev/files/2012/12/screen+shot+1+2+3-150x150.png

Uploaded same file in WordPress 3.5-RC4-23121 (Multisite), upload appears fine with proper resized versions in /blogs.dir/files/... Image is not accessible via direct URL (404) at http://domain.dev/files/2012/12/screen+shot+1+2+3.png (no thumbnail size attached)

comment:4 @jeremyfelt2 years ago

This may also be server related now that I look more. My trunk and 3.4.2 environments are slightly different.

$_GET['file'] on my 3.4.2 Multisite (nginx / PHP 5.3.6 ) returns the proper filename, so passes the is_file() check in ms-files.php

$_GET['file'] on 3.5-RC4-23121 Multisite ( Apache / PHP 5.3.6 ) returns the filename with spaces instead of +, which fails the is_file()check.

comment:5 @nacin2 years ago

Hmm, in single site, http://localhost/beta/wp-content/uploads/2012/12/Screen-Shot+2012-12-07+at+4.42.18-AM.png worked fine for me.

blogs.dir has always been pretty bad, though it's disconcerting this supposedly worked in 3.4.2 with the same setup?

comment:6 @jeremyfelt2 years ago

Different setup - apache vs nginx. Setting up 3.4.2 locally now.

I *think* it's how the server is handling the URL before it hits WordPress.

comment:7 @Ipstenu2 years ago

Broken on Apache 3.4.2 Multisite.

comment:8 @jeremyfelt2 years ago

Same on 3.4.2 Multisite with Apache for me, broken file.

Problem follows the server setup - Apache - rather than the WordPress setup. Confusing issue, but no regression.

comment:9 @devinreams2 years ago

Just to pile on with a quick update, this was on an existing multisite installation (not a new install) so is an issue in ms-files.

I set my sitemeta to have ms_files_rewriting = 0 then these images with characters like "=" upload fine. But, then all my existing image paths break.

Version 0, edited 2 years ago by devinreams (next)

comment:10 @nacin2 years ago

  • Version changed from trunk to 3.4.2

Not a regression, moving version down.

comment:11 @jamescollins2 years ago

#24117 was marked as a duplicate.

@jamescollins2 years ago

comment:12 @jamescollins2 years ago

  • Keywords has-patch added; needs-patch removed

(as per #24117), I'm also seeing this problem on our Apache servers on our multisite installations that use ms-files.php rewriting.

The safest approach here seems to be to automatically remove + characters from file names during the upload process.

22813.diff achieves this by modifying the sanitize_file_name() function.

This problem is also similar to the one reported in #16330 (which also has a patch).

This solution solves the problem for new files that are uploaded, however it doesn't solve it for any existing media library files that contain a plus sign in their name.

comment:13 @jamescollins22 months ago

Could we please consider this for inclusion in 3.7?

Unless anyone can think of a legitimate reason why the + character should be allowed in file names, then this change is relatively straightforward and ensures that Multisite users don't encounter issues.

comment:14 @nacin22 months ago

  • Keywords needs-unit-tests added

I don't really see why we should allow + in filenames. It's nice that we mostly handle it, I guess, but that's not enough.

I think we should close this as a duplicate of #16330. Very similar problem, same solution.

On the other hand, can we figure out why this is broken? As in, how can we make this work while still supporting + in filenames? Not saying we should, but it would help to understand why this is broken.

This sounds like a good candidate for unit tests.

comment:15 follow-up: @jamescollins22 months ago

Thanks nacin.

This bug is a problem for WordPress multisite installations that use ms-files.php rewriting when using Apache. Nginx seems to work fine according to feedback above.

By default WordPress Multisite with ms-files rewriting uses this rule in the .htaccess file:

RewriteRule ^files/(.+) wp-includes/ms-files.php?file=$1 [L]

But doing that, ms-files.php receives a $_GET[ 'file' ] value that uses a space instead of the + sign.

There are two potential ways to fix this so that ms-files.php rewriting in multisite works for files with + characters in them:

  1. Use the B flag in .htaccess, which would mean .htaccess would need to use:

RewriteRule ^files/(.+) wp-includes/ms-files.php?file=$1 [L,B]

The problem with this is that the B flag is only available in Apache 2.2.7 and newer. It would also be difficult to get existing installs to update their .htaccess files.

  1. In ms-files.php, use urlencode( $_GET[ 'file' ] ) instead of $_GET[ 'file' ]) on line 26.

The third (and probably less risky alternative) is to go with the existing patch which makes sure future uploaded files can't contain the + character.

If you know of a way we could write unit tests for any (or all) of this, then I'd love to know. As far as I can tell, the Multisite unit tests sets up a multisite instance that doesn't use ms-files.php rewriting, so we'd have to add a way to do that, and then possibly some unit tests that uploads a test file to the WordPress media library and then performs a wp_remote_get() request on it to make sure it gets a HTTP 200 (not 400) response.

Interestingly, I just tried uploading a file called wordpress logo test file which contains a + character.png to wordpress.com, and it was renamed to wordpress-logo-test-file-which-contains-a-character.png (ie the + character was removed).

comment:16 in reply to: ↑ 15 ; follow-up: @nacin22 months ago

Replying to jamescollins:

  1. In ms-files.php, use urlencode( $_GET[ 'file' ] ) instead of $_GET[ 'file' ]) on line 26.

Perhaps it is just a matter of replacing ' ' with '+'? Beyond spaces, it seems odd to be allowing possible url-encoded values in filenames, but spaces seem like something we should be able to handle. Especially since it is A) very common for spaces to be in filenames created by most computer users, and B) it only breaks when using functionality many of us never wished existed and since retired for new networks (ms-files).

If you know of a way we could write unit tests for any (or all) of this, then I'd love to know.

Oh, I was only referring to updating the existing unit tests for sanitize_file_name(). Sorry for the confusion.

Interestingly, I just tried uploading a file called wordpress logo test file which contains a + character.png to wordpress.com, and it was renamed to wordpress-logo-test-file-which-contains-a-character.png (ie the + character was removed).

I guess we could ask them if they are using the sanitize_file_name_chars filter to add in others, or doing some other kind of en/decoding or sanitization.

comment:17 in reply to: ↑ 16 @jamescollins22 months ago

Replying to nacin:

Perhaps it is just a matter of replacing ' ' with '+'?

Agreed. 22813-ms-files.diff​ is a relatively simple solution I think.

It fixes existing uploaded files for me (Apache 2.2.22). I don't have an nginx install to test the patch on though.

I guess we could ask them if they are using the sanitize_file_name_chars filter to add in others, or doing some other kind of en/decoding or sanitization.

Not a bad idea to check with them. I've been using this on our multisite installs since April, and it hasn't caused any issues.

function my_sanitize_file_name_chars( $special_chars, $filename_raw ) {
	if ( ! in_array( "+", $special_chars ) )
		$special_chars[] = "+";
	return $special_chars;
}
add_filter( 'sanitize_file_name_chars', 'my_sanitize_file_name_chars', 10, 2 );

comment:18 @wonderboymusic8 months ago

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

Fixed in [29290].

comment:19 @jamescollins8 months ago

Is it possible to change the Milestone of this ticket to 4.0 ?

Thanks :)

comment:20 @dd328 months ago

  • Milestone changed from Awaiting Review to 4.0
Note: See TracTickets for help on using tickets.