Make WordPress Core

Opened 9 years ago

Closed 21 months ago

Last modified 14 months ago

#35188 closed feature request (fixed)

Pass nonce action from "nonce_life" filter

Reported by: giuseppemazzapica's profile giuseppe.mazzapica Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.6
Component: General Keywords: has-patch needs-dev-note has-unit-tests
Focuses: Cc:

Description

At the moment, nonce_life https://developer.wordpress.org/reference/hooks/nonce_life/ filter pass to callbacks only the nonce lifespan to be filtered.

There are cases in which a shorter nonce lifespan might be useful (default lifespan is one day), and would be handy being able to recognize the context for the nonce creation.

It means that wp_nonce_tick() https://developer.wordpress.org/reference/functions/wp_nonce_tick/ should receive the action as argument.

Providing a default (probably -1 that is the default none action) this change will be 100% backward compatible.

Currently the only (hackish) way to filter the lifespan only for specific nonces is to add a filter before to call both wp_create_nonce and wp_verify_nonce and remove the filter right after that. Two filter additions and two filter removals that may be replaced with a single filter addition if context would be provided by the nonce_life hook.

Attachments (4)

35188.diff (1.1 KB) - added by dwainm 8 years ago.
First attempt at giving more context to the 'nonce_life' filter
35188-2.patch (1.6 KB) - added by dwainm 8 years ago.
Updated doc blocks and added argument to wp_create_nonce function call to wp_nonce_tick call
35188-3.patch (897 bytes) - added by dwainm 8 years ago.
35188-patch-3
35188-4.patch (1.0 KB) - added by dwainm 8 years ago.
Adding changelog entries

Download all attachments as: .zip

Change History (37)

#1 @johnbillion
9 years ago

  • Keywords needs-patch good-first-bug added

@dwainm
8 years ago

First attempt at giving more context to the 'nonce_life' filter

#2 @dwainm
8 years ago

Hi @giuseppe.mazzapica , would love your feedback on the patch uploaded :)

#3 @giuseppe.mazzapica
8 years ago

Hi @dwainm, thanks.

I think there are some issues in the patch.

In wp_verify_nonce default action is -1 and probably that should be used in wp_nonce_tick as well. (And doc bloc should say string|int).

Moreover, wp_nonce_tick is used in wp_create_nonce and not only in wp_verify_nonce.

Last very minor thing, there's an alignment issue in the doc bloc.

@dwainm
8 years ago

Updated doc blocks and added argument to wp_create_nonce function call to wp_nonce_tick call

#4 @dwainm
8 years ago

Thank you for your feedback @giuseppe.mazzapica . Updated :)

#5 @dwainm
8 years ago

Hi @giuseppe.mazzapica @johnbillion

I would love to your feedback on the latest patch. Thank you.

#6 @DrewAPicture
8 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to dwainm
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

See 35188-2.patch

This ticket was mentioned in Slack in #core by dwainm. View the logs.


8 years ago

#8 @jorbin
8 years ago

  • Keywords needs-patch added; has-patch removed

Thanks for the patch. When a filter or function's signature is updated, the inline documentation should be updated with a changelog entry. Additionally, the patch uses spaces and not tabs, so it needs to be updated for that well. The PHP style guide is in the handbook.

@dwainm
8 years ago

35188-patch-3

@dwainm
8 years ago

Adding changelog entries

#9 @dwainm
8 years ago

  • Keywords has-patch added; needs-patch removed
  • Version set to trunk

Thank you for your feedback @jorbin. I've updated the patch to include tabs not space and have updated the changed for both the function and the filter.

Version 0, edited 8 years ago by dwainm (next)

This ticket was mentioned in Slack in #core by dwainm. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

#12 @SergeyBiryukov
2 years ago

  • Milestone set to 6.1

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#14 @audrasjb
2 years ago

  • Keywords needs-refresh needs-dev-note needs-testing added; good-first-bug removed

Removing good-first-bug and adding needs-refresh / needs-testing.

#15 @audrasjb
2 years ago

  • Owner changed from dwainm to audrasjb
  • Status changed from assigned to accepted

Self assigning for refresh/testing

#16 @SergeyBiryukov
2 years ago

The latest patch appears to miss the part where the $action parameter is actually passed to wp_nonce_tick(), as seen in earlier patches, e.g. 35188-2.patch.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

This ticket was mentioned in PR #3007 on WordPress/wordpress-develop by costdev.


2 years ago
#20

  • Keywords has-unit-tests added; needs-refresh removed

#21 @costdev
2 years ago

  • Keywords needs-testing-info added

