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

[hlashbrooke]

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).

[ocean90]

I noticed the same ​recently.

[dd32]

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).

[hlashbrooke]

@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]

Encoded whitespaces + unit test.

[donmhico]

Corrected the diff.

[donmhico]

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

[azaozz]

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.

[SergeyBiryukov]

In 46462:

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

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