Merge pull request #6282 from kenjis/fix-cachePage-secureheaders

fix:  page cache saves Response data before running after filters

Author: kenjis

system/CodeIgniter.php

@@ -21,7 +21,6 @@
 use CodeIgniter\HTTP\IncomingRequest;
 use CodeIgniter\HTTP\RedirectResponse;
 use CodeIgniter\HTTP\Request;
-use CodeIgniter\HTTP\RequestInterface;
 use CodeIgniter\HTTP\ResponseInterface;
 use CodeIgniter\HTTP\URI;
 use CodeIgniter\Router\Exceptions\RedirectException;
@@ -55,7 +54,7 @@ class CodeIgniter
     /**
      * App startup time.
      *
-     * @var mixed
+     * @var float|null
      */
     protected $startTime;
 
@@ -298,17 +297,18 @@ protected function initializeKint()
      * tries to route the response, loads the controller and generally
      * makes all of the pieces work together.
      *
-     * @throws Exception
      * @throws RedirectException
      *
-     * @return bool|mixed|RequestInterface|ResponseInterface|void
+     * @return ResponseInterface|void
      */
     public function run(?RouteCollectionInterface $routes = null, bool $returnResponse = false)
     {
         if ($this->context === null) {
             throw new LogicException('Context must be set before run() is called. If you are upgrading from 4.1.x, you need to merge `public/index.php` and `spark` file from `vendor/codeigniter4/framework`.');
         }
 
+        static::$cacheTTL = 0;
+
         $this->startBenchmark();
 
         $this->getRequestObject();
@@ -321,7 +321,9 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
         if ($this->request instanceof IncomingRequest && strtolower($this->request->getMethod()) === 'cli') {
             $this->response->setStatusCode(405)->setBody('Method Not Allowed');
 
-            return $this->sendResponse();
+            $this->sendResponse();
+
+            return;
         }
 
         Events::trigger('pre_system');
@@ -409,7 +411,7 @@ private function isWeb(): bool
      * @throws PageNotFoundException
      * @throws RedirectException
      *
-     * @return mixed|RequestInterface|ResponseInterface
+     * @return ResponseInterface
      */
     protected function handleRequest(?RouteCollectionInterface $routes, Cache $cacheConfig, bool $returnResponse = false)
     {
@@ -475,6 +477,9 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
         // so it can be used with the output.
         $this->gatherOutput($cacheConfig, $returned);
 
+        // After filter debug toolbar requires 'total_execution'.
+        $this->totalTime = $this->benchmark->getElapsedTime('total_execution');
+
         // Never run filters when running through Spark cli
         if (! $this->isSparked()) {
             $filters->setResponse($this->response);
@@ -496,6 +501,17 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
             $this->response = $response;
         }
 
+        // Cache it without the performance metrics replaced
+        // so that we can have live speed updates along the way.
+        // Must be run after filters to preserve the Response headers.
+        if (static::$cacheTTL > 0) {
+            $this->cachePage($cacheConfig);
+        }
+
+        // Update the performance metrics
+        $output = $this->displayPerformanceMetrics($this->response->getBody());
+        $this->response->setBody($output);
+
         // Save our current URI as the previous URI in the session
         // for safer, more accurate use with `previous_url()` helper function.
         $this->storePreviousURL(current_url(true));
@@ -641,7 +657,7 @@ protected function forceSecureAccess($duration = 31_536_000)
      *
      * @throws Exception
      *
-     * @return bool|ResponseInterface
+     * @return false|ResponseInterface
      */
     public function displayCache(Cache $config)
     {
@@ -664,7 +680,8 @@ public function displayCache(Cache $config)
                 $this->response->setHeader($name, $value);
             }
 
-            $output = $this->displayPerformanceMetrics($output);
+            $this->totalTime = $this->benchmark->getElapsedTime('total_execution');
+            $output          = $this->displayPerformanceMetrics($output);
             $this->response->setBody($output);
 
             return $this->response;
@@ -734,8 +751,6 @@ protected function generateCacheName(Cache $config): string
      */
     public function displayPerformanceMetrics(string $output): string
     {
-        $this->totalTime = $this->benchmark->getElapsedTime('total_execution');
-
         return str_replace('{elapsed_time}', (string) $this->totalTime, $output);
     }
 
@@ -956,7 +971,10 @@ protected function display404errors(PageNotFoundException $e)
      * Gathers the script output from the buffer, replaces some execution
      * time tag in the output and displays the debug toolbar, if required.
      *
+     * @param Cache|null                    $cacheConfig Deprecated. No longer used.
      * @param ResponseInterface|string|null $returned
+     *
+     * @deprecated $cacheConfig is deprecated.
      */
     protected function gatherOutput(?Cache $cacheConfig = null, $returned = null)
     {
@@ -992,14 +1010,6 @@ protected function gatherOutput(?Cache $cacheConfig = null, $returned = null)
             $this->output .= $returned;
         }
 
-        // Cache it without the performance metrics replaced
-        // so that we can have live speed updates along the way.
-        if (static::$cacheTTL > 0) {
-            $this->cachePage($cacheConfig);
-        }
-
-        $this->output = $this->displayPerformanceMetrics($this->output);
-
         $this->response->setBody($this->output);
     }
 
@@ -1069,6 +1079,8 @@ public function spoofRequestMethod()
     /**
      * Sends the output of this request back to the client.
      * This is what they've been waiting for!
+     *
+     * @return void
      */
     protected function sendResponse()
     {

system/HTTP/DownloadResponse.php

@@ -247,6 +247,8 @@ public function setCache(array $options = [])
     /**
      * {@inheritDoc}
      *
+     * @return $this
+     *
      * @todo Do downloads need CSP or Cookies? Compare with ResponseTrait::send()
      */
     public function send()

tests/system/CodeIgniterTest.php

@@ -15,8 +15,10 @@
 use CodeIgniter\HTTP\Response;
 use CodeIgniter\Router\RouteCollection;
 use CodeIgniter\Test\CIUnitTestCase;
+use CodeIgniter\Test\Filters\CITestStreamFilter;
 use CodeIgniter\Test\Mock\MockCodeIgniter;
 use Config\App;
+use Config\Filters;
 use Config\Modules;
 use Tests\Support\Filters\Customfilter;
 
@@ -531,4 +533,64 @@ public function testSpoofRequestMethodCannotUseGET()
 
         $this->assertSame('post', Services::request()->getMethod());
     }
+
+    /**
+     * @see https://github.com/codeigniter4/CodeIgniter4/issues/6281
+     */
+    public function testPageCacheSendSecureHeaders()
+    {
+        // Suppress command() output
+        CITestStreamFilter::$buffer = '';
+        $outputStreamFilter         = stream_filter_append(STDOUT, 'CITestStreamFilter');
+        $errorStreamFilter          = stream_filter_append(STDERR, 'CITestStreamFilter');
+
+        // Clear Page cache
+        command('cache:clear');
+
+        $_SERVER['REQUEST_URI'] = '/test';
+
+        $routes = Services::routes();
+        $routes->add('test', static function () {
+            CodeIgniter::cache(3600);
+
+            $response = Services::response();
+            $string = 'This is a test page. Elapsed time: {elapsed_time}';
+
+            return $response->setBody($string);
+        });
+        $router = Services::router($routes, Services::request());
+        Services::injectMock('router', $router);
+
+        /** @var Filters $filterConfig */
+        $filterConfig                   = config('Filters');
+        $filterConfig->globals['after'] = ['secureheaders'];
+        Services::filters($filterConfig);
+
+        // The first response to be cached.
+        ob_start();
+        $this->codeigniter->useSafeOutput(true)->run();
+        $output = ob_get_clean();
+
+        $this->assertStringContainsString('This is a test page', $output);
+        $response = Services::response();
+        $headers  = $response->headers();
+        $this->assertArrayHasKey('X-Frame-Options', $headers);
+
+        // The second response from the Page cache.
+        ob_start();
+        $this->codeigniter->useSafeOutput(true)->run();
+        $output = ob_get_clean();
+
+        $this->assertStringContainsString('This is a test page', $output);
+        $response = Services::response();
+        $headers  = $response->headers();
+        $this->assertArrayHasKey('X-Frame-Options', $headers);
+
+        // Clear Page cache
+        command('cache:clear');
+
+        // Remove stream fliters
+        stream_filter_remove($outputStreamFilter);
+        stream_filter_remove($errorStreamFilter);
+    }
 }

