WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 13 months ago

#18412 closed defect (bug) (duplicate)

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

Reported by: ripperdoc Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.1
Component: Media Keywords: has-patch needs-unit-tests dev-feedback
Focuses: Cc:

Description

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.

Attachments (3)

18412.diff (1.5 KB) - added by kurtpayne 4 years ago.
Patch to fix %XX in media filenames
18412.2.diff (1.5 KB) - added by c3mdigital 2 years ago.
Refresh of existing patch
18412.3.diff (1.2 KB) - added by ericlewis 15 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 @ripperdoc4 years ago

  • Cc ripperdoc added

comment:3 @kawauso4 years ago

  • Keywords needs-patch added

comment:4 @ramoonus4 years ago

  • Cc ramoonus@… added
  • Severity changed from minor to normal

not a duplicate then?

@kurtpayne4 years ago

Patch to fix %XX in media filenames

comment:5 @kurtpayne4 years ago

  • Cc kurtpayne added
  • Keywords has-patch added

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

comment:6 @SergeyBiryukov4 years ago

  • Keywords needs-patch removed

comment:7 @c3mdigital2 years ago

  • Keywords needs-refresh added

@c3mdigital2 years ago

Refresh of existing patch

comment:8 @c3mdigital2 years ago

  • Keywords needs-refresh removed
  • Version changed from 3.2.1 to 2.1

comment:9 @SergeyBiryukov22 months ago

#25463 was marked as a duplicate.

comment:10 follow-up: @ericlewis15 months ago

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

comment:11 in reply to: ↑ 10 @kurtpayne15 months ago

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

comment:12 follow-up: @ericlewis15 months ago

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?

comment:13 in reply to: ↑ 12 ; follow-up: @SergeyBiryukov15 months ago

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.

@ericlewis15 months ago

comment:14 @ericlewis15 months ago

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

comment:15 in reply to: ↑ 13 @kurtpayne15 months ago

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.

comment:16 @SergeyBiryukov14 months ago

  • Milestone changed from Awaiting Review to 4.0

comment:17 follow-up: @wonderboymusic14 months ago

  • Keywords needs-unit-tests added

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

comment:18 in reply to: ↑ 17 @simonwheatley14 months ago

  • Keywords dev-feedback added

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

comment:19 @mattheu13 months ago

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.

comment:20 @johnbillion13 months ago

  • Milestone 4.0 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.