Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Fetch: refresh nonces as soon as they expire, then fetch again #16683

Merged
merged 3 commits into from
Aug 9, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 19, 2019

Description

Currently, nonces are refresh through the Heartbeat API, but it requires a valid nonce. When you leave a post open and come back to it the next day (during which time the heartbeat may have stopped), the nonce cannot be refreshed.

Since a simple authenticated request is enough to get a new nonce elsewhere (like a full page reload), we can just create an endpoint with admin-ajax.php to get a new nonce. We CANNOT use the REST API itself because of CORS, but we can create a separate endpoint without CORS.

I updated the API Fetch package to refresh a nonce when the REST API sends a nonce error code. Once a new nonce is obtained, the request is remade. The user wouldn't notice a thing. :)

Notice that with this technique, the nonce lifetime could even be reduced for increased security, without affecting the user experience!

How has this been tested?

Install the e2e test plugin that sets the nonce lifetime to 5 seconds. Create a new post. Wait 5 seconds, then add some text and try to save. Saving should work. Open the console and notice that there is one 403 status error logged.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

)
),
( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' )
'wp.apiFetch.nonceMiddleware = wp.apiFetch.createNonceMiddleware( "%s" );' .
Copy link
Member Author

@ellatrix ellatrix Jul 19, 2019

Choose a reason for hiding this comment

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

I left createNonceMiddleware alone as core is using it.

@ellatrix ellatrix added this to the Gutenberg 6.2 milestone Jul 24, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Curious to have feedback from the REST API team on this one.

return reject( error );
}

// If the nonce is invalid, refresh it and try again.
Copy link
Member

Choose a reason for hiding this comment

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

Can / should this be part of a new or existing middleware?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps. I didn't really find a good way to put the promise callback in the middleware for creating a request. I guess there should be two kinds of middleware then? One for creating the request and one for processing the response?

gutenberg.php Show resolved Hide resolved
@TimothyBJacobs
Copy link
Member

Curious to have feedback from the REST API team on this one.

I'll make sure to bring this ticket up at the next office hours.

@gziolo gziolo removed this from the Gutenberg 6.2 milestone Jul 30, 2019
@ellatrix
Copy link
Member Author

@TimothyBJacobs Thanks!

@aduth
Copy link
Member

aduth commented Jul 30, 2019

@ellatrix
Copy link
Member Author

ellatrix commented Aug 6, 2019

@TimothyBJacobs @kadamwhite Any news on this? :)

@TimothyBJacobs TimothyBJacobs added the REST API Interaction Related to REST API label Aug 6, 2019
@kadamwhite
Copy link
Contributor

@ellatrix Thanks for the bump :) My bad.

I think this makes sense. I'd be very hesitant to change or relax the authentication rules for any route within /wp-json, so I like using admin-ajax for getting the new nonce; the approach you're using feels safest.

My only concern would be to ensure we're guarded against any situation where the admin-ajax endpoint could be exploited by an unauthorized user to get a nonce/cookie combo they could use for mischief. I believe this is probably well in hand given normal admin-ajax authentication rules, I'm just much less familiar with those than I am the REST API! :)

As for whether this should be middleware, I'm impartial. I feel it should be possible to utilize middleware for this, but apiFetch is also a very WordPress-oriented request library so I've no problem supporting this behavior as part of the core request chain logic.

'wp.apiFetch.use( wp.apiFetch.nonceMiddleware );' .
'wp.apiFetch.nonceEndpoint = "%s";',
( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' ),
admin_url( 'admin-ajax.php?action=gutenberg_rest_nonce' )
Copy link
Contributor

Choose a reason for hiding this comment

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

the nonce lifetime could even be reduced for increased security, without affecting the user experience!

This would only be true if we assume all libraries are using apiFetch, or at minimum using a similar base mechanism for refreshing nonces. Perhaps it would be better to drop the gutenberg_ prefix here to encourage consistent use of this approach by plugins writing admin UI that uses alternative HTTP transports?

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention is to drop the gutenberg_ prefix, but only when the PHP code gets merged with core. I'm also fine dropping it here, but this is still a plugin, not core.

Copy link
Member Author

Choose a reason for hiding this comment

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

This would only be true if we assume all libraries are using apiFetch, or at minimum using a similar base mechanism for refreshing nonces.

That's right. It's currently also not possible to change the lifetime of specific nonces I think. That would be cool. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just wanted to highlight the possibility, but yes, right now, that's unrealistic.

@ellatrix
Copy link
Member Author

ellatrix commented Aug 6, 2019

@kadamwhite Thanks for the review. So if I understand you correctly, you have no objections?

I also don't really care about the middleware. I can do it if people like, or we can just do it when the need arises.

@ellatrix ellatrix added this to the Gutenberg 6.3 milestone Aug 6, 2019
Copy link
Contributor

@kadamwhite kadamwhite left a comment

Choose a reason for hiding this comment

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

if I understand you correctly, you have no objections?

Sorry for being vague; yes, I approve of the plan :) haven’t tested it locally but the code and direction sound great so I’ll add my +1 here

