Make WordPress Core

Opened 7 years ago

Closed 8 months ago

Last modified 5 weeks 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 7 years ago.
First attempt at giving more context to the 'nonce_life' filter
35188-2.patch (1.6 KB) - added by dwainm 7 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 7 years ago.
35188-patch-3
35188-4.patch (1.0 KB) - added by dwainm 7 years ago.
Adding changelog entries

Download all attachments as: .zip

Change History (37)

#1 @johnbillion
7 years ago

  • Keywords needs-patch good-first-bug added

@dwainm
7 years ago

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

#2 @dwainm
7 years ago

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

#3 @giuseppe.mazzapica
7 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
7 years ago

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

#4 @dwainm
7 years ago

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

#5 @dwainm
7 years ago

Hi @giuseppe.mazzapica @johnbillion

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

#6 @DrewAPicture
7 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.


7 years ago

#8 @jorbin
7 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
7 years ago

35188-patch-3

@dwainm
7 years ago

Adding changelog entries

#9 @dwainm
7 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 change log for both the function and the filter.

Last edited 7 years ago by dwainm (previous) (diff)

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


7 years ago

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


12 months ago

#12 @SergeyBiryukov
12 months ago

  • Milestone set to 6.1

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


12 months ago

#14 @audrasjb
12 months 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
12 months ago

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

Self assigning for refresh/testing

#16 @SergeyBiryukov
12 months 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.


11 months ago

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


11 months ago

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


10 months ago

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


10 months ago
#20

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

#21 @costdev
10 months 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 10 months ago by costdev (previous) (diff)

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


9 months ago

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


9 months ago

#24 @audrasjb
9 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.


8 months ago

#26 @antonvlasenko
8 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:


8 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
8 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
8 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
8 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.


5 weeks 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:


5 weeks ago
#33

Thanks for the PR! Merged in r55685.

Note: See TracTickets for help on using tickets.