Repository navigation
Fix /token and /revoke OAuth error codes - #3743
mangeshraut712 wants to merge 1 commit into
Conversation
Return unauthorized_client when a client is not registered for a grant the server implements, unsupported_grant_type for any other grant, and invalid_client with a Basic challenge when HTTP Basic authentication fails.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3723. If a maintainer assigns you to #3723, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
Fixes #3723
/tokenand/revokenow use the error codes in RFC 6749 §5.2. RFC 7009 §2.2.1 reuses that presentation for revocation.unauthorized_client.client_credentials,password, or anything else outsidegrant_types_supported) isunsupported_grant_type, decided before the request union is validated, so the body is not a pydantic error./revokeisinvalid_client, the same code/tokenalready returned.Authorization: Basic, the 401 includesWWW-Authenticate: Basic realm="OAuth". RFC 7617 §2 requires the realm on a Basic challenge.client_secret_postfailures do not get the header, because the client did not use the Authorization field.The jwt-bearer path no longer has a separate "grant disabled" branch. That grant is outside
grant_types_supportedunless identity assertion is enabled, so the early check already returnsunsupported_grant_type.Motivation and Context
Clients that branch on these codes were reacting to the wrong condition. The two grant errors were swapped,
/revokestill reported failed authentication asunauthorized_client, and a failed Basic authentication had noWWW-Authenticateheader.How Has This Been Tested?
uv run --frozen ruff check .anduv run --frozen ruff format --check .are clean.uv run --frozen pyright: 0 errors, 0 warnings../scripts/test: 6223 passed, 8 skipped, 1 xfailed, coverage 100%, strict-no-cover clean.tests/server/auth/test_error_handling.pycover the four rows in the issue.test_client_without_the_grant_registered_is_rejectednow expectsunauthorized_client.Breaking Changes
Successful token and revocation responses are unchanged. Clients or tests that matched the old error codes will see the spec codes instead:
unauthorized_clientrather thanunsupported_grant_typewhen a registered client is refused a supported grant,unsupported_grant_typerather thaninvalid_requestforclient_credentialsandpassword, andinvalid_clientrather thanunauthorized_clientwhen/revokeauthentication fails.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
Prepared with AI assistance.
Happy to adjust the realm value or the error descriptions if you'd prefer different wording.