thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.49k stars 1.12k forks source link

Issue with redirect_uri in Authorization Code Grant #1418

Open ls-sean-fraser opened 1 week ago

ls-sean-fraser commented 1 week ago

Using version 9.0.0, using authorization code grant without specifying a redirect_uri in both requests does not seem to be accepted. The spec indicates these are only required if used in both places.

redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in Section 4.1.1, and their
         values MUST be identical.

When using a default Authorization code entity...

class OauthAuthorizationCode implements AuthCodeEntityInterface
{
    use EntityTrait;
    use TokenEntityTrait;
    use AuthCodeTrait;
}

If the redirect_uri is omitted from the authorize request, the authorization code contains a redirect_uri of null.

The access token call then fail here, As the value is null, but it is checking for empty string

https://github.com/thephpleague/oauth2-server/blob/master/src/Grant/AuthCodeGrant.php#L220-L224

if ($authCodePayload->redirect_uri !== '' && $redirectUri === null) {
     throw OAuthServerException::invalidRequest('redirect_uri');
}

Forcing the null redirect_uri to be empty string in the entity doesn't resolve the issue as the subsequent check which will fail to compare redirect_uri of the code and the request, as '' !== null.

I suspect that the check above should be changed to be:

if ($authCodePayload->redirect_uri !== null && $redirectUri === null) { 
Sephster commented 1 week ago

Have you got a redirect Uri registered against your client?

ls-sean-fraser commented 1 week ago

Yes, my ClientEntityInterface implementation returns a value for the client. The redirect itself works fine, it is only validation of it during the code exchange that is the problem

Sephster commented 3 days ago

Thanks for reporting this. I think I have a solution figured out. I'm going to change the code so we validate that if a redirect URI is passed, it must be a valid URI as per the specs. That negates the need to check for non empty strings as we shouldn't be accepting these in the first place.

I'll finalise this tomorrow and submit a fix. Thank you