Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Improve nonce handling by rejecting stale values #3770

Merged
merged 5 commits into from Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Expand Up @@ -5,6 +5,7 @@ module.exports = {
],
globals: {
wcStoreApiNonce: 'readonly',
wcStoreApiNonceTimestamp: 'readonly',
fetchMock: true,
jQuery: 'readonly',
IntersectionObserver: 'readonly',
Expand Down
59 changes: 52 additions & 7 deletions assets/js/middleware/store-api-nonce.js
Expand Up @@ -3,9 +3,18 @@
*/
import apiFetch from '@wordpress/api-fetch';

// @ts-ignore wcStoreApiNonce is window global
// Cache for the initial nonce initialized from hydration.
let nonce = wcStoreApiNonce || '';
// Stores the current nonce for the middleware.
let currentNonce = '';
let currentTimestamp = 0;

try {
const storedNonceValue = window.localStorage.getItem( 'storeApiNonce' );
const storedNonce = storedNonceValue ? JSON.parse( storedNonceValue ) : {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Any security concerns about storing the nonce value in localStorage? Would we be able to get away with just storing the nonce timestamp (and assume if that hasn't changed that the nonce in the request is okay)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nerrad I don't think there is a security concern because the nonces are also injected into the page (so readable from JS), and they are to stop CSRF not used as secure tokens or anything. We need both here because the nonce injected on the page is out of date if you go back using the browser button. The new nonce needs to persist somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, in that case - this gets the 👍 from me (note, I have not tested though...so probably should get confirmed by someone else in testing).

currentNonce = storedNonce?.nonce || '';
currentTimestamp = storedNonce?.timestamp || 0;
} catch {
// We can ignore an error from JSON parse.
}

/**
* Returns whether or not this is a non GET wc/store API request.
Expand All @@ -28,12 +37,44 @@ const isStoreApiGetRequest = ( options ) => {
* @param {Object} headers Headers object.
*/
const setNonce = ( headers ) => {
const newNonce = headers?.get( 'X-WC-Store-API-Nonce' );
if ( newNonce ) {
nonce = newNonce;
const nonce = headers?.get( 'X-WC-Store-API-Nonce' ) || '';
const timestamp = headers?.get( 'X-WC-Store-API-Nonce-Timestamp' ) || 0;

if ( nonce ) {
updateNonce( nonce, timestamp );
}
};

/**
* Updates the stored nonce within localStorage so it is persisted between page loads.
*
* @param {string} nonce Incoming nonce string.
* @param {number} timestamp Timestamp from server of nonce.
*/
const updateNonce = ( nonce, timestamp ) => {
// If the "new" nonce matches the current nonce, we don't need to update.
if ( nonce === currentNonce ) {
return;
}

// Only update the nonce if newer. It might be coming from cache.
if ( currentTimestamp && timestamp < currentTimestamp ) {
return;
}

currentNonce = nonce;
currentTimestamp = timestamp || Date.now() / 1000; // Convert ms to seconds to match php time()

// Update the persisted values.
window.localStorage.setItem(
'storeApiNonce',
JSON.stringify( {
nonce: currentNonce,
timestamp: currentTimestamp,
} )
);
};

/**
* Nonce middleware which updates the nonce after a request, if given.
*
Expand All @@ -47,11 +88,15 @@ const storeNonceMiddleware = ( options, next ) => {
const existingHeaders = options.headers || {};
options.headers = {
...existingHeaders,
'X-WC-Store-API-Nonce': nonce,
'X-WC-Store-API-Nonce': currentNonce,
};
}
return next( options, next );
};

apiFetch.use( storeNonceMiddleware );
apiFetch.setNonce = setNonce;

// @ts-ignore wcStoreApiNonce is window global cache for the initial nonce initialized from hydration.
// @ts-ignore wcStoreApiNonceTimestamp is window global cache for the initial nonce initialized from hydration.
updateNonce( wcStoreApiNonce, wcStoreApiNonceTimestamp );
5 changes: 4 additions & 1 deletion src/Assets.php
Expand Up @@ -63,7 +63,10 @@ public static function register_assets() {
// Inline data.
wp_add_inline_script(
'wc-blocks-middleware',
"var wcStoreApiNonce = '" . esc_js( wp_create_nonce( 'wc_store_api' ) ) . "';",
"
var wcStoreApiNonce = '" . esc_js( wp_create_nonce( 'wc_store_api' ) ) . "';
var wcStoreApiNonceTimestamp = '" . esc_js( time() ) . "';
",
'before'
);

Expand Down
1 change: 1 addition & 0 deletions src/StoreApi/Routes/AbstractRoute.php
Expand Up @@ -81,6 +81,7 @@ public function get_response( \WP_REST_Request $request ) {
}

$response->header( 'X-WC-Store-API-Nonce', wp_create_nonce( 'wc_store_api' ) );
$response->header( 'X-WC-Store-API-Nonce-Timestamp', time() );
$response->header( 'X-WC-Store-API-User', get_current_user_id() );
return $response;
}
Expand Down