Make WordPress Core

Opened 6 years ago

Last modified 4 years ago

#44744 reviewing defect (bug)

Bug on canonical redirect with Hebrew query string.

Reported by: yehudah's profile yehudah Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: major Version: 4.9.8
Component: Canonical Keywords: needs-unit-tests has-patch
Focuses: Cc:

Description

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;
}

Attachments (1)

44744.diff (430 bytes) - added by rellect 6 years ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
6 years ago

#44769 was marked as a duplicate.

#2 @SergeyBiryukov
6 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

@rellect
6 years ago

#3 @rellect
6 years ago

  • Keywords needs-patch removed

#4 @SergeyBiryukov
5 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 4.9.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @pento
5 years ago

  • Milestone changed from 4.9.9 to 5.0.1

#6 @andizer
5 years ago

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

#7 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#8 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#9 @audrasjb
5 years ago

  • Milestone changed from 5.0.3 to 5.1

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.

#10 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

#11 @pento
5 years ago

#45315 was marked as a duplicate.

#12 @atanasangelovdev
4 years ago

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?

Note: See TracTickets for help on using tickets.