WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 22 months ago

Last modified 21 months ago

#16226 closed defect (bug) (duplicate)

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 (3)

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

Download all attachments as: .zip

Change History (16)

comment:1 SergeyBiryukov3 years ago

Possibly related: #15955

solarissmoke3 years ago

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

comment:2 solarissmoke3 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: dd323 years ago

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

comment:4 in reply to: ↑ 3 solarissmoke3 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 dd323 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)

dd323 years ago

comment:6 phogberg3 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 simonwheatley3 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.

simonwheatley3 years ago

Strip + AND % chars from uploaded filenames

comment:8 simonwheatley3 years ago

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

comment:9 dd323 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:10 kawauso2 years ago

Related: #16330

comment:11 jaddle2 years ago

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

comment:12 krembo9922 months ago

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

comment:13 SergeyBiryukov21 months ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.