"%20" in an uploaded image file name breaks thumbnails

[ripperdoc]

When uploading an image file where the name contains %20, it will not show thumbnails. It is still possible to edit the image in the image editor, however.

[SergeyBiryukov]

Related: #16330

[ramoonus]

not a duplicate then?

[kurtpayne]

Patch to fix %XX in media filenames

[kurtpayne]

18412.diff​ will double-encode any %XX patterns in media URLs so they are properly decoded when returned to the browser.

[c3mdigital]

Refresh of existing patch

[SergeyBiryukov]

#25463 was marked as a duplicate.

[ericlewis]

This seems like a bizarre solution. @kurtpayne still think this is the way to go?

[kurtpayne]

Replying to ericlewis:

This seems like a bizarre solution. @kurtpayne still think this is the way to go?

The general answer to these types of problems is to double-encode or use a token replacement (e.g. '-real-percent-plz-'), encode, then replace the token with the original character afterwards.

For example, check out the usage of <WPPreserveNewline />:
​https://github.com/WordPress/WordPress/blob/master/wp-includes/formatting.php#L320

[ericlewis]

Cool, thanks for the detail.

So, if you upload a file named Eric%20Lewis.jpg the URL would end up as siteurl.com/wp-content/uploads/2014/05/Eric%2520Lewis.jpg.

Should we be URL-unencoding before naming the file on the server, so if I upload Eric%20Lewis.jpg, the file on the server would be /path/to/uploads/dir/Eric Lewis.jpg and the URL siteurl.com/wp-content/uploads/2014/05/Eric%20Lewis.jpg?

[SergeyBiryukov]

Replying to ericlewis:

Should we be URL-unencoding before naming the file on the server, so if I upload Eric%20Lewis.jpg, the file on the server would be /path/to/uploads/dir/Eric Lewis.jpg and the URL siteurl.com/wp-content/uploads/2014/05/Eric%20Lewis.jpg?

This seems better to me than double-encoding.

[ericlewis]

In attachment:18412.3.diff​, urldecode() filenames and attachment titles when creating them on the server.

[kurtpayne]

Replying to SergeyBiryukov:

Replying to ericlewis:

Should we be URL-unencoding before naming the file on the server, so if I upload Eric%20Lewis.jpg, the file on the server would be /path/to/uploads/dir/Eric Lewis.jpg and the URL siteurl.com/wp-content/uploads/2014/05/Eric%20Lewis.jpg?

This seems better to me than double-encoding.

I can get behind this, too. There is a precedent for normalizing filenames on upload.

[wonderboymusic]

This looks fine, but the bug and fix need to be demonstrated via a unit test.

[simonwheatley]

Replying to wonderboymusic:

This looks fine, but the bug and fix need to be demonstrated via a unit test.

I cannot see how to demonstrate this bug in wp_handle_upload via a unit test (not to say it's not possible). I've created a not-uploaded fake file, and overridden as many tests as possible. Unfortunately the tests fails (not in a good way) on the move_uploaded_file call, which (I assume) detects that it's not a properly uploaded file and then the wp_handle_upload function fails.

Any ideas for how we can get around this?

My test so far:

	/*
	 * @ticket 18412
	 */
	function test_unique_filename_encoded_space_is_sanitized() {
		$tmp_file_name = wp_tempnam();
		file_put_contents( $tmp_file_name, 'xxxxxxxxxxxxxxxx' );
		$overrides = array(
			'test_form'   => false,
			'test_size'   => false,
			'test_upload' => false,
		);
		$fake_file = array(
			'name'     => 'hello%20world.png',
			'type'     => 'image/png',
			'size'     => '16',
			'tmp_name' => $tmp_file_name,
			'error'    => UPLOAD_ERR_OK,
		);
		$results = wp_handle_upload( $fake_file, $overrides );
		// Some assertions to go here, haven't got that far
		unlink( $tmp_file_name );
	}

[mattheu]

I think the underlying issue is the same as #16330 - the bug affects both uploading and sideloading.

The solution I proposed there is that sanitize_file_name should replace %20 with a - as it does with spaces. I think this is a better solution because if %20 in filenames breaks things then it shouldn't be allowed.

My patch on #16330 should fix the issues described in this ticket as well as sideload issues.

[johnbillion]

Closing in favour of #16330 which has tests and a patch.