Ensured NotFoundRedirectResolver replaces placeholders from the URL

This commit is contained in:
Alejandro Celaya 2021-10-03 16:45:13 +02:00
parent b0a8a03f0a
commit 994a28f31d
3 changed files with 72 additions and 22 deletions

View file

@ -127,7 +127,7 @@ return [
Util\DoctrineBatchHelper::class => ['em'],
Util\RedirectResponseHelper::class => [Options\UrlShortenerOptions::class],
Config\NotFoundRedirectResolver::class => [Util\RedirectResponseHelper::class],
Config\NotFoundRedirectResolver::class => [Util\RedirectResponseHelper::class, 'Logger_Shlink'],
Action\RedirectAction::class => [
Service\ShortUrl\ShortUrlResolver::class,

View file

@ -4,9 +4,11 @@ declare(strict_types=1);
namespace Shlinkio\Shlink\Core\Config;
use League\Uri\Exceptions\SyntaxError;
use League\Uri\Uri;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\UriInterface;
use Psr\Log\LoggerInterface;
use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType;
use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface;
@ -18,8 +20,10 @@ class NotFoundRedirectResolver implements NotFoundRedirectResolverInterface
private const DOMAIN_PLACEHOLDER = '{DOMAIN}';
private const ORIGINAL_PATH_PLACEHOLDER = '{ORIGINAL_PATH}';
public function __construct(private RedirectResponseHelperInterface $redirectResponseHelper)
{
public function __construct(
private RedirectResponseHelperInterface $redirectResponseHelper,
private LoggerInterface $logger,
) {
}
public function resolveRedirectResponse(
@ -48,13 +52,23 @@ class NotFoundRedirectResolver implements NotFoundRedirectResolverInterface
{
$domain = $currentUri->getAuthority();
$path = $currentUri->getPath();
$redirectUri = Uri::createFromString($redirectUrl);
try {
$redirectUri = Uri::createFromString($redirectUrl);
} catch (SyntaxError $e) {
$this->logger->warning('It was not possible to parse "{url}" as a valid URL: {e}', [
'e' => $e,
'url' => $redirectUrl,
]);
return $redirectUrl;
}
$replacePlaceholderForPattern = static fn (string $pattern, string $replace, callable $modifier) =>
static fn (?string $value) =>
$value === null ? null : str_replace($modifier($pattern), $modifier($replace), $value);
$replacePlaceholders = static fn (callable $modifier) => compose(
static fn (?string $value) =>
$value === null ? null : str_replace(self::DOMAIN_PLACEHOLDER, $modifier($domain), $value),
static fn (?string $value) =>
$value === null ? null : str_replace(self::ORIGINAL_PATH_PLACEHOLDER, $modifier($path), $value),
$replacePlaceholderForPattern(self::DOMAIN_PLACEHOLDER, $domain, $modifier),
$replacePlaceholderForPattern(self::ORIGINAL_PATH_PLACEHOLDER, $path, $modifier),
);
$replacePlaceholdersInPath = $replacePlaceholders('\Functional\id');
$replacePlaceholdersInQuery = $replacePlaceholders('\urlencode');

View file

@ -14,9 +14,10 @@ use Prophecy\Argument;
use Prophecy\PhpUnit\ProphecyTrait;
use Prophecy\Prophecy\ObjectProphecy;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\UriInterface;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Log\NullLogger;
use Shlinkio\Shlink\Core\Action\RedirectAction;
use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface;
use Shlinkio\Shlink\Core\Config\NotFoundRedirectResolver;
use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType;
use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions;
@ -28,18 +29,11 @@ class NotFoundRedirectResolverTest extends TestCase
private NotFoundRedirectResolver $resolver;
private ObjectProphecy $helper;
private NotFoundRedirectConfigInterface $config;
protected function setUp(): void
{
$this->helper = $this->prophesize(RedirectResponseHelperInterface::class);
$this->resolver = new NotFoundRedirectResolver($this->helper->reveal());
$this->config = new NotFoundRedirectOptions([
'invalidShortUrl' => 'invalidShortUrl',
'regular404' => 'regular404',
'baseUrl' => 'baseUrl',
]);
$this->resolver = new NotFoundRedirectResolver($this->helper->reveal(), new NullLogger());
}
/**
@ -47,13 +41,15 @@ class NotFoundRedirectResolverTest extends TestCase
* @dataProvider provideRedirects
*/
public function expectedRedirectionIsReturnedDependingOnTheCase(
UriInterface $uri,
NotFoundType $notFoundType,
NotFoundRedirectOptions $redirectConfig,
string $expectedRedirectTo,
): void {
$expectedResp = new Response();
$buildResp = $this->helper->buildRedirectResponse($expectedRedirectTo)->willReturn($expectedResp);
$resp = $this->resolver->resolveRedirectResponse($notFoundType, $this->config, new Uri());
$resp = $this->resolver->resolveRedirectResponse($notFoundType, $redirectConfig, $uri);
self::assertSame($expectedResp, $resp);
$buildResp->shouldHaveBeenCalledOnce();
@ -62,21 +58,61 @@ class NotFoundRedirectResolverTest extends TestCase
public function provideRedirects(): iterable
{
yield 'base URL with trailing slash' => [
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri('/'))),
$uri = new Uri('/'),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions(['baseUrl' => 'baseUrl']),
'baseUrl',
];
yield 'base URL with domain placeholder' => [
$uri = new Uri('https://doma.in'),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions(['baseUrl' => 'https://redirect-here.com/{DOMAIN}']),
'https://redirect-here.com/doma.in',
];
yield 'base URL with domain placeholder in query' => [
$uri = new Uri('https://doma.in'),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions(['baseUrl' => 'https://redirect-here.com/?domain={DOMAIN}']),
'https://redirect-here.com/?domain=doma.in',
];
yield 'base URL without trailing slash' => [
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri(''))),
$uri = new Uri(''),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions(['baseUrl' => 'baseUrl']),
'baseUrl',
];
yield 'regular 404' => [
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri('/foo/bar'))),
$uri = new Uri('/foo/bar'),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions(['regular404' => 'regular404']),
'regular404',
];
yield 'regular 404 with path placeholder in query' => [
$uri = new Uri('/foo/bar'),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions(['regular404' => 'https://redirect-here.com/?path={ORIGINAL_PATH}']),
'https://redirect-here.com/?path=%2Ffoo%2Fbar',
];
yield 'regular 404 with multiple placeholders' => [
$uri = new Uri('https://doma.in/foo/bar'),
$this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)),
new NotFoundRedirectOptions([
'regular404' => 'https://redirect-here.com/{ORIGINAL_PATH}/{DOMAIN}/?d={DOMAIN}&p={ORIGINAL_PATH}',
]),
'https://redirect-here.com//foo/bar/doma.in/?d=doma.in&p=%2Ffoo%2Fbar', // TODO Fix duplicated slash
];
yield 'invalid short URL' => [
new Uri('/foo'),
$this->notFoundType($this->requestForRoute(RedirectAction::class)),
new NotFoundRedirectOptions(['invalidShortUrl' => 'invalidShortUrl']),
'invalidShortUrl',
];
yield 'invalid short URL with path placeholder' => [
new Uri('/foo'),
$this->notFoundType($this->requestForRoute(RedirectAction::class)),
new NotFoundRedirectOptions(['invalidShortUrl' => 'https://redirect-here.com/{ORIGINAL_PATH}']),
'https://redirect-here.com//foo', // TODO Fix duplicated slash
];
}
/** @test */
@ -84,7 +120,7 @@ class NotFoundRedirectResolverTest extends TestCase
{
$notFoundType = $this->notFoundType($this->requestForRoute('foo'));
$result = $this->resolver->resolveRedirectResponse($notFoundType, $this->config, new Uri());
$result = $this->resolver->resolveRedirectResponse($notFoundType, new NotFoundRedirectOptions(), new Uri());
self::assertNull($result);
$this->helper->buildRedirectResponse(Argument::cetera())->shouldNotHaveBeenCalled();