WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 weeks ago

#16226 reopened defect (bug)

Attachment URL filenames are not urlencoded

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

Description

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 4 years ago.
Don't allow % in filenames as it will cause false urlencoding
16226.2.diff (1.5 KB) - added by dd32 4 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 5 weeks ago.
16226-tests.patch (1.5 KB) - added by mordauk 5 weeks ago.

Download all attachments as: .zip

Change History (20)

comment:1 @SergeyBiryukov4 years ago

Possibly related: #15955

@solarissmoke4 years ago

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

comment:2 @solarissmoke4 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.

comment:3 follow-up: @dd324 years ago

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

comment:4 in reply to: ↑ 3 @solarissmoke4 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?

comment:5 @dd324 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)

@dd324 years ago

comment:6 @phogberg4 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.

comment:7 @simonwheatley4 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.

@simonwheatley4 years ago

Strip + AND % chars from uploaded filenames

comment:8 @simonwheatley4 years ago

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

comment:9 @dd324 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.

comment:11 @jaddle3 years ago

  • Cc jonathan.addleman@… added
Last edited 3 years ago by jaddle (previous) (diff)

comment:12 @krembo993 years ago

  • Resolution set to duplicate
  • Status changed from new to closed
Version 0, edited 3 years ago by krembo99 (next)

comment:13 @SergeyBiryukov3 years ago

  • Milestone Future Release deleted

comment:14 @jblifestyles6 weeks 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 6 weeks ago by jblifestyles (previous) (diff)

@mordauk5 weeks ago

@mordauk5 weeks ago

comment:15 @mordauk5 weeks 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.

Note: See TracTickets for help on using tickets.