Make WordPress Core

Opened 19 months ago

Last modified 17 months ago

#55274 reviewing defect (bug)

Special chars in attachment filename breaks srcset

Reported by: dravnic's profile dravnic Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords:
Focuses: Cc:


There is currently fix that handles spaces in filenames, but it doesn't handle other special characters e.g. $! or any other special char

Spaces issue was reported there:

And fixed here:

Attachments (5)

55274.diff (1.0 KB) - added by dravnic 19 months ago.
55274.2.diff (2.2 KB) - added by dravnic 19 months ago.
55274.3.diff (1.0 KB) - added by dravnic 19 months ago.
55274.2.2.diff (2.2 KB) - added by dravnic 19 months ago.
55274.2.3.diff (2.3 KB) - added by dravnic 19 months ago.

Download all attachments as: .zip

Change History (13)

19 months ago

19 months ago

#1 @audrasjb
19 months ago

  • Keywords has-patch has-unit-tests added
  • Milestone changed from Awaiting Review to 6.0
  • Owner set to audrasjb
  • Status changed from new to reviewing

Hello @dravnic, Welcome to WordPress Core Trac and thank you for opening this ticket,

The proposal looks good at a glance. Also, thank you for updating unit tests.
Moving for 6.0 consideration.

#2 @dravnic
19 months ago

  • Keywords has-patch has-unit-tests removed

Thanks @audrasjb, glad to contribute!

19 months ago

19 months ago

#3 @dravnic
19 months ago

Please consider patches 55274.3.diff and 55274.2.2.diff (don't know how to remove 55274.diff and 55274.2.diff)

rawurlencode is better to encode path names than urlencode. e.g. spaces will be converted to "%20" instead "+". As well updated unit test as there was a typo.

19 months ago

#4 @dravnic
19 months ago

Fixed another typo in the unit test. Sorry for multiple patches :)

Final patches are:

#5 @dravnic
19 months ago

I was reviewing this discussion: #36549

The patches proposed here are fixing url special chars before the filter. This could break filter behavior if filter function expects urls that aren't encoded or if the filter itself changes urls and introduces special chars.

The fix proposed by @joemcgill ( seems better as it would escape url after the filter.

Note that esc_attr() that was mentioned in the above discussion didn't properly encode special chars for urls. so esc_url() should be done before.

Last edited 19 months ago by SergeyBiryukov (previous) (diff)

#6 @dravnic
19 months ago

I've tried using esc_url() like in but it didn't help.

The image filename that I used to test this is smeće-1024x614.png the special char in this filename is ć - a Croatian letter. In the src attribute path is properly encoded sme%C4%87e-1024x614.png

With current code and with esc_url() the image was encoded in srcset as smeće-1024x614.png which is an invalid url.

When patched with it was encoded as sme%C4%87e-1024x614.png which is ok.

So rawurlencode() seems a better option, but it cannot be used after filter because then it would encode entire url not only filename. Maybe to extract filename from the full path and then encode it and append back on its path. Although directory name can as well contain special chars. To cover these cases it work require a bit more logic.

It would be great if we could encode before the filter and then leave it to filter to handle its urls properly.

Last edited 19 months ago by dravnic (previous) (diff)

#7 @costdev
18 months ago

  • Version set to 4.4

#8 @peterwilsoncc
17 months ago

  • Milestone changed from 6.0 to Future Release

It seems the various patches on this ticket need consolidation and a little more work so I am moving this of the 6.0 milestone.

Ideally, I think the tests would be separate and in addition to the spaces test. It would also be good to test some accents and non-latin characters as indicated in comment 6.

Note: See TracTickets for help on using tickets.