WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#10057 closed defect (bug) (fixed)

wp_nonce_field() calls wp_referer_field() with too many args

Reported by: coffee2code Owned by: markjaquith
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

In wp_nonce_field() in wp-includes/functions.php, a call is made to wp_referer_field() with two arguments. However, wp_referer_field() only accepts one argument.

wp_original_referer_field() accepts two arguments and is probably what was originally intended.

Refer to r7438 (specifically http://core.trac.wordpress.org/changeset/7438/trunk/wp-includes/functions.php). Mark added a second argument to wp_original_referer_field() while also adding the second (and technically invalid) argument to the call to wp_referer_field() in wp_nonce_field().

This leads me to believe that the wp_referer_field() call in question was intended to be changed to be a call to the updated wp_original_referer_field() function. This is also affirmed by the fact that there are no other calls to wp_referer_field() in the codebase except once in the livejournal importer.

As such, the attached patch changes the name of the function being called.

Attachments (2)

10057.diff (733 bytes) - added by coffee2code 6 years ago.
Patch mentioned in ticket submission
10057.2.diff (712 bytes) - added by coffee2code 6 years ago.
Patch to remove extraneous argument

Download all attachments as: .zip

Change History (11)

@coffee2code6 years ago

Patch mentioned in ticket submission

comment:1 @Denis-de-Bernardy6 years ago

  • Keywords early added

comment:2 @ryan6 years ago

  • Owner set to MarkJaquith
  • Status changed from new to assigned

comment:3 @markjaquith6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12331]) Yes, I did mean original... props coffee2code. fixes #10057

comment:4 @vladimir_kolesnikov6 years ago

  • Cc vladimir@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

What is the reason to call wp_original_referer_field($echo, >>>'previous'<<<)?

It inserts the field with the address the user came from to the page with the form.

Say, we have a form on a page admin.php?page=some/file.php which submits to wp-admin/admin-posts.php. If the user comes to admin.php?page=some/file.php from, say, Google, the generated fields will be:

<input type="hidden" value="ef08c8f103" name="_wpnonce" id="_wpnonce"/>
<input type="hidden" value="http://www.google.ru/" name="_wp_original_http_referer"/>

In this case _wp_original_http_referer field will be meaningless.

Moreover, check_admin_referer() function calls wp_get_referer() to get the referring page. wp_get_referer() checks $_REQUEST_wp_http_referer? (which wp_nonce_field() used to generate) which is not there since wp_original_referer_field() inserts a different field _wp_original_http_referer.

Now the code (plugins, I mean) that relies upon wp_get_referer() is less reliable because wp_get_referer() will use only $_SERVERHTTP_REFERER? which is often empty (blocked by the firewall, browser etc, esp. when FORCE_SSL_ADMIN constnt is set).

In brief:

  1. The patch breaks backward compatibility
  2. wp_get_referer() is now less reliable
  3. If you need to use _wp_http_referer, you have to add a call to wp_referer_field() in addition to wp_nonce_field().

Is this the intended behavior?

comment:5 @markjaquith6 years ago

(In [12368]) Reverting [12331]. see #10057

comment:6 @markjaquith6 years ago

  • Milestone changed from 2.9 to 3.0
  • Owner changed from MarkJaquith to markjaquith
  • Status changed from reopened to accepted

Reverted. I'll take another look after 2.9.

comment:7 @coffee2code6 years ago

Of course, the alternative fix, which if vladimir is correct, is to omit the 'previous' argument in the call to wp_referer_field() that was added in r7438 (specifics mentioned above). The attached patch, 10057.2.diff, does so. This essentially retains the current state of affairs, but deals with the issue I originally raised (too many args sent to wp_referer_field()).

I forget how deeply I looked into it originally, but since that rev added the function wp_original_referer_field() and also modified the aforementioned line of code to send a second arg of 'previous', it appeared consistent with an intent to change the function being called. If not, then that rev unnecessarily added the 'previous' arg.

So, in short, the intent of the original WP code still bears some review, but the second patch should correct an improper call without compatibility issues.

@coffee2code6 years ago

Patch to remove extraneous argument

comment:8 @TobiasBg5 years ago

  • Keywords commit added; early removed
  • Severity changed from minor to normal

Just closed #12550 as a dup of this.

I agree to coffee2code that the extra argument should be removed.
I haven't tested if the patch here still applies cleanly. If not, there's a more recent one in the closed ticket.

comment:9 @markjaquith5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [14522]) Omit extraneous argument. props coffee2code. fixes #10057

Note: See TracTickets for help on using tickets.