[TASK] Merge PSR-7 request and _GET/_POST parameters 43/58443/4
authorBenni Mack <benni@typo3.org>
Fri, 28 Sep 2018 20:24:53 +0000 (22:24 +0200)
committerFrank Naegler <frank.naegler@typo3.org>
Sat, 29 Sep 2018 22:14:05 +0000 (00:14 +0200)
When hooks modify _GET or _POST parameters,
it is important that these changes reflect the PSR-7 request
for now, as long as TYPO3 access the _GET/_POST parameters
via GeneralUtility::_GP().

In order to move away from global access, we still want to avoid
places where it is unclear to use $_GET/$_POST vs.
$GLOBALS['TYPO3_REQUEST'] until all parts are completely
"global-scope free" for GET/POST parameters.

The change adds the initial GET/POST parameters to the
request object in the very first middleware of the frontend.

If these have been modified when the RequestHandler builds
up the content, they are added on top of the PSR-7 request object.

Additionally, if the PSR-7 request object has been modified,
these changes are put back in the global scope to reliably use
_GPmerged within Extbase and

Additionally, if _GET/_POST have been modified, a warning will
be shown in the TimeTracker to find out that there have been
modifications.

Until then, it is safe to continue to access _GET/_POST within
Hooks and Frontend, however it is highly discouraged to *modify*
_GET/_POST directly as this functionality will be breaking in TYPO3 v10.0.

Bottom line: This safety net can now trigger deprecation warnings
if _GET/_POST have been modified during PSR-15 middleware hooks.

Bottom line 2: If these have been modified, they are put inside the
current request object.

Bottom line 3: If the request object has been modified, global state
will be modified ONCE in one place to ensure that we work with
the same object during the request phase.

Bottom line 4: We cannot get away from the current state of
running a TYPO3 Frontend Request from another source, and
we try to maintain compatibilty for legacy scripts for now. However,
this will be breaking in TYPO3 v10.0.

Resolves: #86458
Releases: master
Change-Id: Ic8f4f123bb5ea0d660e500494cf06a965dea03c4
Reviewed-on: https://review.typo3.org/58443
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Frank Naegler <frank.naegler@typo3.org>
Tested-by: Frank Naegler <frank.naegler@typo3.org>
typo3/sysext/frontend/Classes/Http/RequestHandler.php
typo3/sysext/frontend/Classes/Middleware/PreprocessRequestHook.php
typo3/sysext/frontend/Tests/Unit/Http/RequestHandlerTest.php

index 0ca7c3d..c5fd247 100644 (file)
@@ -28,6 +28,7 @@ use TYPO3\CMS\Core\Site\Entity\SiteLanguage;
 use TYPO3\CMS\Core\TimeTracker\TimeTracker;
 use TYPO3\CMS\Core\Type\File\ImageInfo;
 use TYPO3\CMS\Core\TypoScript\TypoScriptService;
+use TYPO3\CMS\Core\Utility\ArrayUtility;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\PathUtility;
 use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
