#27152 closed defect (bug) (fixed)
wp_get_referer() no longer reports off-site referrers
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.5 | Priority: | normal |
Severity: | normal | Version: | 3.6.1 |
Component: | Bootstrap/Load | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
In r25318, a redirect validation was added to wp_get_referer()
and wp_get_original_referer()
by @nacin (there's no ticket for this change btw).
The problem here is that this has broken calls to these functions with the purpose of simply fetching the referrer for logging or stats, and not necessarily for redirection. This is a silent failure since callers were already expected to handle a false return value, and now off-site referers return false as well.
This is a regression from 3.6.
We shouldn't just assume that a call to these methods are strictly for the purpose of redirection, and leave the responsibility of validating the URL for redirection up to the method actually performing the redirection. We have wp_safe_redirect()
for this, and if this is required for form actions or elsewhere, they should be handling validation themselves.
Attachments (4)
Change History (17)
#2
follow-up:
↓ 3
@
11 years ago
- Version changed from 3.7 to 3.6.1
This was security hardening to avoid open redirection vulnerabilities. Too often core and plugins were using wp_get_referer() without wp_safe_redirect(). The addition of wp_validate_redirect() was very deliberate and was signed off by the security team and the lead developers.
This function is specifically a WordPress utility, given its checking of the _wp_http_referer field. (If you wanted HTTP_REFERER, you should really ask for it.) That, combined with the unreliability of referrers, the danger of un-validated referrers, and the nature of it already allowing for a silent failure, made it a pretty obvious decision.
This is a wontfix. It was a bug for this function to ever return a value without first validating it. wp_validate_redirect() has a filter.
#3
in reply to:
↑ 2
@
11 years ago
Just for the sake of argument since this never saw any public debate or discussion, and since what was apparently such a significant oversight is now fixed anyway, I'd appreciate some feedback on this. It doesn't seem like the appropriate response to me, and I'd like to clear my concerns up before something like this happens again without notice.
I wouldn't call it a "bug", it was doing it's job. The code using it might have been incorrect, but that's not the fault of wp_get_referer()
(which is appropriately documented with the correct usage on the Codex).
Why was it implausible to fix any incorrect usage in core? I'm fairly certain it didn't need fixed in more than a few places, but even if it was, I know there wasn't more than a maximum of about 50 locations in core that would have needed a review, and presumably, the reporter (or exploit) would have pointed out the location(s) to be concerned about.
How do you know how frequently it's used incorrectly in plugins? Do you have some numbers?
It just seems to me like this has diminished the reason for even having wp_safe_redirect()
as well as leaving a very generically named wp_get_referer()
method with only one single use case (redirecting form submissions), and no longer able to serve it's more globally identified purpose of (possibly more reliably) reporting where the user was referred from; something it was perfectly able to do before, and something the name of the method still indicates. This really just means that developers will be using wp_get_referer()
incorrectly now instead, so I wouldn't really call it a "pretty obvious decision".
This could have simply been resolved with better documentation and advocating better security practices including providing better examples of proper use in core code (which will never be fixed up to use wp_safe_redirect()
now since there is no point).
Part of my concern here is also that this was much more of an API design decision made by the security team rather than actually fixing security holes at their real points of exposure, all while ignoring the same advice we're still telling plugin developers. Normally this wouldn't even bother me if it was a good, quick fix for a serious issue that would have involved way more work to fix properly, but needed fixed immediately. This wan't one of those cases though.
#4
@
11 years ago
The security team includes a number of lead developers (well, all of them) as well as others who are known for their security and API acumen like dd32 and duck_. Everyone signed off on this change. These changes are not ones we make lightly, whether in public or in private. In this case, we also factored in that dropping a referer would not be critical, especially given how much they are already dropped in practice.
You may be right that it has diminished the need to use wp_safe_redirect(). The fact of the matter was that core was doing it wrong something like 40% of the time. If that's our track record, then we cannot expect a plugin developer to be any better. Sometimes we go with the scalpel; in this case, we chose the hammer.
#5
@
11 years ago
- Milestone changed from 3.9 to Awaiting Review
- Severity changed from major to normal
I could see wp_get_referer() gaining a counterpart, like wp_get_raw_referer() or wp_get_unsafe_referer(), or wp_get_referer(bool).
#8
@
9 years ago
- Milestone changed from Awaiting Review to 4.5
27152.2.diff is an updated patch that makes things more DRY by using wp_get_raw_referer
in wp_get_referer
.
Unit tests for wp_get_referer
are being suggested in #19856. Perhaps those are already enough, because a part of the function is simply extracted into a new one. Otherwise it's easy to add like 2 new tests for wp_get_raw_referer
alone.
#11
@
9 years ago
27152.4.diff adds a note regarding wp_get_referer()
in the new function's docblock and always resets the super globals on setUp()
. Locally, tests suddenly failed because Tests_WP_Customize_Manager::test_return_url()
sets $_SERVER['HTTP_REFERER']
.
There's at least one assertion that can check this on the latest patch submitted to #19856 (ticket-19856-referer.3.patch). Could just apply the unit tests portion of that for tests that cover this.