user_guide_src/source/changelogs/v4.2.2.rst

@@ -15,6 +15,7 @@ BREAKING
 - Now ``Services::request()`` returns ``IncomingRequest`` or ``CLIRequest``.
 - The method signature of ``CodeIgniter\Debug\Exceptions::__construct()`` has been changed. The ``IncomingRequest`` typehint on the ``$request`` parameter was removed. Extending classes should likewise remove the parameter so as not to break LSP.
 - The method signature of ``BaseBuilder.php::insert()`` and ``BaseBuilder.php::update()`` have been changed. The ``?array`` typehint on the ``$set`` parameter was removed.
+- A bug that caused pages to be cached before after filters were executed when using page caching has been fixed. Adding response headers or changing the response body in after filters now caches them correctly.
 
 Enhancements
 ************
@@ -33,6 +34,7 @@ Deprecations
 ************
 
 - The parameters of ``Services::request()`` are deprecated.
+- The first parameter ``$cacheConfig`` of ``CodeIgniter::gatherOutput()`` is deprecated.
 
 Bugs Fixed
 **********

user_guide_src/source/incoming/filters.rst

@@ -50,6 +50,8 @@ If a ``Response`` instance is returned, the Response will be sent back to the cl
 This can be useful for implementing rate limiting for APIs. See :doc:`Throttler </libraries/throttler>` for an
 example.
 
+.. _after-filters:
+
 After Filters
 =============
 
@@ -169,6 +171,8 @@ This filter prohibits user input data (``$_GET``, ``$_POST``, ``$_COOKIE``, ``ph
 - invalid UTF-8 characters
 - control characters except line break and tab code
 
+.. _secureheaders:
+
 SecureHeaders
 =============
 

user_guide_src/source/installation/upgrade_422.rst

@@ -19,6 +19,17 @@ Mandatory File Changes
 Breaking Changes
 ****************
 
+Web Page Caching Bug Fix
+========================
+
+- :doc:`../general/caching` now caches the Response data after :ref:`after-filters` are executed.
+- For example, if you enable :ref:`secureheaders`, the Response headers are now sent when the page comes from the cache.
+
+.. important:: If you have written **code based on this bug** that assumes changes to the Response in "after" filters are not cached then **sensitive information could be cached and compromised**. If this is the case, change your code to disable caching of the page.
+
+Others
+======
+
 - The method ``Forge::createTable()`` no longer executes a ``CREATE TABLE IF NOT EXISTS``. If table is not found in ``$db->tableExists($table)`` then ``CREATE TABLE`` is executed.
 - The second parameter ``$ifNotExists`` of ``Forge::_createTable()`` is deprecated. It is no longer used and will be removed in a future release.
 

user_guide_src/source/installation/upgrading.rst

@@ -12,6 +12,7 @@ upgrading from.
 .. toctree::
     :titlesonly:
 
+    upgrade_422
     upgrade_421
     upgrade_420
     upgrade_418