diff --git a/CHANGELOG.md b/CHANGELOG.md index e82ef13b..8bbfd348 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,31 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## [4.4.3] - 2025-02-15 +### Added +* *Nothing* + +### Changed +* *Nothing* + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* [#2351](https://github.com/shlinkio/shlink/issues/2351) Fix visitor IP address resolution when Shlink is served behind more than one reverse proxy. + + This regression was introduced due to a change in behavior in `akrabat/rka-ip-address-middleware`, that now picks the first address from the right after excluding all trusted proxies. + + Since Shlink does not set trusted proxies, this means the first IP from the right is now picked instead of the first from the left, so we now reverse the list before trying to resolve the IP. + + In the future, Shlink will allow you to define trusted proxies, to avoid other potential side effects because of this reversing of the list. + +* [#2354](https://github.com/shlinkio/shlink/issues/2354) Fix error "NOSCRIPT No matching script. Please use EVAL" thrown when creating a lock in redis. +* [#2319](https://github.com/shlinkio/shlink/issues/2319) Fix unique index for `short_code` and `domain_id` in `short_urls` table not being used in Microsoft SQL engines for rows where `domain_id` is `null`. + ## [4.4.2] - 2025-01-29 ### Added * *Nothing* diff --git a/composer.json b/composer.json index d18802a2..8d4848e4 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,7 @@ "ext-json": "*", "ext-mbstring": "*", "ext-pdo": "*", - "akrabat/ip-address-middleware": "^2.5", + "akrabat/ip-address-middleware": "^2.6", "cakephp/chronos": "^3.1", "doctrine/dbal": "^4.2", "doctrine/migrations": "^3.8", @@ -56,23 +56,23 @@ "spiral/roadrunner-jobs": "^4.6", "symfony/console": "^7.2", "symfony/filesystem": "^7.2", - "symfony/lock": "^7.2", + "symfony/lock": "7.2.0", "symfony/process": "^7.2", "symfony/string": "^7.2" }, "require-dev": { "devizzent/cebe-php-openapi": "^1.1.2", "devster/ubench": "^2.1", - "phpstan/phpstan": "^2.0", + "phpstan/phpstan": "^2.1", "phpstan/phpstan-doctrine": "^2.0", "phpstan/phpstan-phpunit": "^2.0", "phpstan/phpstan-symfony": "^2.0", - "phpunit/php-code-coverage": "^11.0", - "phpunit/phpcov": "^10.0", - "phpunit/phpunit": "^11.5", + "phpunit/php-code-coverage": "^12.0", + "phpunit/phpcov": "^11.0", + "phpunit/phpunit": "^12.0", "roave/security-advisories": "dev-master", "shlinkio/php-coding-standard": "~2.4.0", - "shlinkio/shlink-test-utils": "^4.2", + "shlinkio/shlink-test-utils": "^4.3.1", "symfony/var-dumper": "^7.2", "veewee/composer-run-parallel": "^1.4" }, @@ -154,16 +154,8 @@ "@test:cli", "phpcov merge build/coverage-cli --html build/coverage-cli/coverage-html && rm build/coverage-cli/*.cov" ], - "openapi:validate": "@php -d error_reporting=\"E_ALL & ~E_DEPRECATED & ~E_USER_DEPRECATED\" vendor/bin/php-openapi validate docs/swagger/swagger.json", - "openapi:inline": "@php -d error_reporting=\"E_ALL & ~E_DEPRECATED & ~E_USER_DEPRECATED\" vendor/bin/php-openapi inline docs/swagger/swagger.json docs/swagger/openapi-inlined.json", - "swagger:validate": [ - "echo \"This command is deprecated. Use openapi:validate instead\"", - "@openapi:validate" - ], - "swagger:inline": [ - "echo \"This command is deprecated. Use openapi:inline instead\"", - "@openapi:inline" - ], + "openapi:validate": "php-openapi validate docs/swagger/swagger.json", + "openapi:inline": "php-openapi inline docs/swagger/swagger.json docs/swagger/openapi-inlined.json", "clean:dev": "rm -f data/database.sqlite && rm -f config/params/generated_config.php" }, "config": { diff --git a/config/autoload/ip-address.global.php b/config/autoload/ip-address.global.php index 9d531040..78f5bc6d 100644 --- a/config/autoload/ip-address.global.php +++ b/config/autoload/ip-address.global.php @@ -2,8 +2,10 @@ declare(strict_types=1); +use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory; use RKA\Middleware\IpAddress; use RKA\Middleware\Mezzio\IpAddressFactory; +use Shlinkio\Shlink\Core\Middleware\ReverseForwardedAddressesMiddlewareDecorator; use const Shlinkio\Shlink\IP_ADDRESS_REQUEST_ATTRIBUTE; @@ -30,8 +32,19 @@ return [ 'dependencies' => [ 'factories' => [ - IpAddress::class => IpAddressFactory::class, +// IpAddress::class => IpAddressFactory::class, + 'actual_ip_address_middleware' => IpAddressFactory::class, + ReverseForwardedAddressesMiddlewareDecorator::class => ConfigAbstractFactory::class, + ], + 'aliases' => [ + // Make sure the decorated middleware is resolved when getting IpAddress::class, to make this decoration + // transparent for other parts of the code + IpAddress::class => ReverseForwardedAddressesMiddlewareDecorator::class, ], ], + ConfigAbstractFactory::class => [ + ReverseForwardedAddressesMiddlewareDecorator::class => ['actual_ip_address_middleware'], + ], + ]; diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php index 1eb977bf..4fa4498c 100644 --- a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php +++ b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php @@ -15,7 +15,6 @@ use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use ShlinkioTest\Shlink\CLI\Util\CliTestUtils; -use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Tester\CommandTester; class GenerateKeyCommandTest extends TestCase @@ -27,7 +26,7 @@ class GenerateKeyCommandTest extends TestCase { $this->apiKeyService = $this->createMock(ApiKeyServiceInterface::class); $roleResolver = $this->createMock(RoleResolverInterface::class); - $roleResolver->method('determineRoles')->with($this->isInstanceOf(InputInterface::class))->willReturn([]); + $roleResolver->method('determineRoles')->willReturn([]); $command = new GenerateKeyCommand($this->apiKeyService, $roleResolver); $this->commandTester = CliTestUtils::testerForCommand($command); diff --git a/module/CLI/test/Command/Db/CreateDatabaseCommandTest.php b/module/CLI/test/Command/Db/CreateDatabaseCommandTest.php index 9563ef27..8fd34fd2 100644 --- a/module/CLI/test/Command/Db/CreateDatabaseCommandTest.php +++ b/module/CLI/test/Command/Db/CreateDatabaseCommandTest.php @@ -40,11 +40,11 @@ class CreateDatabaseCommandTest extends TestCase { $locker = $this->createMock(LockFactory::class); $lock = $this->createMock(SharedLockInterface::class); - $lock->method('acquire')->withAnyParameters()->willReturn(true); - $locker->method('createLock')->withAnyParameters()->willReturn($lock); + $lock->method('acquire')->willReturn(true); + $locker->method('createLock')->willReturn($lock); $phpExecutableFinder = $this->createMock(PhpExecutableFinder::class); - $phpExecutableFinder->method('find')->with($this->isFalse())->willReturn('/usr/local/bin/php'); + $phpExecutableFinder->method('find')->willReturn('/usr/local/bin/php'); $this->processHelper = $this->createMock(ProcessRunnerInterface::class); $this->schemaManager = $this->createMock(AbstractSchemaManager::class); @@ -60,7 +60,7 @@ class CreateDatabaseCommandTest extends TestCase $em->method('getMetadataFactory')->willReturn($this->metadataFactory); $noDbNameConn = $this->createMock(Connection::class); - $noDbNameConn->method('createSchemaManager')->withAnyParameters()->willReturn($this->schemaManager); + $noDbNameConn->method('createSchemaManager')->willReturn($this->schemaManager); $command = new CreateDatabaseCommand($locker, $this->processHelper, $phpExecutableFinder, $em, $noDbNameConn); $this->commandTester = CliTestUtils::testerForCommand($command); diff --git a/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php b/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php index 29932202..a9fc07ad 100644 --- a/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php +++ b/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php @@ -25,11 +25,11 @@ class MigrateDatabaseCommandTest extends TestCase { $locker = $this->createMock(LockFactory::class); $lock = $this->createMock(SharedLockInterface::class); - $lock->method('acquire')->withAnyParameters()->willReturn(true); - $locker->method('createLock')->withAnyParameters()->willReturn($lock); + $lock->method('acquire')->willReturn(true); + $locker->method('createLock')->willReturn($lock); $phpExecutableFinder = $this->createMock(PhpExecutableFinder::class); - $phpExecutableFinder->method('find')->with($this->isFalse())->willReturn('/usr/local/bin/php'); + $phpExecutableFinder->method('find')->willReturn('/usr/local/bin/php'); $this->processHelper = $this->createMock(ProcessRunnerInterface::class); diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index bd694e7c..a57a2870 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -70,7 +70,7 @@ class CreateShortUrlCommandTest extends TestCase $this->urlShortener->expects($this->once())->method('shorten')->withAnyParameters()->willThrowException( NonUniqueSlugException::fromSlug('my-slug'), ); - $this->stringifier->method('stringify')->with($this->isInstanceOf(ShortUrl::class))->willReturn(''); + $this->stringifier->method('stringify')->willReturn(''); $this->commandTester->execute(['longUrl' => 'http://domain.com/invalid', '--custom-slug' => 'my-slug']); $output = $this->commandTester->getDisplay(); @@ -112,7 +112,7 @@ class CreateShortUrlCommandTest extends TestCase return true; }), )->willReturn(UrlShorteningResult::withoutErrorOnEventDispatching(ShortUrl::createFake())); - $this->stringifier->method('stringify')->with($this->isInstanceOf(ShortUrl::class))->willReturn(''); + $this->stringifier->method('stringify')->willReturn(''); $input['longUrl'] = 'http://domain.com/foo/bar'; $this->commandTester->execute($input); @@ -139,7 +139,7 @@ class CreateShortUrlCommandTest extends TestCase return true; }), )->willReturn(UrlShorteningResult::withoutErrorOnEventDispatching($shortUrl)); - $this->stringifier->method('stringify')->with($this->isInstanceOf(ShortUrl::class))->willReturn(''); + $this->stringifier->method('stringify')->willReturn(''); $options['longUrl'] = 'http://domain.com/foo/bar'; $this->commandTester->execute($options); diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 9b35324a..aa1f5e25 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -47,7 +47,7 @@ class LocateVisitsCommandTest extends TestCase $locker = $this->createMock(Lock\LockFactory::class); $this->lock = $this->createMock(Lock\SharedLockInterface::class); - $locker->method('createLock')->with($this->isString(), 600.0, false)->willReturn($this->lock); + $locker->method('createLock')->willReturn($this->lock); $command = new LocateVisitsCommand($this->visitService, $this->visitToLocation, $locker); @@ -67,7 +67,7 @@ class LocateVisitsCommandTest extends TestCase $location = VisitLocation::fromGeolocation(Location::empty()); $mockMethodBehavior = $this->invokeHelperMethods($visit, $location); - $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); + $this->lock->method('acquire')->willReturn(true); $this->visitService->expects($this->exactly($expectedUnlocatedCalls)) ->method('locateUnlocatedVisits') ->withAnyParameters() @@ -83,7 +83,7 @@ class LocateVisitsCommandTest extends TestCase $this->visitToLocation->expects( $this->exactly($expectedUnlocatedCalls + $expectedEmptyCalls + $expectedAllCalls), )->method('resolveVisitLocation')->withAnyParameters()->willReturn(Location::emptyInstance()); - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_SUCCESS); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_SUCCESS); $this->commandTester->setInputs(['y']); $this->commandTester->execute($args); @@ -108,15 +108,15 @@ class LocateVisitsCommandTest extends TestCase public function localhostAndEmptyAddressesAreIgnored(IpCannotBeLocatedException $e, string $message): void { $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::empty()); - $location = VisitLocation::fromGeolocation(Location::emptyInstance()); + $location = VisitLocation::fromGeolocation(Location::empty()); - $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); + $this->lock->method('acquire')->willReturn(true); $this->visitService->expects($this->once()) ->method('locateUnlocatedVisits') ->withAnyParameters() ->willReturnCallback($this->invokeHelperMethods($visit, $location)); $this->visitToLocation->expects($this->once())->method('resolveVisitLocation')->willThrowException($e); - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_SUCCESS); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_SUCCESS); $this->commandTester->execute([], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); @@ -137,7 +137,7 @@ class LocateVisitsCommandTest extends TestCase $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')); $location = VisitLocation::fromGeolocation(Location::emptyInstance()); - $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); + $this->lock->method('acquire')->willReturn(true); $this->visitService->expects($this->once()) ->method('locateUnlocatedVisits') ->withAnyParameters() @@ -145,7 +145,7 @@ class LocateVisitsCommandTest extends TestCase $this->visitToLocation->expects($this->once())->method('resolveVisitLocation')->willThrowException( IpCannotBeLocatedException::forError(WrongIpException::fromIpAddress('1.2.3.4')), ); - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_SUCCESS); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_SUCCESS); $this->commandTester->execute([], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); @@ -165,11 +165,11 @@ class LocateVisitsCommandTest extends TestCase #[Test] public function noActionIsPerformedIfLockIsAcquired(): void { - $this->lock->method('acquire')->with($this->isFalse())->willReturn(false); + $this->lock->method('acquire')->willReturn(false); $this->visitService->expects($this->never())->method('locateUnlocatedVisits'); $this->visitToLocation->expects($this->never())->method('resolveVisitLocation'); - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_SUCCESS); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_SUCCESS); $this->commandTester->execute([], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); $output = $this->commandTester->getDisplay(); @@ -183,8 +183,8 @@ class LocateVisitsCommandTest extends TestCase #[Test] public function showsProperMessageWhenGeoLiteUpdateFails(): void { - $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_FAILURE); + $this->lock->method('acquire')->willReturn(true); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_FAILURE); $this->visitService->expects($this->never())->method('locateUnlocatedVisits'); $this->commandTester->execute([]); @@ -196,8 +196,8 @@ class LocateVisitsCommandTest extends TestCase #[Test] public function providingAllFlagOnItsOwnDisplaysNotice(): void { - $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_SUCCESS); + $this->lock->method('acquire')->willReturn(true); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_SUCCESS); $this->commandTester->execute(['--all' => true]); $output = $this->commandTester->getDisplay(); @@ -208,7 +208,7 @@ class LocateVisitsCommandTest extends TestCase #[Test, DataProvider('provideAbortInputs')] public function processingAllCancelsCommandIfUserDoesNotActivelyAgreeToConfirmation(array $inputs): void { - $this->downloadDbCommand->method('run')->withAnyParameters()->willReturn(ExitCode::EXIT_SUCCESS); + $this->downloadDbCommand->method('run')->willReturn(ExitCode::EXIT_SUCCESS); $this->expectException(RuntimeException::class); $this->expectExceptionMessage('Execution aborted'); diff --git a/module/CLI/test/Util/CliTestUtils.php b/module/CLI/test/Util/CliTestUtils.php index c18186ba..5f94c661 100644 --- a/module/CLI/test/Util/CliTestUtils.php +++ b/module/CLI/test/Util/CliTestUtils.php @@ -25,11 +25,8 @@ class CliTestUtils $command = $generator->testDouble( Command::class, mockObject: true, - markAsMockObject: true, callOriginalConstructor: false, callOriginalClone: false, - cloneArguments: false, - allowMockingUnknownTypes: false, ); $command->method('getName')->willReturn($name); $command->method('isEnabled')->willReturn(true); diff --git a/module/CLI/test/Util/ProcessRunnerTest.php b/module/CLI/test/Util/ProcessRunnerTest.php index 16cc2d43..88253273 100644 --- a/module/CLI/test/Util/ProcessRunnerTest.php +++ b/module/CLI/test/Util/ProcessRunnerTest.php @@ -27,8 +27,8 @@ class ProcessRunnerTest extends TestCase $this->helper = $this->createMock(ProcessHelper::class); $this->formatter = $this->createMock(DebugFormatterHelper::class); $helperSet = $this->createMock(HelperSet::class); - $helperSet->method('get')->with('debug_formatter')->willReturn($this->formatter); - $this->helper->method('getHelperSet')->with()->willReturn($helperSet); + $helperSet->method('get')->willReturn($this->formatter); + $this->helper->method('getHelperSet')->willReturn($helperSet); $this->process = $this->createMock(Process::class); $this->output = $this->createMock(OutputInterface::class); diff --git a/module/Core/migrations/Version20250215100756.php b/module/Core/migrations/Version20250215100756.php new file mode 100644 index 00000000..77e9599e --- /dev/null +++ b/module/Core/migrations/Version20250215100756.php @@ -0,0 +1,43 @@ +skipIf(! $this->isMicrosoftSql()); + + // Drop the existing unique index + $shortUrls = $schema->getTable('short_urls'); + $shortUrls->dropIndex('unique_short_code_plus_domain'); + } + + public function postUp(Schema $schema): void + { + // The only way to get the index properly generated is by hardcoding the SQL. + // Since this migration is run Microsoft SQL only, it is safe to use this approach. + $this->connection->executeStatement( + 'CREATE UNIQUE INDEX unique_short_code_plus_domain ON short_urls (short_code, domain_id);', + ); + } + + private function isMicrosoftSql(): bool + { + return $this->connection->getDatabasePlatform() instanceof SQLServerPlatform; + } +} diff --git a/module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php b/module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php new file mode 100644 index 00000000..71d7545c --- /dev/null +++ b/module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php @@ -0,0 +1,49 @@ +hasHeader(self::FORWARDED_FOR_HEADER)) { + $request = $request->withHeader( + self::FORWARDED_FOR_HEADER, + implode(',', array_reverse(explode(',', $request->getHeaderLine(self::FORWARDED_FOR_HEADER)))), + ); + } + + return $this->wrappedMiddleware->process($request, $handler); + } +} diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php index bb6abea2..56921ef0 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php @@ -31,24 +31,33 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU $ordering = $dbPlatform instanceof PostgreSQLPlatform ? 'ASC' : 'DESC'; $isStrict = $shortUrlMode === ShortUrlMode::STRICT; - $qb = $this->createQueryBuilder('s'); - $qb->leftJoin('s.domain', 'd') - ->where($qb->expr()->eq($isStrict ? 's.shortCode' : 'LOWER(s.shortCode)', ':shortCode')) - ->setParameter('shortCode', $isStrict ? $identifier->shortCode : strtolower($identifier->shortCode)) - ->andWhere($qb->expr()->orX( - $qb->expr()->isNull('s.domain'), - $qb->expr()->eq('d.authority', ':domain'), - )) - ->setParameter('domain', $identifier->domain); + // FIXME The `LOWER(s.shortCode)` condition in non-strict mode drops performance dramatically. + // Investigate if the case-insensitive check can be done natively by the DB engine. - // Since we order by domain, we will have first the URL matching provided domain, followed by the one - // with no domain (if any), so it is safe to fetch 1 max result, and we will get: - // * The short URL matching both the short code and the domain, or - // * The short URL matching the short code but without any domain, or - // * No short URL at all - $qb->orderBy('s.domain', $ordering) + $qb = $this->createQueryBuilder('s'); + $qb->where($qb->expr()->eq($isStrict ? 's.shortCode' : 'LOWER(s.shortCode)', ':shortCode')) + ->setParameter('shortCode', $isStrict ? $identifier->shortCode : strtolower($identifier->shortCode)) ->setMaxResults(1); + // If $domain is null, do not join with domains nor do $qb->expr()->eq('d.authority', ':domain') + $domain = $identifier->domain; + if ($domain === null) { + $qb->andWhere($qb->expr()->isNull('s.domain')); + } else { + $qb->leftJoin('s.domain', 'd') + ->andWhere($qb->expr()->orX( + $qb->expr()->isNull('s.domain'), + $qb->expr()->eq('d.authority', ':domain'), + )) + ->setParameter('domain', $domain) + // Since we order by domain, we will have first the URL matching provided domain, followed by the one + // with no domain (if any), so it is safe to fetch 1 max result, and we will get: + // * The short URL matching both the short code and the domain, or + // * The short URL matching the short code but without any domain, or + // * No short URL at all + ->orderBy('s.domain', $ordering); + } + return $qb->getQuery()->getOneOrNullResult(); } diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php index 2cfe9417..799851c7 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/module/Core/test-api/Action/RedirectTest.php @@ -90,7 +90,8 @@ class RedirectTest extends ApiTestCase ]; $ipAddressConfig = require __DIR__ . '/../../../../config/autoload/ip-address.global.php'; - foreach ($ipAddressConfig['rka']['ip_address']['headers_to_inspect'] as $header) { + $headers = $ipAddressConfig['rka']['ip_address']['headers_to_inspect']; + foreach ($headers as $header) { yield sprintf('rule: IP address in "%s" header', $header) => [ [ RequestOptions::HEADERS => [$header => $header !== 'Forwarded' ? '1.2.3.4' : 'for=1.2.3.4'], @@ -98,6 +99,15 @@ class RedirectTest extends ApiTestCase 'https://example.com/static-ip-address', ]; } + + yield 'rule: IP address in "X-Forwarded-For" together with proxy addresses' => [ + [ + RequestOptions::HEADERS => [ + 'X-Forwarded-For' => '1.2.3.4, 192.168.1.1, 192.168.1.2', + ], + ], + 'https://example.com/static-ip-address', + ]; } /** diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index 9db42752..4e987277 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -79,9 +79,7 @@ class QrCodeActionTest extends TestCase string $expectedContentType, ): void { $code = 'abc123'; - $this->urlResolver->method('resolveEnabledShortUrl')->with( - ShortUrlIdentifier::fromShortCodeAndDomain($code, ''), - )->willReturn(ShortUrl::createFake()); + $this->urlResolver->method('resolveEnabledShortUrl')->willReturn(ShortUrl::createFake()); $handler = $this->createMock(RequestHandlerInterface::class); $req = (new ServerRequest())->withAttribute('shortCode', $code)->withQueryParams($query); @@ -109,9 +107,7 @@ class QrCodeActionTest extends TestCase int $expectedSize, ): void { $code = 'abc123'; - $this->urlResolver->method('resolveEnabledShortUrl')->with( - ShortUrlIdentifier::fromShortCodeAndDomain($code, ''), - )->willReturn(ShortUrl::createFake()); + $this->urlResolver->method('resolveEnabledShortUrl')->willReturn(ShortUrl::createFake()); $handler = $this->createMock(RequestHandlerInterface::class); $resp = $this->action($defaultOptions)->process($req->withAttribute('shortCode', $code), $handler); @@ -199,9 +195,7 @@ class QrCodeActionTest extends TestCase ->withQueryParams(['size' => 250, 'roundBlockSize' => $roundBlockSize]) ->withAttribute('shortCode', $code); - $this->urlResolver->method('resolveEnabledShortUrl')->with( - ShortUrlIdentifier::fromShortCodeAndDomain($code, ''), - )->willReturn(ShortUrl::withLongUrl('https://shlink.io')); + $this->urlResolver->method('resolveEnabledShortUrl')->willReturn(ShortUrl::withLongUrl('https://shlink.io')); $handler = $this->createMock(RequestHandlerInterface::class); $resp = $this->action($defaultOptions)->process($req, $handler); @@ -242,9 +236,7 @@ class QrCodeActionTest extends TestCase ->withQueryParams(['color' => $queryColor]) ->withAttribute('shortCode', $code); - $this->urlResolver->method('resolveEnabledShortUrl')->with( - ShortUrlIdentifier::fromShortCodeAndDomain($code), - )->willReturn(ShortUrl::withLongUrl('https://shlink.io')); + $this->urlResolver->method('resolveEnabledShortUrl')->willReturn(ShortUrl::withLongUrl('https://shlink.io')); $handler = $this->createMock(RequestHandlerInterface::class); $resp = $this->action( @@ -306,9 +298,7 @@ class QrCodeActionTest extends TestCase ->withAttribute('shortCode', $code) ->withQueryParams($query); - $this->urlResolver->method('resolveEnabledShortUrl')->with( - ShortUrlIdentifier::fromShortCodeAndDomain($code), - )->willReturn(ShortUrl::withLongUrl('https://shlink.io')); + $this->urlResolver->method('resolveEnabledShortUrl')->willReturn(ShortUrl::withLongUrl('https://shlink.io')); $handler = $this->createMock(RequestHandlerInterface::class); $resp = $this->action(new QrCodeOptions(size: 250, logoUrl: $logoUrl))->process($req, $handler); diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 59e1a5b8..c9f9eaa7 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -37,7 +37,7 @@ class RedirectActionTest extends TestCase $this->redirectRespHelper = $this->createMock(RedirectResponseHelperInterface::class); $redirectBuilder = $this->createMock(ShortUrlRedirectionBuilderInterface::class); - $redirectBuilder->method('buildShortUrlRedirect')->withAnyParameters()->willReturn(self::LONG_URL); + $redirectBuilder->method('buildShortUrlRedirect')->willReturn(self::LONG_URL); $this->action = new RedirectAction( $this->urlResolver, diff --git a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php index 8c9f5a1b..88e256ae 100644 --- a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php +++ b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php @@ -43,7 +43,7 @@ class GeolocationDbUpdaterTest extends TestCase $this->dbUpdater = $this->createMock(DbUpdaterInterface::class); $this->lock = $this->createMock(Lock\SharedLockInterface::class); - $this->lock->method('acquire')->with($this->isTrue())->willReturn(true); + $this->lock->method('acquire')->willReturn(true); $this->em = $this->createMock(EntityManagerInterface::class); $this->repo = $this->createMock(EntityRepository::class); @@ -291,7 +291,7 @@ class GeolocationDbUpdaterTest extends TestCase private function geolocationDbUpdater(TrackingOptions|null $options = null): GeolocationDbUpdater { $locker = $this->createMock(Lock\LockFactory::class); - $locker->method('createLock')->with($this->isString())->willReturn($this->lock); + $locker->method('createLock')->willReturn($this->lock); return new GeolocationDbUpdater($this->dbUpdater, $locker, $options ?? new TrackingOptions(), $this->em, 3); } diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index e8d800a1..84229728 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -104,7 +104,7 @@ class ImportedLinksProcessorTest extends TestCase ]; $expectedCalls = count($urls); - $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + $this->em->method('getRepository')->willReturn($this->repo); $this->repo->expects($this->exactly($expectedCalls))->method('findOneByImportedUrl')->willReturn(null); $this->shortCodeHelper->expects($this->exactly($expectedCalls)) ->method('ensureShortCodeUniqueness') @@ -138,7 +138,7 @@ class ImportedLinksProcessorTest extends TestCase new ImportedShlinkUrl(ImportSource::BITLY, 'https://baz', [], Chronos::now(), null, 'baz', null), ]; - $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + $this->em->method('getRepository')->willReturn($this->repo); $this->repo->expects($this->exactly(3))->method('findOneByImportedUrl')->willReturn(null); $this->shortCodeHelper->expects($this->exactly(3))->method('ensureShortCodeUniqueness')->willReturn(true); $this->em->expects($this->exactly(3))->method('persist')->with( @@ -167,7 +167,7 @@ class ImportedLinksProcessorTest extends TestCase new ImportedShlinkUrl(ImportSource::BITLY, 'https://baz3', [], Chronos::now(), null, 'baz3', null), ]; - $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + $this->em->method('getRepository')->willReturn($this->repo); $this->repo->expects($this->exactly(count($urls)))->method('findOneByImportedUrl')->willReturnCallback( fn (ImportedShlinkUrl $url): ShortUrl|null => contains( $url->longUrl, @@ -195,7 +195,7 @@ class ImportedLinksProcessorTest extends TestCase new ImportedShlinkUrl(ImportSource::BITLY, 'https://baz3', [], Chronos::now(), null, 'baz3', 'bar'), ]; - $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + $this->em->method('getRepository')->willReturn($this->repo); $this->repo->expects($this->exactly(count($urls)))->method('findOneByImportedUrl')->willReturn(null); $this->shortCodeHelper->expects($this->exactly(7))->method('ensureShortCodeUniqueness')->willReturnCallback( fn ($_, bool $hasCustomSlug) => ! $hasCustomSlug, @@ -219,7 +219,7 @@ class ImportedLinksProcessorTest extends TestCase int $amountOfPersistedVisits, ShortUrl|null $foundShortUrl, ): void { - $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + $this->em->method('getRepository')->willReturn($this->repo); $this->repo->expects($this->once())->method('findOneByImportedUrl')->willReturn($foundShortUrl); $this->shortCodeHelper->expects($this->exactly($foundShortUrl === null ? 1 : 0)) ->method('ensureShortCodeUniqueness') @@ -276,7 +276,7 @@ class ImportedLinksProcessorTest extends TestCase #[Test, DataProvider('provideFoundShortUrls')] public function visitsArePersistedWithProperShortUrl(ShortUrl $originalShortUrl, ShortUrl|null $foundShortUrl): void { - $this->em->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + $this->em->method('getRepository')->willReturn($this->repo); $this->repo->expects($this->once())->method('findOneByImportedUrl')->willReturn($originalShortUrl); if (!$originalShortUrl->getId()) { $this->em->expects($this->never())->method('find'); diff --git a/module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php b/module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php new file mode 100644 index 00000000..d3e1bd5e --- /dev/null +++ b/module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php @@ -0,0 +1,59 @@ +decoratedMiddleware = $this->createMock(MiddlewareInterface::class); + $this->requestHandler = $this->createMock(RequestHandlerInterface::class); + $this->middleware = new ReverseForwardedAddressesMiddlewareDecorator($this->decoratedMiddleware); + } + + #[Test] + public function processesRequestAsIsWhenHeadersIsNotFound(): void + { + $request = ServerRequestFactory::fromGlobals(); + $this->decoratedMiddleware->expects($this->once())->method('process')->with( + $request, + $this->requestHandler, + )->willReturn(new Response()); + + $this->middleware->process($request, $this->requestHandler); + } + + #[Test] + public function revertsListOfAddressesWhenHeaderIsFound(): void + { + $request = ServerRequestFactory::fromGlobals()->withHeader( + ReverseForwardedAddressesMiddlewareDecorator::FORWARDED_FOR_HEADER, + '1.2.3.4,5.6.7.8,9.10.11.12', + ); + + $this->decoratedMiddleware->expects($this->once())->method('process')->with( + $this->callback(fn (ServerRequestInterface $req): bool => $req->getHeaderLine( + ReverseForwardedAddressesMiddlewareDecorator::FORWARDED_FOR_HEADER, + ) === '9.10.11.12,5.6.7.8,1.2.3.4'), + $this->requestHandler, + )->willReturn(new Response()); + + $this->middleware->process($request, $this->requestHandler); + } +} diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 31f42bc7..4968ccbf 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -116,7 +116,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase { $repo = $this->createMock(DomainRepository::class); $repo->expects($this->exactly(3))->method('findOneBy')->with($this->isArray())->willReturn(null); - $this->em->method('getRepository')->with(Domain::class)->willReturn($repo); + $this->em->method('getRepository')->willReturn($repo); $authority = 'foo.com'; $domain1 = $this->resolver->resolveDomain($authority); @@ -135,7 +135,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase { $tagRepo = $this->createMock(TagRepository::class); $tagRepo->expects($this->exactly(6))->method('findOneBy')->with($this->isArray())->willReturn(null); - $this->em->method('getRepository')->with(Tag::class)->willReturn($tagRepo); + $this->em->method('getRepository')->willReturn($tagRepo); $tags = ['foo', 'bar']; [$foo1, $bar1] = $this->resolver->resolveTags($tags); diff --git a/module/Core/test/ShortUrl/UrlShortenerTest.php b/module/Core/test/ShortUrl/UrlShortenerTest.php index fb191e95..f2467dbd 100644 --- a/module/Core/test/ShortUrl/UrlShortenerTest.php +++ b/module/Core/test/ShortUrl/UrlShortenerTest.php @@ -5,7 +5,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ShortUrl; use Cake\Chronos\Chronos; -use Doctrine\ORM\EntityManager; +use Doctrine\ORM\EntityManagerInterface; use Laminas\ServiceManager\Exception\ServiceNotFoundException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; @@ -24,7 +24,7 @@ use Shlinkio\Shlink\Core\ShortUrl\UrlShortener; class UrlShortenerTest extends TestCase { private UrlShortener $urlShortener; - private MockObject & EntityManager $em; + private MockObject & EntityManagerInterface $em; private MockObject & ShortUrlTitleResolutionHelperInterface $titleResolutionHelper; private MockObject & ShortCodeUniquenessHelperInterface $shortCodeHelper; private MockObject & EventDispatcherInterface $dispatcher; @@ -35,12 +35,9 @@ class UrlShortenerTest extends TestCase $this->titleResolutionHelper = $this->createMock(ShortUrlTitleResolutionHelperInterface::class); $this->shortCodeHelper = $this->createMock(ShortCodeUniquenessHelperInterface::class); - // FIXME Should use the interface, but it doe snot define wrapInTransaction explicitly - $this->em = $this->createMock(EntityManager::class); + $this->em = $this->createMock(EntityManagerInterface::class); $this->em->method('persist')->willReturnCallback(fn (ShortUrl $shortUrl) => $shortUrl->setId('10')); - $this->em->method('wrapInTransaction')->with($this->isCallable())->willReturnCallback( - fn (callable $callback) => $callback(), - ); + $this->em->method('wrapInTransaction')->willReturnCallback(fn (callable $callback) => $callback()); $this->dispatcher = $this->createMock(EventDispatcherInterface::class); $this->repo = $this->createMock(ShortUrlRepositoryInterface::class); diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index d6762c00..070ea7e6 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -108,14 +108,8 @@ class VisitsStatsHelperTest extends TestCase range(0, 1), ); $repo2 = $this->createMock(VisitRepository::class); - $repo2->method('findVisitsByShortCode')->with( - $identifier, - $this->isInstanceOf(VisitsListFiltering::class), - )->willReturn($list); - $repo2->method('countVisitsByShortCode')->with( - $identifier, - $this->isInstanceOf(VisitsCountFiltering::class), - )->willReturn(1); + $repo2->method('findVisitsByShortCode')->willReturn($list); + $repo2->method('countVisitsByShortCode')->willReturn(1); $this->em->expects($this->exactly(2))->method('getRepository')->willReturnMap([ [ShortUrl::class, $repo], @@ -168,10 +162,8 @@ class VisitsStatsHelperTest extends TestCase range(0, 1), ); $repo2 = $this->createMock(VisitRepository::class); - $repo2->method('findVisitsByTag')->with($tag, $this->isInstanceOf(VisitsListFiltering::class))->willReturn( - $list, - ); - $repo2->method('countVisitsByTag')->with($tag, $this->isInstanceOf(VisitsCountFiltering::class))->willReturn(1); + $repo2->method('findVisitsByTag')->willReturn($list); + $repo2->method('countVisitsByTag')->willReturn(1); $this->em->expects($this->exactly(2))->method('getRepository')->willReturnMap([ [Tag::class, $repo], @@ -209,14 +201,8 @@ class VisitsStatsHelperTest extends TestCase range(0, 1), ); $repo2 = $this->createMock(VisitRepository::class); - $repo2->method('findVisitsByDomain')->with( - $domain, - $this->isInstanceOf(VisitsListFiltering::class), - )->willReturn($list); - $repo2->method('countVisitsByDomain')->with( - $domain, - $this->isInstanceOf(VisitsCountFiltering::class), - )->willReturn(1); + $repo2->method('findVisitsByDomain')->willReturn($list); + $repo2->method('countVisitsByDomain')->willReturn(1); $this->em->expects($this->exactly(2))->method('getRepository')->willReturnMap([ [Domain::class, $repo], @@ -239,14 +225,8 @@ class VisitsStatsHelperTest extends TestCase range(0, 1), ); $repo2 = $this->createMock(VisitRepository::class); - $repo2->method('findVisitsByDomain')->with( - Domain::DEFAULT_AUTHORITY, - $this->isInstanceOf(VisitsListFiltering::class), - )->willReturn($list); - $repo2->method('countVisitsByDomain')->with( - Domain::DEFAULT_AUTHORITY, - $this->isInstanceOf(VisitsCountFiltering::class), - )->willReturn(1); + $repo2->method('findVisitsByDomain')->willReturn($list); + $repo2->method('countVisitsByDomain')->willReturn(1); $this->em->expects($this->exactly(2))->method('getRepository')->willReturnMap([ [Domain::class, $repo], diff --git a/module/Rest/test/Middleware/ShortUrl/CreateShortUrlContentNegotiationMiddlewareTest.php b/module/Rest/test/Middleware/ShortUrl/CreateShortUrlContentNegotiationMiddlewareTest.php index 7f4d0eba..de438a44 100644 --- a/module/Rest/test/Middleware/ShortUrl/CreateShortUrlContentNegotiationMiddlewareTest.php +++ b/module/Rest/test/Middleware/ShortUrl/CreateShortUrlContentNegotiationMiddlewareTest.php @@ -30,9 +30,7 @@ class CreateShortUrlContentNegotiationMiddlewareTest extends TestCase public function whenNoJsonResponseIsReturnedNoFurtherOperationsArePerformed(): void { $expectedResp = new Response(); - $this->requestHandler->method('handle')->with($this->isInstanceOf(ServerRequestInterface::class))->willReturn( - $expectedResp, - ); + $this->requestHandler->method('handle')->willReturn($expectedResp); $resp = $this->middleware->process(new ServerRequest(), $this->requestHandler);