#57670 closed defect (bug) (fixed)
Invalid return type for wp_get_raw_referer()
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 4.6 |
Component: | General | Keywords: | php81 has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
The return type for wp_get_raw_referer
indicates it returns string|false
. However, this is not the case if the user provided $_REQUEST['_wp_http_referer']
variable is an array for example.
This results in PHP 8.x fatals in code that assumes this function only returns strings/booleans.
strpos( wp_get_raw_referer(), 'foo' );
Change History (13)
This ticket was mentioned in PR #4034 on WordPress/wordpress-develop by xknown.
2 years ago
#1
- Keywords has-patch has-unit-tests added
#3
@
2 years ago
- Keywords commit added
- Milestone changed from Awaiting Review to 6.3
- Owner set to johnbillion
- Status changed from new to accepted
2 years ago
#4
The patch looks good to me.
I am left wondering about two things though:
- Is the
_wp_http_referer
REQUEST variable documented anywhere ? And if so, is that documentation clear enough that this should always be set to astring
? And if not documented, is there anywhere we can add documentation about this ?
- Can we think of any legitimate reasons for the variable to be set to an array ? The only thing I can come up with is a redirect-chain.
I don't think it's a documentation problem, or developers using it on the wrong way. We noticed this issue because there were lots of PHP fatals related to people running automated security tools.
2 years ago
#5
- Can we think of any legitimate reasons for the variable to be set to an array ? The only thing I can come up with is a redirect-chain.
`
I don't think it's a documentation problem, or developers using it on the wrong way. We noticed this issue because there were lots of PHP fatals related to people running automated security tools.
Well, servers don't set that variable by default, so it gets set somewhere.... whether it is by WP Core, by a plugin or by an automated security tool. So, what I'm really asking is, can we identify in some way, what code is setting the variable to an array and for what purpose.
Based on _this_ function's documentation, setting the variable to an array is _doing it wrong_, but I'm wondering if there are legitimate reasons for that and what the source is (and if it is Core, we need to fix Core or decide that arrays are allowed).
#7
@
21 months ago
Hi @xknown,
Some questions:
- Have you reproduced this in Core's default functionality, or is it only when using automatic security tools?
- What code produced the array for
_wp_http_referer
? (link if possible)
Thanks!
#8
@
21 months ago
Hi @costdev!
Have you reproduced this in Core's default functionality, or is it only when using automatic security tools?
I didn't test it in core, but it took me less than five minutes to find an example:
'http://WP.site/wp-login.php?action=postpass&_wp_http_referer[]='
.
Fatal error: Uncaught TypeError: trim(): Argument #1 ($string) must be of type string, array given in /var/www/html/wp-includes/pluggable.php:1558 Stack trace: #0 /var/www/html/wp-includes/pluggable.php(1558): trim(Array, ' \t\n\r\x00\x08\v') #1 /var/www/html/wp-includes/functions.php(1962): wp_validate_redirect(Array, false) #2 /var/www/html/wp-login.php(724): wp_get_referer() #3 {main} thrown in /var/www/html/wp-includes/pluggable.php on line 1558
And even if we can only reproduce it via security tools, I think this is something we need to fix.
What code produced the array for _wp_http_referer? (link if possible)
It's likely somewhere in the PHP internals, but I don't think it's relevant for this ticket.
#9
@
21 months ago
Thanks @xknown!
My understanding is that _wp_http_referer
, like HTTP_REFERER
, should always be a string. I haven't found any references to it being acceptable or used as an array.
So if the issue is just that wp_get_referer()
isn't enforcing its documented return types, then the PR looks good to me! - I just left a comment with a small change to the test method's name.
#10
@
21 months ago
- Keywords commit added
even if we can only reproduce it via security tools, I think this is something we need to fix
+1. Thanks for catching this and for the PR!
My understanding is that
_wp_http_referer
, likeHTTP_REFERER
, should always be a string.
Yep, that's right. As far as I know _wp_http_referer
is intended as a replacement/override for HTTP_REFERER
and is used for redirects (in combination with wp_safe_redirect()
). It can either be a string or not set.
21 months ago
#11
Yep, more/better docs are always a big plus! The question is: where would be a good place to add them :)
servers don't set that variable by default, so it gets set somewhere...
Right. It gets set in few places, mostly through wp_nonce_field()
(which is used a lot) that calls wp_referer_field()
that returns a hidden HTML input element.
Seems the docblock for wp_referer_field()
is the most appropriate place although not sure how "visible" it would be as that function is rarely used on its own. So perhaps can add the docs in two places: the docblocks for wp_referer_field()
and for wp_get_raw_referer()
?
Trac ticket: https://core.trac.wordpress.org/ticket/57670#ticket