Require confidential clients to authenticate on the authorization_code grant - #83
Open
roborourke wants to merge 3 commits into
Open
roborourke wants to merge 3 commits into
roborourke wants to merge 3 commits into
Conversation
The token endpoint calls check_secret() and is_client_credentials_enabled() on whatever get_client() returns, but neither method was on ClientInterface and PersonalClient implemented neither. A POST to /oauth2/access_token with grant_type=client_credentials and client_id=__personal_access_token reached handle_client_credentials(), called an undefined method on the singleton and fataled. That is an unauthenticated 500 on a public endpoint. Put both methods on the interface, along with requires_secret() which the authorization_code grant needs next, so the endpoint can rely on them. Give PersonalClient implementations that all return false: it has no secret, it is issued to a user rather than a client, and it must never authenticate as one. Adding methods to a published interface breaks any third-party class that implements it. That is the lesser evil here, since such a class already fatals on this path; failing at declaration time is louder but honest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The admin form has always asked whether a client is Private or Public, and described Private with the RFC 6749 section 2.1 wording for a confidential client. Nothing ever read the answer back: get_type() had exactly one caller, the code that re-populates that same form. Expose the answer as requires_secret(), taken from rmccue's WIP in WP-API#36. Only an explicit 'private' counts. Clients created before the meta existed return '', and the test suite's own helper stores 'web', so keying off anything looser would lock out installs that never made the choice. Make it filterable on 'oauth2.client.requires_secret' so a site can demand authentication from clients the admin never marked Private, or exempt one client while an integration is updated. Enforcing this is a breaking change for anyone whose Private client never sent its secret, and the escape hatch should land with it rather than after the first support ticket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The grant resolved client_id and code, then went straight to validating the code and issuing a token. A client_secret sent in the body was accepted as a request parameter and never read. Anyone holding a leaked or intercepted code could redeem it without the secret, which is what RFC 6749 section 4.1.3 requires the server to prevent for a confidential client. Check the secret when the client requires one, before the code is looked up, so a failed attempt cannot consume or probe a code. Public clients still exchange codes with no secret, which section 4.1.3 also allows, so this only bites clients an admin explicitly marked Private. A Private client whose stored secret is empty now always fails. That is a broken registration rather than a supported configuration, and failing it closed beats hash_equals( '', '' ) returning true for an empty submission. Reuse the error the client_credentials grant already returns rather than adding a third failure shape: one invalid_client code, 401, and no hint about which half of the credentials was wrong. Both paths now also send WWW-Authenticate when the client authenticated with the Authorization header, per section 5.2. WP_Error cannot carry a header, so those responses go through rest_convert_error_to_response() to get one. Finally, read the Basic header for a secret even when client_id arrived in the body, provided the header names the same client. Before, the header was only consulted when client_id was absent, so a client splitting its credentials across the two would have been rejected for sending no secret at all. Co-Authored-By: Claude Opus 5 <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
1 participant
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.
Builds on #36. Closes #27.
The admin form asks if a client is Private or Public. Nothing ever read the answer.
get_type()had one caller: the code that fills that same form back in.So the
authorization_codegrant never checked the client. It looked up the client ID and the code, then issued a token. Aclient_secretsent by the client was ignored. Anyone with a stolen code could swap it for a token.This adds
Client::requires_secret()from #36 and makes the token endpoint act on it.What changed:
invalid_clienterror theclient_credentialsgrant already returns. It doesn't say which half of the credentials was wrong.WWW-Authenticateheader when the client used the Authorization header. RFC 6749 section 5.2 asks for this.client_idcame from the body, as long as the header names the same client.check_secret()andis_client_credentials_enabled()move ontoClientInterface.PersonalClienthad neither, soclient_credentialswithclient_id=__personal_access_tokenfataled. That was an unauthenticated 500.Breaking change: a Private client that never sent its secret will now get a 401. Filter
oauth2.client.requires_secretto control this per client.Note for reviewers: adding methods to
ClientInterfacebreaks any third-party class that implements it. Those classes already fatal on theclient_credentialspath today.23 new tests. 156 pass on single site and multisite, phpcs clean.
🤖 Generated with Claude Code