From b637211c57d9b52eb17feb2264d34ad017695ee3 Mon Sep 17 00:00:00 2001 From: TLG-Gildas <129415070+TLG-Gildas@users.noreply.github.com> Date: Wed, 24 Jul 2024 16:21:30 +0000 Subject: [PATCH 1/4] fix: authorization code error should be redirect --- src/Controller/AuthorizationController.php | 36 +++++++++++++++++++ .../Acceptance/AuthorizationEndpointTest.php | 16 ++++----- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/Controller/AuthorizationController.php b/src/Controller/AuthorizationController.php index 7b5215e4..7045b9eb 100644 --- a/src/Controller/AuthorizationController.php +++ b/src/Controller/AuthorizationController.php @@ -111,6 +111,42 @@ public function indexAction(Request $request): Response $response = $this->server->completeAuthorizationRequest($authRequest, $serverResponse); } catch (OAuthServerException $e) { + // https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + // If the request fails due to a missing, invalid, or mismatching + // redirection URI, or if the client identifier is missing or invalid, + // the authorization server SHOULD inform the resource owner of the + // error and MUST NOT automatically redirect the user-agent to the + // invalid redirection URI. + // + // If the resource owner denies the access request or if the request + // fails for reasons other than a missing or invalid redirection URI, + // the authorization server informs the client by adding the following + // parameters to the query component of the redirection URI using the + // "application/x-www-form-urlencoded" format + // + // so if redirectUri is not already set, we try to set request redirect_uri params, fallback to first redirectUri of client + /** @psalm-suppress RiskyTruthyFalsyComparison !empty($e->getHint()),empty($e->getRedirectUri()) we really want to check null and empty */ + if (!empty($client) + && ('invalid_client' === $e->getErrorType() + || ('invalid_request' === $e->getErrorType() && !empty($e->getHint()) + && !\in_array(sscanf($e->getHint() ?? '', 'Check the `%s` parameter')[0] ?? null, ['client_id', 'client_secret', 'redirect_uri']))) + && empty($e->getRedirectUri())) { + /** @var \League\Bundle\OAuth2ServerBundle\Model\ClientInterface $client */ + $redirectUri = $request->query->get('redirect_uri', // query string has priority + (string)$request->request->get('redirect_uri', // then we check body to support POST request + $client->getRedirectUris()[0]?->__toString() ?? '')); // then first client redirect uri + if (!empty($redirectUri)) { + $e = new OAuthServerException( + $e->getMessage(), + $e->getCode(), + $e->getErrorType(), + $e->getHttpStatusCode(), + $e->getHint(), + $redirectUri, + $e->getPrevious(), + ); + } + } $response = $e->generateHttpResponse($serverResponse); } diff --git a/tests/Acceptance/AuthorizationEndpointTest.php b/tests/Acceptance/AuthorizationEndpointTest.php index a467a9f8..d8638f0c 100644 --- a/tests/Acceptance/AuthorizationEndpointTest.php +++ b/tests/Acceptance/AuthorizationEndpointTest.php @@ -191,15 +191,15 @@ public function testAuthCodeRequestWithClientWhoIsNotAllowedToMakeARequestWithPl $response = $this->client->getResponse(); - $this->assertSame(400, $response->getStatusCode()); - - $this->assertSame('application/json', $response->headers->get('Content-Type')); - - $jsonResponse = json_decode($response->getContent(), true); + $this->assertSame(302, $response->getStatusCode()); + $redirectUri = $response->headers->get('Location'); - $this->assertSame('invalid_request', $jsonResponse['error']); - $this->assertSame('The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed.', $jsonResponse['message']); - $this->assertSame('Plain code challenge method is not allowed for this client', $jsonResponse['hint']); + $this->assertStringStartsWith(FixtureFactory::FIXTURE_CLIENT_FIRST_REDIRECT_URI, $redirectUri); + $query = []; + parse_str(parse_url($redirectUri, \PHP_URL_QUERY), $query); + $this->assertEquals('invalid_request', $query['error']); + $this->assertEquals('The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed.', $query['message']); + $this->assertEquals('Plain code challenge method is not allowed for this client', $query['hint']); } public function testAuthCodeRequestWithClientWhoIsAllowedToMakeARequestWithPlainCodeChallengeMethod(): void From 95808613bc5bae39004cbba483d64aa0cfe86bc6 Mon Sep 17 00:00:00 2001 From: TLG-Gildas <129415070+TLG-Gildas@users.noreply.github.com> Date: Mon, 3 Mar 2025 22:47:51 +0000 Subject: [PATCH 2/4] fix: use explicit checks instead of empty on client object --- src/Controller/AuthorizationController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/AuthorizationController.php b/src/Controller/AuthorizationController.php index 4cf6c11c..b1141aaa 100644 --- a/src/Controller/AuthorizationController.php +++ b/src/Controller/AuthorizationController.php @@ -126,7 +126,7 @@ public function indexAction(Request $request): Response // // so if redirectUri is not already set, we try to set request redirect_uri params, fallback to first redirectUri of client /** @psalm-suppress RiskyTruthyFalsyComparison !empty($e->getHint()),empty($e->getRedirectUri()) we really want to check null and empty */ - if (!empty($client) + if (isset($client) && $client !== null && ('invalid_client' === $e->getErrorType() || ('invalid_request' === $e->getErrorType() && !empty($e->getHint()) && !\in_array(sscanf($e->getHint() ?? '', 'Check the `%s` parameter')[0] ?? null, ['client_id', 'client_secret', 'redirect_uri']))) From 90efb0f966eaec70419ed922b05312470eccd1b1 Mon Sep 17 00:00:00 2001 From: TLG-Gildas <129415070+TLG-Gildas@users.noreply.github.com> Date: Mon, 3 Mar 2025 23:49:31 +0000 Subject: [PATCH 3/4] fix: replace all usage of empty --- src/Controller/AuthorizationController.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Controller/AuthorizationController.php b/src/Controller/AuthorizationController.php index b1141aaa..66e903cc 100644 --- a/src/Controller/AuthorizationController.php +++ b/src/Controller/AuthorizationController.php @@ -125,17 +125,17 @@ public function indexAction(Request $request): Response // "application/x-www-form-urlencoded" format // // so if redirectUri is not already set, we try to set request redirect_uri params, fallback to first redirectUri of client - /** @psalm-suppress RiskyTruthyFalsyComparison !empty($e->getHint()),empty($e->getRedirectUri()) we really want to check null and empty */ - if (isset($client) && $client !== null + if (isset($client) && null !== $client && ('invalid_client' === $e->getErrorType() - || ('invalid_request' === $e->getErrorType() && !empty($e->getHint()) + || ('invalid_request' === $e->getErrorType() && null !== $e->getHint() && 0 < \strlen($e->getHint()) && !\in_array(sscanf($e->getHint() ?? '', 'Check the `%s` parameter')[0] ?? null, ['client_id', 'client_secret', 'redirect_uri']))) - && empty($e->getRedirectUri())) { + && (null === $e->getRedirectUri() || 0 === \strlen($e->getRedirectUri()))) { /** @var \League\Bundle\OAuth2ServerBundle\Model\ClientInterface $client */ $redirectUri = $request->query->get('redirect_uri', // query string has priority - (string)$request->request->get('redirect_uri', // then we check body to support POST request - $client->getRedirectUris()[0]?->__toString() ?? '')); // then first client redirect uri - if (!empty($redirectUri)) { + (string) $request->request->get('redirect_uri', // then we check body to support POST request + $client->getRedirectUris()[0]?->__toString() ?? '') // then first client redirect uri + ); + if (0 < \strlen($redirectUri)) { $e = new OAuthServerException( $e->getMessage(), $e->getCode(), From 58104d07246478cc060f44bf5d26b0d33c3a963e Mon Sep 17 00:00:00 2001 From: TLG-Gildas <129415070+TLG-Gildas@users.noreply.github.com> Date: Tue, 4 Mar 2025 00:09:48 +0000 Subject: [PATCH 4/4] fix: coding standards --- src/Controller/AuthorizationController.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Controller/AuthorizationController.php b/src/Controller/AuthorizationController.php index 66e903cc..f0930ac1 100644 --- a/src/Controller/AuthorizationController.php +++ b/src/Controller/AuthorizationController.php @@ -127,15 +127,15 @@ public function indexAction(Request $request): Response // so if redirectUri is not already set, we try to set request redirect_uri params, fallback to first redirectUri of client if (isset($client) && null !== $client && ('invalid_client' === $e->getErrorType() - || ('invalid_request' === $e->getErrorType() && null !== $e->getHint() && 0 < \strlen($e->getHint()) + || ('invalid_request' === $e->getErrorType() && null !== $e->getHint() && '' !== $e->getHint() && !\in_array(sscanf($e->getHint() ?? '', 'Check the `%s` parameter')[0] ?? null, ['client_id', 'client_secret', 'redirect_uri']))) - && (null === $e->getRedirectUri() || 0 === \strlen($e->getRedirectUri()))) { + && (null === $e->getRedirectUri() || '' === $e->getRedirectUri())) { /** @var \League\Bundle\OAuth2ServerBundle\Model\ClientInterface $client */ - $redirectUri = $request->query->get('redirect_uri', // query string has priority - (string) $request->request->get('redirect_uri', // then we check body to support POST request - $client->getRedirectUris()[0]?->__toString() ?? '') // then first client redirect uri + $redirectUri = $request->query->get('redirect_uri', // query string has priority + (string) $request->request->get('redirect_uri', // then we check body to support POST request + $client->getRedirectUris()[0]?->__toString() ?? '') // then first client redirect uri ); - if (0 < \strlen($redirectUri)) { + if ('' !== $redirectUri) { $e = new OAuthServerException( $e->getMessage(), $e->getCode(),