Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#51733 new defect (bug)

redirect_canonical unneeded redirect

Reported by: aidvu's profile aidvu Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 3.3
Component: Canonical Keywords: needs-patch
Focuses: Cc:

Description

Recently caught an unneeded redirect in redirect_canonical. Started caching responses, and ended in a redirect loop.

tl;dr
Per RFC3986, + is a reserved character, and we should use %20 to encode a space in URLs (percent encoding).
Per 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:

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
// 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
);

Change History (1)

#1 @aidvu
3 years ago

Anyone from core interested in discussing this one?

Note: See TracTickets for help on using tickets.