From 32f6f4068078823f6fd8649f0288888842de5f7c Mon Sep 17 00:00:00 2001 From: Sergei Mikhailov Date: Mon, 27 Jan 2025 18:25:55 +0100 Subject: [PATCH] fix: #160 redirect to the actual redirect_uri after verifying that it matches the registered tool host --- src/Security/Oidc/OidcAuthenticator.php | 26 +++- .../Security/Oidc/OidcAuthenticatorTest.php | 120 +++++++++++++++++- 2 files changed, 142 insertions(+), 4 deletions(-) diff --git a/src/Security/Oidc/OidcAuthenticator.php b/src/Security/Oidc/OidcAuthenticator.php index e1eb31f..8461875 100644 --- a/src/Security/Oidc/OidcAuthenticator.php +++ b/src/Security/Oidc/OidcAuthenticator.php @@ -30,6 +30,7 @@ use OAT\Library\Lti1p3Core\Message\Payload\Builder\MessagePayloadBuilder; use OAT\Library\Lti1p3Core\Message\Payload\Builder\MessagePayloadBuilderInterface; use OAT\Library\Lti1p3Core\Message\Payload\LtiMessagePayloadInterface; +use OAT\Library\Lti1p3Core\Registration\RegistrationInterface; use OAT\Library\Lti1p3Core\Registration\RegistrationRepositoryInterface; use OAT\Library\Lti1p3Core\Security\Jwt\Parser\Parser; use OAT\Library\Lti1p3Core\Security\Jwt\Parser\ParserInterface; @@ -99,6 +100,8 @@ public function authenticate(ServerRequestInterface $request): LtiMessageInterfa throw new LtiBadRequestException('Invalid message hint'); } + $redirectUri = $this->createToolRedirectUrl($registration, $oidcRequest); + $authenticationResult = $this->authenticator->authenticate( $registration, $oidcRequest->getParameters()->getMandatory('login_hint') @@ -128,7 +131,7 @@ public function authenticate(ServerRequestInterface $request): LtiMessageInterfa $payload = $this->builder->buildMessagePayload($registration->getPlatformKeyChain()); return new LtiMessage( - $originalToken->getClaims()->getMandatory(LtiMessagePayloadInterface::CLAIM_LTI_TARGET_LINK_URI), + $redirectUri, [ 'id_token' => $payload->getToken()->toString(), 'state' => $oidcRequest->getParameters()->getMandatory('state') @@ -153,4 +156,25 @@ private function sanitizeClaims(array $claims): array return $claims; } + + private function createToolRedirectUrl( + RegistrationInterface $registration, + LtiMessageInterface $oidcRequest + ): string { + $uri = $oidcRequest->getParameters()->getMandatory('redirect_uri'); + $host = parse_url($uri, PHP_URL_HOST); + /** + * redirect_uri must match one of the values associated with the tool's registration + * however, the library does not support multiple redirect_uris per registration, + * and relies on the launch URL to represent the tool's redirect_uri + * + * in order to support multiple redirect_uris, while still keeping the authentication + * relatively safe, the redirect_uri's host is compared against the pre-registered launch URL's host + */ + if (!$host || $host !== parse_url($registration->getTool()->getLaunchUrl() ?? '', PHP_URL_HOST)) { + throw new LtiBadRequestException('Invalid redirect_uri'); + } + + return $uri; + } } diff --git a/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php b/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php index def31ca..2087efe 100644 --- a/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php +++ b/tests/Unit/Security/Oidc/OidcAuthenticatorTest.php @@ -85,7 +85,8 @@ public function testAuthenticationSuccess(): void sprintf('http://platform.com/init?%s', http_build_query([ 'login_hint' => 'login_hint', 'lti_message_hint' => $messageHint->toString(), - 'state' => 'state' + 'state' => 'state', + 'redirect_uri' => 'http://tool.com/redirect', ])) ); @@ -112,6 +113,10 @@ public function testAuthenticationSuccess(): void 'nonce', $idToken->getClaims()->get(LtiMessagePayloadInterface::CLAIM_NONCE) ); + $this->assertEquals( + 'http://tool.com/redirect', + $result->getUrl() + ); } public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): void @@ -145,7 +150,8 @@ public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): v sprintf('http://platform.com/init?%s', http_build_query([ 'login_hint' => 'login_hint', 'lti_message_hint' => $messageHint->toString(), - 'state' => 'state' + 'state' => 'state', + 'redirect_uri' => 'http://tool.com/redirect', ])) ); @@ -169,6 +175,113 @@ public function testAuthenticationSuccessWithUserIdentityClaimsSanitization(): v $this->assertEquals('userPicture', $idToken->getClaims()->get(LtiMessagePayloadInterface::CLAIM_USER_PICTURE)); } + public function testAuthenticationEmptyRedirectUri(): void + { + $this->expectException(LtiException::class); + $this->expectExceptionMessage('OIDC authentication failed: Missing mandatory redirect_uri'); + + $registration = $this->createTestRegistration(); + + $messageHint = $this->buildJwt( + [ + MessagePayloadInterface::HEADER_KID => $registration->getToolKeyChain()->getIdentifier() + ], + [ + MessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier() + ], + $registration->getToolKeyChain()->getPrivateKey() + ); + + $this->repositoryMock + ->expects($this->once()) + ->method('find') + ->with($registration->getIdentifier()) + ->willReturn($registration); + + $request = $this->createServerRequest( + 'GET', + sprintf('http://platform.com/init?%s', http_build_query([ + 'login_hint' => 'login_hint', + 'lti_message_hint' => $messageHint->toString(), + 'state' => 'state', + ])) + ); + + $this->subject->authenticate($request); + } + + public function testAuthenticationMalformedRedirectUri(): void + { + $this->expectException(LtiBadRequestException::class); + $this->expectExceptionMessage('Invalid redirect_uri'); + + $registration = $this->createTestRegistration(); + + $messageHint = $this->buildJwt( + [ + MessagePayloadInterface::HEADER_KID => $registration->getToolKeyChain()->getIdentifier() + ], + [ + MessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier() + ], + $registration->getToolKeyChain()->getPrivateKey() + ); + + $this->repositoryMock + ->expects($this->once()) + ->method('find') + ->with($registration->getIdentifier()) + ->willReturn($registration); + + $request = $this->createServerRequest( + 'GET', + sprintf('http://platform.com/init?%s', http_build_query([ + 'login_hint' => 'login_hint', + 'lti_message_hint' => $messageHint->toString(), + 'state' => 'state', + 'redirect_uri' => 'tool.com/redirect' + ])) + ); + + $this->subject->authenticate($request); + } + + public function testAuthenticationMismatchingRedirectUri(): void + { + $this->expectException(LtiBadRequestException::class); + $this->expectExceptionMessage('Invalid redirect_uri'); + + $registration = $this->createTestRegistration(); + + $messageHint = $this->buildJwt( + [ + MessagePayloadInterface::HEADER_KID => $registration->getToolKeyChain()->getIdentifier() + ], + [ + MessagePayloadInterface::CLAIM_REGISTRATION_ID => $registration->getIdentifier() + ], + $registration->getToolKeyChain()->getPrivateKey() + ); + + $this->repositoryMock + ->expects($this->once()) + ->method('find') + ->with($registration->getIdentifier()) + ->willReturn($registration); + + $request = $this->createServerRequest( + 'GET', + sprintf('http://platform.com/init?%s', http_build_query([ + 'login_hint' => 'login_hint', + 'lti_message_hint' => $messageHint->toString(), + 'state' => 'state', + 'redirect_uri' => 'http://tool.malicious.com/redirect' + ])) + ); + + $this->subject->authenticate($request); + } + public function testAuthenticationFailureOnExpiredMessageHint(): void { $this->expectException(LtiBadRequestException::class); @@ -274,7 +387,8 @@ public function testAuthenticationFailureOnAuthenticationFailure(): void 'GET', sprintf('http://platform.com/init?%s', http_build_query([ 'lti_message_hint' => $messageHint->toString(), - 'login_hint' => 'login_hint' + 'login_hint' => 'login_hint', + 'redirect_uri' => 'http://tool.com/redirect', ])) );