Merge pull request #1433 from thephpleague/fix-invalid-grant-reply

Issue invalid_grant instead of invalid_request when authcode is not correct
thephpleague · Nov 14, 2024 · f843573 · f843573
2 parents dd22d86 + 66c2a3a
commit f843573
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Support for PHP 8.4 (PR #1454)
 
 ### Fixed
+- In the Auth Code grant, when requesting an access token with an invalid auth code, we now respond with an invalid_grant error instead of invalid_request (PR #1433)
 - Fixed spec compliance issue  where device access token request was mistakenly expecting to receive scopes in the request (PR #1412)
 - Refresh tokens pre version 9 might have had user IDs set as ints which meant they were incorrectly rejected. We now cast these values to strings to allow old refresh tokens (PR #1436)
 

diff --git a/src/CryptTrait.php b/src/CryptTrait.php
@@ -15,8 +15,11 @@
 namespace League\OAuth2\Server;
 
 use Defuse\Crypto\Crypto;
+use Defuse\Crypto\Exception\EnvironmentIsBrokenException;
+use Defuse\Crypto\Exception\WrongKeyOrModifiedCiphertextException;
 use Defuse\Crypto\Key;
 use Exception;
+use InvalidArgumentException;
 use LogicException;
 
 use function is_string;
@@ -64,6 +67,17 @@ protected function decrypt(string $encryptedData): string
             }
 
             throw new LogicException('Encryption key not set when attempting to decrypt');
+        } catch (WrongKeyOrModifiedCiphertextException $e) {
+            $exceptionMessage = 'The authcode or decryption key/password used '
+                . 'is not correct';
+
+            throw new InvalidArgumentException($exceptionMessage, 0, $e);
+        } catch (EnvironmentIsBrokenException $e) {
+            $exceptionMessage = 'Auth code decryption failed. This is likely '
+                . 'due to an environment issue or runtime bug in the '
+                . 'decryption library';
+
+            throw new LogicException($exceptionMessage, 0, $e);
         } catch (Exception $e) {
             throw new LogicException($e->getMessage(), 0, $e);
         }

diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php
@@ -15,6 +15,7 @@
 use DateInterval;
 use DateTimeImmutable;
 use Exception;
+use InvalidArgumentException;
 use League\OAuth2\Server\CodeChallengeVerifiers\CodeChallengeVerifierInterface;
 use League\OAuth2\Server\CodeChallengeVerifiers\PlainVerifier;
 use League\OAuth2\Server\CodeChallengeVerifiers\S256Verifier;
@@ -123,8 +124,10 @@ public function respondToAccessTokenRequest(
                 $authCodePayload->user_id,
                 $authCodePayload->auth_code_id
             );
+        } catch (InvalidArgumentException $e) {
+            throw OAuthServerException::invalidGrant('Cannot validate the provided authorization code');
         } catch (LogicException $e) {
-            throw OAuthServerException::invalidRequest('code', 'Cannot decrypt the authorization code', $e);
+            throw OAuthServerException::invalidRequest('code', 'Issue decrypting the authorization code', $e);
         }
 
         $codeVerifier = $this->getRequestParameter('code_verifier', $request);
@@ -206,7 +209,7 @@ private function validateAuthorizationCode(
         }
 
         if (time() > $authCodePayload->expire_time) {
-            throw OAuthServerException::invalidRequest('code', 'Authorization code has expired');
+            throw OAuthServerException::invalidGrant('Authorization code has expired');
         }
 
         if ($this->authCodeRepository->isAuthCodeRevoked($authCodePayload->auth_code_id) === true) {

diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php
@@ -1621,7 +1621,7 @@ public function testRespondToAccessTokenRequestClientMismatch(): void
         }
     }
 
-    public function testRespondToAccessTokenRequestBadCodeEncryption(): void
+    public function testRespondToAccessTokenRequestBadCode(): void
     {
         $client = new ClientEntity();
 
@@ -1663,15 +1663,71 @@ public function testRespondToAccessTokenRequestBadCodeEncryption(): void
                 'grant_type'   => 'authorization_code',
                 'client_id'    => 'foo',
                 'redirect_uri' => self::REDIRECT_URI,
-                'code'         => 'sdfsfsd',
+                'code'         => 'badCode',
             ]
         );
 
         try {
             /* @var StubResponseType $response */
             $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M'));
         } catch (OAuthServerException $e) {
-            self::assertEquals($e->getHint(), 'Cannot decrypt the authorization code');
+            self::assertEquals($e->getErrorType(), 'invalid_grant');
+            self::assertEquals($e->getHint(), 'Cannot validate the provided authorization code');
+        }
+    }
+
+    public function testRespondToAccessTokenRequestNoEncryptionKey(): void
+    {
+        $client = new ClientEntity();
+
+        $client->setIdentifier('foo');
+        $client->setRedirectUri(self::REDIRECT_URI);
+        $client->setConfidential();
+
+        $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
+
+        $clientRepositoryMock->method('getClientEntity')->willReturn($client);
+        $clientRepositoryMock->method('validateClient')->willReturn(true);
+
+        $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
+        $accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();
+
+        $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
+        $refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf();
+
+        $grant = new AuthCodeGrant(
+            $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
+            $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
+            new DateInterval('PT10M')
+        );
+        $grant->setClientRepository($clientRepositoryMock);
+        $grant->setAccessTokenRepository($accessTokenRepositoryMock);
+        $grant->setRefreshTokenRepository($refreshTokenRepositoryMock);
+        // We deliberately don't set an encryption key here
+
+        $request = new ServerRequest(
+            [],
+            [],
+            null,
+            'POST',
+            'php://input',
+            [],
+            [],
+            [],
+            [
+            'grant_type'   => 'authorization_code',
+            'client_id'    => 'foo',
+            'redirect_uri' => self::REDIRECT_URI,
+            'code'         => 'badCode',
+            ]
+        );
+
+        try {
+            /* @var StubResponseType $response */
+            $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M'));
+        } catch (OAuthServerException $e) {
+            self::assertEquals($e->getErrorType(), 'invalid_request');
+            self::assertEquals($e->getHint(), 'Issue decrypting the authorization code');
         }
     }