[TASK] Ensure PageArgumentValidator is not dependent on TSFE 41/60841/6
authorBenni Mack <benni@typo3.org>
Wed, 29 May 2019 20:49:25 +0000 (22:49 +0200)
committerAnja Leichsenring <aleichsenring@ab-softlab.de>
Wed, 26 Jun 2019 15:26:29 +0000 (17:26 +0200)
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>
typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php
typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
typo3/sysext/frontend/Classes/Page/PageAccessFailureReasons.php
typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php

index 6ece676..d51773c 100644 (file)
@@ -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 = [];
+        }
     }
 
     /**
index 6d95e54..7d6ff74 100644 (file)
@@ -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;
     }
 
index 4646325..88a5fc8 100644 (file)
@@ -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)',
 
index b8ac1be..698a216 100644 (file)
@@ -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());
+    }
 }