Make WordPress Core

Opened 8 years ago

Closed 4 years ago

#37569 closed enhancement (worksforme)

REST API: refresh expired nonces

Reported by: iseulde's profile iseulde Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: dev-feedback has-patch
Focuses: Cc:

Description

Is there a reason to not refresh an expired nonce?

At the moment the API only refreshes nonces when it's given an unexpired nonce, so that the client can refresh it for future requests. When the nonce expired though, it will just error. The only thing a client can do then is to refresh the page, or get a new nonce some other way. Since a simple authenticated request is enough to get a new nonce elsewhere, I don't see why the API itself can't provide the client with a new one, so that the client can refresh its nonce, and repeat the request with the new nonce.

Attachments (5)

37569.patch (830 bytes) - added by iseulde 8 years ago.
37569.diff (3.2 KB) - added by rmccue 8 years ago.
Refresh nonce via Heartbeat
37569.2.diff (3.6 KB) - added by adamsilverstein 7 years ago.
37569.3.diff (1.5 KB) - added by adamsilverstein 7 years ago.
37569.4.diff (1.9 KB) - added by adamsilverstein 7 years ago.
prevent recursion

Download all attachments as: .zip

Change History (39)

@iseulde
8 years ago

#1 @iseulde
8 years ago

  • Keywords has-patch added

#2 @iseulde
8 years ago

Okay, so the reason is CORS. In that case the client should be able to refresh the nonce some other way. Maybe a separate endpoint, maybe admin-ajax.php, and some docs?

See https://wordpress.slack.com/archives/core-restapi/p1470310813000622

Last edited 8 years ago by iseulde (previous) (diff)

#3 @iseulde
8 years ago

  • Keywords needs-patch needs-docs dev-feedback added; has-patch removed

So sending the nonce with admin-ajax.php is exactly what I end up doing, as there is nothing that core provides. It would be great if core had some documentation on how to do this best and/or provided an endpoint for cookie based auth to renew an expired nonce.

@rmccue
8 years ago

Refresh nonce via Heartbeat

#4 @rmccue
8 years ago

  • Keywords has-patch added; needs-patch removed

We can refresh the nonce via Heartbeat, similar to the existing code that does the same for the regular nonces in the admin. 37569.diff adds a new wp-api-nonce script that does just that. It only refreshes the nonce in the second tick (12-24 hours).

This is a separate script to avoid creating a dependency on heartbeat for wp-api, as you may not want Heartbeat loading constantly on the frontend of your site.

@iseulde Does this suit your use case?

#5 @rmccue
8 years ago

Note: I used heartbeat_received rather than wp_refresh_nonces, as the latter only fires if the heartbeat-nonce is in the second tick. These should be the same, but with the pluggability of nonces, I'd rather avoid that.

#6 @rmccue
8 years ago

  • Milestone changed from Awaiting Review to 4.8
  • Type changed from defect (bug) to enhancement

#7 follow-up: @iseulde
8 years ago

At first glance this looks great. It's fine not having a separate script IMO, as long as there is an endpoint for it, it's the canonical way to refresh the nonce, and it's documented as such.

#8 in reply to: ↑ 7 ; follow-up: @rmccue
8 years ago

Replying to iseulde:

At first glance this looks great. It's fine not having a separate script IMO, as long as there is an endpoint for it, it's the canonical way to refresh the nonce, and it's documented as such.

Do you mean we should roll it into the regular Heartbeat refreshes? We could potentially roll it into the base wp-api library as well, if @adamsilverstein thinks that's a good idea.

#9 in reply to: ↑ 8 @adamsilverstein
8 years ago

Do you mean we should roll it into the regular Heartbeat refreshes? We could potentially roll it into the base wp-api library as well, if @adamsilverstein thinks that's a good idea.

@rmccue I definitely think this is a good idea, that was my plan all along with #35662. Let's open a separate ticket to track this enhancement.

#10 @adamsilverstein
8 years ago

Created a new ticket to add nonce refreshing to the JS client in #40422

#11 @iseulde
8 years ago

Not sure about rolling into heartbeat, but yes to the base wp-api library.

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


8 years ago

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


8 years ago

#14 @jbpaul17
8 years ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

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


7 years ago

#16 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

#17 @jnylen0
7 years ago

Do we still want this change, given that #40422 is going to add this functionality to the wp-api.js client?

If so, let's commit #40919 first and then roll this change into the script that will be added there.

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


7 years ago

#19 @iseulde
7 years ago

@rmccue Stumbled upon this ticket again and saw that the patch has a check to see if the old nonce is in its second half life. So this patch doesn't really solve this issue? We need to send a new nonce regardless of the old nonce's validity.

