Make WordPress Core

Opened 5 years ago

Closed 7 weeks ago

Last modified 7 weeks 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 5 years ago.
Don't allow % in filenames as it will cause false urlencoding
16226.2.diff (1.5 KB) - added by dd32 5 years ago.
Add plus to special chars.diff (833 bytes) - added by simonwheatley 4 years ago.
Strip + AND % chars from uploaded filenames
16226.patch (713 bytes) - added by mordauk 8 months ago.
16226-tests.patch (1.5 KB) - added by mordauk 8 months ago.

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
5 years ago

Possibly related: #15955

5 years ago

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

#2 @solarissmoke
5 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
5 years ago

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

#4 in reply to: ↑ 3 @solarissmoke
5 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
5 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)

5 years ago

#6 @phogberg
5 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
4 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.

4 years ago

Strip + AND % chars from uploaded filenames

#8 @simonwheatley
4 years ago

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

#9 @dd32
4 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
4 years ago

Related: #16330

#11 @jaddle
4 years ago

  • Cc jonathan.addleman@… added

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

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

#12 @krembo99
3 years ago

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

#13 @SergeyBiryukov
3 years ago

  • Milestone Future Release deleted

#14 @jblifestyles
9 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 9 months ago by jblifestyles (previous) (diff)

8 months ago

#15 @mordauk
8 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
7 months ago

  • Milestone set to Awaiting Review

#17 @jblifestyles
6 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
5 months ago

#32673 was marked as a duplicate.

#19 @ocean90
5 months ago

#19447 was marked as a duplicate.

#20 @wonderboymusic
7 weeks ago

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

#21 @wonderboymusic
7 weeks 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
7 weeks ago

In 35124:

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

See #16226.

Note: See TracTickets for help on using tickets.