Make WordPress Core

Opened 12 years ago

Closed 8 years ago

#19856 closed defect (bug) (fixed)

wp_get_referer() doesn't return false when the referer URL is the same as the current URL

Reported by: garyc40's profile garyc40 Owned by: swissspidy's profile swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.3.1
Component: Bootstrap/Load Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Inside wp_get_referer(), there's this conditional statement:

if ( $ref && $ref !== $_SERVER['REQUEST_URI'] )

It is there to ensure that wp_get_referer() doesn't return the same page I'm on. This is useful when redirecting because I can detect and avoid infinite redirection.

According to PHP documentation, $_SERVER['REQUEST_URI'] is only the URI on the host. As a result, the conditional statement above fails in this case:

Let's say I was redirected from http://example.com/sample-uri to itself (either by clicking a link or a form submission). Then:

$ref = 'http://example.com/sample-uri';
$_SERVER['REQUEST_URI'] = '/sample-uri';

So technically, the referrer is the same page, but wp_get_referer() doesn't return false as expected, because $ref !== $_SERVER['REQUEST_URI'].

A better conditional statement would be:

if ( $ref && parse_url( $ref, PHP_URL_PATH ) !== $_SERVER['REQUEST_URI'] )

Patch attached.

I'm using PHP 5.3.6, Apache 2.2.20.

Attachments (6)

19856.diff (488 bytes) - added by garyc40 12 years ago.
19856-2.patch (455 bytes) - added by bpetty 12 years ago.
use get_site_url() in wp_get_referer() checks
19856-tests.patch (2.1 KB) - added by bpetty 12 years ago.
unit tests
ticket-19856-referer.3.patch (3.9 KB) - added by bpetty 10 years ago.
19856.2.diff (4.5 KB) - added by swissspidy 8 years ago.
19856.3.diff (5.0 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (22)

@garyc40
12 years ago

#1 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.4

#2 @nacin
12 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.4 to Future Release

@bpetty
12 years ago

use get_site_url() in wp_get_referer() checks

@bpetty
12 years ago

unit tests

#3 @bpetty
12 years ago

  • Keywords needs-unit-tests removed

The original patch here breaks referer URLs from different query variables, and also doesn't account for URLs from the different servers, but (coincidentally) the same resource.

If we use get_site_url() in wp_get_referer() for comparison instead, it fixes this and accounts for query variables and different servers.

I've also attached unit tests for this.

#4 @bpetty
12 years ago

  • Cc bpetty added

#5 @bpetty
11 years ago

  • Milestone changed from Future Release to 3.6

#6 @mordauk
11 years ago

  • Cc pippin@… added

#7 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#8 @nacin
10 years ago

  • Component changed from General to Bootstrap/Load
  • Keywords needs-patch needs-unit-tests added; has-patch removed

The patch would break if WordPress was not at the root of a domain.

#9 @nacinbot
10 years ago

  • Milestone changed from Future Release to 3.9

#10 @bpetty
10 years ago

  • Keywords needs-unit-tests removed
  • Milestone changed from 3.9 to Future Release

I've refreshed the patch, and added subfolder unit tests, however, this patch does not actually fix the broken subfolder detection from the previous patch. It can at least now be confirmed by explicitly running the unit tests for this ticket.

I was hoping to avoid using anything complex like parse_url(), but it's looking like it would be necessary for properly handling subfolder installations unless someone else has a better suggestion for retrieving just the path from site_url without using parse_url() (maybe I'm not aware of such a method in WP already).

I'm punting this back out of 3.9 milestone for now.

#11 @chriscct7
8 years ago

  • Keywords needs-unit-tests added

@swissspidy
8 years ago

#12 @swissspidy
8 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
  • Milestone changed from Future Release to 4.5

Patch needed a refresh because wp_get_referer() now uses wp_validate_redirect() (see [25318] and #27152). 19856.2.diff is an updated patch accommodating for that.

#13 follow-up: @nacin
8 years ago

This patch wold suggest that this condition is *never* hit. wp_get_referer() likely sometimes sees a root-relative referrer, no? That would imply we should add to this, not replace.

@swissspidy
8 years ago

#14 in reply to: ↑ 13 @swissspidy
8 years ago

Replying to nacin:

This patch wold suggest that this condition is *never* hit. wp_get_referer() likely sometimes sees a root-relative referrer, no? That would imply we should add to this, not replace.

Makes sense. 19856.3.diff adjusts the condition and adds unit tests for this case.

#15 @swissspidy
8 years ago

  • Keywords commit added

Considering for commit if there are no objections. Will implement #27152 afterwards.

#16 @swissspidy
8 years ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

In 36242:

Ensure wp_get_referer() returns false when the referrer URL is the current URL.

Adds unit tests.

Fixes #19856.

Note: See TracTickets for help on using tickets.