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

[devinreams]

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.

[SergeyBiryukov]

Sounds like a duplicate of #16330.

[devinreams]

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

[jeremyfelt]

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)

[jeremyfelt]

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.

[nacin]

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?

[jeremyfelt]

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.

[Ipstenu]

Broken on Apache 3.4.2 Multisite.

[jeremyfelt]

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.

[devinreams]

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.

[nacin]

Not a regression, moving version down.

[jamescollins]

#24117 was marked as a duplicate.

[jamescollins]

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

[jamescollins]

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.

[nacin]

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.

[jamescollins]

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.

2. 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).

[nacin]

Replying to jamescollins:

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

[jamescollins]

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

[wonderboymusic]

Fixed in [29290].

[jamescollins]

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

Thanks :)