WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 16 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 18 months ago.
Escapes the dot in the regex.
36827_guess_url_patch_and_test (1.5 KB) - added by curdin 16 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 16 months ago.
Patch and unit tests ( with .patch extension - thanks @netweb )

Download all attachments as: .zip

Change History (16)

@cfinke
18 months ago

Escapes the dot in the regex.

#1 @voldemortensen
18 months ago

  • Milestone changed from Awaiting Review to 4.6

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


18 months ago

#3 @ocean90
18 months ago

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

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

#4 @swissspidy
18 months ago

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

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

  • Keywords needs-unit-tests added; commit removed

#7 in reply to: ↑ 5 @voldemortensen
18 months 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
17 months 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
16 months ago

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

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

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

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

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

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