WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 weeks ago

#36998 reviewing defect (bug)

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

Reported by: hlashbrooke Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

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 2 weeks ago.
Encoded whitespaces + unit test.
36998.1.diff (1.3 KB) - added by donmhico 2 weeks ago.
Corrected the diff.

Download all attachments as: .zip

Change History (7)

#1 @ocean90
3 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
3 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 3 years ago by dd32 (previous) (diff)

#3 @hlashbrooke
3 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.

@donmhico
2 weeks ago

Encoded whitespaces + unit test.

@donmhico
2 weeks ago

Corrected the diff.

#4 @donmhico
2 weeks 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
2 weeks ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.