Make WordPress Core

Opened 22 months ago

Last modified 11 days ago

#58902 new defect (bug)

add_query_arg() should esc_url_raw() REQUEST_URI

Reported by: jorbin's profile jorbin Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch has-unit-tests dev-feedback has-test-info
Focuses: Cc:

Description

add_query_arg assumes that the query argument is an acceptable query argument. In order to help developers from accidently making a URL an unacceptable URL.

Some related tickets: #16859, #22951, and #22300.

The security team has reviewed this and ok'd it being worked on in public.

Attachments (3)

58902.diff (1.1 KB) - added by ivanzhuck 21 months ago.
58902.2.diff (1.1 KB) - added by ivanzhuck 21 months ago.
58902.3.diff (3.2 KB) - added by ivanzhuck 20 months ago.

Download all attachments as: .zip

Change History (30)

This ticket was mentioned in PR #4910 on WordPress/wordpress-develop by @audrasjb.


22 months ago
#1

  • Keywords has-patch added; needs-patch removed

@audrasjb commented on PR #4910:


22 months ago
#2

This will also need unit tests.

#3 @wpsharif
22 months ago

  • Keywords has-testing-info added

I've checked this and I did error log the URL with this changes. It's working fine.
https://ibb.co/BPtvksH

My configuration:
-Windows 11
-Apache Version: 2.4.54.2
-PHP Version: 8.1.13
-MySQL Version: 8.0.31

This ticket was mentioned in PR #5057 on WordPress/wordpress-develop by @ivanzhuck.


21 months ago
#4

  • Keywords has-unit-tests added; needs-unit-tests removed

Updated unit tests

@ivanzhuck
21 months ago

#5 follow-ups: @SergeyBiryukov
21 months ago

Since esc_url_raw() is a wrapper for sanitize_url(), could we use the latter directly here?

All of the other instances in core were replaced in [53455] / #55852, except for two that accidentally snuck in later.

#6 in reply to: ↑ 5 @SergeyBiryukov
21 months ago

Replying to SergeyBiryukov:

Since esc_url_raw() is a wrapper for sanitize_url(), could we use the latter directly here?

All of the other instances in core were replaced in [53455] / #55852, except for two that accidentally snuck in later.

Created a ticket for those: #59247.

#7 in reply to: ↑ 5 @ivanzhuck
21 months ago

@SergeyBiryukov, I've replaced esc_url_raw() with sanitize_url() in the PR

@ivanzhuck
21 months ago

#8 @oglekler
20 months ago

  • Keywords changes-requested added

I think that the problem in question is not covered by Unit tests and change in the last patch is changing test that according to comment is dedicated to test another issue: #4903

#9 @oglekler
20 months ago

@jorbin can you provide an example URL for Unit test? Thank you!

#10 @ivanzhuck
20 months ago

@oglekler

The string baz=1 is not a valid relative URL. If we send it as a parameter for the function
esc_url_raw() it returns http://baz=1 that is also not valid URL. We can't use unacceptable URL as a positive test case, because the ticket is about preventing that. So we should add ? before baz=1 to make the URL correct. Do you agree with me?

P.S. The change doesn't affect the issue from #4903

Last edited 20 months ago by ivanzhuck (previous) (diff)

#11 @oglekler
20 months ago

@ivanzhuck you are changing the test line that was added for another issue 11 years ago:
https://core.trac.wordpress.org/changeset/1192/tests
I believe that it should be left like it is.

This is the tests dedicated to URLs:
https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/formatting/escUrl.php

To make sure that add_query_arg() will always return a sanitized URL, I assume it needs a separate test with URLs where there is something to sanitize.

#12 @ivanzhuck
20 months ago

@oglekler

well... I think we can't leave the old test line as is, because the test fails with it after applying the patch from @audrasjb. We should change one of both: the test or the code)

root@83c80e784dc4:/var/www# ./vendor/bin/phpunit --group add_query_arg
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.13 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

F......                                                             7 / 7 (100%)

Time: 00:00.382, Memory: 171.50 MB

There was 1 failure:

1) Tests_Functions::test_add_query_arg
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'baz=1&foo=1'
+'http://baz=1?foo=1'

/var/www/tests/phpunit/tests/functions.php:539
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:106

FAILURES!
Tests: 7, Assertions: 91, Failures: 1.
Last edited 20 months ago by ivanzhuck (previous) (diff)

#13 @ivanzhuck
20 months ago

@oglekler

  1. I moved the checkup for the issue #4903 to the end of the test function. Now it runs only if URL passed as a parameter to add_query_arg(). And doesn't run for cases when URL was taken from $_SERVERREQUEST_URI? as sinitize_url()returns not valid value for the line 'baz=1'as it is unacceptable URL.
  2. I added a separate test case to make sure add_query_arg() returns sanitized URLs

