diff --git a/CHANGELOG.md b/CHANGELOG.md index abb8bf89..e646df82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ 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). -## [Unreleased] +## [4.0.3] - 2024-03-15 ### Added * *Nothing* @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### 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 diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 3a6a6d06..768fb60e 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -5,7 +5,7 @@ declare(strict_types=1); 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 @@ readonly class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderI $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 @@ readonly class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderI } // 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 c8f96bba..7780f91b 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -87,7 +87,7 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface } /** - * @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 e8cf211a..2c54e8a7 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/module/Core/test-api/Action/RedirectTest.php @@ -7,6 +7,7 @@ namespace ShlinkioApiTest\Shlink\Core\Action; 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 @@ use const ShlinkioTest\Shlink\IOS_USER_AGENT; 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 @@ class RedirectTest extends ApiTestCase 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 @@ class RedirectTest extends ApiTestCase '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 53a86322..f1ffc012 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -7,6 +7,7 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper; 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 @@ class ShortUrlRedirectionBuilderTest extends TestCase ?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 @@ class ShortUrlRedirectionBuilderTest extends TestCase { $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 c53986c1..8f4efdda 100644 --- a/module/Rest/test-api/Action/ListRedirectRulesTest.php +++ b/module/Rest/test-api/Action/ListRedirectRulesTest.php @@ -66,7 +66,7 @@ class ListRedirectRulesTest extends ApiTestCase 'conditions' => [self::LANGUAGE_EN_CONDITION], ], [ - 'longUrl' => 'https://blog.alejandrocelaya.com/android', + 'longUrl' => 'android://foo/bar', 'priority' => 4, 'conditions' => [ [ @@ -77,7 +77,7 @@ class ListRedirectRulesTest extends ApiTestCase ], ], [ - '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 267969f1..b21f86e1 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php @@ -49,7 +49,7 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF $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 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF $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);