Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#36998 closed defect (bug) (fixed)

wp_sanitize_redirect() strips spaces out of URLs instead of encoding them

Reported by: hlashbrooke's profile hlashbrooke Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch has-unit-tests
Focuses: Cc:


This is similar to #17052, #10690 and one or two others that relate to non-ASCII characters being stripped from URLs, however I couldn't find a ticket that relates directly to spaces being stripped out in the wp_sanitize_redirect() function. This function is called when using wp_redirect() or wp_safe_redirect() and, when it is called, it strips out the spaces from URLs instead of encoding them as %20 (which I would think would be the correct way of doing things). This results in unexpected behaviour and broken redirects when passing a URL with spaces to wp_redirect().

The cause for this will likely be the same as what was fixed in #23605 for the esc_url() function, but this is in a separate location so I'm not 100% if the same fix will apply.

The fix for esc_url() was to simply add $url = str_replace( ' ', '%20', $url );, which works just fine, but the case in wp_sanitize_redirect() may be a bit different (although I don't see why it would be).

Attachments (2)

36998.diff (1.3 KB) - added by donmhico 5 years ago.
Encoded whitespaces + unit test.
36998.1.diff (1.3 KB) - added by donmhico 5 years ago.
Corrected the diff.

Download all attachments as: .zip

Change History (10)

#1 @ocean90
8 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
  • Version trunk deleted

I noticed the same recently.

#2 @dd32
8 years ago

Technically speaking, spaces are invalid in a URL and stripping them out is correct - you should pass a well-formed URL into the redirect (so spaces should already be encoded). Just the same as you don't (shouldn't) pass (unencoded) multibyte characters in a URL to it.

Realistically though, encoding spaces, multibyte characters, and other url components as long as it doesn't affect the security of the url being checked, is a sane thing to do. And it appears that we already urlencode multibyte characters.

Also, technically speaking, In the linked example that @ocean90 gave, the initial URL format is invalid, as it's not properly URL encoded (the entire redirect_url parameter should be encoded, so %20 would've became %2520 and the change wouldn't have been needed in the first place).

Last edited 8 years ago by dd32 (previous) (diff)

#3 @hlashbrooke
8 years ago

@dd32 I agree with you there. Spaces should be formatted correctly before being passed to wp_redirect(), but like you said - the sane thing to do would be to double check that before handling the redirect. I think patching this would be a useful improvement, if only to prevent unexpected situations when handling redirects.

5 years ago

Encoded whitespaces + unit test.

5 years ago

Corrected the diff.

#4 @donmhico
5 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

Please ignore my first patch, it was weird. The latest patch was the correct one 36998.1.diff.

Basically, I just replace the whitespace with %20 at the top before the $location is passed to the other functions.

$location = str_replace( ' ', '%20', $location );

I don't see any problem with this solution, and as @dd32 mentioned on comment 2, encoding such components is a sane thing to do as long as there's no security risk. And I think that encoding whitespaces to %20 is better than just stripping it out. Typing a url with whitespaces in the browser will convert the whitespaces to %20 as well, so I think there's no issue simulating the same behaviour in wp_sanitize_redirect().

#5 @SergeyBiryukov
5 years ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @azaozz
5 years ago

  • Keywords has-unit-tests added

36998.1.diff looks good. As @dd32 mentions above I'm more concerned that it means the URL passed to the redirect is not well-formed. As the function already url-encodes multibyte characters, seems it should encode spaces as well.

This ticket was mentioned in Slack in #core by marybaum. View the logs.

5 years ago

#8 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46462:

Formatting: Make sure wp_sanitize_redirect() encodes spaces in URLs instead of stripping them out.

Props donmhico, hlashbrooke, dd32, azaozz.
Fixes #36998.

Note: See TracTickets for help on using tickets.