Opened 5 years ago
Last modified 21 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
@
5 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
@
5 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
@
5 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
@
5 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 further comments if needed and update the status as needed
Escapes the dot in the regex.