Repository navigation
Return RFC 6749 error fields from the token endpoint - #89
Merged
Merged
Conversation
RFC 6749 section 5.2 says a token endpoint error is a JSON object with a top-level "error" code such as invalid_grant, plus an optional "error_description". The endpoint returned the WordPress error shape instead ({"code","message","data"}), so a standard OAuth client could not tell why a request failed.
A rest_request_after_callbacks filter on the token route now adds "error" and "error_description" to every error. That filter also sees argument validation errors, such as an unknown grant_type, which happen before the endpoint callback runs. rest_post_dispatch would work in production too, but WP_REST_Server::dispatch() does not run it, so it would be untested.
The WordPress fields are kept alongside the new ones, so existing clients that read "code" keep working. Replacing the body outright would be closer to the RFC but would break them.
Error codes come from a map of the endpoint's known WP_Error codes. An "error" key in the WP_Error data takes precedence, so new errors can set their OAuth code where they are created. The PKCE errors in WP-API#85 already do this. Unknown errors become server_error with a 500 status. Every other error is sent as a 400, except invalid_client, which keeps its 401 when the client failed to authenticate. A 401 now carries a WWW-Authenticate: Basic challenge, as section 5.2 requires. The resource server's Bearer challenge still stays off this route, so the WWW-Authenticate test now checks for the Basic challenge instead of no header.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Dispatched token errors are not formatted, and server_error can retain an incorrect status.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Updates token endpoint errors with RFC 6749 OAuth fields while preserving WordPress fields, status handling, and authentication challenges.
Changes:
- Maps token errors to OAuth error codes.
- Adds RFC-aligned statuses and Basic authentication challenges.
- Expands token endpoint and header tests.
| File | Summary |
|---|---|
tests/test-www-authenticate.php |
Tests Basic authentication challenge behavior. |
tests/test-token-endpoint.php |
Tests OAuth error responses. |
inc/endpoints/namespace.php |
Registers token endpoint hooks. |
inc/endpoints/class-token.php |
Formats OAuth errors and statuses; contains unresolved response-hook and server_error status issues. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
An unknown error that carried a 4xx status became server_error but kept its 4xx status, so the response said "server error" with a client-error status. Unknown errors now pick their OAuth code from the status: a 4xx becomes invalid_request with a 400, and anything else becomes server_error. server_error is always sent as a 500, including when an error sets it through its data. Forcing every unknown error to 500 was the other option, but an error that already says it is the client's fault should not be reported as a server failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
joehoyle
approved these changes
Oct 7, 2026
roborourke
added a commit
to humanmade/OAuth2
that referenced
this pull request
Oct 7, 2026
Upstream WP-API#89 added a token endpoint filter that turns WP_Error responses into RFC 6749 section 5.2 errors. It uses the `error` key in the error data when present, otherwise a fixed map, and falls back to `invalid_request` for any other 4xx. The PKCE verifier errors already set `error => invalid_grant`, so they come through unchanged. The fail-closed error for a stored challenge with no stored method did not, so after the merge it would have surfaced as `invalid_request`. The problem is the authorization code, not the request, so it now sets `invalid_grant`, matching how WP-API#89 maps the other corrupted-code error (`get_user.invalid_data`). The array-valued verifier test only checked for a 400. With WP-API#89 in place the schema rejection becomes a proper `invalid_request` OAuth error, so the test now asserts that with the shared helper. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
roborourke
added a commit
to humanmade/OAuth2
that referenced
this pull request
Oct 7, 2026
…th-check Upstream WP-API#89 added a token endpoint error formatter that adds the RFC 6749 section 5.2 error fields to every WP_Error from the token route, and sends a `Basic realm="oauth2"` challenge with every 401. This branch built its own challenge response, but only when the client authenticated with the Authorization header. The formatter now owns the challenge. client_authentication_failed() always returns the plain WP_Error, so header and body failures both get the error fields and the same challenge. Before, a header failure came back as a pre-built WP_REST_Response that the formatter skipped, so it had a different realm and no `error` field. As a result, a failed body authentication now gets a challenge too. RFC 6749 allows this, and RFC 7235 requires a challenge on every 401, so the body-auth test now expects it. Both sides added a request_token() test helper. Upstream's generic version keeps the name. This branch's code-exchange helper is renamed to exchange_code(). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
roborourke
added a commit
to humanmade/OAuth2
that referenced
this pull request
Oct 7, 2026
Upstream WP-API#89 sends every token endpoint error except a 401 invalid_client as a 400, as RFC 6749 section 5.2 requires. Reusing an unknown code used to return 404, so the burn test's retry assertion broke after the merge and failed across the whole CI matrix. The retry exists to prove the code was deleted, so it fails as an unknown code rather than as a bad verifier. The status no longer tells those apart, so the test now checks the invalid_grant OAuth error and the unknown-code WP_Error code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Token endpoint errors now include the
erroranderror_descriptionfields from RFC 6749 section 5.2.Before this change, errors used the WordPress shape only:
code,messageanddata. A standard OAuth client looks for a top-levelerrorlikeinvalid_grant, so it could not tell why a request failed. The WordPress fields stay, so clients that readcodekeep working.Each known error maps to an OAuth code. For example, an unknown client gives
invalid_client, an expired code givesinvalid_grant, and an unknowngrant_typegivesunsupported_grant_type. An error can also set its own code with anerrorkey in its data. The PKCE errors in #85 already do this, so they will be covered once both PRs merge. Unknown errors becomeserver_error.Errors now use status 400, as the RFC asks. The exceptions are
server_error, which stays 500, and a failed client login, which stays 401. That 401 now sends aWWW-Authenticate: Basicheader.Copilot flagged this on #85. It predates that PR, so the fix is here.
🤖 Generated with Claude Code