Improve nonce handling by rejecting stale values #3770
Conversation
|
Size Change: +163 B (0%) Total Size: 1.18 MB
ℹ️ View Unchanged
|
| } | ||
|
|
||
| currentNonce = nonce; | ||
| currentTimestamp = timestamp || Date.now() / 1000; |
There was a problem hiding this comment.
Perhaps add a comment for why we're dividing by 1000 here, without looking at L68 of Assets.php it's not immediately obvious. Not a big deal I think but may be helpful.
There was a problem hiding this comment.
Thanks, added in 7aa1866
|
|
||
| try { | ||
| const storedNonceValue = window.localStorage.getItem( 'storeApiNonce' ); | ||
| const storedNonce = storedNonceValue ? JSON.parse( storedNonceValue ) : {}; |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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).
|
|
||
| try { | ||
| const storedNonceValue = window.localStorage.getItem( 'storeApiNonce' ); | ||
| const storedNonce = storedNonceValue ? JSON.parse( storedNonceValue ) : {}; |
There was a problem hiding this comment.
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).
|
works as expected on Firefox, Chrome and Safari. |
#3766 fixes the cache issue reported in #3757 by disabling caches.
As an alternative, this PR adds some additional logic to the nonce middleware which allows the nonce to be stored within browser localStorage, and some new logic to detect if a nonce is older or newer than the current.
Fixes #3757
Closes #3766
How to test the changes in this Pull Request:
If the add to cart worked, this fix is working. Also check browser localStorage and you should see a new field called storeApiNonce.
Changelog