Merge pull request #7610 from iRedds/rework-redirect-exception

[4.4] Rework redirect exception

system/CodeIgniter.php

@@ -22,6 +22,7 @@
 use CodeIgniter\HTTP\IncomingRequest;
 use CodeIgniter\HTTP\RedirectResponse;
 use CodeIgniter\HTTP\Request;
+use CodeIgniter\HTTP\ResponsableInterface;
 use CodeIgniter\HTTP\ResponseInterface;
 use CodeIgniter\HTTP\URI;
 use CodeIgniter\Router\Exceptions\RedirectException as DeprecatedRedirectException;
@@ -337,20 +338,17 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
         $this->getRequestObject();
         $this->getResponseObject();
 
-        $this->forceSecureAccess();
-
         $this->spoofRequestMethod();
 
         try {
             $this->response = $this->handleRequest($routes, config(Cache::class), $returnResponse);
-        } catch (RedirectException|DeprecatedRedirectException $e) {
+        } catch (ResponsableInterface|DeprecatedRedirectException $e) {
             $this->outputBufferingEnd();
-            $logger = Services::logger();
-            $logger->info('REDIRECTED ROUTE at ' . $e->getMessage());
+            if ($e instanceof DeprecatedRedirectException) {
+                $e = new RedirectException($e->getMessage(), $e->getCode(), $e);
+            }
 
-            // If the route is a 'redirect' route, it throws
-            // the exception with the $to as the message
-            $this->response->redirect(base_url($e->getMessage()), 'auto', $e->getCode());
+            $this->response = $e->getResponse();
         } catch (PageNotFoundException $e) {
             $this->response = $this->display404errors($e);
         } catch (Throwable $e) {
@@ -419,6 +417,8 @@ public function disableFilters(): void
      */
     protected function handleRequest(?RouteCollectionInterface $routes, Cache $cacheConfig, bool $returnResponse = false)
     {
+        $this->forceSecureAccess();
+
         if ($this->request instanceof IncomingRequest && strtolower($this->request->getMethod()) === 'cli') {
             return $this->response->setStatusCode(405)->setBody('Method Not Allowed');
         }

system/Common.php

@@ -21,6 +21,7 @@
 use CodeIgniter\Files\Exceptions\FileNotFoundException;
 use CodeIgniter\HTTP\CLIRequest;
 use CodeIgniter\HTTP\Exceptions\HTTPException;
+use CodeIgniter\HTTP\Exceptions\RedirectException;
 use CodeIgniter\HTTP\IncomingRequest;
 use CodeIgniter\HTTP\RedirectResponse;
 use CodeIgniter\HTTP\RequestInterface;
@@ -476,22 +477,24 @@ function esc($data, string $context = 'html', ?string $encoding = null)
      * @param ResponseInterface $response
      *
      * @throws HTTPException
+     * @throws RedirectException
      */
-    function force_https(int $duration = 31_536_000, ?RequestInterface $request = null, ?ResponseInterface $response = null)
-    {
-        if ($request === null) {
-            $request = Services::request(null, true);
-        }
+    function force_https(
+        int $duration = 31_536_000,
+        ?RequestInterface $request = null,
+        ?ResponseInterface $response = null
+    ) {
+        $request ??= Services::request();
 
         if (! $request instanceof IncomingRequest) {
             return;
         }
 
-        if ($response === null) {
-            $response = Services::response(null, true);
-        }
+        $response ??= Services::response();
 
-        if ((ENVIRONMENT !== 'testing' && (is_cli() || $request->isSecure())) || (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'test')) {
+        if ((ENVIRONMENT !== 'testing' && (is_cli() || $request->isSecure()))
+            || $request->getServer('HTTPS') === 'test'
+        ) {
             return; // @codeCoverageIgnore
         }
 
@@ -520,13 +523,14 @@ function force_https(int $duration = 31_536_000, ?RequestInterface $request = nu
         );
 
         // Set an HSTS header
-        $response->setHeader('Strict-Transport-Security', 'max-age=' . $duration);
-        $response->redirect($uri);
-        $response->sendHeaders();
-
-        if (ENVIRONMENT !== 'testing') {
-            exit(); // @codeCoverageIgnore
-        }
+        $response->setHeader('Strict-Transport-Security', 'max-age=' . $duration)
+            ->redirect($uri)
+            ->setStatusCode(307)
+            ->setBody('')
+            ->getCookieStore()
+            ->clear();
+
+        throw new RedirectException($response);
     }
 }
 

system/HTTP/Exceptions/RedirectException.php

@@ -12,17 +12,72 @@
 namespace CodeIgniter\HTTP\Exceptions;
 
 use CodeIgniter\Exceptions\HTTPExceptionInterface;
+use CodeIgniter\HTTP\ResponsableInterface;
+use CodeIgniter\HTTP\ResponseInterface;
+use Config\Services;
 use Exception;
+use InvalidArgumentException;
+use LogicException;
+use Throwable;
 
 /**
  * RedirectException
  */
-class RedirectException extends Exception implements HTTPExceptionInterface
+class RedirectException extends Exception implements ResponsableInterface, HTTPExceptionInterface
 {
     /**
      * HTTP status code for redirects
      *
      * @var int
      */
     protected $code = 302;
+
+    protected ?ResponseInterface $response = null;
+
+    /**
+     * @param ResponseInterface|string $message Response object or a string containing a relative URI.
+     * @param int                      $code    HTTP status code to redirect if $message is a string.
+     */
+    public function __construct($message = '', int $code = 0, ?Throwable $previous = null)
+    {
+        if (! is_string($message) && ! $message instanceof ResponseInterface) {
+            throw new InvalidArgumentException(
+                'RedirectException::__construct() first argument must be a string or ResponseInterface',
+                0,
+                $this
+            );
+        }
+
+        if ($message instanceof ResponseInterface) {
+            $this->response = $message;
+            $message        = '';
+
+            if ($this->response->getHeaderLine('Location') === '' && $this->response->getHeaderLine('Refresh') === '') {
+                throw new LogicException(
+                    'The Response object passed to RedirectException does not contain a redirect address.'
+                );
+            }
+
+            if ($this->response->getStatusCode() < 301 || $this->response->getStatusCode() > 308) {
+                $this->response->setStatusCode($this->code);
+            }
+        }
+
+        parent::__construct($message, $code, $previous);
+    }
+
+    public function getResponse(): ResponseInterface
+    {
+        if (null === $this->response) {
+            $this->response = Services::response()
+                ->redirect(base_url($this->getMessage()), 'auto', $this->getCode());
+        }
+
+        Services::logger()->info(
+            'REDIRECTED ROUTE at '
+            . ($this->response->getHeaderLine('Location') ?: substr($this->response->getHeaderLine('Refresh'), 6))
+        );
+
+        return $this->response;
+    }
 }

system/HTTP/ResponsableInterface.php

@@ -0,0 +1,17 @@
+<?php
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <admin@codeigniter.com>
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+namespace CodeIgniter\HTTP;
+
+interface ResponsableInterface
+{
+    public function getResponse(): ResponseInterface;
+}

tests/system/CodeIgniterTest.php

@@ -441,9 +441,7 @@ public function testRunForceSecure()
         $response = $this->getPrivateProperty($codeigniter, 'response');
         $this->assertNull($response->header('Location'));
 
-        ob_start();
-        $codeigniter->run();
-        ob_get_clean();
+        $response = $codeigniter->run(null, true);
 
         $this->assertSame('https://example.com/', $response->header('Location')->getValue());
     }

tests/system/CommonFunctionsTest.php

@@ -14,6 +14,7 @@
 use CodeIgniter\Config\BaseService;
 use CodeIgniter\Config\Factories;
 use CodeIgniter\HTTP\CLIRequest;
+use CodeIgniter\HTTP\Exceptions\RedirectException;
 use CodeIgniter\HTTP\IncomingRequest;
 use CodeIgniter\HTTP\RedirectResponse;
 use CodeIgniter\HTTP\Response;
@@ -35,6 +36,7 @@
 use Config\Routing;
 use Config\Services;
 use Config\Session as SessionConfig;
+use Exception;
 use Kint;
 use RuntimeException;
 use stdClass;
@@ -599,10 +601,23 @@ public function testViewNotSaveData()
     public function testForceHttpsNullRequestAndResponse()
     {
         $this->assertNull(Services::response()->header('Location'));
+        Services::response()->setCookie('force', 'cookie');
+        Services::response()->setHeader('Force', 'header');
+        Services::response()->setBody('default body');
+
+        try {
+            force_https();
+        } catch (Exception $e) {
+            $this->assertInstanceOf(RedirectException::class, $e);
+            $this->assertSame('https://example.com/', $e->getResponse()->header('Location')->getValue());
+            $this->assertFalse($e->getResponse()->hasCookie('force'));
+            $this->assertSame('header', $e->getResponse()->getHeaderLine('Force'));
+            $this->assertSame('', $e->getResponse()->getBody());
+            $this->assertSame(307, $e->getResponse()->getStatusCode());
+        }
 
+        $this->expectException(RedirectException::class);
         force_https();
-
-        $this->assertSame('https://example.com/', Services::response()->header('Location')->getValue());
     }
 
     /**

tests/system/ControllerTest.php

@@ -12,6 +12,7 @@
 namespace CodeIgniter;
 
 use CodeIgniter\Config\Factories;
+use CodeIgniter\HTTP\Exceptions\RedirectException;
 use CodeIgniter\HTTP\IncomingRequest;
 use CodeIgniter\HTTP\Request;
 use CodeIgniter\HTTP\Response;
@@ -75,10 +76,13 @@ public function testConstructorHTTPS()
         $original = $_SERVER;
         $_SERVER  = ['HTTPS' => 'on'];
         // make sure we can instantiate one
-        $this->controller = new class () extends Controller {
-            protected $forceHTTPS = 1;
-        };
-        $this->controller->initController($this->request, $this->response, $this->logger);
+        try {
+            $this->controller = new class () extends Controller {
+                protected $forceHTTPS = 1;
+            };
+            $this->controller->initController($this->request, $this->response, $this->logger);
+        } catch (RedirectException $e) {
+        }
 
         $this->assertInstanceOf(Controller::class, $this->controller);
         $_SERVER = $original; // restore so code coverage doesn't break

tests/system/HTTP/RedirectExceptionTest.php

@@ -0,0 +1,93 @@
+<?php
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <admin@codeigniter.com>
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+namespace CodeIgniter\HTTP;
+
+use CodeIgniter\HTTP\Exceptions\RedirectException;
+use CodeIgniter\Log\Logger;
+use CodeIgniter\Test\Mock\MockLogger as LoggerConfig;
+use Config\Services;
+use LogicException;
+use PHPUnit\Framework\TestCase;
+use Tests\Support\Log\Handlers\TestHandler;
+
+/**
+ * @internal
+ *
+ * @group Others
+ */
+final class RedirectExceptionTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        Services::reset();
+        Services::injectMock('logger', new Logger(new LoggerConfig()));
+    }
+
+    public function testResponse(): void
+    {
+        $response = (new RedirectException(
+            Services::response()
+                ->redirect('redirect')
+                ->setCookie('cookie', 'value')
+                ->setHeader('Redirect-Header', 'value')
+        ))->getResponse();
+
+        $this->assertSame('redirect', $response->getHeaderLine('location'));
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertSame('value', $response->getHeaderLine('Redirect-Header'));
+        $this->assertSame('value', $response->getCookie('cookie')->getValue());
+    }
+
+    public function testResponseWithoutLocation(): void
+    {
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage(
+            'The Response object passed to RedirectException does not contain a redirect address.'
+        );
+
+        new RedirectException(Services::response());
+    }
+
+    public function testResponseWithoutStatusCode(): void
+    {
+        $response = (new RedirectException(Services::response()->setHeader('Location', 'location')))->getResponse();
+
+        $this->assertSame('location', $response->getHeaderLine('location'));
+        $this->assertSame(302, $response->getStatusCode());
+    }
+
+    public function testLoggingLocationHeader(): void
+    {
+        $uri      = 'http://location';
+        $expected = 'INFO - ' . date('Y-m-d') . ' --> REDIRECTED ROUTE at ' . $uri;
+        $response = (new RedirectException(Services::response()->redirect($uri)))->getResponse();
+
+        $logs = TestHandler::getLogs();
+
+        $this->assertSame($uri, $response->getHeaderLine('Location'));
+        $this->assertSame('', $response->getHeaderLine('Refresh'));
+        $this->assertSame($expected, $logs[0]);
+    }
+
+    public function testLoggingRefreshHeader(): void
+    {
+        $uri      = 'http://location';
+        $expected = 'INFO - ' . date('Y-m-d') . ' --> REDIRECTED ROUTE at ' . $uri;
+        $response = (new RedirectException(Services::response()->redirect($uri, 'refresh')))->getResponse();
+
+        $logs = TestHandler::getLogs();
+
+        $this->assertSame($uri, substr($response->getHeaderLine('Refresh'), 6));
+        $this->assertSame('', $response->getHeaderLine('Location'));
+        $this->assertSame($expected, $logs[0]);
+    }
+}

