Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

API Fetch: refresh nonces as soon as they expire, then fetch again #16683

Merged
merged 3 commits into from
Aug 9, 2019
Merged

API Fetch: refresh nonces as soon as they expire, then fetch again #16683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04d8ae1
API Fetch: refresh nonces as soon as they expired, then fetch again
ellatrix Jul 19, 2019
087fe0b
Use exit()
ellatrix Jul 19, 2019
c62b839
Fix PHP linting errors
ellatrix Jul 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,11 @@ function gutenberg_pre_init() {

require_once dirname( __FILE__ ) . '/lib/load.php';
}

/**
* Outputs a WP REST API nonce.
*/
function gutenberg_rest_nonce() {
exit( wp_create_nonce( 'wp_rest' ) );
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
}
add_action( 'wp_ajax_gutenberg_rest_nonce', 'gutenberg_rest_nonce' );
24 changes: 5 additions & 19 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,25 +246,11 @@ function gutenberg_register_scripts_and_styles() {
wp_add_inline_script(
'wp-api-fetch',
sprintf(
implode(
"\n",
array(
'( function() {',
' var nonceMiddleware = wp.apiFetch.createNonceMiddleware( "%s" );',
' wp.apiFetch.use( nonceMiddleware );',
' wp.hooks.addAction(',
' "heartbeat.tick",',
' "core/api-fetch/create-nonce-middleware",',
' function( response ) {',
' if ( response[ "rest_nonce" ] ) {',
' nonceMiddleware.nonce = response[ "rest_nonce" ];',
' }',
' }',
' )',
'} )();',
)
),
( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' )
'wp.apiFetch.nonceMiddleware = wp.apiFetch.createNonceMiddleware( "%s" );' .
Copy link
Member Author

@ellatrix ellatrix Jul 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left createNonceMiddleware alone as core is using it.

'wp.apiFetch.use( wp.apiFetch.nonceMiddleware );' .
'wp.apiFetch.nonceEndpoint = "%s";',
( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' ),
admin_url( 'admin-ajax.php?action=gutenberg_rest_nonce' )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nonce lifetime could even be reduced for increased security, without affecting the user experience!

This would only be true if we assume all libraries are using apiFetch, or at minimum using a similar base mechanism for refreshing nonces. Perhaps it would be better to drop the gutenberg_ prefix here to encourage consistent use of this approach by plugins writing admin UI that uses alternative HTTP transports?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention is to drop the gutenberg_ prefix, but only when the PHP code gets merged with core. I'm also fine dropping it here, but this is still a plugin, not core.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would only be true if we assume all libraries are using apiFetch, or at minimum using a similar base mechanism for refreshing nonces.

That's right. It's currently also not possible to change the lifetime of specific nonces I think. That would be cool. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to highlight the possibility, but yes, right now, that's unrealistic.

),
'after'
);
Expand Down
37 changes: 29 additions & 8 deletions packages/api-fetch/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ function registerMiddleware( middleware ) {
middlewares.unshift( middleware );
}

const checkStatus = ( response ) => {
if ( response.status >= 200 && response.status < 300 ) {
return response;
}

throw response;
};

const defaultFetchHandler = ( nextOptions ) => {
const { url, path, data, parse = true, ...remainingOptions } = nextOptions;
let { body, headers } = nextOptions;
Expand All @@ -71,13 +79,6 @@ const defaultFetchHandler = ( nextOptions ) => {
headers,
}
);
const checkStatus = ( response ) => {
if ( response.status >= 200 && response.status < 300 ) {
return response;
}

throw response;
};

const parseResponse = ( response ) => {
if ( parse ) {
Expand Down Expand Up @@ -148,7 +149,27 @@ function apiFetch( options ) {
return step( workingOptions, next );
};

return createRunStep( 0 )( options );
return new Promise( function( resolve, reject ) {
createRunStep( 0 )( options )
.then( resolve )
.catch( ( error ) => {
if ( error.code !== 'rest_cookie_invalid_nonce' ) {
return reject( error );
}

// If the nonce is invalid, refresh it and try again.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can / should this be part of a new or existing middleware?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I didn't really find a good way to put the promise callback in the middleware for creating a request. I guess there should be two kinds of middleware then? One for creating the request and one for processing the response?

window.fetch( apiFetch.nonceEndpoint )
.then( checkStatus )
.then( ( data ) => data.text() )
.then( ( text ) => {
apiFetch.nonceMiddleware.nonce = text;
apiFetch( options )
.then( resolve )
.catch( reject );
} )
.catch( reject );
} );
} );
}

apiFetch.use = registerMiddleware;
Expand Down
16 changes: 16 additions & 0 deletions packages/e2e-tests/plugins/nonce.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php
/**
* Plugin Name: Gutenberg Test Plugin, Nonce
* Plugin URI: https://github.com/WordPress/gutenberg
* Author: Gutenberg Team
*
* @package gutenberg-test-plugin-nonce
*/

/**
* Returns the nonce life time.
*/
function gutenberg_test_plugin_nonce_life() {
return 5;
}
add_filter( 'nonce_life', 'gutenberg_test_plugin_nonce_life' );
35 changes: 35 additions & 0 deletions packages/e2e-tests/specs/plugins/nonce.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* WordPress dependencies
*/
import {
activatePlugin,
createNewPost,
deactivatePlugin,
saveDraft,
} from '@wordpress/e2e-test-utils';

describe( 'Nonce', () => {
beforeAll( async () => {
await activatePlugin( 'gutenberg-test-plugin-nonce' );
} );

afterAll( async () => {
await deactivatePlugin( 'gutenberg-test-plugin-nonce' );
} );

beforeEach( async () => {
await createNewPost();
} );

it( 'should refresh when expired', async () => {
await page.keyboard.press( 'Enter' );
// eslint-disable-next-line no-restricted-syntax
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
await page.waitFor( 5000 );
await page.keyboard.type( 'test' );
// `saveDraft` waits for saving to be successful, so this test would
// timeout if it's not.
await saveDraft();
// We expect a 403 status once.
expect( console ).toHaveErrored();
} );
} );