Include a refreshed nonce when responding to an authenticated REST API response

[adamsilverstein]

In ​https://github.com/WP-API/WP-API/issues/2146 @kadamwhite points out that in order for long lived JavaScript applications to remain authenticated. Without this, the nonce localized at load time will expire. My proposal is to add a X-WP-Nonce header with a new nonce in responses to authenticated requests.

[adamsilverstein]

In 35662.diff​:

• Add a X-WP-Nonce header with a refreshed nonce when the request is authenticated.

Going to update slightly so its only sent when there is an existing nonce.

[adamsilverstein]

In 35662.2.diff​ Only send the refreshed nonce when the request contains a valid nonce.

[adamsilverstein]

35662.3.diff​

• code/docblock cleanup
• only send nonce header when logic check passes

[welcher]

Adds unit tests

[rachelbaker]

@welcher thanks for the unit tests! Can you add a . to the end of your test method Docblocks?

@rmccue any other feedback here?

[azaozz]

Few things:

• Generating nonces for non-logged in users is almost useless. There may be edge cases where this could be used for something, but better to leave that for plugins to enable/set.
• Currently the nonces code block is before the REST API enabled check so it will return a nonce even when the API is disabled. This doesn't seem right?
• Generating a nonce on every request (which will be the same for 12 hours) seems redundant. Perhaps it is better when a client looks for the presence of a new nonce and replaces the current one? As mentioned in the Slack chat, maybe add new nonce only when wp_verify_nonce() returns 2.
• Consider separating the filter parameters: $nonce_is_valid and user_logged_in and maybe drop $user_and_nonce. Plugins don't need to check again why $user_and_nonce is false.

[welcher]

Unit Tests with updated docblocks

[adamsilverstein]

Related: #31897

[adamsilverstein]

@azaozz

Thanks for the feedback.

• Note that we only send the nonce back when the request contains a valid nonce.
• I agree we should only send a nonce back if the api is enabled.
• I like the suggestion of only sending a nonce when the verify nonce returns 2 indicating a nonce with a later expiration is available. It is straightforward to work with this on the client side.

@rmccue is working on a refreshed patch.

[kirasong]

Punting this to Future Release -- too late for 4.5 beta deadline.

[markjaquith]

Been a while with no updated patch. Anyone else want to provide a new patch for this?

[markjaquith]

Just refreshing the existing patch so it applies to core

[markjaquith]

35662.7.diff​:

• Only sends the header if REST API is active.
• By default, only sends if user is logged in and nonce status is 2.
• Sends nonce status as second parameter in the filter. (is_user_logged_in() can be checked by anything hooking in).

Annoyingly, I had to remove the unit test that tests the default functionality (without messing with the filter) because I couldn't figure out how to generate a nonce that is already in status 2. (Anyone have any ideas here? Might be good for general nonce testing).

[rmccue]

I believe @aidvu was working on an updated patch that moves this to rest_cookie_check_errors instead, which is a nicer place for it to live.

Would it be worth introducing a wp_create_outdated_nonce() or something? Seems like a very niche use case, and only really useful in the unit tests.

[markjaquith]

Replying to rmccue:

I believe @aidvu was working on an updated patch that moves this to rest_cookie_check_errors instead, which is a nicer place for it to live.

No opinion here.

Would it be worth introducing a wp_create_outdated_nonce() or something? Seems like a very niche use case, and only really useful in the unit tests.

Seems very niche. If we do that, maybe just make it available as a tests helper function — not in core.

[aidvu]

Move the nonce refreshing to rest_cookie_check_errors. Update tests.

[aidvu]

Updated patch as discussed today with @rmccue and @markjaquith at WCEU Contributor Day.

Thanks for all the help. :) It was an awesome experience.

[adamsilverstein]

Thanks for the effort here, especially working out the updated unit tests @aidvu! I like this placement for the functionality better and can see why it is a little better here. I have a slightly updated patch (added some periods in unit test description blocks and space before single line comments) I will upload when track db cooperates. I ran the unit tests and verified they work as expected.

[adamsilverstein]

slight doc block updates

[aidvu]

Thanks for cleaning and discussing the stuff with me @adamsilverstein. :) Forgot to mention you up there!

[rachelbaker]

Committed in [37905]:

REST API: Include a refreshed nonce in a X-WP-Nonce header when responding to an authenticated request.

Props adamsilverstein, welcher, markjaquith, aidvu.
Fixes #35662.

[rachelbaker]

In 37905:

REST API: Include a refreshed nonce in a X-WP-Nonce header when responding to an authenticated request.

Props adamsilverstein, welcher, markjaquith, aidvu.
Fixes #35662.