user_guide_src/source/changelogs/v4.4.0.rst

@@ -104,6 +104,7 @@ Helpers and Functions
 
 - **Array:** Added :php:func:`array_group_by()` helper function to group data
   values together. Supports dot-notation syntax.
+- **Common:** :php:func:`force_https()` no longer terminates the application, but throws a ``RedirectException``.
 
 Others
 ======
@@ -123,6 +124,8 @@ Others
 - **Request:** Added ``IncomingRequest::setValidLocales()`` method to set valid locales.
 - **Table:** Added ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with headings. See :ref:`table-sync-rows-with-headings` for details.
 - **Error Handling:** Now you can use :ref:`custom-exception-handlers`.
+- **RedirectException:** can also take an object that implements ResponseInterface as its first argument.
+- **RedirectException:** implements ResponsableInterface.
 
 Message Changes
 ***************
@@ -146,6 +149,10 @@ Changes
   this restriction has been removed.
 - **RouteCollection:** The array structure of the protected property ``$routes``
   has been modified for performance.
+- **HSTS:** Now :php:func:`force_https()` or
+  ``Config\App::$forceGlobalSecureRequests = true`` sets the HTTP status code 307,
+  which allows the HTTP request method to be preserved after the redirect.
+  In previous versions, it was 302.
 
 Deprecations
 ************