Opened 19 months ago
Last modified 3 months ago
#58902 new defect (bug)
add_query_arg() should esc_url_raw() REQUEST_URI
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Formatting | Keywords: | has-patch has-unit-tests needs-testing needs-testing-info |
Focuses: | Cc: |
Attachments (3)
Change History (28)
This ticket was mentioned in PR #4910 on WordPress/wordpress-develop by @audrasjb.
19 months ago
#1
- Keywords has-patch added; needs-patch removed
@audrasjb commented on PR #4910:
19 months ago
#2
This will also need unit tests.
This ticket was mentioned in PR #5057 on WordPress/wordpress-develop by @ivanzhuck.
18 months ago
#4
- Keywords has-unit-tests added; needs-unit-tests removed
Updated unit tests
#6
in reply to:
↑ 5
@
18 months ago
Replying to SergeyBiryukov:
Since
esc_url_raw()
is a wrapper forsanitize_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
@
17 months ago
@SergeyBiryukov, I've replaced esc_url_raw()
with sanitize_url()
in the PR
#8
@
17 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
#10
@
16 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
#11
@
16 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
@
16 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.
#13
@
16 months ago
@oglekler
- 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? assinitize_url()
returns not valid value for the line 'baz=1'as it is unacceptable URL. - I added a separate test case to make sure
add_query_arg()
returns sanitized URLs
Please review
#15
@
16 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.
12 months ago
This ticket was mentioned in Slack in #core by oglekler. View the logs.
8 months ago
#19
@
8 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.
8 months ago
This ticket was mentioned in Slack in #core-test by tremidkhar. View the logs.
8 months ago
#22
@
8 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.
4 months ago
#24
@
4 months ago
- Milestone changed from 6.7 to Future Release
Moving this one to a Future Release as it was punted multiple times.
https://core.trac.wordpress.org/ticket/58902