mirrors/shlink
1
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2024-11-24 05:38:06 +03:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #2063 from acelaya-forks/feature/non-http-url

Browse source 
Fix error when redirecting to a non-http URL
This commit is contained in:
Alejandro Celaya 2024-03-15 23:05:02 +01:00 committed by GitHub
commit 98992c656f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 89 additions and 40 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
CHANGELOG.md
View file

@ -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

19
module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php
View file

@ -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);
}
}

2
module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php
View file

@ -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
{

34
module/Core/test-api/Action/RedirectTest.php
View file

@ -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'));
}
}

63
module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php
View file

@ -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);
}
}

4
module/Rest/test-api/Action/ListRedirectRulesTest.php
View file

@ -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' => [
[

4
module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php
View file

@ -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);