Make WordPress Core

Opened 6 years ago

Closed 16 months ago

Last modified 16 months ago

#16226 closed defect (bug) (fixed)

Attachment URL filenames are not urlencoded

Reported by: mdawaffe Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.1
Component: Upload Keywords: has-patch
Focuses: Cc:


In looking at #16191, I discovered a related bug.

Upload a file called a%22b.jpg. The file will be stored on the filesystem as a%22b.jpg, but the resulting attachment URL will be http://example.com/wp-content/uploads/2011/01/a%22b.jpg, which will point to a file named a"b.jpg.

Attachments (5)

16226.diff (833 bytes) - added by solarissmoke 6 years ago.
Don't allow % in filenames as it will cause false urlencoding
16226.2.diff (1.5 KB) - added by dd32 6 years ago.
Add plus to special chars.diff (833 bytes) - added by simonwheatley 6 years ago.
Strip + AND % chars from uploaded filenames
16226.patch (713 bytes) - added by mordauk 23 months ago.
16226-tests.patch (1.5 KB) - added by mordauk 23 months ago.

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
6 years ago

Possibly related: #15955

6 years ago

Don't allow % in filenames as it will cause false urlencoding

#2 @solarissmoke
6 years ago

  • Keywords has-patch added; needs-patch removed

% should be added to the list of special characters. Browsers will treat it (and what follows) as a urlencoded entity, which means you will never be able to access the saved file.

#3 follow-up: @dd32
6 years ago

shouldn't % be encoded to %25 instead? ie. the filename should be urlencoded.

#4 in reply to: ↑ 3 @solarissmoke
6 years ago

Replying to dd32:

shouldn't % be encoded to %25 instead? ie. the filename should be urlencoded.

The difficulty is urlencoding only the filename portion... we can't urlencode the whole url. And we can't do the encoding in sanitize_file_name() because that is used in other non-URL contexts. Is there some other place where it can be done?

#5 @dd32
6 years ago

See the attached patch, it's a rough patch, but seems to work in the few test cases i've got here. I've not tested it against international characters, nor have i tested it against "store uploads in year/month folders" being disabled (which will probably break this patch)

6 years ago

#6 @phogberg
6 years ago

Similar to this:

Upload a file called a+b.jpg. The file will be stored on the filesystem as a+b.jpg, but the resulting attachment URL will be "http://example.com/wp-content/uploads/2011/04/a+b.jpg", which will point to a file named "a b.jpg" wich does not exist.

The plus sign translates to a space in an urlencoded string. 16226.2.diff will probably solve this as well, but not patch 16226.diff.

#7 @simonwheatley
6 years ago

  • Keywords dev-feedback added
  • Version changed from 3.1 to 3.2

It seems to me that the simplest thing to do for these edge cases (albeit that I've just had a real user complaining about a file with a "+" in the filename not uploading) is to strip "+" and "%" in the sanitise_file_name function. I'm attaching a patch which does just this.

6 years ago

Strip + AND % chars from uploaded filenames

#8 @simonwheatley
6 years ago

Note that this simplistic approach would also work for #16191. :)

#9 @dd32
6 years ago

  • Version changed from 3.2 to 3.1

The Version field is used to track the first version in which a issue is identified in, You can assume that any version between Version and Milestone will be affected by the ticket, so no need to update it to the current branch.

#10 @kawauso
5 years ago

Related: #16330

#11 @jaddle
5 years ago

  • Cc jonathan.addleman@… added

I just noticed another similar bug - I uploaded some files with spaces,

Version 0, edited 5 years ago by jaddle (next)

#12 @krembo99
5 years ago

  • Resolution set to duplicate
  • Status changed from new to closed
Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#13 @SergeyBiryukov
5 years ago

  • Milestone Future Release deleted

#14 @jblifestyles
23 months ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Why hasn't this been implemented?

I'm running into issues importing images that had a % in their URL.. Simply adding it to the $special_chars fixed the problem, but I'm wondering why this hasn't made it into core... does this cause other conflicts?

Last edited 23 months ago by jblifestyles (previous) (diff)

23 months ago

#15 @mordauk
23 months ago

16226.patch is a refreshed patch that applies cleanly.

16226-tests.patch refreshes unit tests so they pass properly.

The only issue I see with this is that encoded spaces and other characters (such as %20) get their % removed but leave the rest. This means that multi %20 +space.png becomes multi-20-space.png.

#16 @SergeyBiryukov
21 months ago

  • Milestone set to Awaiting Review

#17 @jblifestyles
20 months ago

If someone is looking to address this issue temporarily while waiting on the patch to get integrated, and happen to sit on a server, like Flywheel, that doesn't let you edit wp-includes.... You can simply add this to your theme's functions.php file

add_filter ('sanitize_file_name', 'remove_percents_filter', 10 );
function remove_percents_filter( $filename ) {
    $filename = str_replace( '%', '', $filename );
    return $filename;

#18 @ocean90
20 months ago

#32673 was marked as a duplicate.

#19 @ocean90
20 months ago

#19447 was marked as a duplicate.

#20 @wonderboymusic
16 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 4.4

#21 @wonderboymusic
16 months ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from reopened to closed

In 35122:

Fotmatting: in sanitize_file_name(), escape % when uploads contain them, otherwise attachment URLs will unescape the char and break.

Adds unit tests.

Props mordauk, simonwheatley, dd32, solarissmoke.
Fixes #16226.

#22 @wonderboymusic
16 months ago

In 35124:

After [35122], update test_wp_unique_filename() to reflect the change.

See #16226.

Note: See TracTickets for help on using tickets.