[BUGFIX] Ignore cHash when given but not needed

If a cHash GET parameter is given, but there
are no GET parameters that are relevant, a
hash_calc() call against an empty string is done.

However, the change now allows an invalid
cHash if no check is necessary. This could
happen when upgrading from older instances
where a cHash is not needed anymore.

Bots would not then fill up the error log
but get the new page (with a valid 200 result)

Resolves: #41033
Releases: master, 9.5
Change-Id: Id02701fcbece371a6b9ce0f92fe0be55dd972325
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60895
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Georg Ringer <georg.ringer@gmail.com>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Georg Ringer <georg.ringer@gmail.com>

diff --git a/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php b/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
index 016ba00..65d64a5 100644 (file)
--- a/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
+++ b/typo3/sysext/frontend/Classes/Middleware/PageArgumentValidator.php
@@ -102,15 +102,23 @@ class PageArgumentValidator implements MiddlewareInterface
     {
         if ($this->controller->cHash) {
             $queryParams['id'] = $this->controller->id;
-            $this->controller->cHash_array = $this->cacheHashCalculator->getRelevantParameters(HttpUtility::buildQueryString($queryParams));
-            $cHash_calc = $this->cacheHashCalculator->calculateCacheHash($this->controller->cHash_array);
-            if (!hash_equals($cHash_calc, $this->controller->cHash)) {
+            $relevantParameters = $this->cacheHashCalculator->getRelevantParameters(HttpUtility::buildQueryString($queryParams));
+            $this->controller->cHash_array = $relevantParameters;
+            $calculatedCacheHash = $this->cacheHashCalculator->calculateCacheHash($relevantParameters);
+            // cHash was given, but nothing to be calculated, so cHash is unset and all is good.
+            if (empty($relevantParameters)) {
+                $this->getTimeTracker()->setTSlogMessage('The incoming cHash "' . $this->controller->cHash . '" is given but not needed. cHash is unset', 2);
+                // We do not need to update the query params as everything is checked via $TSFE->cHash, see $TSFE->reqCHash()
+                $this->controller->cHash = '';
+                return true;
+            }
+            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 "' . $cHash_calc . '" did not match, so caching was disabled. The fieldlist used was "' . implode(',', array_keys($this->controller->cHash_array)) . '"', 2);
+                $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))) {

diff --git a/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php b/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php
new file mode 100644 (file)
index 0000000..af4fa0f
--- /dev/null
+++ b/typo3/sysext/frontend/Tests/Unit/Middleware/PageArgumentValidatorTest.php
@@ -0,0 +1,109 @@
+<?php
+declare(strict_types = 1);
+
+namespace TYPO3\CMS\Frontend\Tests\Unit\Middleware;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+use TYPO3\CMS\Core\Http\Response;
+use TYPO3\CMS\Core\Http\ServerRequest;
+use TYPO3\CMS\Core\Routing\PageArguments;
+use TYPO3\CMS\Core\TimeTracker\TimeTracker;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController;
+use TYPO3\CMS\Frontend\Middleware\PageArgumentValidator;
+use TYPO3\CMS\Frontend\Middleware\PageResolver;
+use TYPO3\TestingFramework\Core\AccessibleObjectInterface;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+class PageArgumentValidatorTest extends UnitTestCase
+{
+    /**
+     * @var bool Reset singletons created by subject
+     */
+    protected $resetSingletonInstances = true;
+
+    /**
+     * @var TypoScriptFrontendController|AccessibleObjectInterface
+     */
+    protected $controller;
+
+    /**
+     * @var RequestHandlerInterface
+     */
+    protected $responseOutputHandler;
+
+    /**
+     * @var PageResolver|AccessibleObjectInterface
+     */
+    protected $subject;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $timeTrackerStub = new TimeTracker(false);
+        GeneralUtility::setSingletonInstance(TimeTracker::class, $timeTrackerStub);
+        $this->controller = $this->getAccessibleMock(TypoScriptFrontendController::class, ['dummy'], [], '', false);
+
+        // A request handler which only runs through
+        $this->responseOutputHandler = new class implements RequestHandlerInterface {
+            public function handle(ServerRequestInterface $request): ResponseInterface
+            {
+                return new Response();
+            }
+        };
+    }
+
+    /**
+     * @test
+     */
+    public function givenCacheHashWithoutRequiredParametersIgnoresCacheHashAndUnsetsCacheHash(): void
+    {
+        $incomingUrl = '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']);
+
+        $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());
+        static::assertEquals('', $this->controller->cHash);
+    }
+
+    /**
+     * @test
+     */
+    public function givenCacheHashNotMatchingCalculatedCacheHashTriggers404(): void
+    {
+        $incomingUrl = 'https://example.com/lotus-flower/en/mr-magpie/bloom/';
+        $this->controller->id = 13;
+        $this->controller->cHash = 'XYZ';
+
+        $pageArguments = new PageArguments(13, '1', ['cHash' => 'XYZ', 'dynamic' => 'argument'], ['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());
+    }
+}