[wp-trac] [WordPress Trac] #51733: redirect_canonical unneeded redirect

[WordPress Trac]

#51733: redirect_canonical unneeded redirect
--------------------------+-----------------------------
 Reporter:  aidvu         |      Owner:  (none)
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Canonical     |    Version:  3.3
 Severity:  minor         |   Keywords:  needs-patch
  Focuses:                |
--------------------------+-----------------------------
 Recently caught an unneeded redirect in `redirect_canonical`. Started
 caching responses, and ended in a redirect loop.

 tl;dr
 Per [https://tools.ietf.org/html/rfc3986#section-2.2 RFC3986], `+` is a
 reserved character, and we should use `%20` to encode a space in URLs
 (percent encoding).
 Per [https://tools.ietf.org/html/rfc1866#section-8.2.2 RFC1866], encode
 query args as `application/x-www-form-urlencoded` in which case a space is
 encoded as a `+`.

 Posting here to discuss whether we want to fix this in core, or simply
 leave it as is.

 To reproduce:
 - Use any `twenty{year}` theme
 - Customize homepage
 - Select `A static page`
 - Visit homepage with query args, e.g.
 https://vubuesinessplantest.help/?test=a+b+c
 - A redirect occurs to https://vubuesinessplantest.help/?test=a%20b%20c

 Considering we're using `parse_str()` to parse query variables, and it
 correctly parses both RFCs I'd argue we shouldn't redirect in the case
 when query args are the same.

 Here's a filter I've written to skip redirects in this case:
 {{{#!php
 <?php
 // Don't redirect if we're only re-encoding query params
 add_filter(
         'redirect_canonical',
         function ( $redirect_url, $requested_url ) {
                 $parsed_redirect = parse_url( $redirect_url );
                 $parsed_requested = parse_url( $requested_url );
                 // Scheme changed, do redirect
                 if ( $parsed_requested['scheme'] !==
 $parsed_redirect['scheme'] ) {
                         return $redirect_url;
                 }
                 // Host changed, do redirect
                 if ( $parsed_requested['host'] !==
 $parsed_redirect['host'] ) {
                         return $redirect_url;
                 }
                 // Path changed, do redirect
                 if ( $parsed_requested['path'] !==
 $parsed_redirect['path'] ) {
                         return $redirect_url;
                 }
                 // Parse query args
                 parse_str( $parsed_redirect['query'], $query_redirect );
                 parse_str( $parsed_requested['query'], $query_request );
                 // Sort by keys, if the order changed
                 ksort( $query_redirect );
                 ksort( $query_request );
                 // If parsed query args are the same, skip redirect
                 if ( $query_redirect === $query_request ) {
                         return false;
                 }
                 // Otherwise, do redirect
                 return $redirect_url;
         },
         10,
         2
 );
 }}}

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/51733>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform