diff --git a/.github/ISSUE_TEMPLATE/Bug.yml b/.github/ISSUE_TEMPLATE/Bug.yml index 2fce5cf4a..f9671a9f5 100644 --- a/.github/ISSUE_TEMPLATE/Bug.yml +++ b/.github/ISSUE_TEMPLATE/Bug.yml @@ -58,5 +58,10 @@ body: validations: required: true attributes: - label: How to reproduce - value: '' + label: Minimum steps to reproduce + value: | + diff --git a/CHANGELOG.md b/CHANGELOG.md index 15502e0d6..e646df824 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ 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.0.3] - 2024-03-15 +### Added +* *Nothing* + +### Changed +* *Nothing* + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* [#2058](https://github.com/shlinkio/shlink/issues/2058) Fix DB credentials provided as env vars being casted to `int` if they include only numbers. +* [#2060](https://github.com/shlinkio/shlink/issues/2060) Fix error when trying to redirect to a non-http long URL. + + ## [4.0.2] - 2024-03-09 ### Added * *Nothing* diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index 849c91af0..3eb43edf2 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -16,6 +16,10 @@ 'mssql' => 'pdo_sqlsrv', default => 'pdo_mysql', }; + $readCredentialAsString = static function (EnvVars $envVar): string|null { + $value = $envVar->loadFromEnv(); + return $value === null ? null : (string) $value; + }; $resolveDefaultPort = static fn () => match ($driver) { 'postgres' => '5432', 'mssql' => '1433', @@ -28,6 +32,7 @@ 'postgres' => 'utf8', default => null, }; + $resolveConnection = static fn () => match ($driver) { null, 'sqlite' => [ 'driver' => 'pdo_sqlite', @@ -36,8 +41,8 @@ default => [ 'driver' => $resolveDriver(), 'dbname' => EnvVars::DB_NAME->loadFromEnv('shlink'), - 'user' => EnvVars::DB_USER->loadFromEnv(), - 'password' => EnvVars::DB_PASSWORD->loadFromEnv(), + 'user' => $readCredentialAsString(EnvVars::DB_USER), + 'password' => $readCredentialAsString(EnvVars::DB_PASSWORD), 'host' => EnvVars::DB_HOST->loadFromEnv(EnvVars::DB_UNIX_SOCKET->loadFromEnv()), 'port' => EnvVars::DB_PORT->loadFromEnv($resolveDefaultPort()), 'unix_socket' => $isMysqlCompatible ? EnvVars::DB_UNIX_SOCKET->loadFromEnv() : null, diff --git a/module/Core/functions/array-utils.php b/module/Core/functions/array-utils.php index 7b9ca7e50..d68851d88 100644 --- a/module/Core/functions/array-utils.php +++ b/module/Core/functions/array-utils.php @@ -17,7 +17,6 @@ function contains(mixed $value, array $array): bool /** * @param array[] $multiArray - * @return array */ function flatten(array $multiArray): array { diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 3a6a6d064..768fb60ee 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -5,7 +5,7 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Helper; use GuzzleHttp\Psr7\Query; -use Laminas\Diactoros\Uri; +use GuzzleHttp\Psr7\Uri; use Laminas\Stdlib\ArrayUtils; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Options\TrackingOptions; @@ -30,16 +30,18 @@ public function buildShortUrlRedirect( $uri = new Uri($this->redirectionResolver->resolveLongUrl($shortUrl, $request)); $currentQuery = $request->getQueryParams(); $shouldForwardQuery = $shortUrl->forwardQuery(); + $baseQueryString = $uri->getQuery(); + $basePath = $uri->getPath(); return $uri - ->withQuery($shouldForwardQuery ? $this->resolveQuery($uri, $currentQuery) : $uri->getQuery()) - ->withPath($this->resolvePath($uri, $extraPath)) + ->withQuery($shouldForwardQuery ? $this->resolveQuery($baseQueryString, $currentQuery) : $baseQueryString) + ->withPath($this->resolvePath($basePath, $extraPath)) ->__toString(); } - private function resolveQuery(Uri $uri, array $currentQuery): string + private function resolveQuery(string $baseQueryString, array $currentQuery): string { - $hardcodedQuery = Query::parse($uri->getQuery()); + $hardcodedQuery = Query::parse($baseQueryString); $disableTrackParam = $this->trackingOptions->disableTrackParam; if ($disableTrackParam !== null) { @@ -47,14 +49,13 @@ private function resolveQuery(Uri $uri, array $currentQuery): string } // We want to merge preserving numeric keys, as some params might be numbers - $mergedQuery = ArrayUtils::merge($hardcodedQuery, $currentQuery, true); + $mergedQuery = ArrayUtils::merge($hardcodedQuery, $currentQuery, preserveNumericKeys: true); return Query::build($mergedQuery); } - private function resolvePath(Uri $uri, ?string $extraPath): string + private function resolvePath(string $basePath, ?string $extraPath): string { - $hardcodedPath = $uri->getPath(); - return $extraPath === null ? $hardcodedPath : sprintf('%s%s', $hardcodedPath, $extraPath); + return $extraPath === null ? $basePath : sprintf('%s%s', $basePath, $extraPath); } } diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php index c8f96bba9..7780f91be 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -87,7 +87,7 @@ private function tryToResolveRedirect( } /** - * @return array{0: string, 1: string|null} + * @return array{string, string|null} */ private function resolvePotentialShortCodeAndExtraPath(UriInterface $uri, int $shortCodeSegments): array { diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php index e8cf211a2..2c54e8a7e 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/module/Core/test-api/Action/RedirectTest.php @@ -7,6 +7,7 @@ use GuzzleHttp\RequestOptions; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\TestWith; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; use const ShlinkioTest\Shlink\ANDROID_USER_AGENT; @@ -15,8 +16,8 @@ class RedirectTest extends ApiTestCase { - #[Test, DataProvider('provideUserAgents')] - public function properRedirectHappensBasedOnUserAgent(array $options, string $expectedRedirect): void + #[Test, DataProvider('provideRequestOptions')] + public function properRedirectHappensBasedOnRedirectRules(array $options, string $expectedRedirect): void { $response = $this->callShortUrl('def456', $options); @@ -24,19 +25,19 @@ public function properRedirectHappensBasedOnUserAgent(array $options, string $ex self::assertEquals($expectedRedirect, $response->getHeaderLine('Location')); } - public static function provideUserAgents(): iterable + public static function provideRequestOptions(): iterable { yield 'android' => [ [ RequestOptions::HEADERS => ['User-Agent' => ANDROID_USER_AGENT], ], - 'https://blog.alejandrocelaya.com/android', + 'android://foo/bar', ]; yield 'ios' => [ [ RequestOptions::HEADERS => ['User-Agent' => IOS_USER_AGENT], ], - 'https://blog.alejandrocelaya.com/ios', + 'fb://profile/33138223345', ]; yield 'desktop' => [ [ @@ -86,4 +87,27 @@ public static function provideUserAgents(): iterable 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', ]; } + + /** + * @param non-empty-string $longUrl + */ + #[Test] + #[TestWith(['android://foo/bar'])] + #[TestWith(['fb://profile/33138223345'])] + #[TestWith(['viber://pa?chatURI=1234'])] + public function properRedirectHappensForNonHttpLongUrls(string $longUrl): void + { + $slug = 'non-http-schema'; + $this->callApiWithKey('POST', '/short-urls', [ + RequestOptions::JSON => [ + 'longUrl' => $longUrl, + 'customSlug' => $slug, + ], + ]); + + $response = $this->callShortUrl($slug); + + self::assertEquals(302, $response->getStatusCode()); + self::assertEquals($longUrl, $response->getHeaderLine('Location')); + } } diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index 53a863229..f1ffc012a 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -7,6 +7,7 @@ use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; @@ -37,7 +38,7 @@ public function buildShortUrlRedirectBuildsExpectedUrl( ?bool $forwardQuery, ): void { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ - 'longUrl' => 'https://domain.com/foo/bar?some=thing', + 'longUrl' => 'https://example.com/foo/bar?some=thing', 'forwardQuery' => $forwardQuery, ])); $this->redirectionResolver->expects($this->once())->method('resolveLongUrl')->with( @@ -54,59 +55,81 @@ public static function provideData(): iterable { $request = static fn (array $query = []) => ServerRequestFactory::fromGlobals()->withQueryParams($query); - yield ['https://domain.com/foo/bar?some=thing', $request(), null, true]; - yield ['https://domain.com/foo/bar?some=thing', $request(), null, null]; - yield ['https://domain.com/foo/bar?some=thing', $request(), null, false]; - yield ['https://domain.com/foo/bar?some=thing&else', $request(['else' => null]), null, true]; - yield ['https://domain.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, true]; - yield ['https://domain.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, null]; - yield ['https://domain.com/foo/bar?some=thing', $request(['foo' => 'bar']), null, false]; - yield ['https://domain.com/foo/bar?some=thing&123=foo', $request(['123' => 'foo']), null, true]; - yield ['https://domain.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, true]; - yield ['https://domain.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, null]; - yield ['https://domain.com/foo/bar?some=thing', $request([456 => 'foo']), null, false]; + yield ['https://example.com/foo/bar?some=thing', $request(), null, true]; + yield ['https://example.com/foo/bar?some=thing', $request(), null, null]; + yield ['https://example.com/foo/bar?some=thing', $request(), null, false]; + yield ['https://example.com/foo/bar?some=thing&else', $request(['else' => null]), null, true]; + yield ['https://example.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, true]; + yield ['https://example.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, null]; + yield ['https://example.com/foo/bar?some=thing', $request(['foo' => 'bar']), null, false]; + yield ['https://example.com/foo/bar?some=thing&123=foo', $request(['123' => 'foo']), null, true]; + yield ['https://example.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, true]; + yield ['https://example.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, null]; + yield ['https://example.com/foo/bar?some=thing', $request([456 => 'foo']), null, false]; yield [ - 'https://domain.com/foo/bar?some=overwritten&foo=bar', + 'https://example.com/foo/bar?some=overwritten&foo=bar', $request(['foo' => 'bar', 'some' => 'overwritten']), null, true, ]; yield [ - 'https://domain.com/foo/bar?some=overwritten', + 'https://example.com/foo/bar?some=overwritten', $request(['foobar' => 'notrack', 'some' => 'overwritten']), null, true, ]; yield [ - 'https://domain.com/foo/bar?some=overwritten', + 'https://example.com/foo/bar?some=overwritten', $request(['foobar' => 'notrack', 'some' => 'overwritten']), null, null, ]; yield [ - 'https://domain.com/foo/bar?some=thing', + 'https://example.com/foo/bar?some=thing', $request(['foobar' => 'notrack', 'some' => 'overwritten']), null, false, ]; - yield ['https://domain.com/foo/bar/something/else-baz?some=thing', $request(), '/something/else-baz', true]; + yield ['https://example.com/foo/bar/something/else-baz?some=thing', $request(), '/something/else-baz', true]; yield [ - 'https://domain.com/foo/bar/something/else-baz?some=thing&hello=world', + 'https://example.com/foo/bar/something/else-baz?some=thing&hello=world', $request(['hello' => 'world']), '/something/else-baz', true, ]; yield [ - 'https://domain.com/foo/bar/something/else-baz?some=thing&hello=world', + 'https://example.com/foo/bar/something/else-baz?some=thing&hello=world', $request(['hello' => 'world']), '/something/else-baz', null, ]; yield [ - 'https://domain.com/foo/bar/something/else-baz?some=thing', + 'https://example.com/foo/bar/something/else-baz?some=thing', $request(['hello' => 'world']), '/something/else-baz', false, ]; } + + /** + * @param non-empty-string $longUrl + */ + #[Test] + #[TestWith(['android://foo/bar'])] + #[TestWith(['fb://profile/33138223345'])] + #[TestWith(['viber://pa?chatURI=1234'])] + public function buildShortUrlRedirectBuildsNonHttpUrls(string $longUrl): void + { + $shortUrl = ShortUrl::withLongUrl($longUrl); + $request = ServerRequestFactory::fromGlobals(); + + $this->redirectionResolver->expects($this->once())->method('resolveLongUrl')->with( + $shortUrl, + $request, + )->willReturn($shortUrl->getLongUrl()); + + $result = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request); + + self::assertEquals($longUrl, $result); + } } diff --git a/module/Rest/test-api/Action/ListRedirectRulesTest.php b/module/Rest/test-api/Action/ListRedirectRulesTest.php index c53986c17..8f4efddae 100644 --- a/module/Rest/test-api/Action/ListRedirectRulesTest.php +++ b/module/Rest/test-api/Action/ListRedirectRulesTest.php @@ -66,7 +66,7 @@ public function errorIsReturnedWhenInvalidUrlIsFetched(): void 'conditions' => [self::LANGUAGE_EN_CONDITION], ], [ - 'longUrl' => 'https://blog.alejandrocelaya.com/android', + 'longUrl' => 'android://foo/bar', 'priority' => 4, 'conditions' => [ [ @@ -77,7 +77,7 @@ public function errorIsReturnedWhenInvalidUrlIsFetched(): void ], ], [ - 'longUrl' => 'https://blog.alejandrocelaya.com/ios', + 'longUrl' => 'fb://profile/33138223345', 'priority' => 5, 'conditions' => [ [ diff --git a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php index 267969f1c..b21f86e14 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php @@ -49,7 +49,7 @@ public function load(ObjectManager $manager): void $androidRule = new ShortUrlRedirectRule( shortUrl: $defShortUrl, priority: 4, - longUrl: 'https://blog.alejandrocelaya.com/android', + longUrl: 'android://foo/bar', conditions: new ArrayCollection([RedirectCondition::forDevice(DeviceType::ANDROID)]), ); $manager->persist($androidRule); @@ -65,7 +65,7 @@ public function load(ObjectManager $manager): void $iosRule = new ShortUrlRedirectRule( shortUrl: $defShortUrl, priority: 5, - longUrl: 'https://blog.alejandrocelaya.com/ios', + longUrl: 'fb://profile/33138223345', conditions: new ArrayCollection([RedirectCondition::forDevice(DeviceType::IOS)]), ); $manager->persist($iosRule);