From 1312352f26c8ab019c7639fef74d7bb54c236c4b Mon Sep 17 00:00:00 2001
From: Benni Mack <benni@typo3.org>
Date: Wed, 29 May 2019 22:49:25 +0200
Subject: [PATCH] [TASK] Ensure PageArgumentValidator is not dependent on TSFE
Checking if cHash matches any GET parameters can be
done without $TSFE->cHash_array and $TSFE->hash as
all data is built inside PageArguments already.
However, $TSFE->cHash_array is still necessary and
filled as before when ->setPageArguments() is called.
This is a precursor to re-structure the dependencies within
TSFE and PSR-15 middlewares.
Resolves: #88460
Releases: master
Change-Id: I43c2fdc1049d451b3fc9bc06a57b744703a7a323
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60841
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Daniel Gorges <daniel.gorges@b13.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Daniel Gorges <daniel.gorges@b13.de>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
---
.../TypoScriptFrontendController.php | 11 ++
.../Middleware/PageArgumentValidator.php | 122 ++++++++++++------
.../Classes/Page/PageAccessFailureReasons.php | 2 +
.../Middleware/PageArgumentValidatorTest.php | 51 +++++++-
4 files changed, 143 insertions(+), 43 deletions(-)
diff --git a/typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php b/typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php
index 6ece67671bd1..d51773c150c4 100644
--- a/typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php
+++ b/typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php
@@ -63,6 +63,7 @@ use TYPO3\CMS\Core\Utility\PathUtility;
use TYPO3\CMS\Core\Utility\RootlineUtility;
use TYPO3\CMS\Frontend\Authentication\FrontendUserAuthentication;
use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
+use TYPO3\CMS\Frontend\Page\CacheHashCalculator;
use TYPO3\CMS\Frontend\Page\PageAccessFailureReasons;
use TYPO3\CMS\Frontend\Page\PageRepository;
use TYPO3\CMS\Frontend\Resource\FilePathSanitizer;
@@ -1562,6 +1563,16 @@ class TypoScriptFrontendController implements LoggerAwareInterface
public function setPageArguments(PageArguments $pageArguments)
{
$this->pageArguments = $pageArguments;
+ $queryParams = $pageArguments->getDynamicArguments();
+ // Calculated hash is stored in $this->cHash_array.
+ // This is used to cache pages with more parameters than just id and type.
+ if (!empty($queryParams) && $pageArguments->getArguments()['cHash'] ?? false) {
+ $queryParams['id'] = $pageArguments->getPageId();
+ $this->cHash_array = GeneralUtility::makeInstance(CacheHashCalculator::class)
+ ->getRelevantParameters(HttpUtility::buildQueryString($queryParams));
+ } else {
+ $this->cHash_array = [];
+ }
}
/**
diff --git a/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php b/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
index 6d95e54f5c21..7d6ff74ac032 100644
--- a/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
+++ b/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
@@ -70,29 +70,49 @@ class PageArgumentValidator implements MiddlewareInterface, LoggerAwareInterface
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{
$pageNotFoundOnValidationError = (bool)($GLOBALS['TYPO3_CONF_VARS']['FE']['pageNotFoundOnCHashError'] ?? true);
+ /** @var PageArguments $pageArguments */
$pageArguments = $request->getAttribute('routing', null);
+ if (!($pageArguments instanceof PageArguments)) {
+ // Page Arguments must be set in order to validate. This middleware only works if PageArguments
+ // is available, and is usually combined with the Page Resolver middleware
+ return GeneralUtility::makeInstance(ErrorController::class)->pageNotFoundAction(
+ $request,
+ 'Page Arguments could not be resolved',
+ ['code' => PageAccessFailureReasons::INVALID_PAGE_ARGUMENTS]
+ );
+ }
if ($this->controller->no_cache && !$pageNotFoundOnValidationError) {
// No need to test anything if caching was already disabled.
- } else {
- // Evaluate the cache hash parameter or dynamic arguments when coming from a Site-based routing
- if ($pageArguments instanceof PageArguments) {
- $queryParams = $pageArguments->getDynamicArguments();
- } else {
- $queryParams = $request->getQueryParams();
- }
- if (!empty($queryParams) && !$this->evaluateCacheHashParameter($queryParams, $pageNotFoundOnValidationError)) {
- // cHash was given, but nothing to be calculated, so let's do a redirect to the current page
- // but without the cHash
- if ($this->controller->cHash && empty($this->controller->cHash_array)) {
+ return $handler->handle($request);
+ }
+ // Evaluate the cache hash parameter or dynamic arguments when coming from a Site-based routing
+ $cHash = $pageArguments->getArguments()['cHash'] ?? '';
+ $queryParams = $pageArguments->getDynamicArguments();
+ if ($cHash || !empty($queryParams)) {
+ $relevantParametersForCacheHashArgument = $this->getRelevantParametersForCacheHashCalculation($pageArguments);
+ if ($cHash) {
+ if (empty($relevantParametersForCacheHashArgument)) {
+ // cHash was given, but nothing to be calculated, so let's do a redirect to the current page
+ // but without the cHash
+ $this->logger->notice('The incoming cHash "' . $cHash . '" is given but not needed. cHash is unset');
$uri = $request->getUri();
unset($queryParams['cHash']);
$uri = $uri->withQuery(HttpUtility::buildQueryString($queryParams));
return new RedirectResponse($uri, 308);
}
+ if (!$this->evaluateCacheHashParameter($cHash, $relevantParametersForCacheHashArgument, $pageNotFoundOnValidationError)) {
+ return GeneralUtility::makeInstance(ErrorController::class)->pageNotFoundAction(
+ $request,
+ 'Request parameters could not be validated (&cHash comparison failed)',
+ ['code' => PageAccessFailureReasons::CACHEHASH_COMPARISON_FAILED]
+ );
+ }
+ // No cHash given but was required
+ } elseif (!$this->evaluateQueryParametersWithoutCacheHash($queryParams, $pageNotFoundOnValidationError)) {
return GeneralUtility::makeInstance(ErrorController::class)->pageNotFoundAction(
$request,
- 'Request parameters could not be validated (&cHash comparison failed)',
- ['code' => PageAccessFailureReasons::CACHEHASH_COMPARISON_FAILED]
+ 'Request parameters could not be validated (&cHash empty)',
+ ['code' => PageAccessFailureReasons::CACHEHASH_EMPTY]
);
}
}
@@ -100,41 +120,65 @@ class PageArgumentValidator implements MiddlewareInterface, LoggerAwareInterface
}
/**
- * Calculates a hash string based on additional parameters in the url.
+ * Filters out the arguments that are necessary for calculating cHash
*
- * Calculated hash is stored in $this->controller->cHash_array.
+ * @param PageArguments $pageArguments
+ * @return array
+ */
+ protected function getRelevantParametersForCacheHashCalculation(PageArguments $pageArguments): array
+ {
+ $queryParams = $pageArguments->getDynamicArguments();
+ $queryParams['id'] = $pageArguments->getPageId();
+ return $this->cacheHashCalculator->getRelevantParameters(HttpUtility::buildQueryString($queryParams));
+ }
+
+ /**
+ * Calculates a hash string based on additional parameters in the url.
* This is used to cache pages with more parameters than just id and type.
*
* @see TypoScriptFrontendController::reqCHash()
- * @param array $queryParams GET parameters
+ * @param string $cHash the chash to check
+ * @param array $relevantParameters GET parameters necessary for cHash calculation
* @param bool $pageNotFoundOnCacheHashError see $GLOBALS['TYPO3_CONF_VARS']['FE']['pageNotFoundOnCHashError']
* @return bool if false, then a PageNotFound response is triggered
*/
- protected function evaluateCacheHashParameter(array $queryParams, bool $pageNotFoundOnCacheHashError): bool
+ protected function evaluateCacheHashParameter(string $cHash, array $relevantParameters, bool $pageNotFoundOnCacheHashError): bool
{
- if ($this->controller->cHash) {
- $queryParams['id'] = $this->controller->id;
- $relevantParameters = $this->cacheHashCalculator->getRelevantParameters(HttpUtility::buildQueryString($queryParams));
- $this->controller->cHash_array = $relevantParameters;
- // cHash was given, but nothing to be calculated, so cHash is unset and all is good.
- if (empty($relevantParameters)) {
- $this->logger->notice('The incoming cHash "' . $this->controller->cHash . '" is given but not needed. cHash is unset');
- return false;
- }
- $calculatedCacheHash = $this->cacheHashCalculator->calculateCacheHash($relevantParameters);
- if (!hash_equals($calculatedCacheHash, $this->controller->cHash)) {
- // Early return to trigger the error controller
- if ($pageNotFoundOnCacheHashError) {
- return false;
- }
- $this->controller->no_cache = true;
- $this->getTimeTracker()->setTSlogMessage('The incoming cHash "' . $this->controller->cHash . '" and calculated cHash "' . $calculatedCacheHash . '" did not match, so caching was disabled. The fieldlist used was "' . implode(',', array_keys($this->controller->cHash_array)) . '"', 2);
- }
- // No cHash is set, check if that is correct
- } elseif ($this->cacheHashCalculator->doParametersRequireCacheHash(HttpUtility::buildQueryString($queryParams))) {
- // Will disable caching
- $this->controller->reqCHash();
+ $calculatedCacheHash = $this->cacheHashCalculator->calculateCacheHash($relevantParameters);
+ if (hash_equals($calculatedCacheHash, $cHash)) {
+ return true;
+ }
+ // Early return to trigger the error controller
+ if ($pageNotFoundOnCacheHashError) {
+ return false;
+ }
+ // Caching is disabled now (but no 404)
+ $this->controller->no_cache = true;
+ $this->getTimeTracker()->setTSlogMessage('The incoming cHash "' . $cHash . '" and calculated cHash "' . $calculatedCacheHash . '" did not match, so caching was disabled. The fieldlist used was "' . implode(',', array_keys($relevantParameters)) . '"', 2);
+ return true;
+ }
+
+ /**
+ * No cHash is set but there are query parameters, check if that is correct
+ *
+ * Should only be called if NO cHash parameter is given.
+ *
+ * @param array $dynamicArguments
+ * @param bool $pageNotFoundOnCacheHashError
+ * @return bool
+ */
+ protected function evaluateQueryParametersWithoutCacheHash(array $dynamicArguments, bool $pageNotFoundOnCacheHashError): bool
+ {
+ if (!$this->cacheHashCalculator->doParametersRequireCacheHash(HttpUtility::buildQueryString($dynamicArguments))) {
+ return true;
+ }
+ // cHash is required, but not given, so trigger a 404
+ if ($pageNotFoundOnCacheHashError) {
+ return false;
}
+ // Caching is disabled now (but no 404)
+ $this->controller->no_cache = true;
+ $this->getTimeTracker()->setTSlogMessage('TSFE->reqCHash(): No &cHash parameter was sent for GET vars though required so caching is disabled', 2);
return true;
}
diff --git a/typo3/sysext/frontend/Classes/Page/PageAccessFailureReasons.php b/typo3/sysext/frontend/Classes/Page/PageAccessFailureReasons.php
index 464632586c46..88a5fc8846c8 100644
--- a/typo3/sysext/frontend/Classes/Page/PageAccessFailureReasons.php
+++ b/typo3/sysext/frontend/Classes/Page/PageAccessFailureReasons.php
@@ -31,6 +31,7 @@ final class PageAccessFailureReasons
public const RENDERING_INSTRUCTIONS_NOT_CONFIGURED = 'rendering_instructions.type';
// Validation errors
+ public const INVALID_PAGE_ARGUMENTS = 'page.invalid_arguments';
public const CACHEHASH_COMPARISON_FAILED = 'cache_hash.comparison';
public const CACHEHASH_EMPTY = 'cache_hash.empty';
@@ -62,6 +63,7 @@ final class PageAccessFailureReasons
self::RENDERING_INSTRUCTIONS_NOT_FOUND => 'No TypoScript template found',
self::RENDERING_INSTRUCTIONS_NOT_CONFIGURED => 'The page is not configured',
+ self::INVALID_PAGE_ARGUMENTS => 'Page Arguments could not be resolved',
self::CACHEHASH_COMPARISON_FAILED => 'Request parameters could not be validated (&cHash comparison failed)',
self::CACHEHASH_EMPTY => 'Request parameters could not be validated (&cHash empty)',
diff --git a/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php b/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php
index b8ac1bea1ae1..698a216a1173 100644
--- a/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php
+++ b/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php
@@ -76,8 +76,6 @@ class PageArgumentValidatorTest extends UnitTestCase
{
$incomingUrl = 'https://example.com/lotus-flower/en/mr-magpie/bloom/?cHash=XYZ';
$expectedResult = 'https://example.com/lotus-flower/en/mr-magpie/bloom/';
- $this->controller->id = 13;
- $this->controller->cHash = 'XYZ';
$pageArguments = new PageArguments(13, '1', ['cHash' => 'XYZ'], ['parameter-from' => 'path']);
@@ -98,8 +96,6 @@ class PageArgumentValidatorTest extends UnitTestCase
public function givenCacheHashNotMatchingCalculatedCacheHashTriggers404(): void
{
$incomingUrl = 'https://example.com/lotus-flower/en/mr-magpie/bloom/?cHash=YAZ';
- $this->controller->id = 13;
- $this->controller->cHash = 'XYZ';
$pageArguments = new PageArguments(13, '1', ['cHash' => 'XYZ', 'dynamic' => 'argument'], ['parameter-from' => 'path']);
@@ -110,4 +106,51 @@ class PageArgumentValidatorTest extends UnitTestCase
$response = $subject->process($request, $this->responseOutputHandler);
static::assertEquals(404, $response->getStatusCode());
}
+
+ /**
+ * @test
+ */
+ public function noPageArgumentsReturnsErrorResponse()
+ {
+ $incomingUrl = 'https://king.com/lotus-flower/en/mr-magpie/bloom/';
+ $request = new ServerRequest($incomingUrl, 'GET');
+
+ $subject = new PageArgumentValidator($this->controller);
+ $response = $subject->process($request, $this->responseOutputHandler);
+ static::assertEquals(404, $response->getStatusCode());
+ }
+
+ /**
+ * @test
+ */
+ public function staticPageArgumentsSkipProcessingAndReturnsSuccess()
+ {
+ $incomingUrl = 'https://example.com/lotus-flower/en/mr-magpie/bloom/';
+
+ $pageArguments = new PageArguments(13, '1', [], ['parameter-from' => 'path']);
+
+ $request = new ServerRequest($incomingUrl, 'GET');
+ $request = $request->withAttribute('routing', $pageArguments);
+
+ $subject = new PageArgumentValidator($this->controller);
+ $response = $subject->process($request, $this->responseOutputHandler);
+ static::assertEquals(200, $response->getStatusCode());
+ }
+
+ /**
+ * @test
+ */
+ public function invalidCacheHashWithDynamicArgumentsTriggers404()
+ {
+ $incomingUrl = 'https://example.com/lotus-flower/en/mr-magpie/bloom/';
+
+ $pageArguments = new PageArguments(13, '1', ['cHash' => 'coolio', 'download' => true], ['parameter-from' => 'path']);
+
+ $request = new ServerRequest($incomingUrl, 'GET');
+ $request = $request->withAttribute('routing', $pageArguments);
+
+ $subject = new PageArgumentValidator($this->controller);
+ $response = $subject->process($request, $this->responseOutputHandler);
+ static::assertEquals(404, $response->getStatusCode());
+ }
}
--
GitLab