Opened 5 years ago
Last modified 20 months ago
#36827 assigned defect (bug)
Regular expression in wp_guess_url() is slightly too permissive.
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 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)
Change History (16)
This ticket was mentioned in Slack in #core by voldemortensen. View the logs.
5 years ago
#5
follow-up:
↓ 7
@
5 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'
#7
in reply to:
↑ 5
@
5 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=somevarI'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
@
5 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
@
4 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 ;)
#11
in reply to:
↑ 10
@
4 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:
↓ 13
@
4 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?
#13
in reply to:
↑ 12
@
4 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 comments if needed.
Escapes the dot in the regex.