WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 14 months ago

Last modified 5 months ago

#35662 closed enhancement (fixed)

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

Reported by: adamsilverstein Owned by: rachelbaker
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

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.

Attachments (9)

35662.diff (753 bytes) - added by adamsilverstein 19 months ago.
35662.2.diff (1.1 KB) - added by adamsilverstein 19 months ago.
35662.3.diff (1.4 KB) - added by adamsilverstein 18 months ago.
35662.4.diff (5.1 KB) - added by welcher 18 months ago.
Adds unit tests
35662.5.diff (5.2 KB) - added by welcher 18 months ago.
Unit Tests with updated docblocks
35662.6.diff (5.2 KB) - added by markjaquith 14 months ago.
Just refreshing the existing patch so it applies to core
35662.7.diff (4.8 KB) - added by markjaquith 14 months ago.
35662.8.diff (4.1 KB) - added by aidvu 14 months ago.
Move the nonce refreshing to rest_cookie_check_errors. Update tests.
35662.11.diff (4.1 KB) - added by adamsilverstein 14 months ago.
slight doc block updates

Download all attachments as: .zip

Change History (37)

#1 @adamsilverstein
19 months ago

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.

#2 @adamsilverstein
19 months ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 4.5
  • Owner set to adamsilverstein
  • Status changed from new to assigned

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

#3 @adamsilverstein
18 months ago

  • Keywords needs-unit-tests added

35662.3.diff

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

@welcher
18 months ago

Adds unit tests

#4 @welcher
18 months ago

  • Keywords needs-unit-tests removed

#5 @adamsilverstein
18 months ago

  • Component changed from General to REST API

#6 @rachelbaker
18 months ago

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

@rmccue any other feedback here?

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


18 months ago

#8 @rmccue
18 months ago

  • Owner changed from adamsilverstein to rmccue
  • Status changed from assigned to reviewing

#9 @azaozz
18 months ago

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. Plugins don't need to check again why $user_and_nonce is false.
Last edited 18 months ago by azaozz (previous) (diff)

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


18 months ago

@welcher
18 months ago

Unit Tests with updated docblocks

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


18 months ago

#13 @adamsilverstein
18 months ago

@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.

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


18 months ago

#15 @mikeschroder
18 months ago

  • Milestone changed from 4.5 to Future Release

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

#16 @markjaquith
14 months ago

  • Keywords needs-refresh added

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

@markjaquith
14 months ago

Just refreshing the existing patch so it applies to core

#17 @markjaquith
14 months ago

  • Keywords needs-refresh removed

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).

#18 follow-up: @rmccue
14 months ago

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.

#19 in reply to: ↑ 18 @markjaquith
14 months ago

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
14 months ago

Move the nonce refreshing to rest_cookie_check_errors. Update tests.

#20 @aidvu
14 months ago

  • Keywords has-unit-tests added

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

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

#21 @adamsilverstein
14 months ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Future Release to 4.6

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
14 months ago

slight doc block updates

#22 @aidvu
14 months ago

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

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


14 months ago

#24 @ocean90
14 months ago

  • Owner changed from rmccue to rachelbaker

#25 @rachelbaker
14 months ago

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.

#26 @rachelbaker
14 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

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


13 months ago

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


5 months ago

Note: See TracTickets for help on using tickets.