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

[petitphp]

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.

[SergeyBiryukov]

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?

[costdev]

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

[davidbaumwald]

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.

[SergeyBiryukov]

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.