@@ -75,6 +76,63 @@ class RequestHandler implements RequestHandlerInterface, PsrRequestHandlerInterf
     }
 
     /**
+     * Puts parameters that have been added or removed from the global _GET or _POST arrays
+     * into the given request (however, the PSR-7 request information takes precedence).
+     *
+     * @param ServerRequestInterface $request
+     * @return ServerRequestInterface
+     */
+    protected function addModifiedGlobalsToIncomingRequest(ServerRequestInterface $request): ServerRequestInterface
+    {
+        $originalGetParameters = $request->getAttribute('_originalGetParameters', null);
+        if ($originalGetParameters !== null && !empty($_GET) && $_GET !== $originalGetParameters) {
+            // Find out what has been changed.
+            $modifiedGetParameters = ArrayUtility::arrayDiffAssocRecursive($_GET ?? [], $originalGetParameters);
+            if (!empty($modifiedGetParameters)) {
+                $queryParams = array_replace_recursive($modifiedGetParameters, $request->getQueryParams());
+                $request = $request->withQueryParams($queryParams);
+                $GLOBALS['TYPO3_REQUEST'] = $request;
+                $this->timeTracker->setTSlogMessage('GET parameters have been modified during Request building in a hook.');
+            }
+        }
+        // do same for $_POST if the request is a POST request
+        $originalPostParameters = $request->getAttribute('_originalPostParameters', null);
+        if ($request->getMethod() === 'POST' && $originalPostParameters !== null && !empty($_POST) && $_POST !== $originalPostParameters) {
+            // Find out what has been changed
+            $modifiedPostParameters = ArrayUtility::arrayDiffAssocRecursive($_POST ?? [], $originalPostParameters);
+            if (!empty($modifiedPostParameters)) {
+                $parsedBody = array_replace_recursive($modifiedPostParameters, $request->getParsedBody());
+                $request = $request->withParsedBody($parsedBody);
+                $GLOBALS['TYPO3_REQUEST'] = $request;
+                $this->timeTracker->setTSlogMessage('POST parameters have been modified during Request building in a hook.');
+            }
+        }
+        return $request;
+    }
+
+    /**
+     * Sets the global GET and POST to the values, so if people access $_GET and $_POST
+     * Within hooks starting NOW (e.g. cObject), they get the "enriched" data from query params.
+     *
+     * This needs to be run after the request object has been enriched with modified GET/POST variables.
+     *
+     * @param ServerRequestInterface $request
+     * @internal this safety net will be removed in TYPO3 v10.
+     */
+    protected function resetGlobalsToCurrentRequest(ServerRequestInterface $request)
+    {
+        if ($request->getQueryParams() !== $_GET) {
+            $queryParams = $request->getQueryParams();
+            $_GET = $queryParams;
+            $GLOBALS['HTTP_GET_VARS'] = $_GET;
+        }
+        if ($request->getMethod() === 'POST' && $request->getParsedBody() !== $_POST) {
+            $parsedBody = $request->getParsedBody();
+            $_POST = $parsedBody;
+            $GLOBALS['HTTP_POST_VARS'] = $_POST;
+        }
+    }
+    /**
      * Handles a frontend request, after finishing running middlewares
      *
      * @param ServerRequestInterface $request
@@ -87,6 +145,10 @@ class RequestHandler implements RequestHandlerInterface, PsrRequestHandlerInterf
         /** @var TypoScriptFrontendController $controller */
         $controller = $GLOBALS['TSFE'];
 
