Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.4] Rework redirect exception #7610

Merged
merged 8 commits into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions system/CodeIgniter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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');
}
Expand Down
36 changes: 20 additions & 16 deletions system/Common.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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);
}
}

Expand Down
57 changes: 56 additions & 1 deletion system/HTTP/Exceptions/RedirectException.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
17 changes: 17 additions & 0 deletions system/HTTP/ResponsableInterface.php
Original file line number Diff line number Diff line change
@@ -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;
}
4 changes: 1 addition & 3 deletions tests/system/CodeIgniterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
19 changes: 17 additions & 2 deletions tests/system/CommonFunctionsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -35,6 +36,7 @@
use Config\Routing;
use Config\Services;
use Config\Session as SessionConfig;
use Exception;
use Kint;
use RuntimeException;
use stdClass;
Expand Down Expand Up @@ -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());
}

/**
Expand Down
12 changes: 8 additions & 4 deletions tests/system/ControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
93 changes: 93 additions & 0 deletions tests/system/HTTP/RedirectExceptionTest.php
Original file line number Diff line number Diff line change
@@ -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]);
}
}
7 changes: 7 additions & 0 deletions user_guide_src/source/changelogs/v4.4.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
======
Expand All @@ -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
***************
Expand All @@ -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
************
Expand Down
Loading