Commit aeba438a authored by Jörn Wagner's avatar Jörn Wagner Committed by Benni Mack
Browse files

[BUGFIX] Prevent redirects causing infinite redirect loops

This patch adds a check to detect self referencing
redirects, thus avoiding them and instead log an error
in the corresponding frontend redirect middleware.

Furthermore, add a bunch of tests along the way to cover
this change and the different constellations, for example
not avoiding redirect with the same path but external host.

Resolves: #96427
Releases: main, 11.5, 10.4
Change-Id: I554ba51b53065dd754068e379f69c2a5dffc3054
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/72918

Tested-by: core-ci's avatarcore-ci <typo3@b13.com>
Tested-by: Stefan Bürk's avatarStefan Bürk <stefan@buerk.tech>
Tested-by: Benni Mack's avatarBenni Mack <benni@typo3.org>
Reviewed-by: Stefan Bürk's avatarStefan Bürk <stefan@buerk.tech>
Reviewed-by: Benni Mack's avatarBenni Mack <benni@typo3.org>
parent 8bfa7b9f
......@@ -28,6 +28,7 @@ use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Http\RedirectResponse;
use TYPO3\CMS\Core\Utility\GeneralUtility;
use TYPO3\CMS\Core\Utility\HttpUtility;
use TYPO3\CMS\Redirects\Service\RedirectService;
/**
......@@ -63,7 +64,16 @@ class RedirectHandler implements MiddlewareInterface, LoggerAwareInterface
if (is_array($matchedRedirect)) {
$url = $this->redirectService->getTargetUrl($matchedRedirect, $request);
if ($url instanceof UriInterface) {
$this->logger->debug('Redirecting', ['record' => $matchedRedirect, 'uri' => $url]);
if ($this->redirectUriWillRedirectToCurrentUri($request, $url)) {
if ($url->getFragment()) {
// Enrich error message for unsharp check with target url fragment.
$this->logger->error('Redirect ' . $url->getPath() . ' eventually points to itself! Target with fragment can not be checked and we take the safe check to avoid redirect loops. Aborting.', ['record' => $matchedRedirect, 'uri' => (string)$url]);
} else {
$this->logger->error('Redirect ' . $url->getPath() . ' points to itself! Aborting.', ['record' => $matchedRedirect, 'uri' => (string)$url]);
}
return $handler->handle($request);
}
$this->logger->debug('Redirecting', ['record' => $matchedRedirect, 'uri' => (string)$url]);
$response = $this->buildRedirectResponse($url, $matchedRedirect);
$this->incrementHitCount($matchedRedirect);
......@@ -103,4 +113,70 @@ class RedirectHandler implements MiddlewareInterface, LoggerAwareInterface
->set('lasthiton', $GLOBALS['EXEC_TIME'])
->execute();
}
/**
* Checks if redirect uri matches current request uri.
*/
protected function redirectUriWillRedirectToCurrentUri(ServerRequestInterface $request, UriInterface $redirectUri): bool
{
$requestUri = $request->getUri();
$redirectIsAbsolute = $redirectUri->getHost() && $redirectUri->getScheme();
$requestUri = $this->sanitizeUriForComparison($requestUri, !$redirectIsAbsolute);
$redirectUri = $this->sanitizeUriForComparison($redirectUri, !$redirectIsAbsolute);
return (string)$requestUri === (string)$redirectUri;
}
/**
* Strip down uri to be suitable to make valid comparison in 'redirectUriWillRedirectToCurrentUri()'
* if uri is pointing to itself and redirect should be processed.
*/
protected function sanitizeUriForComparison(UriInterface $uri, bool $relativeCheck): UriInterface
{
// Remove schema, host and port if we need to sanitize for relative check.
if ($relativeCheck) {
$uri = $uri->withScheme('')->withHost('')->withPort(null);
}
// Remove default port by schema, as they are superfluous and not meaningful enough, and even not
// set in a request uri as this depends a lot on the used webserver setup and infrastructure.
$portDefaultSchemaMap = [
// we only need web ports here, as web request could not be done over another
// schema at all, ex. ftp or mailto.
80 => 'http',
443 => 'https',
];
if (
!$relativeCheck
&& $uri->getScheme()
&& isset($portDefaultSchemaMap[$uri->getPort()])
&& $uri->getScheme() === $portDefaultSchemaMap[$uri->getPort()]
) {
$uri = $uri->withPort(null);
}
// Remove userinfo, as request would not hold it and so comparing would lead to a false-positive result
if ($uri->getUserInfo()) {
$uri = $uri->withUserInfo('');
}
// Browser should and do not hand over the fragment part in a request as this is defined to be handled
// by clients only in the protocol, thus we remove the fragment to be safe and do not end in redirect loop
// for targets with fragments because we do not get it in the request. Still not optimal but the best we
// can do in this case.
if ($uri->getFragment()) {
$uri = $uri->withFragment('');
}
// Query arguments do not have to be in the same order to be the same outcome, thus sorting them will
// give us a valid comparison, and we can correctly determine if we would have a redirect to the same uri.
// Arguments with empty values are kept, because removing them might lead to false-positives in some cases.
if ($uri->getQuery()) {
$parts = [];
parse_str($uri->getQuery(), $parts);
ksort($parts);
$uri = $uri->withQuery(HttpUtility::buildQueryString($parts));
}
return $uri;
}
}
"pages",,,,,,,,,,,
,"uid","pid","hidden","deleted","sys_language_uid","l10n_parent","slug",,,,
,1,0,0,0,0,0,"/",,,,
,10,1,0,0,0,0,"/flat-samehost-1",,,,
,11,1,0,0,0,0,"/flat-samehost-2",,,,
,12,1,0,0,0,0,"/flat-samehost-3",,,,
,13,1,0,0,0,0,"/flat-samehost-4",,,,
,20,1,0,0,0,0,"/regexp-samehost-1",,,,
,21,1,0,0,0,0,"/regexp-samehost-2",,,,
,22,1,0,0,0,0,"/regexp-samehost-3",,,,
,100,1,0,0,0,0,"/sanatize-samehost-1",,,,
,101,1,0,0,0,0,"/sanatize-samehost-2",,,,
,102,1,0,0,0,0,"/sanatize-samehost-3",,,,
,103,1,0,0,0,0,"/sanatize-samehost-4",,,,
"sys_redirect",,,,,,,,,,,
,"uid","pid","target_statuscode","disabled","deleted","source_host","source_path","target","is_regexp","respect_query_parameters","keep_query_parameters"
,1,0,301,0,0,"acme.com","/flat-samehost-1","https://external.acme.com/flat-samehost-1",0,0,0
,2,0,301,0,0,"acme.com","/flat-samehost-2","https://external.acme.com/flat-samehost-2",0,0,1
,3,0,301,0,0,"acme.com","/flat-samehost-3?param1=value1","https://external.acme.com/flat-samehost-3",0,1,0
,4,0,301,0,0,"acme.com","/flat-samehost-4?param1=value1","https://external.acme.com/flat-samehost-4",0,1,1
,5,0,301,0,0,"acme.com","#^/regexp-(samehost-1)$#","https://external.acme.com/regexp-samehost-1",1,0,0
,6,0,301,0,0,"acme.com","#^/regexp-(samehost-2)$#","https://external.acme.com/regexp-samehost-2",1,0,1
,7,0,301,0,0,"acme.com","#^/regexp-(samehost-3)(|\?param1=value1)$#","https://external.acme.com/regexp-samehost-3",1,1,0
,100,0,301,0,0,"acme.com","/sanatize-samehost-1","https://acme.com:443/sanatize-samehost-1",0,0,0
,101,0,301,0,0,"acme.com","/sanatize-samehost-2","https://user:password@acme.com/sanatize-samehost-2",0,0,0
,102,0,301,0,0,"acme.com","/sanatize-samehost-3?param1=value1&param2=value2&param3=","https://acme.com/sanatize-samehost-3?param3=&param2=value2&param1=value1",0,1,0
,103,0,301,0,0,"acme.com","/sanatize-samehost-4","https://acme.com/sanatize-samehost-4#some-fragment",0,0,0
"pages",,,,,,,,,,,
,"uid","pid","hidden","deleted","sys_language_uid","l10n_parent","slug",,,,
,1,0,0,0,0,0,"/",,,,
,10,1,0,0,0,0,"/flat-samehost-1",,,,
,11,1,0,0,0,0,"/flat-samehost-2",,,,
,12,1,0,0,0,0,"/flat-samehost-3",,,,
,13,1,0,0,0,0,"/flat-samehost-4",,,,
,20,1,0,0,0,0,"/regexp-samehost-1",,,,
,21,1,0,0,0,0,"/regexp-samehost-2",,,,
,22,1,0,0,0,0,"/regexp-samehost-3",,,,
,102,1,0,0,0,0,"/sanatize-samehost-3",,,,
,103,1,0,0,0,0,"/sanatize-samehost-4",,,,
"sys_redirect",,,,,,,,,,,
,"uid","pid","target_statuscode","disabled","deleted","source_host","source_path","target","is_regexp","respect_query_parameters","keep_query_parameters"
,1,0,301,0,0,"acme.com","/flat-samehost-1","/flat-samehost-1",0,0,0
,2,0,301,0,0,"acme.com","/flat-samehost-2","/flat-samehost-2",0,0,1
,3,0,301,0,0,"acme.com","/flat-samehost-3?param1=value1","/flat-samehost-3",0,1,0
,4,0,301,0,0,"acme.com","/flat-samehost-4?param1=value1","/flat-samehost-4",0,1,1
,5,0,301,0,0,"acme.com","#^/regexp-(samehost-1)$#","/regexp-samehost-1",1,0,0
,6,0,301,0,0,"acme.com","#^/regexp-(samehost-2)$#","/regexp-samehost-2",1,0,1
,7,0,301,0,0,"acme.com","#^/regexp-(samehost-3)(|\?param1=value1)$#","/regexp-samehost-3",1,1,0
,102,0,301,0,0,"acme.com","/sanatize-samehost-3?param1=value1&param2=value2&param3=&cHash=69f1b01feb7ed14b95b85cbc66ee2a3a","/sanatize-samehost-3?param3=&param2=value2&param1=value1&cHash=69f1b01feb7ed14b95b85cbc66ee2a3a",0,1,0
,103,0,301,0,0,"acme.com","/sanatize-samehost-4","/sanatize-samehost-4#some-fragment",0,0,0
"pages",,,,,,,,,,,
,"uid","pid","hidden","deleted","sys_language_uid","l10n_parent","slug",,,,
,1,0,0,0,0,0,"/",,,,
,10,1,0,0,0,0,"/flat-samehost-1",,,,
,11,1,0,0,0,0,"/flat-samehost-2",,,,
,12,1,0,0,0,0,"/flat-samehost-3",,,,
,13,1,0,0,0,0,"/flat-samehost-4",,,,
,20,1,0,0,0,0,"/regexp-samehost-1",,,,
,21,1,0,0,0,0,"/regexp-samehost-2",,,,
,22,1,0,0,0,0,"/regexp-samehost-3",,,,
,102,1,0,0,0,0,"/sanatize-samehost-3",,,,
,103,1,0,0,0,0,"/sanatize-samehost-4",,,,
"sys_redirect",,,,,,,,,,,
,"uid","pid","target_statuscode","disabled","deleted","source_host","source_path","target","is_regexp","respect_query_parameters","keep_query_parameters"
,1,0,301,0,0,"acme.com","/flat-samehost-1","t3://page?uid=10",0,0,0
,2,0,301,0,0,"acme.com","/flat-samehost-2","t3://page?uid=11",0,0,1
,3,0,301,0,0,"acme.com","/flat-samehost-3?param1=value1","t3://page?uid=12",0,1,0
,4,0,301,0,0,"acme.com","/flat-samehost-4?param1=value1","t3://page?uid=13",0,1,1
,5,0,301,0,0,"acme.com","#^/regexp-(samehost-1)$#","t3://page?uid=20",1,0,0
,6,0,301,0,0,"acme.com","#^/regexp-(samehost-2)$#","t3://page?uid=21",1,0,1
,7,0,301,0,0,"acme.com","#^/regexp-(samehost-3)(|\?param1=value1)$#","t3://page?uid=22",1,1,0
,102,0,301,0,0,"acme.com","/sanatize-samehost-3?param1=value1&param2=value2&param3=&cHash=69f1b01feb7ed14b95b85cbc66ee2a3a","t3://page?uid=102&param3=&param2=value2&param1=value1&cHash=69f1b01feb7ed14b95b85cbc66ee2a3a",0,1,0
,103,0,301,0,0,"acme.com","/sanatize-samehost-4","t3://page?uid=103#some-fragment",0,0,0
......@@ -319,4 +319,474 @@ class RedirectServiceTest extends FunctionalTestCase
self::assertEquals('TYPO3 Redirect ' . $expectedRedirectUid, $response->getHeader('X-Redirect-By')[0]);
self::assertEquals($expectedRedirectUri, $response->getHeader('location')[0]);
}
public function samePathWithSameDomainT3TargetDataProvider(): array
{
return [
'flat' => [
'https://acme.com/flat-samehost-1',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'flat - with query parameters' => [
'https://acme.com/flat-samehost-1?param1=value1&cHash=e0527192caa60a6dac1e30af7cfeaf64',
'https://acme.com/',
301,
'https://acme.com/flat-samehost-1',
1,
],
'flat keep_query_parameters' => [
'https://acme.com/flat-samehost-2',
'https://acme.com/',
200,
null,
null,
],
'flat keep_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-2?param1=value1&cHash=e0527192caa60a6dac1e30af7cfeaf64',
'https://acme.com/',
200,
null,
null,
],
'flat respect_query_parameters' => [
'https://acme.com/flat-samehost-3',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'flat respect_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-3?param1=value1',
'https://acme.com/',
301,
'https://acme.com/flat-samehost-3',
3,
],
'flat respect_query_parameters and keep_query_parameters' => [
'https://acme.com/flat-samehost-4',
'https://acme.com/',
200,
null,
null,
],
'flat respect_query_parameters and keep_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-4?param1=value1&cHash=caa2156411affc2d7c8c5169652c6e13',
'https://acme.com/',
200,
null,
null,
],
'regexp' => [
'https://acme.com/regexp-samehost-1',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'regexp - with query parameters' => [
'https://acme.com/regexp-samehost-1?param1=value1',
'https://acme.com/',
301,
'https://acme.com/regexp-samehost-1',
5,
],
'regexp keep_query_parameters' => [
'https://acme.com/regexp-samehost-2',
'https://acme.com/',
200,
null,
null,
],
'regexp keep_query_parameters - with query parameters' => [
'https://acme.com/regexp-samehost-2?param1=value1&cHash=feced69fa13ce7d3bf0483c21ff03064',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'regexp keep_query_parameters - with query parameters but without cHash' => [
'https://acme.com/regexp-samehost-2?param1=value1',
'https://acme.com/',
301,
'https://acme.com/regexp-samehost-2?param1=value1&cHash=feced69fa13ce7d3bf0483c21ff03064',
6,
],
'regexp respect_query_parameters' => [
'https://acme.com/regexp-samehost-3',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'regexp respect_query_parameters - with query parameters but without cHash' => [
'https://acme.com/regexp-samehost-3?param1=value1',
'https://acme.com/',
301,
'https://acme.com/regexp-samehost-3',
7,
],
'same host as external target with query arguments in another order than target should pass instead of redirect' => [
'https://acme.com/sanatize-samehost-3?param1=value1&param2=value2&param3=&cHash=69f1b01feb7ed14b95b85cbc66ee2a3a',
'https://acme.com/',
200,
null,
null,
],
'same host as external target with fragment should pass instead of redirect' => [
'https://acme.com/sanatize-samehost-4',
'https://acme.com/',
200,
null,
null,
],
];
}
/**
* @test
* @dataProvider samePathWithSameDomainT3TargetDataProvider
*/
public function samePathWithSameDomainT3Target(string $url, string $baseUri, int $expectedStatusCode, ?string $expectedRedirectUri, ?int $expectedRedirectUid): void
{
$this->importCSVDataSet(__DIR__ . '/Fixtures/RedirectService_samePathWithSameDomainT3Target.csv');
$this->writeSiteConfiguration(
'acme-com',
$this->buildSiteConfiguration(1, $baseUri)
);
$this->setUpFrontendRootPage(
1,
['typo3/sysext/redirects/Tests/Functional/Service/Fixtures/Redirects.typoscript']
);
$response = $this->executeFrontendSubRequest(
new InternalRequest($url),
null,
false
);
self::assertEquals($expectedStatusCode, $response->getStatusCode());
if ($expectedRedirectUri) {
self::assertIsArray($response->getHeader('X-Redirect-By'));
self::assertIsArray($response->getHeader('location'));
self::assertEquals('TYPO3 Redirect ' . $expectedRedirectUid, $response->getHeader('X-Redirect-By')[0]);
self::assertEquals($expectedRedirectUri, $response->getHeader('location')[0]);
}
}
public function samePathWithSameDomainAndRelativeTargetDataProvider(): array
{
return [
'flat' => [
'https://acme.com/flat-samehost-1',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'flat - with query parameters' => [
'https://acme.com/flat-samehost-1?param1=value1&cHash=e0527192caa60a6dac1e30af7cfeaf64',
'https://acme.com/',
301,
'/flat-samehost-1',
1,
],
'flat keep_query_parameters' => [
'https://acme.com/flat-samehost-2',
'https://acme.com/',
200,
null,
null,
],
'flat keep_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-2?param1=value1&cHash=e0527192caa60a6dac1e30af7cfeaf64',
'https://acme.com/',
200,
null,
null,
],
'flat respect_query_parameters' => [
'https://acme.com/flat-samehost-3',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'flat respect_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-3?param1=value1',
'https://acme.com/',
301,
'/flat-samehost-3',
3,
],
'flat respect_query_parameters and keep_query_parameters' => [
'https://acme.com/flat-samehost-4',
'https://acme.com/',
200,
null,
null,
],
'flat respect_query_parameters and keep_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-4?param1=value1&cHash=caa2156411affc2d7c8c5169652c6e13',
'https://acme.com/',
200,
null,
null,
],
'regexp' => [
'https://acme.com/regexp-samehost-1',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'regexp - with query parameters' => [
'https://acme.com/regexp-samehost-1?param1=value1',
'https://acme.com/',
301,
'/regexp-samehost-1',
5,
],
'regexp keep_query_parameters' => [
'https://acme.com/regexp-samehost-2',
'https://acme.com/',
200,
null,
null,
],
'regexp keep_query_parameters - with query parameters' => [
'https://acme.com/regexp-samehost-2?param1=value1&cHash=feced69fa13ce7d3bf0483c21ff03064',
'https://acme.com/',
200,
null,
null,
],
'regexp keep_query_parameters - with query parameters but without cHash' => [
'https://acme.com/regexp-samehost-2?param1=value1',
'https://acme.com/',
200,
null,
null,
],
'regexp respect_query_parameters' => [
'https://acme.com/regexp-samehost-3',
'https://acme.com/',
200,
null,
null,
],
// this should redirect and not pass through
'regexp respect_query_parameters - with query parameters but without cHash' => [
'https://acme.com/regexp-samehost-3?param1=value1',
'https://acme.com/',
301,
'/regexp-samehost-3',
7,
],
];
}
/**
* @test
* @dataProvider samePathWithSameDomainAndRelativeTargetDataProvider
*/
public function samePathWithSameDomainAndRelativeTarget(string $url, string $baseUri, int $expectedStatusCode, ?string $expectedRedirectUri, ?int $expectedRedirectUid): void
{
$this->importCSVDataSet(__DIR__ . '/Fixtures/RedirectService_samePathWithSameDomainAndRelativeTarget.csv');
$this->writeSiteConfiguration(
'acme-com',
$this->buildSiteConfiguration(1, $baseUri)
);
$this->setUpFrontendRootPage(
1,
['typo3/sysext/redirects/Tests/Functional/Service/Fixtures/Redirects.typoscript']
);
$response = $this->executeFrontendSubRequest(
new InternalRequest($url),
null,
false
);
self::assertEquals($expectedStatusCode, $response->getStatusCode());
if ($expectedRedirectUri) {
self::assertIsArray($response->getHeader('X-Redirect-By'));
self::assertIsArray($response->getHeader('location'));
self::assertEquals('TYPO3 Redirect ' . $expectedRedirectUid, $response->getHeader('X-Redirect-By')[0]);
self::assertEquals($expectedRedirectUri, $response->getHeader('location')[0]);
}
}
public function samePathRedirectsWithExternalTargetDataProvider(): array
{
return [
'flat' => [
'https://acme.com/flat-samehost-1',
'https://acme.com/',
301,
'https://external.acme.com/flat-samehost-1',
1,
],
'flat - with query parameters' => [
'https://acme.com/flat-samehost-1?param1=value1&cHash=e0527192caa60a6dac1e30af7cfeaf64',
'https://acme.com/',
301,
'https://external.acme.com/flat-samehost-1',
1,
],
'flat keep_query_parameters' => [
'https://acme.com/flat-samehost-2',
'https://acme.com/',
301,
'https://external.acme.com/flat-samehost-2',
2,
],
'flat keep_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-2?param1=value1',
'https://acme.com/',
301,
'https://external.acme.com/flat-samehost-2?param1=value1',
2,
],
// following will not match at all, so it is expected to be resolved with 200
'flat respect_query_parameters' => [
'https://acme.com/flat-samehost-3',
'https://acme.com/',
200,
null,
null,
],
'flat respect_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-3?param1=value1',
'https://acme.com/',
301,
'https://external.acme.com/flat-samehost-3',
3,
],
// following will not match at all, so it is expected to be resolved with 200
'flat respect_query_parameters and keep_query_parameters' => [
'https://acme.com/flat-samehost-4',
'https://acme.com/',
200,
null,
null,
],
'flat respect_query_parameters and keep_query_parameters - with query parameters' => [
'https://acme.com/flat-samehost-4?param1=value1',
'https://acme.com/',
301,
'https://external.acme.com/flat-samehost-4?param1=value1',
4,
],
'regexp' => [
'https://acme.com/regexp-samehost-1',
'https://acme.com/',
301,
'https://external.acme.com/regexp-samehost-1',
5,
],
'regexp - with query parameters' => [
'https://acme.com/regexp-samehost-1?param1=value1',
'https://acme.com/',
301,
'https://external.acme.com/regexp-samehost-1',
5,
],
'regexp keep_query_parameters' => [
'https://acme.com/regexp-samehost-2',
'https://acme.com/',
301,
'https://external.acme.com/regexp-samehost-2',
6,
],
'regexp keep_query_parameters - with query parameters' => [
'https://acme.com/regexp-samehost-2?param1=value1',
'https://acme.com/',
301,
'https://external.acme.com/regexp-samehost-2?param1=value1',
6,
],
'regexp respect_query_parameters' => [
'https://acme.com/regexp-samehost-3',
'https://acme.com/',
301,
'https://external.acme.com/regexp-samehost-3',
7,
],
// this should redirect and not pass through
'regexp respect_query_parameters - with query parameters but without cHash' => [
'https://acme.com/regexp-samehost-3?param1=value1',
'https://acme.com/',
301,