Repository: thephpleague/oauth2-server (php)

Issue:
TypeError in mallformed tokens
I'm using https://github.com/mezzio/mezzio-authentication-oauth2 and noticed thousands of 500 errors on our server.
(Someone tried to access something he should not but I don't expect this bug a security issue.)

The problem happens if someone passes a token not as string.

E.g:
```php
        $data = [
            'grant_type'    => 'refresh_token',
            'refresh_token' => ['not', 'a', 'string'],
            'client_id'     => $ssoClient['client_id'],
            'client_secret' => $ssoClient['client_secret'],
        ];
        $I->sendPOST('/oauth2/token', $data);
        $I->seeResponseCodeIs(401);
```
```
TypeError raised in file /app/vendor/defuse/php-encryption/src/Crypto.php line 136:
Message: String expected for argument 1. Array given instead.
Stack Trace:
#0 /app/vendor/league/oauth2-server/src/CryptTrait.php(69): Defuse\Crypto\Crypto::decryptWithPassword(Array, '/app/config/aut...')
#1 /app/vendor/league/oauth2-server/src/Grant/RefreshTokenGrant.php(107): League\OAuth2\Server\Grant\AbstractGrant->decrypt(Array)
#2 /app/vendor/league/oauth2-server/src/Grant/RefreshTokenGrant.php(47): League\OAuth2\Server\Grant\RefreshTokenGrant->validateOldRefreshToken(Object(Laminas\Diactoros\ServerRequest), 'accounts.fronte...')
#3 /app/vendor/league/oauth2-server/src/AuthorizationServer.php(198): League\OAuth2\Server\Grant\RefreshTokenGrant->respondToAccessTokenRequest(Object(Laminas\Diactoros\ServerRequest), Object(League\OAuth2\Server\ResponseTypes\BearerTokenResponse), Object(DateInterval))
#4 /app/vendor/mezzio/mezzio-authentication-oauth2/src/TokenEndpointHandler.php(71): League\OAuth2\Server\AuthorizationServer->respondToAccessTokenRequest(Object(Laminas\Diactoros\ServerRequest), Object(Laminas\Diactoros\Response))
```

The bug seems to be fixed with but other grant types are very likely effected, too
```php
        if (!\is_string($encryptedRefreshToken)) {
            throw OAuthServerException::invalidRefreshToken('Refresh token mallformed');
        }
```


Pull request:
Validate expected type of input parameter
fixes #1209

Patch:
diff --git a/src/Grant/AbstractGrant.php b/src/Grant/AbstractGrant.php
index c4797292..f4239170 100644
--- a/src/Grant/AbstractGrant.php
+++ b/src/Grant/AbstractGrant.php
@@ -191,6 +191,10 @@ abstract class AbstractGrant implements GrantTypeInterface
         $redirectUri = $this->getRequestParameter('redirect_uri', $request, null);
 
         if ($redirectUri !== null) {
+            if (!\is_string($redirectUri)) {
+                throw OAuthServerException::invalidRequest('redirect_uri');
+            }
+
             $this->validateRedirectUri($redirectUri, $client, $request);
         }
 
@@ -238,12 +242,16 @@ abstract class AbstractGrant implements GrantTypeInterface
 
         $clientId = $this->getRequestParameter('client_id', $request, $basicAuthUser);
 
-        if (\is_null($clientId)) {
+        if (!\is_string($clientId)) {
             throw OAuthServerException::invalidRequest('client_id');
         }
 
         $clientSecret = $this->getRequestParameter('client_secret', $request, $basicAuthPassword);
 
+        if ($clientSecret !== null && !\is_string($clientSecret)) {
+            throw OAuthServerException::invalidRequest('client_secret');
+        }
+
         return [$clientId, $clientSecret];
     }
 
@@ -287,10 +295,16 @@ abstract class AbstractGrant implements GrantTypeInterface
      */
     public function validateScopes($scopes, $redirectUri = null)
     {
-        if (!\is_array($scopes)) {
+        if ($scopes === null) {
+            $scopes = [];
+        } elseif (\is_string($scopes)) {
             $scopes = $this->convertScopesQueryStringToArray($scopes);
         }
 
+        if (!\is_array($scopes)) {
+            throw OAuthServerException::invalidRequest('scope');
+        }
+
         $validScopes = [];
 
         foreach ($scopes as $scopeItem) {
@@ -313,7 +327,7 @@ abstract class AbstractGrant implements GrantTypeInterface
      *
      * @return array
      */
-    private function convertScopesQueryStringToArray($scopes)
+    private function convertScopesQueryStringToArray(string $scopes)
     {
         return \array_filter(\explode(self::SCOPE_DELIMITER_STRING, \trim($scopes)), function ($scope) {
             return !empty($scope);
diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php
index 5cce97e5..474d1026 100644
--- a/src/Grant/AuthCodeGrant.php
+++ b/src/Grant/AuthCodeGrant.php
@@ -106,7 +106,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant
 
         $encryptedAuthCode = $this->getRequestParameter('code', $request, null);
 
-        if ($encryptedAuthCode === null) {
+        if (!\is_string($encryptedAuthCode)) {
             throw OAuthServerException::invalidRequest('code');
         }
 
@@ -260,6 +260,10 @@ class AuthCodeGrant extends AbstractAuthorizeGrant
         $redirectUri = $this->getQueryStringParameter('redirect_uri', $request);
 
         if ($redirectUri !== null) {
+            if (!\is_string($redirectUri)) {
+                throw OAuthServerException::invalidRequest('redirect_uri');
+            }
+
             $this->validateRedirectUri($redirectUri, $client, $request);
         } elseif (empty($client->getRedirectUri()) ||
             (\is_array($client->getRedirectUri()) && \count($client->getRedirectUri()) !== 1)) {
diff --git a/src/Grant/ImplicitGrant.php b/src/Grant/ImplicitGrant.php
index 17f289bf..21e06ed4 100644
--- a/src/Grant/ImplicitGrant.php
+++ b/src/Grant/ImplicitGrant.php
@@ -120,7 +120,7 @@ class ImplicitGrant extends AbstractAuthorizeGrant
             $this->getServerParameter('PHP_AUTH_USER', $request)
         );
 
-        if (\is_null($clientId)) {
+        if (!\is_string($clientId)) {
             throw OAuthServerException::invalidRequest('client_id');
         }
 
@@ -129,6 +129,10 @@ class ImplicitGrant extends AbstractAuthorizeGrant
         $redirectUri = $this->getQueryStringParameter('redirect_uri', $request);
 
         if ($redirectUri !== null) {
+            if (!\is_string($redirectUri)) {
+                throw OAuthServerException::invalidRequest('redirect_uri');
+            }
+
             $this->validateRedirectUri($redirectUri, $client, $request);
         } elseif (\is_array($client->getRedirectUri()) && \count($client->getRedirectUri()) !== 1
             || empty($client->getRedirectUri())) {
@@ -147,6 +151,10 @@ class ImplicitGrant extends AbstractAuthorizeGrant
 
         $stateParameter = $this->getQueryStringParameter('state', $request);
 
+        if ($stateParameter !== null && !\is_string($stateParameter)) {
+            throw OAuthServerException::invalidRequest('state');
+        }
+
         $authorizationRequest = new AuthorizationRequest();
         $authorizationRequest->setGrantTypeId($this->getIdentifier());
         $authorizationRequest->setClient($client);
diff --git a/src/Grant/PasswordGrant.php b/src/Grant/PasswordGrant.php
index 7579fd0d..db4eab9f 100644
--- a/src/Grant/PasswordGrant.php
+++ b/src/Grant/PasswordGrant.php
@@ -84,13 +84,13 @@ class PasswordGrant extends AbstractGrant
     {
         $username = $this->getRequestParameter('username', $request);
 
-        if (\is_null($username)) {
+        if (!\is_string($username)) {
             throw OAuthServerException::invalidRequest('username');
         }
 
         $password = $this->getRequestParameter('password', $request);
 
-        if (\is_null($password)) {
+        if (!\is_string($password)) {
             throw OAuthServerException::invalidRequest('password');
         }
 
diff --git a/src/Grant/RefreshTokenGrant.php b/src/Grant/RefreshTokenGrant.php
index 19945f5c..837fead8 100644
--- a/src/Grant/RefreshTokenGrant.php
+++ b/src/Grant/RefreshTokenGrant.php
@@ -92,7 +92,7 @@ class RefreshTokenGrant extends AbstractGrant
     protected function validateOldRefreshToken(ServerRequestInterface $request, $clientId)
     {
         $encryptedRefreshToken = $this->getRequestParameter('refresh_token', $request);
-        if (\is_null($encryptedRefreshToken)) {
+        if (!\is_string($encryptedRefreshToken)) {
             throw OAuthServerException::invalidRequest('refresh_token');
         }
 


Test patch:
diff --git a/tests/Grant/AbstractGrantTest.php b/tests/Grant/AbstractGrantTest.php
index 93f95fa5..48307cce 100644
--- a/tests/Grant/AbstractGrantTest.php
+++ b/tests/Grant/AbstractGrantTest.php
@@ -89,6 +89,70 @@ class AbstractGrantTest extends TestCase
         $this->assertSame([null, null], $basicAuthMethod->invoke($grantMock, $serverRequest));
     }
 
+    public function testGetClientCredentialsClientIdNotAString()
+    {
+        $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
+
+        /** @var AbstractGrant $grantMock */
+        $grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
+        $grantMock->setClientRepository($clientRepositoryMock);
+
+        $abstractGrantReflection = new \ReflectionClass($grantMock);
+
+        $serverRequest = new ServerRequest(
+            [],
+            [],
+            null,
+            'POST',
+            'php://input',
+            [],
+            [],
+            [],
+            [
+                'client_id'     => ['not', 'a', 'string'],
+                'client_secret' => 'client_secret',
+            ]
+        );
+        $getClientCredentialsMethod = $abstractGrantReflection->getMethod('getClientCredentials');
+        $getClientCredentialsMethod->setAccessible(true);
+
+        $this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class);
+
+        $getClientCredentialsMethod->invoke($grantMock, $serverRequest, true, true);
+    }
+
+    public function testGetClientCredentialsClientSecretNotAString()
+    {
+        $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
+
+        /** @var AbstractGrant $grantMock */
+        $grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
+        $grantMock->setClientRepository($clientRepositoryMock);
+
+        $abstractGrantReflection = new \ReflectionClass($grantMock);
+
+        $serverRequest = new ServerRequest(
+            [],
+            [],
+            null,
+            'POST',
+            'php://input',
+            [],
+            [],
+            [],
+            [
+                'client_id'     => 'client_id',
+                'client_secret' => ['not', 'a', 'string'],
+            ]
+        );
+        $getClientCredentialsMethod = $abstractGrantReflection->getMethod('getClientCredentials');
+        $getClientCredentialsMethod->setAccessible(true);
+
+        $this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class);
+
+        $getClientCredentialsMethod->invoke($grantMock, $serverRequest, true, true);
+    }
+
     public function testValidateClientPublic()
     {
         $client = new ClientEntity();
@@ -261,6 +325,32 @@ class AbstractGrantTest extends TestCase
         $validateClientMethod->invoke($grantMock, $serverRequest, true, true);
     }
 
+    public function testValidateClientMalformedRedirectUri()
+    {
+        $client = new ClientEntity();
+        $client->setRedirectUri('http://foo/bar');
+        $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
+        $clientRepositoryMock->method('getClientEntity')->willReturn($client);
+
+        /** @var AbstractGrant $grantMock */
+        $grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
+        $grantMock->setClientRepository($clientRepositoryMock);
+
+        $abstractGrantReflection = new \ReflectionClass($grantMock);
+
+        $serverRequest = (new ServerRequest())->withParsedBody([
+            'client_id'    => 'foo',
+            'redirect_uri' => ['not', 'a', 'string'],
+        ]);
+
+        $validateClientMethod = $abstractGrantReflection->getMethod('validateClient');
+        $validateClientMethod->setAccessible(true);
+
+        $this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class);
+
+        $validateClientMethod->invoke($grantMock, $serverRequest, true, true);
+    }
+
     public function testValidateClientBadClient()
     {
         $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php
index a080da31..860c6e86 100644
--- a/tests/Grant/AuthCodeGrantTest.php
+++ b/tests/Grant/AuthCodeGrantTest.php
@@ -1112,6 +1112,44 @@ class AuthCodeGrantTest extends TestCase
         }
     }
 
+    public function testRespondToAccessTokenRequestWithAuthCodeNotAString()
+    {
+        $client = new ClientEntity();
+        $client->setRedirectUri('http://foo/bar');
+
+        $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
+        $clientRepositoryMock->method('getClientEntity')->willReturn($client);
+
+        $grant = new AuthCodeGrant(
+            $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
+            $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
+            new DateInterval('PT10M')
+        );
+
+        $grant->setClientRepository($clientRepositoryMock);
+        $grant->setEncryptionKey($this->cryptStub->getKey());
+
+        $request = new ServerRequest(
+            [],
+            [],
+            null,
+            'POST',
+            'php://input',
+            [],
+            [],
+            [],
+            [
+                'grant_type'   => 'authorization_code',
+                'client_id'    => 'foo',
+                'redirect_uri' => 'http://foo/bar',
+                'code'         => ['not', 'a', 'string'],
+            ]
+        );
+
+        $this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class);
+        $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M'));
+    }
+
     public function testRespondToAccessTokenRequestExpiredCode()
     {
         $client = new ClientEntity();