Please review

@ivanzhuck
20 months ago

#14 @oglekler
20 months ago

@SergeyBiryukov please take a look, possibly we overcomplicate things :)

#15 @oglekler
20 months ago

  • Milestone changed from 6.4 to 6.5

It looks like right now we are running out of time before RC1, so, I am moving this ticket to the next milestone.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


15 months ago

#17 @swissspidy
15 months ago

  • Milestone changed from 6.5 to 6.6

This ticket was mentioned in Slack in #core by oglekler. View the logs.


12 months ago

#19 @oglekler
12 months ago

  • Keywords needs-testing needs-testing-info added; has-testing-info changes-requested removed

It is possible to test the patch somehow? 🤔 Possibly the patch needs refresh.

I hope we can make it in this milestone and will not be drugging it again 😅

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-test by tremidkhar. View the logs.


11 months ago

#22 @oglekler
11 months ago

  • Milestone changed from 6.6 to 6.7

We have RC1 today, and it looks like we will not make it in this milestone, so I am moving this ticket to the next one.

Most likely, the patch needs refresh and possibly testing instructions.

Hypothetically, Can unescape or escaped $_SERVERREQUEST_URI? in this place break something? 🤔

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


8 months ago

#24 @stoyangeorgiev
8 months ago

  • Milestone changed from 6.7 to Future Release

Moving this one to a Future Release as it was punted multiple times.

This ticket was mentioned in Slack in #core-test by sppramodh. View the logs.


7 months ago

#26 @wordpressdotorg
12 days ago

  • Keywords needs-test-info added; needs-testing-info removed

#27 @SirLouen
11 days ago

  • Keywords dev-feedback has-test-info added; needs-testing needs-test-info removed

Reproduction and Patch Test Report

Description

🟠 This report validates that the issue can be reproduced and the patch is working as expected with some warnings included in the notes.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.4.6
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.4.6)
  • Browser: Chrome 136.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty 2.9
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Reproduction Steps

  1. Add the coded provided in Supplemental Artifacts to whatever method you prefer to run code in WP
  2. Create a post and add the shortcode [url_query_demo_shortcode]
  3. 🐞 Resulting URLs are wrong

Expected Results (according to the report)

  • Using add_query_arg the resulting URL should be sanitized, hence right despite being poorly made by the developer.

Actual Results with the Patch

  • ✅ The resulting URL are correctly formatted.

Additional Notes

  • In this case, unit tests are pretty straightforward and testing could be done with them, but still, I have added a very simple tests to showcase this patch in case anyone wants to see visually where is the problem with this.
  • 🟠 I'm finding that not all proposed test cases are useful (they are still passing with or without URL sanitization, so they are basically useless). I think some of them can be trimmed, although none will cause any harm being left there.
  • ⚠️ I'm not 100% confident if it's the right solution that the URL is sanitized after adding a add_query_arg, or if it should return some sort of notice, for a poorly formatted URL.

Supplemental Artifacts

Here I provide the code to create a shortcode and test this issue:

function url_query_demo_shortcode() {
        $output  = '<div>';
        $output .= '<h2>URL Query String Demo</h2>';

        $urls_with_query_string = array(
                'http://example.com/two words?foo=1' => 'http://example.com/two%20words?foo=1&bar=2',
                'http;//example.com?foo=1'                       => 'http://example.com?foo=1&bar=2',
                'example.com?foo=1'                              => 'http://example.com?foo=1&bar=2',
        );

        $output .= '<table>';
        $output .= '<tr><th>Original URL</th><th>Expected</th><th>Result w/ QV</th></tr>';

        foreach ( $urls_with_query_string as $wrong_url => $expected ) {
                $_SERVER['REQUEST_URI'] = $wrong_url;
                $with_bar               = add_query_arg( array( 'bar' => '2' ) );
                $result = ( $with_bar !== $expected ) ? '❌' : '✅';
                $output .= '<tr>';
                $output .= '<td>' . $wrong_url . '</td>';
                $output .= '<td>' . $expected . '</td>';
                $output .= '<td>' . $with_bar . '</td>';
                $output .= '<td>' . $result . '</td>';
                $output .= '</tr>';
        }

        $output .= '</table>';

        $output .= '</div>';

        return $output;
}
add_shortcode( 'url_query_demo', 'url_query_demo_shortcode' );
Last edited 11 days ago by SirLouen (previous) (diff)
Note: See TracTickets for help on using tickets.