WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#36998 new defect (bug)

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

Reported by: hlashbrooke Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: needs-patch needs-unit-tests
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).

Change History (3)

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

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

Note: See TracTickets for help on using tickets.