From be6df854dd97ea2359c298b1c4bc85ddc4340135 Mon Sep 17 00:00:00 2001 From: Nicola Saliu Date: Tue, 10 Dec 2024 09:52:27 +0100 Subject: [PATCH 1/3] Sanitize Shop URL on update --- src/Registration/RegistrationService.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Registration/RegistrationService.php b/src/Registration/RegistrationService.php index b132c1d..57c9d8b 100644 --- a/src/Registration/RegistrationService.php +++ b/src/Registration/RegistrationService.php @@ -66,7 +66,7 @@ public function register(RequestInterface $request): ResponseInterface $this->shopRepository->createShop($shop); } else { - $shop->setShopUrl($queries['shop-url']); + $shop->setShopUrl($this->sanitizeShopUrl($queries['shop-url'])); $this->eventDispatcher?->dispatch(new BeforeRegistrationStartsEvent($request, $shop)); @@ -78,7 +78,6 @@ public function register(RequestInterface $request): ResponseInterface 'shop-url' => $shop->getShopUrl(), ]); - $psrFactory = new Psr17Factory(); $data = [ From f7278bd95a86c21a69fc5d86f2b9281158cc5a4e Mon Sep 17 00:00:00 2001 From: Nicola Saliu Date: Fri, 13 Dec 2024 11:27:55 +0100 Subject: [PATCH 2/3] NEXT-39991 - Fix the Shop URL in the registration to create the correct proof if the URL should be sanitized --- src/Registration/RegistrationService.php | 16 +- .../Registration/RegistrationServiceTest.php | 137 ++++++++++++++++-- 2 files changed, 142 insertions(+), 11 deletions(-) diff --git a/src/Registration/RegistrationService.php b/src/Registration/RegistrationService.php index 57c9d8b..e012d8a 100644 --- a/src/Registration/RegistrationService.php +++ b/src/Registration/RegistrationService.php @@ -58,7 +58,7 @@ public function register(RequestInterface $request): ResponseInterface if ($shop === null) { $shop = $this->shopRepository->createShopStruct( $queries['shop-id'], - $this->sanitizeShopUrl($queries['shop-url']), + $queries['shop-url'], $this->shopSecretGeneratorInterface->generate() ); @@ -66,7 +66,7 @@ public function register(RequestInterface $request): ResponseInterface $this->shopRepository->createShop($shop); } else { - $shop->setShopUrl($this->sanitizeShopUrl($queries['shop-url'])); + $shop->setShopUrl($queries['shop-url']); $this->eventDispatcher?->dispatch(new BeforeRegistrationStartsEvent($request, $shop)); @@ -86,6 +86,8 @@ public function register(RequestInterface $request): ResponseInterface 'secret' => $shop->getShopSecret(), ]; + $this->fixShopUrlInDatabase($shop); + $response = $psrFactory->createResponse(200); return $response @@ -162,4 +164,14 @@ private function sanitizeShopUrl(string $shopUrl): string $normalizedPath ); } + + private function fixShopUrlInDatabase(ShopInterface $shop): void + { + $sanitizedShopUrl = $this->sanitizeShopUrl($shop->getShopUrl()); + + if ($shop->getShopUrl() !== $sanitizedShopUrl) { + $shop->setShopUrl($sanitizedShopUrl); + $this->shopRepository->updateShop($shop); + } + } } diff --git a/tests/Registration/RegistrationServiceTest.php b/tests/Registration/RegistrationServiceTest.php index fda22e0..4730d01 100644 --- a/tests/Registration/RegistrationServiceTest.php +++ b/tests/Registration/RegistrationServiceTest.php @@ -91,6 +91,11 @@ public function testRegisterCreate(): void ->method('createShopStruct') ->willReturn($shop); + $shopRepository + ->expects(static::never()) + ->method('updateShop') + ->with($shop); + $eventDispatcher ->expects(static::once()) ->method('dispatch'); @@ -137,6 +142,11 @@ public function testRegisterCreateMustNotDispatchBeforeRegistrationStartsEvent() ->method('createShopStruct') ->willReturn($shop); + $shopRepository + ->expects(static::never()) + ->method('updateShop') + ->with($shop); + $response = $registrationService->register( new Request('GET', 'http://localhost?shop-id=123&shop-url=https://my-shop.com×tamp=1234567890') ); @@ -162,9 +172,27 @@ public function testRegisterUpdate(): void $events[] = $event; }); + $shopRepository = $this->createMock(ShopRepositoryInterface::class); + + $shop = new MockShop('123', 'https://my-shop.com', '1234567890'); + + $shopRepository + ->expects(static::once()) + ->method('getShopFromId') + ->willReturn($shop); + + $shopRepository + ->expects(static::never()) + ->method('createShopStruct'); + + $shopRepository + ->expects(static::once()) + ->method('updateShop') + ->with($shop); + $registrationService = new RegistrationService( $this->appConfiguration, - $this->shopRepository, + $shopRepository, $this->createMock(RequestVerifier::class), new ResponseSigner(), new RandomStringShopSecretGenerator(), @@ -176,10 +204,6 @@ public function testRegisterUpdate(): void new Request('GET', 'http://localhost?shop-id=123&shop-url=https://my-shop.com×tamp=1234567890') ); - $shop = $this->shopRepository->getShopFromId('123'); - - $this->shopRepository->updateShop($shop); - static::assertNotNull($shop); static::assertCount(1, $events); @@ -465,18 +489,101 @@ public function testRegisterConfirmMissingShopParameters(array $params): void } #[DataProvider('shopUrlsProvider')] - public function testRegisterShopUrlIsSanitized( + public function testRegisterCreateShopUrlIsSanitized( string $shopUrl, string $expectedUrl, + bool $sanitizeShopUrlInDatabase ): void { + $shopRepository = $this->createMock(ShopRepositoryInterface::class); + + $shop = new MockShop('123', $shopUrl, '1234567890'); + + $shopRepository + ->expects(static::once()) + ->method('getShopFromId') + ->with('123') + ->willReturn(null); + + $shopRepository + ->expects(static::once()) + ->method('createShopStruct') + ->willReturn($shop); + + if ($sanitizeShopUrlInDatabase) { + $shopRepository + ->expects(static::once()) + ->method('updateShop') + ->with($shop); + } + + $registrationService = new RegistrationService( + $this->appConfiguration, + $shopRepository, + $this->createMock(RequestVerifier::class), + new ResponseSigner(), + new RandomStringShopSecretGenerator(), + new NullLogger(), + null + ); + $request = new Request( 'GET', sprintf('http://localhost?shop-id=123&shop-url=%s×tamp=1234567890', $shopUrl) ); - $this->registerService->register($request); + $registrationService->register($request); - $shop = $this->shopRepository->getShopFromId('123'); + static::assertSame($expectedUrl, $shop->getShopUrl()); + } + + #[DataProvider('shopUrlsProvider')] + public function testRegisterUpdateShopUrlIsSanitized( + string $shopUrl, + string $expectedUrl, + bool $sanitizeShopUrlInDatabase + ): void { + $shopRepository = $this->createMock(ShopRepositoryInterface::class); + + $shop = new MockShop('123', $shopUrl, '1234567890'); + + $shopRepository + ->expects(static::once()) + ->method('getShopFromId') + ->willReturn($shop); + + $shopRepository + ->expects(static::never()) + ->method('createShopStruct') + ->willReturn($shop); + + if ($sanitizeShopUrlInDatabase) { + $shopRepository + ->expects(static::exactly(2)) + ->method('updateShop') + ->with($shop); + } else { + $shopRepository + ->expects(static::once()) + ->method('updateShop') + ->with($shop); + } + + $registrationService = new RegistrationService( + $this->appConfiguration, + $shopRepository, + $this->createMock(RequestVerifier::class), + new ResponseSigner(), + new RandomStringShopSecretGenerator(), + new NullLogger(), + null + ); + + $request = new Request( + 'GET', + sprintf('http://localhost?shop-id=123&shop-url=%s×tamp=1234567890', $shopUrl) + ); + + $registrationService->register($request); static::assertSame($expectedUrl, $shop->getShopUrl()); } @@ -526,68 +633,80 @@ public static function missingRegisterConfirmShopParametersProvider(): iterable } /** - * @return iterable> + * @return iterable> */ public static function shopUrlsProvider(): iterable { yield 'Valid URL with port' => [ 'shopUrl' => 'https://my-shop.com:80', 'expectedUrl' => 'https://my-shop.com:80', + 'sanitizeShopUrlInDatabase' => false, ]; yield 'Valid URL with port and trailing slash' => [ 'shopUrl' => 'https://my-shop.com:8080/', 'expectedUrl' => 'https://my-shop.com:8080', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Valid URL with port, path and trailing slash' => [ 'shopUrl' => 'https://my-shop.com:8080//test/', 'expectedUrl' => 'https://my-shop.com:8080/test', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Valid URL without trailing slash' => [ 'shopUrl' => 'https://my-shop.com', 'expectedUrl' => 'https://my-shop.com', + 'sanitizeShopUrlInDatabase' => false, ]; yield 'Valid URL with trailing slash' => [ 'shopUrl' => 'https://my-shop.com/', 'expectedUrl' => 'https://my-shop.com', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with trailing slash' => [ 'shopUrl' => 'https://my-shop.com/test/', 'expectedUrl' => 'https://my-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with double slashes' => [ 'shopUrl' => 'https://my-shop.com//test', 'expectedUrl' => 'https://my-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with 2 slashes and trailing slash' => [ 'shopUrl' => 'https://my-shop.com//test/', 'expectedUrl' => 'https://my-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with 3 slashes and trailing slash' => [ 'shopUrl' => 'https://my-shop.com///test/', 'expectedUrl' => 'https://my-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with multiple slashes' => [ 'shopUrl' => 'https://my-shop.com///test/test1//test2', 'expectedUrl' => 'https://my-shop.com/test/test1/test2', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with multiple slashes and trailing slash' => [ 'shopUrl' => 'https://my-shop.com///test/test1//test2/', 'expectedUrl' => 'https://my-shop.com/test/test1/test2', + 'sanitizeShopUrlInDatabase' => true, ]; yield 'Invalid URL with multiple slashes and multplie trailing slash' => [ 'shopUrl' => 'https://my-shop.com///test/test1//test2//', 'expectedUrl' => 'https://my-shop.com/test/test1/test2', + 'sanitizeShopUrlInDatabase' => true, ]; } } From 2bfb8744529071b1148567518b2979ae3ad63749 Mon Sep 17 00:00:00 2001 From: Nicola Saliu Date: Fri, 13 Dec 2024 17:32:54 +0100 Subject: [PATCH 3/3] NEXT-39991 - Improve unit tests --- src/Registration/RegistrationService.php | 1 + .../Registration/RegistrationServiceTest.php | 141 ++++++++++++++++-- 2 files changed, 127 insertions(+), 15 deletions(-) diff --git a/src/Registration/RegistrationService.php b/src/Registration/RegistrationService.php index e012d8a..deb6024 100644 --- a/src/Registration/RegistrationService.php +++ b/src/Registration/RegistrationService.php @@ -169,6 +169,7 @@ private function fixShopUrlInDatabase(ShopInterface $shop): void { $sanitizedShopUrl = $this->sanitizeShopUrl($shop->getShopUrl()); + if ($shop->getShopUrl() !== $sanitizedShopUrl) { $shop->setShopUrl($sanitizedShopUrl); $this->shopRepository->updateShop($shop); diff --git a/tests/Registration/RegistrationServiceTest.php b/tests/Registration/RegistrationServiceTest.php index 4730d01..f85cc54 100644 --- a/tests/Registration/RegistrationServiceTest.php +++ b/tests/Registration/RegistrationServiceTest.php @@ -50,7 +50,7 @@ public function testRegisterMissingParameters(): void { $request = new Request('GET', 'http://localhost'); - static::expectException(MissingShopParameterException::class); + $this->expectException(MissingShopParameterException::class); $this->registerService->register($request); } @@ -105,6 +105,8 @@ public function testRegisterCreate(): void ); static::assertSame(200, $response->getStatusCode()); + static::assertSame('https://my-shop.com', $shop->getShopUrl()); + $json = json_decode((string) $response->getBody()->getContents(), true); static::assertCount(1, $events); @@ -152,6 +154,8 @@ public function testRegisterCreateMustNotDispatchBeforeRegistrationStartsEvent() ); static::assertSame(200, $response->getStatusCode()); + static::assertSame('https://my-shop.com', $shop->getShopUrl()); + $json = json_decode((string) $response->getBody()->getContents(), true); static::assertIsArray($json); @@ -188,7 +192,9 @@ public function testRegisterUpdate(): void $shopRepository ->expects(static::once()) ->method('updateShop') - ->with($shop); + ->with($this->callback(function (MockShop $shop) { + return $shop->getShopUrl() === 'https://my-shop.com'; + })); $registrationService = new RegistrationService( $this->appConfiguration, @@ -245,7 +251,7 @@ public function testConfirmMissingParameter(): void { $request = new Request('POST', 'http://localhost', [], '{}'); - static::expectException(MissingShopParameterException::class); + $this->expectException(MissingShopParameterException::class); $this->registerService->registerConfirm($request); } @@ -253,7 +259,7 @@ public function testConfirmNotExistingShop(): void { $request = new Request('POST', 'http://localhost', [], '{"shopId": "123", "apiKey": "1", "secretKey": "1"}'); - static::expectException(ShopNotFoundException::class); + $this->expectException(ShopNotFoundException::class); $this->registerService->registerConfirm($request); } @@ -464,7 +470,7 @@ public function testRegisterMissingShopParameters(array $params): void new NullLogger() ); - static::expectException(MissingShopParameterException::class); + $this->expectException(MissingShopParameterException::class); $registrationService->register($request); } @@ -484,11 +490,11 @@ public function testRegisterConfirmMissingShopParameters(array $params): void new NullLogger() ); - static::expectException(MissingShopParameterException::class); + $this->expectException(MissingShopParameterException::class); $registrationService->registerConfirm($request); } - #[DataProvider('shopUrlsProvider')] + #[DataProvider('shopUrlsProviderForCreation')] public function testRegisterCreateShopUrlIsSanitized( string $shopUrl, string $expectedUrl, @@ -536,9 +542,11 @@ public function testRegisterCreateShopUrlIsSanitized( static::assertSame($expectedUrl, $shop->getShopUrl()); } - #[DataProvider('shopUrlsProvider')] + + #[DataProvider('shopUrlsProviderForUpdate')] public function testRegisterUpdateShopUrlIsSanitized( string $shopUrl, + string $newShopUrl, string $expectedUrl, bool $sanitizeShopUrlInDatabase ): void { @@ -553,19 +561,32 @@ public function testRegisterUpdateShopUrlIsSanitized( $shopRepository ->expects(static::never()) - ->method('createShopStruct') - ->willReturn($shop); + ->method('createShopStruct'); + + $expectedUrls = [ + $newShopUrl, + $expectedUrl, + ]; + + $callCount = 0; if ($sanitizeShopUrlInDatabase) { $shopRepository ->expects(static::exactly(2)) ->method('updateShop') - ->with($shop); + ->with($this->callback(function (MockShop $shop) use (&$callCount, $expectedUrls) { + $expectedUrl = $expectedUrls[$callCount] ?? null; + $result = $shop->getShopUrl() === $expectedUrl; + $callCount++; + return $result; + })); } else { $shopRepository ->expects(static::once()) ->method('updateShop') - ->with($shop); + ->with($this->callback(function (MockShop $shop) use ($newShopUrl) { + return $shop->getShopUrl() === $newShopUrl; + })); } $registrationService = new RegistrationService( @@ -580,7 +601,7 @@ public function testRegisterUpdateShopUrlIsSanitized( $request = new Request( 'GET', - sprintf('http://localhost?shop-id=123&shop-url=%s×tamp=1234567890', $shopUrl) + sprintf('http://localhost?shop-id=123&shop-url=%s×tamp=1234567890', $newShopUrl) ); $registrationService->register($request); @@ -635,7 +656,7 @@ public static function missingRegisterConfirmShopParametersProvider(): iterable /** * @return iterable> */ - public static function shopUrlsProvider(): iterable + public function shopUrlsProviderForCreation(): iterable { yield 'Valid URL with port' => [ 'shopUrl' => 'https://my-shop.com:80', @@ -703,10 +724,100 @@ public static function shopUrlsProvider(): iterable 'sanitizeShopUrlInDatabase' => true, ]; - yield 'Invalid URL with multiple slashes and multplie trailing slash' => [ + yield 'Invalid URL with multiple slashes and multiple trailing slash' => [ 'shopUrl' => 'https://my-shop.com///test/test1//test2//', 'expectedUrl' => 'https://my-shop.com/test/test1/test2', 'sanitizeShopUrlInDatabase' => true, ]; } + + /** + * @return iterable> + */ + public static function shopUrlsProviderForUpdate(): iterable + { + yield 'Valid URL with port' => [ + 'shopUrl' => 'https://my-shop.com:80', + 'newShopUrl' => 'https://my-changed-shop.de:80', + 'expectedUrl' => 'https://my-changed-shop.de:80', + 'sanitizeShopUrlInDatabase' => false, + ]; + + yield 'Valid URL with port and trailing slash' => [ + 'shopUrl' => 'https://my-shop.com:8080/', + 'newShopUrl' => 'https://my-changed-shop.com:8080/', + 'expectedUrl' => 'https://my-changed-shop.com:8080', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Valid URL with port, path and trailing slash' => [ + 'shopUrl' => 'https://my-shop.com:8080//test/', + 'newShopUrl' => 'https://my-changed-shop.com:8080//test/', + 'expectedUrl' => 'https://my-changed-shop.com:8080/test', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Valid URL without trailing slash' => [ + 'shopUrl' => 'https://my-shop.com', + 'newShopUrl' => 'https://my-changed-shop.com', + 'expectedUrl' => 'https://my-changed-shop.com', + 'sanitizeShopUrlInDatabase' => false, + ]; + + yield 'Valid URL with trailing slash' => [ + 'shopUrl' => 'https://my-shop.com/', + 'newShopUrl' => 'https://my-changed-shop.com/', + 'expectedUrl' => 'https://my-changed-shop.com', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with trailing slash' => [ + 'shopUrl' => 'https://my-shop.com/test/', + 'newShopUrl' => 'https://my-changed-shop.com/test/', + 'expectedUrl' => 'https://my-changed-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with double slashes' => [ + 'shopUrl' => 'https://my-shop.com//test', + 'newShopUrl' => 'https://my-changed-shop.com//test', + 'expectedUrl' => 'https://my-changed-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with 2 slashes and trailing slash' => [ + 'shopUrl' => 'https://my-shop.com//test/', + 'newShopUrl' => 'https://my-changed-shop.com//test/', + 'expectedUrl' => 'https://my-changed-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with 3 slashes and trailing slash' => [ + 'shopUrl' => 'https://my-shop.com///test/', + 'newShopUrl' => 'https://my-changed-shop.com///test/', + 'expectedUrl' => 'https://my-changed-shop.com/test', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with multiple slashes' => [ + 'shopUrl' => 'https://my-shop.com///test/test1//test2', + 'newShopUrl' => 'https://my-changed-shop.com///test/test1//test2', + 'expectedUrl' => 'https://my-changed-shop.com/test/test1/test2', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with multiple slashes and trailing slash' => [ + 'shopUrl' => 'https://my-shop.com///test/test1//test2/', + 'newShopUrl' => 'https://my-changed-shop.com///test/test1//test2/', + 'expectedUrl' => 'https://my-changed-shop.com/test/test1/test2', + 'sanitizeShopUrlInDatabase' => true, + ]; + + yield 'Invalid URL with multiple slashes and multiple trailing slash' => [ + 'shopUrl' => 'https://my-shop.com///test/test1//test2//', + 'newShopUrl' => 'https://my-changed-shop.com///test/test1//test2//', + 'expectedUrl' => 'https://my-changed-shop.com/test/test1/test2', + 'sanitizeShopUrlInDatabase' => true, + ]; + } }