From 1773e6ecae8283d6b2577b0039253739f7a08673 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 10 Oct 2024 11:28:31 +0200 Subject: [PATCH] Ensure query parameters are preserved verbatim when forwarded to long URL --- CHANGELOG.md | 2 +- config/autoload/rabbit.local.php.dist | 2 +- .../Helper/ShortUrlRedirectionBuilder.php | 6 ++- module/Core/test-api/Action/RedirectTest.php | 18 ++++++++ .../Helper/ShortUrlRedirectionBuilderTest.php | 41 +++++++++++-------- 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a350897f4..129e7a16c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * *Nothing* ### Fixed -* *Nothing* +* [#2213](https://github.com/shlinkio/shlink/issues/2213) Fix spaces being replaced with underscores in query parameter names, when forwarded from short URL to long URL. ## [4.2.1] - 2024-10-04 diff --git a/config/autoload/rabbit.local.php.dist b/config/autoload/rabbit.local.php.dist index d19f82c6a..37faf181c 100644 --- a/config/autoload/rabbit.local.php.dist +++ b/config/autoload/rabbit.local.php.dist @@ -7,7 +7,7 @@ return [ 'rabbitmq' => [ 'enabled' => true, 'host' => 'shlink_rabbitmq', - 'port' => '5672', + 'port' => 5672, 'user' => 'rabbit', 'password' => 'rabbit', ], diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 768fb60ee..4d801c6fa 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -28,11 +28,15 @@ public function buildShortUrlRedirect( ?string $extraPath = null, ): string { $uri = new Uri($this->redirectionResolver->resolveLongUrl($shortUrl, $request)); - $currentQuery = $request->getQueryParams(); $shouldForwardQuery = $shortUrl->forwardQuery(); $baseQueryString = $uri->getQuery(); $basePath = $uri->getPath(); + // Get current query by manually parsing query string, instead of using $request->getQueryParams(). + // That prevents some weird PHP logic in which some characters in param names are converted to ensure resulting + // names are valid variable names. + $currentQuery = Query::parse($request->getUri()->getQuery()); + return $uri ->withQuery($shouldForwardQuery ? $this->resolveQuery($baseQueryString, $currentQuery) : $baseQueryString) ->withPath($this->resolvePath($basePath, $extraPath)) diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php index dc6ca1745..36031da8e 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/module/Core/test-api/Action/RedirectTest.php @@ -122,4 +122,22 @@ public function properRedirectHappensForNonHttpLongUrls(string $longUrl): void self::assertEquals(302, $response->getStatusCode()); self::assertEquals($longUrl, $response->getHeaderLine('Location')); } + + #[Test] + public function queryParametersAreProperlyForwarded(): void + { + $slug = 'forward-query-params'; + $this->callApiWithKey('POST', '/short-urls', [ + RequestOptions::JSON => [ + 'longUrl' => 'https://example.com', + 'customSlug' => $slug, + 'forwardQuery' => true, + ], + ]); + + $response = $this->callShortUrl($slug, [RequestOptions::QUERY => ['foo bar' => '123']]); + + self::assertEquals(302, $response->getStatusCode()); + self::assertEquals('https://example.com?foo%20bar=123', $response->getHeaderLine('Location')); + } } diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index f1ffc012a..afe83c8d8 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -5,6 +5,7 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper; use Laminas\Diactoros\ServerRequestFactory; +use Laminas\Diactoros\Uri; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\TestWith; @@ -53,62 +54,70 @@ public function buildShortUrlRedirectBuildsExpectedUrl( public static function provideData(): iterable { - $request = static fn (array $query = []) => ServerRequestFactory::fromGlobals()->withQueryParams($query); + $request = static fn (string $query = '') => ServerRequestFactory::fromGlobals()->withUri( + (new Uri())->withQuery($query), + ); 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://example.com/foo/bar?some=thing&else', $request('else'), 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://example.com/foo/bar?some=overwritten&foo=bar', - $request(['foo' => 'bar', 'some' => 'overwritten']), + $request('foo=bar&some=overwritten'), null, true, ]; yield [ 'https://example.com/foo/bar?some=overwritten', - $request(['foobar' => 'notrack', 'some' => 'overwritten']), + $request('foobar=notrack&some=overwritten'), null, true, ]; yield [ 'https://example.com/foo/bar?some=overwritten', - $request(['foobar' => 'notrack', 'some' => 'overwritten']), + $request('foobar=notrack&some=overwritten'), null, null, ]; yield [ 'https://example.com/foo/bar?some=thing', - $request(['foobar' => 'notrack', 'some' => 'overwritten']), + $request('foobar=notrack&some=overwritten'), null, false, ]; yield ['https://example.com/foo/bar/something/else-baz?some=thing', $request(), '/something/else-baz', true]; yield [ 'https://example.com/foo/bar/something/else-baz?some=thing&hello=world', - $request(['hello' => 'world']), + $request('hello=world',), '/something/else-baz', true, ]; yield [ 'https://example.com/foo/bar/something/else-baz?some=thing&hello=world', - $request(['hello' => 'world']), + $request('hello=world',), '/something/else-baz', null, ]; yield [ 'https://example.com/foo/bar/something/else-baz?some=thing', - $request(['hello' => 'world']), + $request('hello=world',), '/something/else-baz', false, ]; + yield [ + 'https://example.com/foo/bar/something/else-baz?some=thing¶meter%20with%20spaces=world', + $request('parameter with spaces=world',), + '/something/else-baz', + true, + ]; } /**