Gutenberg issue: https://github.com/WordPress/gutenberg/pull/2790

#20 @adamsilverstein
7 years ago

@iseulde the way this should work is if your 24 hr nonce is more than 12 hrs old, you get a new 24 hr nonce. So every 12 hours you should get a new nonce that would last 24 hours. As long as you keep checking at least once every < 12 hrs, you should always have a nonce that will last you 12+ hrs. Can you test the latest patch on the PR, it should work as expected.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


7 years ago

#22 @adamsilverstein
7 years ago

Probably makes the most sense to test this feature in Gutenberg first since its needed there and we don't really need it in core otherwise and it would be easy for clients to add if they need it. In 37569.2.diff I cleaned up a tiny bit and added wp-api-nonce as a dependency of wp-api, however we should probably only load it if heartbeat is active. Also, it would be great to add some unit tests demonstrating how this works.

#23 @adamsilverstein
7 years ago

  • Milestone changed from 4.9 to Future Release

#24 @iseulde
7 years ago

I have some concerns with the latest patch.

It still doesn't solve anything for an expired nonce, as there's a nonce check...

As mentioned in a previous comment, I'm not sure if it's a good idea to roll this into the Heartbeat API. The API client might be a better place for it. A possible approach could be to create a separate endpoint for cookie auth only, either on the REST API root, or admin-ajax.php. I feel that setting it on the root would make it more official for other clients to adopt. With this endpoint, the client could get a new nonce if a request fails because of an invalid nonce. No need for Heartbeat. With this new nonce, the client should resend the same request (as if nothing happened). To the user of this API, the resolution of the promise will just take a bit longer, but it doesn't need to do anything.

A long time ago, I had to do something similar for a plugin.

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

#25 @adamsilverstein
7 years ago

create a separate endpoint for cookie auth only

@iseulde I like this idea! What would be required to get a fresh nonce? Does offering up the nonce without requiring a previous valid nonce weaken their value? Previous thinking was you could always keep your nonce fresh by checking at least once every 12 hrs and returning a new nonce in the second 'half'.

#26 @iseulde
7 years ago

See https://github.com/WordPress/gutenberg/pull/3006#issuecomment-337241027. We wouldn't require anything. It's the same mechanism as a page refresh, which checks if the user is still logged in and has the right capabilities to otherwise get the nonce on first page load. It's like requesting the the same page in JS and then parsing the right nonce out of it*. :) May be a good idea to pass by other security team members, but as far as I see, this is no problem.

As of https://github.com/WordPress/gutenberg/pull/2790, we're already doing it without requiring anything.

*

fetch( http://wordpress.test/wp-admin/admin.php?page=gutenberg-demo, {credentials: "same-origin"})

would return:

<!DOCTYPE html>
/* ... */
<script type='text/javascript'>
var wpApiSettings = {"root":"http:\/\/wordpress.test\/wp-json\/","nonce":"...","versionString":"wp\/v2\/"};
</script>
/* ... */

#27 @adamsilverstein
7 years ago

@iseulde Thanks for the feedback and suggestions, I'll work on adding a new endpoint and a fallback mechanism to the client.

#28 @adamsilverstein
7 years ago

  • Keywords needs-docs removed
  • Milestone changed from Future Release to 5.0

In 37569.3.diff

  • include a fresh nonce in rest responses when the user is logged in, even if the nonce check fails
  • retry the sync when a nonce failure is detected and a new nonce is available

My only concern here is potentially infinite recursion, if the returned nonce continues to change, the request will be made repeatedly. This shouldn't happen, but might be worth explicitly preventing it.

#29 @adamsilverstein
7 years ago

  • Milestone changed from 5.0 to 4.9.1

#30 @iseulde
7 years ago

Would be grateful for a second opinion here.

My only concern here is potentially infinite recursion, if the returned nonce continues to change, the request will be made repeatedly. This shouldn't happen, but might be worth explicitly preventing it.

Yeah, I agree.

@adamsilverstein
7 years ago

prevent recursion

#31 @adamsilverstein
7 years ago

In 37569.4.diff

  • explicitly prevent recursion, resyncing with a fresh nonce at most once each time a nonce failure occurs.

#32 @johnbillion
7 years ago

  • Milestone changed from 4.9.1 to 5.0

#33 @peterwilsoncc
6 years ago

  • Milestone changed from 5.0 to Future Release

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

#34 @TimothyBlynJacobs
4 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

I don't think this is necessary anymore after #48076. If I'm incorrect, please reopen.

Note: See TracTickets for help on using tickets.