WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 4 months ago

#36827 assigned defect (bug)

Regular expression in wp_guess_url() is slightly too permissive.

Reported by: cfinke Owned by: curdin
Milestone: Priority: normal
Severity: normal Version: 3.4
Component: General Keywords: has-patch good-first-bug has-unit-tests
Focuses: Cc:
PR Number:

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 (3)

wp_guess_url_regex.patch (672 bytes) - added by cfinke 3 years ago.
Escapes the dot in the regex.
36827_guess_url_patch_and_test (1.5 KB) - added by curdin 3 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 3 years ago.
Patch and unit tests ( with .patch extension - thanks @netweb )

Download all attachments as: .zip

Change History (16)

@cfinke
3 years ago

Escapes the dot in the regex.

#1 @voldemortensen
3 years ago

  • Milestone changed from Awaiting Review to 4.6

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


3 years ago

#3 @ocean90
3 years ago

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

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

#4 @swissspidy
3 years ago

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

#5 follow-up: @jrf
3 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
3 years ago

  • Keywords needs-unit-tests added; commit removed

#7 in reply to: ↑ 5 @voldemortensen
3 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
3 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
3 years ago

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

@curdin
3 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
3 years ago

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

#11 in reply to: ↑ 10 @netweb
3 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
3 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
3 years ago

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

#13 in reply to: ↑ 12 @netweb
3 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 3 years ago by netweb (previous) (diff)
Note: See TracTickets for help on using tickets.