Skip to content

Commit

Permalink
Merge pull request #744 from erickjth/fix-pkce-implementation
Browse files Browse the repository at this point in the history 
Fix PKCE implementation with strict verifications
  • Loading branch information
@Sephster
Sephster authored Feb 18, 2018
2 parents bcd2fc3 + e0cc5ee commit e04b8f4
Show file tree
Hide file tree
Showing 3 changed files with 174 additions and 18 deletions.
1 change: 1 addition & 0 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
language: php

dist: trusty
sudo: false

cache:
Expand Down
25 changes: 18 additions & 7 deletions src/Grant/AuthCodeGrant.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,15 @@ public function respondToAccessTokenRequest(
throw OAuthServerException::invalidRequest('code_verifier');
}

// Validate code_verifier according to RFC-7636
// @see: https://tools.ietf.org/html/rfc7636#section-4.1
if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeVerifier) !== 1) {
throw OAuthServerException::invalidRequest(
'code_verifier',
'Code Verifier must follow the specifications of RFC-7636.'
);
}

switch ($authCodePayload->code_challenge_method) {
case 'plain':
if (hash_equals($codeVerifier, $authCodePayload->code_challenge) === false) {
Expand Down Expand Up @@ -270,13 +279,6 @@ public function validateAuthorizationRequest(ServerRequestInterface $request)
throw OAuthServerException::invalidRequest('code_challenge');
}

if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeChallenge) !== 1) {
throw OAuthServerException::invalidRequest(
'code_challenge',
'The code_challenge must be between 43 and 128 characters'
);
}

$codeChallengeMethod = $this->getQueryStringParameter('code_challenge_method', $request, 'plain');
if (in_array($codeChallengeMethod, ['plain', 'S256'], true) === false) {
throw OAuthServerException::invalidRequest(
Expand All @@ -285,6 +287,15 @@ public function validateAuthorizationRequest(ServerRequestInterface $request)
);
}

// Validate code_challenge according to RFC-7636
// @see: https://tools.ietf.org/html/rfc7636#section-4.2
if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeChallenge) !== 1) {
throw OAuthServerException::invalidRequest(
'code_challenged',
'Code challenge must follow the specifications of RFC-7636.'
);
}

$authorizationRequest->setCodeChallenge($codeChallenge);
$authorizationRequest->setCodeChallengeMethod($codeChallengeMethod);
}
Expand Down
166 changes: 155 additions & 11 deletions tests/Grant/AuthCodeGrantTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class AuthCodeGrantTest extends TestCase
*/
protected $cryptStub;

const CODE_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk';

const CODE_CHALLENGE = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM';

public function setUp()
{
$this->cryptStub = new CryptTraitStub;
Expand Down Expand Up @@ -185,7 +189,7 @@ public function testValidateAuthorizationRequestCodeChallenge()
'response_type' => 'code',
'client_id' => 'foo',
'redirect_uri' => 'http://foo/bar',
'code_challenge' => str_repeat('A', 43),
'code_challenge' => self::CODE_CHALLENGE,
]
);

