Opened 17 years ago
Closed 16 years ago
#10057 closed defect (bug) (fixed)
wp_nonce_field() calls wp_referer_field() with too many args
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (11)
#4
@
16 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:
- The patch breaks backward compatibility
- wp_get_referer() is now less reliable
- 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?
#6
@
16 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.
#7
@
16 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.
#8
@
16 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.
Patch mentioned in ticket submission