Skip to content

Commit 56b2deb

Browse files
eugene-borovoveugene-borovov
authored
Merge branch 'master' into psr-event-dispatcher
2 parents 1f75b46 + 9bfb699 commit 56b2deb

File tree

11 files changed

+374
-52
lines changed

11 files changed

+374
-52
lines changed

‎.github/dependabot.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
version: 2
2+
updates:
3+
- package-ecosystem: composer
4+
directory: "/"
5+
schedule:
6+
interval: daily
7+
time: "11:00"
8+
open-pull-requests-limit: 10
9+
ignore:
10+
- dependency-name: league/event
11+
versions:
12+
- 3.0.0

‎.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/vendor
22
**/composer.lock
33
phpunit.xml
4+
.phpunit.result.cache
45
.idea
56
/examples/vendor
67
examples/public.key

‎CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
66

77
## [Unreleased]
88
### Added
9+
- The server will now validate redirect uris according to rfc8252 (PR #1203)
910
- Events emitted now include the refresh token and access token payloads (PR #1211)
1011

12+
### Changed
13+
- Keys are now validated using `openssl_pkey_get_private()` and openssl_pkey_get_public()` instead of regex matching (PR #1215)
14+
1115
### Fixed
1216
- The server will now only recognise and handle an authorization header if the value of the header is non-empty. This is to circumvent issues where some common frameworks set this header even if no value is present (PR #1170)
1317
- Added type validation for redirect uri, client ID, client secret, scopes, auth code, state, username, and password inputs (PR #1210)
18+
- Allow scope "0" to be used. Previously this was removed from a request because it failed an `empty()` check (PR #1181)
1419

1520
## [8.2.4] - released 2020-12-10
1621
### Fixed

‎src/CryptKey.php

Lines changed: 55 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,12 @@
1616

1717
class CryptKey
1818
{
19+
/** @deprecated left for backward compatibility check */
1920
const RSA_KEY_PATTERN =
2021
'/^(-----BEGIN (RSA )?(PUBLIC|PRIVATE) KEY-----)\R.*(-----END (RSA )?(PUBLIC|PRIVATE) KEY-----)\R?$/s';
2122

23+
private const FILE_PREFIX = 'file://';
24+
2225
/**
2326
* @var string
2427
*/
@@ -36,36 +39,43 @@ class CryptKey
3639
*/
3740
public function __construct($keyPath, $passPhrase = null, $keyPermissionsCheck = true)
3841
{
39-
if ($rsaMatch = \preg_match(static::RSA_KEY_PATTERN, $keyPath)) {
40-
$keyPath = $this->saveKeyToFile($keyPath);
41-
} elseif ($rsaMatch === false) {
42-
throw new \RuntimeException(
43-
\sprintf('PCRE error [%d] encountered during key match attempt', \preg_last_error())
44-
);
45-
}
42+
$this->passPhrase = $passPhrase;
4643

47-
if (\strpos($keyPath, 'file://') !== 0) {
48-
$keyPath = 'file://' . $keyPath;
49-
}
44+
if (\is_file($keyPath)) {
45+
if (\strpos($keyPath, self::FILE_PREFIX) !== 0) {
46+
$keyPath = self::FILE_PREFIX . $keyPath;
47+
}
5048

51-
if (!\file_exists($keyPath) || !\is_readable($keyPath)) {
52-
throw new LogicException(\sprintf('Key path "%s" does not exist or is not readable', $keyPath));
49+
if (!\is_readable($keyPath)) {
50+
throw new LogicException(\sprintf('Key path "%s" does not exist or is not readable', $keyPath));
51+
}
52+
$isFileKey = true;
53+
$contents = \file_get_contents($keyPath);
54+
$this->keyPath = $keyPath;
55+
} else {
56+
$isFileKey = false;
57+
$contents = $keyPath;
58+
$this->keyPath = $this->saveKeyToFile($keyPath);
5359
}
5460

5561
if ($keyPermissionsCheck === true) {
5662
// Verify the permissions of the key
57-
$keyPathPerms = \decoct(\fileperms($keyPath) & 0777);
63+
$keyPathPerms = \decoct(\fileperms($this->keyPath) & 0777);
5864
if (\in_array($keyPathPerms, ['400', '440', '600', '640', '660'], true) === false) {
59-
\trigger_error(\sprintf(
60-
'Key file "%s" permissions are not correct, recommend changing to 600 or 660 instead of %s',
61-
$keyPath,
62-
$keyPathPerms
63-
), E_USER_NOTICE);
65+
\trigger_error(
66+
\sprintf(
67+
'Key file "%s" permissions are not correct, recommend changing to 600 or 660 instead of %s',
68+
$this->keyPath,
69+
$keyPathPerms
70+
),
71+
E_USER_NOTICE
72+
);
6473
}
6574
}
6675

67-
$this->keyPath = $keyPath;
68-
$this->passPhrase = $passPhrase;
76+
if (!$this->isValidKey($contents, $this->passPhrase ?? '')) {
77+
throw new LogicException('Unable to read key' . ($isFileKey ? " from file $keyPath" : ''));
78+
}
6979
}
7080

7181
/**
@@ -81,7 +91,7 @@ private function saveKeyToFile($key)
8191
$keyPath = $tmpDir . '/' . \sha1($key) . '.key';
8292

8393
if (\file_exists($keyPath)) {
84-
return 'file://' . $keyPath;
94+
return self::FILE_PREFIX . $keyPath;
8595
}
8696

8797
if (\file_put_contents($keyPath, $key) === false) {
@@ -96,7 +106,30 @@ private function saveKeyToFile($key)
96106
// @codeCoverageIgnoreEnd
97107
}
98108

99-
return 'file://' . $keyPath;
109+
return self::FILE_PREFIX . $keyPath;
110+
}
111+
112+
/**
113+
* Validate key contents.
114+
*
115+
* @param string $contents
116+
* @param string $passPhrase
117+
*
118+
* @return bool
119+
*/
120+
private function isValidKey($contents, $passPhrase)
121+
{
122+
$pkey = \openssl_pkey_get_private($contents, $passPhrase) ?: \openssl_pkey_get_public($contents);
123+
if ($pkey === false) {
124+
return false;
125+
}
126+
$details = \openssl_pkey_get_details($pkey);
127+
128+
return $details !== false && \in_array(
129+
$details['type'] ?? -1,
130+
[OPENSSL_KEYTYPE_RSA, OPENSSL_KEYTYPE_EC],
131+
true
132+
);
100133
}
101134

102135
/**

‎src/Grant/AbstractGrant.php

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
use League\OAuth2\Server\Events\EventDispatcherAwareTrait;
2626
use League\OAuth2\Server\Exception\OAuthServerException;
2727
use League\OAuth2\Server\Exception\UniqueTokenIdentifierConstraintViolationException;
28+
use League\OAuth2\Server\RedirectUriValidators\RedirectUriValidator;
2829
use League\OAuth2\Server\Repositories\AccessTokenRepositoryInterface;
2930
use League\OAuth2\Server\Repositories\AuthCodeRepositoryInterface;
3031
use League\OAuth2\Server\Repositories\ClientRepositoryInterface;
@@ -270,14 +271,8 @@ protected function validateRedirectUri(
270271
ClientEntityInterface $client,
271272
ServerRequestInterface $request
272273
) {
273-
if (\is_string($client->getRedirectUri())
274-
&& (\strcmp($client->getRedirectUri(), $redirectUri) !== 0)
275-
) {
276-
$this->dispatchEvent(new ClientAuthenticationFailed($client->getIdentifier(), $request));
277-
throw OAuthServerException::invalidClient($request);
278-
} elseif (\is_array($client->getRedirectUri())
279-
&& \in_array($redirectUri, $client->getRedirectUri(), true) === false
280-
) {
274+
$validator = new RedirectUriValidator($client->getRedirectUri());
275+
if (!$validator->validateRedirectUri($redirectUri)) {
281276
$this->dispatchEvent(new ClientAuthenticationFailed($client->getIdentifier(), $request));
282277
throw OAuthServerException::invalidClient($request);
283278
}
@@ -330,7 +325,7 @@ public function validateScopes($scopes, $redirectUri = null)
330325
private function convertScopesQueryStringToArray(string $scopes)
331326
{
332327
return \array_filter(\explode(self::SCOPE_DELIMITER_STRING, \trim($scopes)), function ($scope) {
333-
return !empty($scope);
328+
return $scope !== '';
334329
});
335330
}
336331

‎src/RedirectUriValidators/RedirectUriValidator.php

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
<?php
2+
/**
3+
* @author Sebastiano Degan <[email protected]>
4+
* @copyright Copyright (c) Alex Bilbie
5+
* @license http://mit-license.org/
6+
*
7+
* @link https://github.com/thephpleague/oauth2-server
8+
*/
9+
10+
namespace League\OAuth2\Server\RedirectUriValidators;
11+
12+
class RedirectUriValidator implements RedirectUriValidatorInterface
13+
{
14+
/**
15+
* @var array
16+
*/
17+
private $allowedRedirectUris;
18+
19+
/**
20+
* New validator instance for the given uri
21+
*
22+
* @param string|array $allowedRedirectUris
23+
*/
24+
public function __construct($allowedRedirectUri)
25+
{
26+
if (\is_string($allowedRedirectUri)) {
27+
$this->allowedRedirectUris = [$allowedRedirectUri];
28+
} elseif (\is_array($allowedRedirectUri)) {
29+
$this->allowedRedirectUris = $allowedRedirectUri;
30+
} else {
31+
$this->allowedRedirectUris = [];
32+
}
33+
}
34+
35+
/**
36+
* Validates the redirect uri.
37+
*
38+
* @param string $redirectUri
39+
*
40+
* @return bool Return true if valid, false otherwise
41+
*/
42+
public function validateRedirectUri($redirectUri)
43+
{
44+
if ($this->isLoopbackUri($redirectUri)) {
45+
return $this->matchUriExcludingPort($redirectUri);
46+
}
47+
48+
return $this->matchExactUri($redirectUri);
49+
}
50+
51+
/**
52+
* According to section 7.3 of rfc8252, loopback uris are:
53+
* - "http://127.0.0.1:{port}/{path}" for IPv4
54+
* - "http://[::1]:{port}/{path}" for IPv6
55+
*
56+
* @param string $redirectUri
57+
*
58+
* @return bool
59+
*/
60+
private function isLoopbackUri($redirectUri)
61+
{
62+
$parsedUrl = \parse_url($redirectUri);
63+
64+
return $parsedUrl['scheme'] === 'http'
65+
&& (\in_array($parsedUrl['host'], ['127.0.0.1', '[::1]'], true));
66+
}
67+
68+
/**
69+
* Find an exact match among allowed uris
70+
*
71+
* @param string $redirectUri
72+
*
73+
* @return bool Return true if an exact match is found, false otherwise
74+
*/
75+
private function matchExactUri($redirectUri)
76+
{
77+
return \in_array($redirectUri, $this->allowedRedirectUris, true);
78+
}
79+
80+
/**
81+
* Find a match among allowed uris, allowing for different port numbers
82+
*
83+
* @param string $redirectUri
84+
*
85+
* @return bool Return true if a match is found, false otherwise
86+
*/
87+
private function matchUriExcludingPort($redirectUri)
88+
{
89+
$parsedUrl = $this->parseUrlAndRemovePort($redirectUri);
90+
91+
foreach ($this->allowedRedirectUris as $allowedRedirectUri) {
92+
if ($parsedUrl === $this->parseUrlAndRemovePort($allowedRedirectUri)) {
93+
return true;
94+
}
95+
}
96+
97+
return false;
98+
}
99+
100+
/**
101+
* Parse an url like \parse_url, excluding the port
102+
*
103+
* @param string $url
104+
*
105+
* @return array
106+
*/
107+
private function parseUrlAndRemovePort($url)
108+
{
109+
$parsedUrl = \parse_url($url);
110+
unset($parsedUrl['port']);
111+
112+
return $parsedUrl;
113+
}
114+
}

‎src/RedirectUriValidators/RedirectUriValidatorInterface.php

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
/**
3+
* @author Sebastiano Degan <[email protected]>
4+
* @copyright Copyright (c) Alex Bilbie
5+
* @license http://mit-license.org/
6+
*
7+
* @link https://github.com/thephpleague/oauth2-server
8+
*/
9+
10+
namespace League\OAuth2\Server\RedirectUriValidators;
11+
12+
interface RedirectUriValidatorInterface
13+
{
14+
/**
15+
* Validates the redirect uri.
16+
*
17+
* @param string $redirectUri
18+
*
19+
* @return bool Return true if valid, false otherwise
20+
*/
21+
public function validateRedirectUri($redirectUri);
22+
}

‎tests/AuthorizationValidators/BearerTokenValidatorTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ public function testBearerTokenValidatorAcceptsValidToken()
3838

3939
$request = (new ServerRequest())->withHeader('authorization', \sprintf('Bearer %s', $validJwt->toString()));
4040

41-
$response = $bearerTokenValidator->validateAuthorization($request);
41+
$validRequest = $bearerTokenValidator->validateAuthorization($request);
4242

43-
$this->assertArrayHasKey('authorization', $response->getHeaders());
43+
$this->assertArrayHasKey('authorization', $validRequest->getHeaders());
4444
}
4545

4646
public function testBearerTokenValidatorRejectsExpiredToken()

‎tests/Grant/AbstractGrantTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -521,13 +521,13 @@ public function testValidateScopes()
521521
{
522522
$scope = new ScopeEntity();
523523
$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
524-
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scope);
524+
$scopeRepositoryMock->expects($this->exactly(3))->method('getScopeEntityByIdentifier')->willReturn($scope);
525525

526526
/** @var AbstractGrant $grantMock */
527527
$grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
528528
$grantMock->setScopeRepository($scopeRepositoryMock);
529529

530-
$this->assertEquals([$scope], $grantMock->validateScopes('basic '));
530+
$this->assertEquals([$scope, $scope, $scope], $grantMock->validateScopes('basic test 0 '));
531531
}
532532

533533
public function testValidateScopesBadScope()

0 commit comments

Comments
 (0)