Expand Down Expand Up @@ -686,7 +690,7 @@ public function testRespondToAccessTokenRequestCodeChallengePlain()
'grant_type' => 'authorization_code',
'client_id' => 'foo',
'redirect_uri' => 'http://foo/bar',
'code_verifier' => 'foobar',
'code_verifier' => self::CODE_VERIFIER,
'code' => $this->cryptStub->doEncrypt(
json_encode(
[
Expand All @@ -696,7 +700,7 @@ public function testRespondToAccessTokenRequestCodeChallengePlain()
'user_id' => 123,
'scopes' => ['foo'],
'redirect_uri' => 'http://foo/bar',
'code_challenge' => 'foobar',
'code_challenge' => self::CODE_VERIFIER,
'code_challenge_method' => 'plain',
]
)
Expand Down Expand Up @@ -744,10 +748,6 @@ public function testRespondToAccessTokenRequestCodeChallengeS256()
$grant->setRefreshTokenRepository($refreshTokenRepositoryMock);
$grant->setEncryptionKey($this->cryptStub->getKey());

// [RFC 7636] Appendix B. Example for the S256 code_challenge_method
$codeVerifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk';
$codeChallenge = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM';

$request = new ServerRequest(
[],
[],
Expand All @@ -761,7 +761,7 @@ public function testRespondToAccessTokenRequestCodeChallengeS256()
'grant_type' => 'authorization_code',
'client_id' => 'foo',
'redirect_uri' => 'http://foo/bar',
'code_verifier' => $codeVerifier,
'code_verifier' => self::CODE_VERIFIER,
'code' => $this->cryptStub->doEncrypt(
json_encode(
[
Expand All @@ -771,7 +771,7 @@ public function testRespondToAccessTokenRequestCodeChallengeS256()
'user_id' => 123,
'scopes' => ['foo'],
'redirect_uri' => 'http://foo/bar',
'code_challenge' => $codeChallenge,
'code_challenge' => self::CODE_CHALLENGE,
'code_challenge_method' => 'S256',
]
)
Expand Down Expand Up @@ -1204,7 +1204,7 @@ public function testRespondToAccessTokenRequestBadCodeVerifierPlain()
'grant_type' => 'authorization_code',
'client_id' => 'foo',
'redirect_uri' => 'http://foo/bar',
'code_verifier' => 'nope',
'code_verifier' => self::CODE_VERIFIER,
'code' => $this->cryptStub->doEncrypt(
json_encode(
[
Expand Down Expand Up @@ -1298,7 +1298,151 @@ public function testRespondToAccessTokenRequestBadCodeVerifierS256()
/* @var StubResponseType $response */
$grant->respondToAccessTokenRequest($request, new StubResponseType(), new \DateInterval('PT10M'));
} catch (OAuthServerException $e) {
$this->assertEquals($e->getHint(), 'Failed to verify `code_verifier`.');
$this->assertEquals($e->getHint(), 'Code Verifier must follow the specifications of RFC-7636.');
}
}

public function testRespondToAccessTokenRequestMalformedCodeVerifierS256WithInvalidChars()
{
$client = new ClientEntity();
$client->setIdentifier('foo');
$client->setRedirectUri('http://foo/bar');
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
$refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf();
$refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity());

$grant = new AuthCodeGrant(
$this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
$this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
new \DateInterval('PT10M')
);
$grant->enableCodeExchangeProof();
$grant->setClientRepository($clientRepositoryMock);
$grant->setAccessTokenRepository($accessTokenRepositoryMock);
$grant->setRefreshTokenRepository($refreshTokenRepositoryMock);
$grant->setScopeRepository($scopeRepositoryMock);
$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_verifier' => 'dqX7C-RbqjHYtytmhGTigKdZCXfxq-+xbsk9_GxUcaE', // Malformed code. Contains `+`.
'code' => $this->cryptStub->doEncrypt(
json_encode(
[
'auth_code_id' => uniqid(),
'expire_time' => time() + 3600,
'client_id' => 'foo',
'user_id' => 123,
'scopes' => ['foo'],
'redirect_uri' => 'http://foo/bar',
'code_challenge' => self::CODE_CHALLENGE,
'code_challenge_method' => 'S256',
]
)
),
]
);

try {
/* @var StubResponseType $response */
$grant->respondToAccessTokenRequest($request, new StubResponseType(), new \DateInterval('PT10M'));
} catch (OAuthServerException $e) {
$this->assertEquals($e->getHint(), 'Code Verifier must follow the specifications of RFC-7636.');
}
}

public function testRespondToAccessTokenRequestMalformedCodeVerifierS256WithInvalidLength()
{
$client = new ClientEntity();
$client->setIdentifier('foo');
$client->setRedirectUri('http://foo/bar');
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
$refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf();
$refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity());

$grant = new AuthCodeGrant(
$this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
$this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
new \DateInterval('PT10M')
);
$grant->enableCodeExchangeProof();
$grant->setClientRepository($clientRepositoryMock);
$grant->setAccessTokenRepository($accessTokenRepositoryMock);
$grant->setRefreshTokenRepository($refreshTokenRepositoryMock);
$grant->setScopeRepository($scopeRepositoryMock);
$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_verifier' => 'dqX7C-RbqjHY', // Malformed code. Invalid length.
'code' => $this->cryptStub->doEncrypt(
json_encode(
[
'auth_code_id' => uniqid(),
'expire_time' => time() + 3600,
'client_id' => 'foo',
'user_id' => 123,
'scopes' => ['foo'],
'redirect_uri' => 'http://foo/bar',
'code_challenge' => 'R7T1y1HPNFvs1WDCrx4lfoBS6KD2c71pr8OHvULjvv8',
'code_challenge_method' => 'S256',
]
)
),
]
);

try {
/* @var StubResponseType $response */
$grant->respondToAccessTokenRequest($request, new StubResponseType(), new \DateInterval('PT10M'));
} catch (OAuthServerException $e) {
$this->assertEquals($e->getHint(), 'Code Verifier must follow the specifications of RFC-7636.');
}
}

Expand Down

0 comments on commit e04b8f4

Please sign in to comment.