WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 6 weeks ago

#50855 accepted defect (bug)

sanitize_file_name function not working as expected if there is '%20' in filename

Reported by: dishitpala Owned by: audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.4.2
Component: Formatting Keywords: has-patch has-screenshots needs-unit-tests
Focuses: Cc:

Description

We have added '%' as a special character in $special_char variable
We are also replacing '%20' with '-'

Here the sequence of str_replace was not appropriate

We need to replace '%20' with '-' before all the special character are replaced

Current behavior:

  • Filename Before: this%20is%20example.png
  • Filename after sanitization: this20is20example.png

Expected behavior:

  • Filename Before: this%20is%20example.png
  • Filename after sanitization: this-is-example.png

File reference: https://github.com/WordPress/WordPress/blob/master/wp-includes/formatting.php

Function name: sanitize_file_name

Attachments (5)

sanitize-file-name.patch (841 bytes) - added by dishitpala 12 months ago.
Capture d’écran 2021-04-26 à 21.17.56.png (296.2 KB) - added by audrasjb 3 months ago.
Before 50855.diff
Capture d’écran 2021-04-26 à 21.18.08.png (323.0 KB) - added by audrasjb 3 months ago.
After 50855.diff
50855.diff (646 bytes) - added by audrasjb 3 months ago.
Formatting: Reprioritization of the characters to remove from filenames in sanitize_file_name()
50855.2.diff (1.6 KB) - added by boblinthorst 6 weeks ago.

Download all attachments as: .zip

Change History (12)

#1 @audrasjb
3 months ago

  • Keywords needs-refresh added

It would be better to keep this line after the sanitize_file_name_chars hook.

@audrasjb
3 months ago

Formatting: Reprioritization of the characters to remove from filenames in sanitize_file_name()

#2 @audrasjb
3 months ago

  • Keywords has-screenshots added; needs-refresh removed
  • Milestone changed from Awaiting Review to 5.8
  • Owner set to audrasjb
  • Status changed from new to accepted

Moving for 5.8 consideration.
@SergeyBiryukov since you committed the last few changes we introduced in WP 5.5, any thoughts on this change?

#3 @audrasjb
7 weeks ago

Just re-tested my patch and it still applies cleanly against trunk. Marking this for commit.

#4 @audrasjb
7 weeks ago

  • Keywords commit added

#5 @desrosj
7 weeks ago

  • Keywords needs-unit-tests added; commit removed

I'd love to see a unit test or two added here to demonstrate the incorrect behavior before, and confirm the correct behavior after, and to prevent this from happening again going forward.

#6 @desrosj
7 weeks ago

  • Milestone changed from 5.8 to 5.9

I'm going to punt this one as 5.8 beta 1 is being packaged within the hour. I think with proper unit tests, this could potentially be considered for a 5.8.x minor.

@boblinthorst
6 weeks ago

#7 @boblinthorst
6 weeks ago

Fixed the existing unit tests. Worked on this with @rolfsiebers and @sresok.

Note: See TracTickets for help on using tickets.