-
Notifications
You must be signed in to change notification settings - Fork 4.6k
api-fetch: clean up error handling #71458
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
Conversation
|
Size Change: -82 B (0%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
| // If the nonce refresh fails, it means we failed to recover from the original | ||
| // `rest_cookie_invalid_nonce` error and that it's time to finally re-throw it. | ||
| if ( ! response.ok ) { | ||
| return Promise.reject( error ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker for this refactoring, but the error will never be rethrown here. The response returned by the nonce refresh endpoint is always OK, even when "refreshed" nonces don't work.
I spent some time trying to fix it last year. Here's the failed result (#67812).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I was testing this, it was easy to get an error response from the endpoint. Just send a request that is not authenticated (or incorrectly authenticated):
$ curl -i 'http://localhost:9001/wp-admin/admin-ajax.php?action=rest-nonce'
HTTP/1.1 400 Bad Request
Date: Tue, 02 Sep 2025 11:18:52 GMT
Server: Apache/2.4.65 (Debian)
X-Powered-By: PHP/8.2.29
X-Robots-Tag: noindex
X-Content-Type-Options: nosniff
Expires: Wed, 11 Jan 1984 05:00:00 GMT
Cache-Control: no-cache, must-revalidate, max-age=0, no-store, private
Referrer-Policy: strict-origin-when-cross-origin
X-Frame-Options: SAMEORIGIN
Content-Length: 1
Connection: close
Content-Type: text/html; charset=UTF-8
0%
There is a 400 error and 0 in the body. Sending a similar request with a bad wordpress_logged_in cookie had the same result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this works when the server actually fails.
The problem I mentioned is related to partially tokens "corrupted tokens, nonce refreshing works, but the returned value then fails subsequent requests and apiFetch gets stuck in the loop. Repro steps from #67812 still apply.
Again, this isn't a blocker here, just something I wanted to cross-reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it now 🙂 wp_create_nonce supports creating nonces also for logged-out, anonymous sessions. So, when your wordress_logged_in cookie is expired, then wp_get_current_user() returns 0, wp_get_session_token() returns '', the session is anonymous. But the admin-ajax.php?action=rest-nonce will happily return a nonce, it's just for a logged-out user.
IMO there shouldn't be a nonce refresh infinite loop: the REST request should be also processed anonymously, which often returns a result, and the anonymous session and anonymous nonce should match.
Nonce refresh loop can happen when there is a mismatch between the wordpress_logged_in and the wordpress auth cookies. Then the REST request can be authenticated (uses only wordpress_logged_in) and the admin-ajax.php is not authenticated (uses also wordpress, scoped to /wp-admin path).
I think it's a bug that admin-ajax.php?action=rest-nonce returns a nonce also for an anonymous user. This is completely useless in a WP Admin/REST API context. It's a feature of wp_create_nonce that's probably useful in other use cases, but for rest-nonce we should disable it. WP Admin user is always authenticated.
I didn't read the entire discussion and feedback in #67812, but it looks like something that's definitely worth trying again. I think I know enough about WordPress auth to help get it through the finish line 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, happy to collaborate on that resolution @jsnajdr!
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| } | ||
|
|
||
| throw response; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checkStatus function is identical to what native response.ok does. We can replace it completely. Also note that it's synchronous.
| } | ||
|
|
||
| const responsePromise = window.fetch( | ||
| const responsePromise = globalThis.fetch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm replacing window with the more universal globalThis. Not that useful right away, but good for potential compatibility with web workers or Node.js.
| } | ||
|
|
||
| return parseResponseAndNormalizeError( response, parse ); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's been a lot of promises that can be replaced with synchronous code. value was not a promise, it's the Response object. No need to turn it into a promise. The handler can be converted into a simple if/else block. If the response is OK, return response, if it's not OK, extract the error from the response (it can be a JSON body of a 4xx response, for example) and throw the object as an error.
| } | ||
|
|
||
| return response.text(); | ||
| } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the error thrown when nonce refresh fails. The workflow is:
- Try to
fetchfrom the original endpoint, e.g.,/wp/v2/posts - If the response is
rest_cookie_invalid_nonce, we fetch a new nonce from/wp-admin/admin-ajax.php?action=rest-nonce - This
admin-ajax.phpis very traditional, it doesn't return a JSON, but a text response with the new nonce. - If the nonce refresh fails, it's a 400 error with a text body that contains
0. - If the nonce refresh succeeds, we try the original fetch again.
Now, the original code, on nonce refresh failure, threw the Response object with the { status: 200, text: '0' } content! Because that's what the checkStatus helper throws. For an apiFetch( { path: '/wp/v2/posts' } ) call this doesn't make any sense. It's much better to throw the original rest_cookie_invalid_nonce error. Because it comes from a /wp/v2/posts response. We failed to recover from the error, so we have to throw it to the caller.
I added two new unit tests that test for this. One of them will fail without this change.
| .catch( ( response: Response ) => { | ||
| // `response` could actually be an error thrown by `defaultFetchHandler`. | ||
| if ( ! response.headers ) { | ||
| if ( ! ( response instanceof globalThis.Response ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apiFetch with parse: false throws either a Response or a regular Error. Seems to me that instanceOf is a better check than checking presence of .headers.
| // Reset the `apiFetch` module before each test to clear | ||
| // internal variables (middlewares, fetch handler, etc.). | ||
| jest.resetModules(); | ||
| apiFetch = ( await import( '../' ) ).default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I load a new instance of apiFetch module for every test, so that internal variables (middlewares, fetch handler) are reset to initial values for each test.
| // options object. It also assumes that some built-in middleware would | ||
| // either mutate or clone the original options if the extra middleware | ||
| // had been pushed to the stack. | ||
| expect.assertions( 1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is now isolated from other tests, so it doesn't need to run as the last test any more.
It also needs its own setFetchHandler call, because it no longer gets one for free from the previous (custom fetch handler) test 🙂
| } | ||
|
|
||
| return response; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now fully inlined into parseResponseAndNormalizeError.
| ) => { | ||
| return Promise.resolve( | ||
| parseResponse( response, shouldParseResponse ) | ||
| ).catch( ( res ) => parseAndThrowError( res, shouldParseResponse ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code didn't make much sense. parseResponse can fail only when response.json() throws an error. But then parseJsonAndNormalizeError will detect that it's not a real Response (doesn't have a .json field) and will throw invalid_json error. Instead of this, we can just call parseJsonAndNormalizeError right away.
| }; | ||
|
|
||
| throw error || unknownError; | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseJsonAndNormalizeError always either succeeds or throws an invalid_json error. This catch handler is redundant, the unknown_error is never thrown.
|
Flaky tests detected in c74cc48. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17401278360
|
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup, @jsnajdr!
|
Great cleanup, @jsnajdr! |
This PR (and also #71439) is inspired by @mcsf comment in #67141 (comment):
Here I'm trying to deliver some love for
api-fetch🙂defaultFetchHandler. The current code is a bit chaotic: using promises in synchronous code, doing some unnecessary check or doing them twice...I'll be adding much more detail in inline comments.