Bug on canonical redirect with Hebrew query string.

[yehudah]

Hello,

Example URL:
​http://domain.com/?שלום

The example comes after having problems with Woocommerce product filters.

Line 491-493 in /wp-includes/canonical.php we have this:

<?php
if ( ! $redirect_url || $redirect_url == $requested_url ) {
        return;
}

The problem:

This will fail because the $redirect_url is not URL safe format and encoded like $requested_url.

Possible solution:

One line before we can use wp_sanitize_redirect function like this:

<?php
$redirect_url = wp_sanitize_redirect( $redirect_url );
if ( ! $redirect_url || $redirect_url == $requested_url ) {
        return;
}

[SergeyBiryukov]

#44769 was marked as a duplicate.

[andizer]

Wouldn't it be better to use rawurldecode instead of urldecode?

[audrasjb]

Hello, and thanks for the ticket and patches,

This ticket still needs some unit-tests.

Since we are looking to retrieve "normal" minor release workflow, let's address this ticket in milestone 5.1.

[pento]

#45315 was marked as a duplicate.

[atanasangelovdev]

I wanted to write a unit test for this but I failed to reproduce the issue. Here are the URLs I tested with and none of them produce a redirect ($redirect_url is false):

http://example.org/?שלום
http://example.org/?%D7%A9%D7%9C%D7%95%D7%9D
http://example.org/ticket-44744/?שלום
http://example.org/ticket-44744/?%D7%A9%D7%9C%D7%95%D7%9D
http://example.org/ticket-44744/?foo=שלום
http://example.org/ticket-44744/?foo=%D7%A9%D7%9C%D7%95%D7%9D

Here's the test class I used:

class Tests_Canonical_Loop extends WP_Canonical_UnitTestCase {
        /**
         * @dataProvider data
         */
        function test( $test_url, $expected, $ticket = 0, $expected_doing_it_wrong = array() ) {
                $this->assertCanonical( $test_url, $expected, $ticket, $expected_doing_it_wrong );
        }

        function data() {
                $post_slug = 'ticket-44744';
                $post_id   = self::factory()->post->create(
                        array(
                                'post_type' => 'page',
                                'post_status' => 'publish',
                                'post_title'  => $post_slug,
                                'post_name'   => $post_slug,
                        )
                );

                /* Format:
                 * [0]: $test_url,
                 * [1]: expected results: Any of the following can be used
                 *      array( 'url': expected redirection location, 'qv': expected query vars to be set via the rewrite AND $_GET );
                 *      array( expected query vars to be set, same as 'qv' above )
                 *      (string) expected redirect location
                 * [3]: (optional) The ticket the test refers to, Can be skipped if unknown.
                 */
                return array(
                        array( "/?שלום", "/?שלום", 44744 ),
                        array( "/?%D7%A9%D7%9C%D7%95%D7%9D", "/?%D7%A9%D7%9C%D7%95%D7%9D", 44744 ),
                        array( "/{$post_slug}/?שלום", "/{$post_slug}/?שלום", 44744 ),
                        array( "/{$post_slug}/?%D7%A9%D7%9C%D7%95%D7%9D", "/{$post_slug}/?%D7%A9%D7%9C%D7%95%D7%9D", 44744 ),
                        array( "/{$post_slug}/?foo=שלום", "/{$post_slug}/?foo=שלום", 44744 ),
                        array( "/{$post_slug}/?foo=%D7%A9%D7%9C%D7%95%D7%9D", "/{$post_slug}/?foo=%D7%A9%D7%9C%D7%95%D7%9D", 44744 ),
                );
        }
}

Maybe I've misunderstood the issue?