Make WordPress Core

Opened 8 years ago

Closed 18 months ago

Last modified 18 months ago

#36827 closed defect (bug) (fixed)

Regular expression in wp_guess_url() is slightly too permissive.

Reported by: cfinke's profile cfinke Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.4
Component: General Keywords: has-patch good-first-bug has-unit-tests
Focuses: Cc:

Description

In wp_guess_url(), there is a regular expression is supposed to replace wp-admin/* or wp-login.php in a URL:

$path = preg_replace( '#/(wp-admin/.*|wp-login.php)#i', '', $_SERVER['REQUEST_URI'] );

However, the dot in wp-login.php is not escaped, so the replacement will also run when any character is in that position, not just a period. The enclosing if () statement makes this exceedingly unlikely to happen, but the dot should be escaped regardless.

Attachments (4)

wp_guess_url_regex.patch (672 bytes) - added by cfinke 8 years ago.
Escapes the dot in the regex.
36827_guess_url_patch_and_test (1.5 KB) - added by curdin 8 years ago.
Regex pattern updated to correct wp-login dot issue and capture urls with query parameters. Added common urls identified earlier as unit tests. This is my first core patch, please let me know if I don't follow some convention ;)
36827.patch (1.5 KB) - added by curdin 8 years ago.
Patch and unit tests ( with .patch extension - thanks @netweb )
36827.2.diff (2.0 KB) - added by costdev 19 months ago.
This refreshes 36827.patch with PHPCS fixes, test standards updates, and a new file for the tests: tests/phpunit/tests/functions/wpGuessUrl.php.

Download all attachments as: .zip

Change History (27)

@cfinke
8 years ago

Escapes the dot in the regex.

#1 @voldemortensen
8 years ago

  • Milestone changed from Awaiting Review to 4.6

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


8 years ago

#3 @ocean90
8 years ago

  • Keywords has-patch commit added
  • Version changed from trunk to 3.4

Introduced in [19923], changed in [21664], [21797], [25396].

#4 @swissspidy
8 years ago

Are there already unit tests for that function? They could catch such oversights.

#5 follow-up: @jrf
8 years ago

Looking at the regex, I can see two more variants which would not be caught:

http://mysite.com/wp-admin
http://mysite.com/wp-login.php?var=somevar

I've seen that last one sometimes when plugins try to secure a url.

Changing the regex to the below will fix that and catch both:
'#/(wp-admin(?:$|/.*)|wp-login\.php.*)#i'

#6 @ocean90
8 years ago

  • Keywords needs-unit-tests added; commit removed

#7 in reply to: ↑ 5 @voldemortensen
8 years ago

Replying to jrf:

Looking at the regex, I can see two more variants which would not be caught:

http://mysite.com/wp-admin
http://mysite.com/wp-login.php?var=somevar

I've seen that last one sometimes when plugins try to secure a url.

Changing the regex to the below will fix that and catch both:
'#/(wp-admin(?:$|/.*)|wp-login\.php.*)#i'

In the case of http://mysite.com/wp-login.php?var=somevar it will still remove wp-login.php and return http://mysite.com?var=somevar. We may need to keep that behavior for back-compat (although I'm not aware of anything that depends on this).

#8 @jdgrimes
8 years ago

If you try to access a site that hasn't been installed yet using http://example.com/wp-login.php?redirect_to=/somewhere, you will be shown the error page saying that the site is not installed yet. But the "Create wp-config.php" button won't work, because wp_guess_url() doesn't strip the query string from the URL. The link URL will be wrong, because wp-load.php just does this:

        $path = wp_guess_url() . '/wp-admin/setup-config.php';

Resulting in http://example.com/?redirect_to=/somewhere/wp-admin/setup-config.php

#9 @ocean90
8 years ago

  • Keywords good-first-bug added
  • Milestone changed from 4.6 to Future Release

@curdin
8 years ago

Regex pattern updated to correct wp-login dot issue and capture urls with query parameters. Added common urls identified earlier as unit tests. This is my first core patch, please let me know if I don't follow some convention ;)

#10 follow-up: @curdin
8 years ago

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

#11 in reply to: ↑ 10 @netweb
8 years ago

  • Owner set to curdin
  • Status changed from new to assigned

Assigning the ticket to mark the good-first-bug as "claimed".

Replying to curdin:

This is my first core patch, please let me know if I don't follow some convention ;)

@curdin Thanks for the patch :)

In the future you should add a file extension to the filename, e.g. 36827.diff, this makes the patch more easily read here in Trac, e.g. you see the red and green diff highlights when viewing wp_guess_url_regex.patch but not 36827_guess_url_patch_and_test ;)

See https://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/

#12 follow-up: @curdin
8 years ago

Thanks for the headsup @netweb. I'll reupload with the correct file extension.

A couple of other questions - is it ok to create patches that also contain the test?
And what's the expected change to status so it may progress? I added the has-unit-tests tag (it already had has-patch) - is this useful and expected?

@curdin
8 years ago

Patch and unit tests ( with .patch extension - thanks @netweb )

#13 in reply to: ↑ 12 @netweb
8 years ago

Replying to curdin:

A couple of other questions - is it ok to create patches that also contain the test?

Yes

And what's the expected change to status so it may progress? I added the has-unit-tests tag (it already had has-patch) - is this useful and expected?

Nothing else is needed for now, someone will review the patch and add further comments if needed and update the status as needed

Last edited 8 years ago by netweb (previous) (diff)

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


21 months ago

#15 @SergeyBiryukov
21 months ago

  • Milestone set to 6.1

#16 @petitphp
21 months ago

I took a look at the latest patch and I didn't see any issues, tests ran successfully.

I also checked the edge case mentioned in comment:8 and the patch did fix it.

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

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


20 months ago

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


20 months ago

#19 @davidbaumwald
20 months ago

  • Owner changed from curdin to davidbaumwald
  • Status changed from assigned to reviewing

#20 @SergeyBiryukov
20 months ago

Thanks for the patch! 36827.patch looks OK to me in general, though it could use some cleanup:

  • Changing camelCase to snake_case and resolving WPCS issues.
  • Moving the test to a better place, as tests/includes/helpers.php is only meant for some helper functions from the test suite itself.

As we don't have a dedicated test file for wp_guess_url() yet, and it's located in wp-includes/functions.php, the test should probably be somewhere in tests/functions.php, perhaps after the validate_file() test. Or we could create a new file and add some more tests :)

Testing the patch with all the cases mentioned above would be very welcome! Are they all included in the unit test?

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


19 months ago

@costdev
19 months ago

This refreshes 36827.patch with PHPCS fixes, test standards updates, and a new file for the tests: tests/phpunit/tests/functions/wpGuessUrl.php.

#22 @davidbaumwald
18 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54146:

General: Correct path replacement regex in wp_guess_url.

In wp_guess_url, the regex to check for wp-login.php in the URL is slightly too permissive, not escaping . in "wp-login.php". . is a token in regex that matches any character.

This change simply escapes the . and adds unit test coverage for wp_guess_url.

Props cfinke, ocean90, jrf, voldemortensen, jdgrimes, curdin, netweb, petitphp, SergeyBiryukov, costdev.
Fixes #36827.

#23 @SergeyBiryukov
18 months ago

In 54148:

Tests: Rename the test for wp_guess_url() to match the function name.

Includes adding public visibilty keyword for the data provider.

Follow-up to [54146].

See #36827.

Note: See TracTickets for help on using tickets.