+        // safety net, will be removed in TYPO3 v10.0. Aligns $_GET/$_POST to the incoming request.
+        $request = $this->addModifiedGlobalsToIncomingRequest($request);
+        $this->resetGlobalsToCurrentRequest($request);
+
         // Generate page
         if ($controller->isGeneratePage()) {
             $this->timeTracker->push('Page generation');
index f84cc4d..c2e49db 100644 (file)
@@ -39,6 +39,14 @@ class PreprocessRequestHook implements MiddlewareInterface
      */
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
+        // Legacy functionality to check if any hook modified global GET/POST
+        // This is a safety net, see RequestHandler for how this is validated.
+        // This information is just a compat layer which will be removed in TYPO3 v10.0.
+        $request = $request->withAttribute('_originalGetParameters', $_GET);
+        if ($request->getMethod() === 'POST') {
+            $request = $request->withAttribute('_originalPostParameters', $_POST);
+        }
+        // Set original parameters
         if (!empty($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['tslib/index_ts.php']['preprocessRequest'])) {
             trigger_error('The "preprocessRequest" hook will be removed in TYPO3 v10.0 in favor of PSR-15. Use a middleware instead.', E_USER_DEPRECATED);
             foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['tslib/index_ts.php']['preprocessRequest'] as $hookFunction) {
index 41d31cc..19c2ad4 100644 (file)
@@ -16,9 +16,11 @@ namespace TYPO3\CMS\Frontend\Tests\Unit\Http;
  */
 
 use Prophecy\Argument;
+use TYPO3\CMS\Core\Http\ServerRequestFactory;
 use TYPO3\CMS\Core\Page\PageRenderer;
 use TYPO3\CMS\Core\TimeTracker\TimeTracker;
 use TYPO3\CMS\Core\TypoScript\TemplateService;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
 use TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController;
 use TYPO3\CMS\Frontend\Http\RequestHandler;
@@ -352,4 +354,199 @@ class RequestHandlerTest extends UnitTestCase
         $pageRendererProphecy->setMetaTag($expectedTags[0]['type'], $expectedTags[0]['name'], $expectedTags[0]['content'], [], false)->shouldHaveBeenCalled();
         $pageRendererProphecy->setMetaTag($expectedTags[1]['type'], $expectedTags[1]['name'], $expectedTags[1]['content'], [], false)->shouldHaveBeenCalled();
     }
+
+    /**
+     * Test if the method is called, and the object is still the same.
+     *
+     * @test
+     */
+    public function addModifiedGlobalsToIncomingRequestFindsSameObject()
+    {
+        GeneralUtility::flushInternalRuntimeCaches();
+        $_SERVER['REQUEST_METHOD'] = 'POST';
+        $_SERVER['HTTP_HOST'] = 'https://www.example.com/my/path/';
+        $_GET = ['foo' => '1'];
+        $_POST = ['bar' => 'yo'];
+        $request = ServerRequestFactory::fromGlobals();
+        $request = $request->withAttribute('_originalGetParameters', $_GET);
+        $request = $request->withAttribute('_originalPostParameters', $_POST);
+
+        $subject = $this->getAccessibleMock(RequestHandler::class, ['dummy'], [], '', false);
+        $resultRequest = $subject->_call('addModifiedGlobalsToIncomingRequest', $request);
+        $this->assertSame($request, $resultRequest);
+    }
+
+    /**
+     * @return array
+     */
+    public function addModifiedGlobalsToIncomingRequestDataProvider()
+    {
+        return [
+            'No parameters have been modified via hook or middleware' => [
+                ['batman' => '1'],
+                ['no_cache' => 1],
+                // Enriched within PSR-7 query params + parsed body
+                [],
+                [],
+                // modified GET / POST parameters
+                [],
+                [],
+                // expected merged results
+                ['batman' => '1'],
+                ['no_cache' => 1],
+            ],
+            'No parameters have been modified via hook' => [
+                ['batman' => '1'],
+                [],
+                // Enriched within PSR-7 query params + parsed body
+                ['ARD' => 'TV', 'Oscars' => 'Cinema'],
+                ['no_cache' => '1'],
+                // modified GET / POST parameters
+                [],
+                [],
+                // expected merged results
+                ['batman' => '1', 'ARD' => 'TV', 'Oscars' => 'Cinema'],
+                ['no_cache' => 1],
+            ],
+            'Hooks and Middlewares modified' => [
+                ['batman' => '1'],
+                [],
+                // Enriched within PSR-7 query params + parsed body
+                ['ARD' => 'TV', 'Oscars' => 'Cinema'],
+                ['no_cache' => '1'],
+                // modified GET / POST parameters
+                ['batman' => '1', 'add_via_hook' => 'yes'],
+                ['submitForm' => 'download now'],
+                // expected merged results
+                ['batman' => '1', 'add_via_hook' => 'yes', 'ARD' => 'TV', 'Oscars' => 'Cinema'],
+                ['submitForm' => 'download now', 'no_cache' => 1],
+            ],
+            'Hooks and Middlewares modified with middleware overruling hooks' => [
+                ['batman' => '1'],
+                [],
+                // Enriched within PSR-7 query params + parsed body
+                ['ARD' => 'TV', 'Oscars' => 'Cinema'],
+                ['no_cache' => '1'],
+                // modified GET / POST parameters
+                ['batman' => '0', 'add_via_hook' => 'yes'],
+                ['submitForm' => 'download now', 'no_cache' => 0],
+                // expected merged results
+                ['batman' => '1', 'add_via_hook' => 'yes', 'ARD' => 'TV', 'Oscars' => 'Cinema'],
+                ['submitForm' => 'download now', 'no_cache' => 1],
+            ],
+            'Hooks and Middlewares modified with middleware overruling hooks with nested parameters' => [
+                ['batman' => '1'],
+                [['tx_siteexample_pi2' => ['uid' => 13]]],
+                // Enriched within PSR-7 query params + parsed body
+                ['ARD' => 'TV', 'Oscars' => 'Cinema', ['tx_blogexample_pi1' => ['uid' => 123]]],
+                ['no_cache' => '1', ['tx_siteexample_pi2' => ['name' => 'empty-tail']]],
+                // modified GET / POST parameters
+                ['batman' => '0', 'add_via_hook' => 'yes', ['tx_blogexample_pi1' => ['uid' => 234]]],
+                ['submitForm' => 'download now', 'no_cache' => 0],
+                // expected merged results
+                ['batman' => '1', 'add_via_hook' => 'yes', 'ARD' => 'TV', 'Oscars' => 'Cinema', ['tx_blogexample_pi1' => ['uid' => 123]]],
+                ['submitForm' => 'download now', 'no_cache' => '1', ['tx_siteexample_pi2' => ['uid' => 13, 'name' => 'empty-tail']]],
+            ],
+        ];
+    }
+
+    /**
+     * Test if the method modifies GET and POST to the expected result, when enriching an object.
+     *
+     * @param array $initialGetParams
+     * @param array $initialPostParams
+     * @param array $addedQueryParams
+     * @param array $addedParsedBody
+     * @param array $modifiedGetParams
+     * @param array $modifiedPostParams
+     * @param array $expectedQueryParams
+     * @param array $expectedParsedBody
+     * @dataProvider addModifiedGlobalsToIncomingRequestDataProvider
+     * @test
+     */
+    public function addModifiedGlobalsToIncomingRequestModifiesObject(
+        $initialGetParams,
+        $initialPostParams,
+        $addedQueryParams,
+        $addedParsedBody,
+        $modifiedGetParams,
+        $modifiedPostParams,
+        $expectedQueryParams,
+        $expectedParsedBody
+    ) {
+        GeneralUtility::flushInternalRuntimeCaches();
+        $_SERVER['REQUEST_METHOD'] = 'POST';
+        $_SERVER['HTTP_HOST'] = 'https://www.example.com/my/path/';
+        $_GET = $initialGetParams;
+        $_POST = $initialPostParams;
+        $request = ServerRequestFactory::fromGlobals();
+        $request = $request->withAttribute('_originalGetParameters', $initialGetParams);
+        $request = $request->withAttribute('_originalPostParameters', $initialPostParams);
+
+        // Now enriching the request object with other GET / POST parameters
+        $queryParams = $request->getQueryParams();
+        $queryParams = array_replace_recursive($queryParams, $addedQueryParams);
+        $request = $request->withQueryParams($queryParams);
+        $parsedBody = $request->getParsedBody() ?? [];
+        $parsedBody = array_replace_recursive($parsedBody, $addedParsedBody);
+        $request = $request->withParsedBody($parsedBody);
+
+        // Now overriding GET and POST parameters
+        $_GET = $modifiedGetParams;
+        $_POST = $modifiedPostParams;
+
+        $subject = $this->getAccessibleMock(RequestHandler::class, ['dummy'], [], '', false);
+        $subject->_set('timeTracker', new TimeTracker(false));
+        $resultRequest = $subject->_call('addModifiedGlobalsToIncomingRequest', $request);
+        $this->assertEquals($expectedQueryParams, $resultRequest->getQueryParams());
+        $this->assertEquals($expectedParsedBody, $resultRequest->getParsedBody());
+    }
+
+    /**
+     * Test if the method is called, and the globals are still the same after calling the method
+     *
+     * @test
+     */
+    public function resetGlobalsToCurrentRequestDoesNotModifyAnything()
+    {
+        $getVars = ['outside' => '1'];
+        $postVars = ['world' => 'yo'];
+        GeneralUtility::flushInternalRuntimeCaches();
+        $_SERVER['REQUEST_METHOD'] = 'POST';
+        $_SERVER['HTTP_HOST'] = 'https://www.example.com/my/path/';
+        $_GET = $getVars;
+        $_POST = $postVars;
+        $request = ServerRequestFactory::fromGlobals();
+
+        $subject = $this->getAccessibleMock(RequestHandler::class, ['dummy'], [], '', false);
+        $subject->_call('resetGlobalsToCurrentRequest', $request);
+        $this->assertEquals($_GET, $getVars);
+        $this->assertEquals($_POST, $postVars);
+    }
+
+    /**
+     * Test if the method is called, and the globals are still the same after calling the method
+     *
+     * @test
+     */
+    public function resetGlobalsToCurrentRequestWithModifiedRequestOverridesGlobals()
+    {
+        $getVars = ['typical' => '1'];
+        $postVars = ['mixtape' => 'wheels'];
+        $modifiedGetVars = ['typical' => 1, 'dont-stop' => 'the-music'];
+        $modifiedPostVars = ['mixtape' => 'wheels', 'tx_blogexample_pi1' => ['uid' => 13]];
+        GeneralUtility::flushInternalRuntimeCaches();
+        $_SERVER['REQUEST_METHOD'] = 'POST';
+        $_SERVER['HTTP_HOST'] = 'https://www.example.com/my/path/';
+        $_GET = $getVars;
+        $_POST = $postVars;
+        $request = ServerRequestFactory::fromGlobals();
+        $request = $request->withQueryParams($modifiedGetVars);
+        $request = $request->withParsedBody($modifiedPostVars);
+
+        $subject = $this->getAccessibleMock(RequestHandler::class, ['dummy'], [], '', false);
+        $subject->_call('resetGlobalsToCurrentRequest', $request);
+        $this->assertEquals($_GET, $modifiedGetVars);
+        $this->assertEquals($_POST, $modifiedPostVars);
+    }
 }