Regular expression in wp_guess_url() is slightly too permissive.

[cfinke]

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.

[cfinke]

Escapes the dot in the regex.

[ocean90]

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

[swissspidy]

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

[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'

[voldemortensen]

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).

[jdgrimes]

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

[curdin]

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 ;)

[netweb]

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/

[curdin]

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]

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

[netweb]

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