@audrasjb @SergeyBiryukov PR 3007 refreshes 35188-4.patch against trunk, updates the @since tags and adds the two missing changes from 35188-2.patch.

This still needs-testing. If someone can test the PR or add testing instructions for others to do so, that would be great!

Last edited 2 years ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core by chaion07. View the logs.


22 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


22 months ago

#24 @audrasjb
22 months ago

  • Keywords dev-feedback added

So @costdev I went to some tests and it looks like it doesn't work as expected… or maybe it's just me… :D

First, create a simple plugin which will generate a link at the end of each singular post of your test site.
This link contains a nonce, and when you click on the link, you'll get the following message:

  • if the nonce is valid (24 hours time limit by default), it will display "Nonce is valid".
  • if the nonce is invalid, it will display "Nonce is invalid"

Then we'll add a small snippet to this plugin, to change the time limitation of our nonce only for the nonce-life-tester.

Here is the code of the plugin:

<?php
/*
Plugin Name: nonce-life-tester
Author: audrasjb
Version: 0.1
Author URI: https://profiles.wordpress.org/audrasjb
*/

function nonce_life_tester_display_link( $content ) {
        // Check if we're inside the main loop in a single Post.
        $nonce = $_GET['_wpnonce'];
        if ( isset( $nonce ) && ! empty( $nonce ) ) {
                // Check Nonce and display verification results.
                $verify = wp_verify_nonce( $nonce, 'nonce-life-tester' );
                switch ( $verify ) {
                        case 1:
                                $result = 'Nonce is valid (less than 12 hours old)';
                                break;
                        case 2:
                                $result = 'Nonce is valid (between 12 and 24 hours old)';
                                break;
                        default:
                                $result = 'Nonce is invalid';
                }
                $content .= '<p>Nonce verification: <code>' . $result . '</code></p>';
        } else {
                // Display a link with a Nonce.
                if ( is_singular() && in_the_loop() && is_main_query() ) {
                        $url = wp_nonce_url( get_permalink(), 'nonce-life-tester' );
                        $content .= '<p><a href="' . $url . '">Testing nonces</a></p>';
                }
        }
        return $content;
}
add_action( 'the_content', 'nonce_life_tester_display_link' );

function nonce_life_tester_reduce_time_limit( $lifespan, $action ) {
        // Modify the lifespan of our specific Nonce.
        if ( 'nonce-life-tester' === $action ) {
                return 10; // 10 Seconds.
        } else {
                return $lifespan;
        }
}
add_filter( 'nonce_life', 'nonce_life_tester_reduce_time_limit', 10, 2 );

Using the current PR, I always get Nonce is invalid message.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


22 months ago

#26 @antonvlasenko
21 months ago

I've tried to reproduce the issue that @audrasjb outlined here, and I got the same result: Nonce is invalid.
But it might be enough to implement this simple fix to make it work:
https://github.com/WordPress/wordpress-develop/pull/3007#pullrequestreview-1112720674

costdev commented on PR #3007:


21 months ago
#27

@costdev I think the $action parameter should also be added to this place where the wp_nonce_tick() function gets called:
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/pluggable.php#L2351
E.g.:

$i     = wp_nonce_tick( $action );

Nicely spotted @anton-vlasenko! That's the last time I refresh a PR without testing it properly! :joy:

#28 @costdev
21 months ago

@audrasjb I have updated the PR with a fix that @antonvlasenko pointed out. When you get time, can you take it for a test spin? 🙂

#29 @audrasjb
21 months ago

  • Keywords needs-testing needs-testing-info dev-feedback removed

Ah thank for fixing this @costdev now it works fine with my little test plugin \o/

I think we're good to go!

#30 @audrasjb
21 months ago

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

In 54218:

General: Pass $action to nonce_life filter.

This changeset contextualizes the usage of nonce_life filter by passing the $action parameter. It allows to alterate the default lifespan of nonces on a case by case basis.

Props giuseppemazzapica, dwainm, DrewAPicture, jorbin, audrasjb, SergeyBiryukov, costdev, antonvlasenko.
Fixes #35188.

This ticket was mentioned in PR #4374 on WordPress/wordpress-develop by malavvasita.


14 months ago
#32

Ticket: #58181

Description:
In [54218] / #35188 $action was accidentally passed to wp_get_session_token(). This function doesn't accept any parameters.
In this PR, I have removed it to avoid consequences.

Trac ticket: https://core.trac.wordpress.org/ticket/58181

@SergeyBiryukov commented on PR #4374:


14 months ago
#33

Thanks for the PR! Merged in r55685.

Note: See TracTickets for help on using tickets.