WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 22 months ago

#36827 assigned defect (bug)

Regular expression in wp_guess_url() is slightly too permissive.

Reported by: cfinke Owned by: curdin
Milestone: Future Release 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 (3)

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

Download all attachments as: .zip

Change History (16)

@cfinke
2 years ago

Escapes the dot in the regex.

#1 @voldemortensen
2 years ago

  • Milestone changed from Awaiting Review to 4.6

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


2 years ago

#3 @ocean90
2 years ago

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

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

#4 @swissspidy
2 years ago

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

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

  • Keywords needs-unit-tests added; commit removed

#7 in reply to: ↑ 5 @voldemortensen
2 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
2 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
23 months ago

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

@curdin
22 months 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
22 months ago

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

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

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

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