@ellatrix
Copy link
Member Author

ellatrix commented Aug 9, 2019

Thanks for the review @kadamwhite! Let's iterate if we see a need for response middleware. This is a great step forward in ensure the user doesn't lose their content. :)

@ellatrix ellatrix merged commit be95231 into master Aug 9, 2019
@ellatrix ellatrix deleted the try/better-nonce-refresh branch August 9, 2019 17:35
@ellatrix
Copy link
Member Author

ellatrix commented Aug 9, 2019

I'll help out if this needs some renaming/adjustments on the PHP side when merging into core.

@ellatrix ellatrix changed the title API Fetch: refresh nonces as soon as they expired, then fetch again API Fetch: refresh nonces as soon as they expire, then fetch again Aug 15, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
…16683)

* API Fetch: refresh nonces as soon as they expired, then fetch again

* Use exit()

* Fix PHP linting errors
gziolo pushed a commit that referenced this pull request Aug 29, 2019
…16683)

* API Fetch: refresh nonces as soon as they expired, then fetch again

* Use exit()

* Fix PHP linting errors
@youknowriad
Copy link
Contributor

@ellatrix do you know if this change have been packported into Core (the php change) or whether it's included in the last patch here https://core.trac.wordpress.org/ticket/47843

cc @gziolo

@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 13, 2019
@gziolo
Copy link
Member

gziolo commented Sep 14, 2019

@ellatrix
Copy link
Member Author

I'll try to look today, if not tomorrow (Monday).

@ellatrix
Copy link
Member Author

@gziolo The package hasn't been updated yet in core so it wouldn't have been adjusted it the core repository yet.

@ellatrix
Copy link
Member Author

I'll update you patch https://core.trac.wordpress.org/ticket/47843

@gziolo
Copy link
Member

gziolo commented Sep 15, 2019

I'll update you patch https://core.trac.wordpress.org/ticket/47843

Awesome, thank you for your help 💯

nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Sep 23, 2019
This is a follow up to #47843, implementing a PHP endpoint and inline scripts 
after the editor package updates. The action was originally added in
WordPress/gutenberg#16683.

Fixes #48076.




git-svn-id: https://develop.svn.wordpress.org/trunk@46253 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Sep 23, 2019
This is a follow up to #47843, implementing a PHP endpoint and inline scripts 
after the editor package updates. The action was originally added in
WordPress/gutenberg#16683.

Fixes #48076.




git-svn-id: https://develop.svn.wordpress.org/trunk@46253 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 23, 2019
This is a follow up to #47843, implementing a PHP endpoint and inline scripts 
after the editor package updates. The action was originally added in
WordPress/gutenberg#16683.

Fixes #48076.



Built from https://develop.svn.wordpress.org/trunk@46253


git-svn-id: http://core.svn.wordpress.org/trunk@46065 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Sep 23, 2019
This is a follow up to #47843, implementing a PHP endpoint and inline scripts 
after the editor package updates. The action was originally added in
WordPress/gutenberg#16683.

Fixes #48076.



Built from https://develop.svn.wordpress.org/trunk@46253


git-svn-id: https://core.svn.wordpress.org/trunk@46065 1a063a9b-81f0-0310-95a4-ce76da25c4cd
miya0001 pushed a commit to cjk4wp/wordpress that referenced this pull request Oct 26, 2019
This is a follow up to #47843, implementing a PHP endpoint and inline scripts 
after the editor package updates. The action was originally added in
WordPress/gutenberg#16683.

Fixes #48076.




git-svn-id: http://develop.svn.wordpress.org/trunk@46253 602fd350-edb4-49c9-b593-d223f7449a82
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] API fetch /packages/api-fetch REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants