Skip to content

Commit aab7037

Browse files
kenjiskenjis
authored
Merge pull request #7610 from iRedds/rework-redirect-exception
[4.4] Rework redirect exception
2 parents 10d3be6 + abb921a commit aab7037

File tree

11 files changed

+240
-34
lines changed

11 files changed

+240
-34
lines changed

‎system/CodeIgniter.php‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use CodeIgniter\HTTP\IncomingRequest;
2323
use CodeIgniter\HTTP\RedirectResponse;
2424
use CodeIgniter\HTTP\Request;
25+
use CodeIgniter\HTTP\ResponsableInterface;
2526
use CodeIgniter\HTTP\ResponseInterface;
2627
use CodeIgniter\HTTP\URI;
2728
use CodeIgniter\Router\Exceptions\RedirectException as DeprecatedRedirectException;
@@ -337,20 +338,17 @@ public function run(?RouteCollectionInterface $routes = null, bool $returnRespon
337338
$this->getRequestObject();
338339
$this->getResponseObject();
339340

340-
$this->forceSecureAccess();
341-
342341
$this->spoofRequestMethod();
343342

344343
try {
345344
$this->response = $this->handleRequest($routes, config(Cache::class), $returnResponse);
346-
} catch (RedirectException|DeprecatedRedirectException $e) {
345+
} catch (ResponsableInterface|DeprecatedRedirectException $e) {
347346
$this->outputBufferingEnd();
348-
$logger = Services::logger();
349-
$logger->info('REDIRECTED ROUTE at ' . $e->getMessage());
347+
if ($e instanceof DeprecatedRedirectException) {
348+
$e = new RedirectException($e->getMessage(), $e->getCode(), $e);
349+
}
350350

351-
// If the route is a 'redirect' route, it throws
352-
// the exception with the $to as the message
353-
$this->response->redirect(base_url($e->getMessage()), 'auto', $e->getCode());
351+
$this->response = $e->getResponse();
354352
} catch (PageNotFoundException $e) {
355353
$this->response = $this->display404errors($e);
356354
} catch (Throwable $e) {
@@ -419,6 +417,8 @@ public function disableFilters(): void
419417
*/
420418
protected function handleRequest(?RouteCollectionInterface $routes, Cache $cacheConfig, bool $returnResponse = false)
421419
{
420+
$this->forceSecureAccess();
421+
422422
if ($this->request instanceof IncomingRequest && strtolower($this->request->getMethod()) === 'cli') {
423423
return $this->response->setStatusCode(405)->setBody('Method Not Allowed');
424424
}

‎system/Common.php‎

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use CodeIgniter\Files\Exceptions\FileNotFoundException;
2222
use CodeIgniter\HTTP\CLIRequest;
2323
use CodeIgniter\HTTP\Exceptions\HTTPException;
24+
use CodeIgniter\HTTP\Exceptions\RedirectException;
2425
use CodeIgniter\HTTP\IncomingRequest;
2526
use CodeIgniter\HTTP\RedirectResponse;
2627
use CodeIgniter\HTTP\RequestInterface;
@@ -476,22 +477,24 @@ function esc($data, string $context = 'html', ?string $encoding = null)
476477
* @param ResponseInterface $response
477478
*
478479
* @throws HTTPException
480+
* @throws RedirectException
479481
*/
480-
function force_https(int $duration = 31_536_000, ?RequestInterface $request = null, ?ResponseInterface $response = null)
481-
{
482-
if ($request === null) {
483-
$request = Services::request(null, true);
484-
}
482+
function force_https(
483+
int $duration = 31_536_000,
484+
?RequestInterface $request = null,
485+
?ResponseInterface $response = null
486+
) {
487+
$request ??= Services::request();
485488

486489
if (! $request instanceof IncomingRequest) {
487490
return;
488491
}
489492

490-
if ($response === null) {
491-
$response = Services::response(null, true);
492-
}
493+
$response ??= Services::response();
493494

494-
if ((ENVIRONMENT !== 'testing' && (is_cli() || $request->isSecure())) || (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'test')) {
495+
if ((ENVIRONMENT !== 'testing' && (is_cli() || $request->isSecure()))
496+
|| $request->getServer('HTTPS') === 'test'
497+
) {
495498
return; // @codeCoverageIgnore
496499
}
497500

@@ -520,13 +523,14 @@ function force_https(int $duration = 31_536_000, ?RequestInterface $request = nu
520523
);
521524

522525
// Set an HSTS header
523-
$response->setHeader('Strict-Transport-Security', 'max-age=' . $duration);
524-
$response->redirect($uri);
525-
$response->sendHeaders();
526-
527-
if (ENVIRONMENT !== 'testing') {
528-
exit(); // @codeCoverageIgnore
529-
}
526+
$response->setHeader('Strict-Transport-Security', 'max-age=' . $duration)
527+
->redirect($uri)
528+
->setStatusCode(307)
529+
->setBody('')
530+
->getCookieStore()
531+
->clear();
532+
533+
throw new RedirectException($response);
530534
}
531535
}
532536

‎system/HTTP/Exceptions/RedirectException.php‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,72 @@
1212
namespace CodeIgniter\HTTP\Exceptions;
1313

1414
use CodeIgniter\Exceptions\HTTPExceptionInterface;
15+
use CodeIgniter\HTTP\ResponsableInterface;
16+
use CodeIgniter\HTTP\ResponseInterface;
17+
use Config\Services;
1518
use Exception;
19+
use InvalidArgumentException;
20+
use LogicException;
21+
use Throwable;
1622

1723
/**
1824
* RedirectException
1925
*/
20-
class RedirectException extends Exception implements HTTPExceptionInterface
26+
class RedirectException extends Exception implements ResponsableInterface, HTTPExceptionInterface
2127
{
2228
/**
2329
* HTTP status code for redirects
2430
*
2531
* @var int
2632
*/
2733
protected $code = 302;
34+
35+
protected ?ResponseInterface $response = null;
36+
37+
/**
38+
* @param ResponseInterface|string $message Response object or a string containing a relative URI.
39+
* @param int $code HTTP status code to redirect if $message is a string.
40+
*/
41+
public function __construct($message = '', int $code = 0, ?Throwable $previous = null)
42+
{
43+
if (! is_string($message) && ! $message instanceof ResponseInterface) {
44+
throw new InvalidArgumentException(
45+
'RedirectException::__construct() first argument must be a string or ResponseInterface',
46+
0,
47+
$this
48+
);
49+
}
50+
51+
if ($message instanceof ResponseInterface) {
52+
$this->response = $message;
53+
$message = '';
54+
55+
if ($this->response->getHeaderLine('Location') === '' && $this->response->getHeaderLine('Refresh') === '') {
56+
throw new LogicException(
57+
'The Response object passed to RedirectException does not contain a redirect address.'
58+
);
59+
}
60+
61+
if ($this->response->getStatusCode() < 301 || $this->response->getStatusCode() > 308) {
62+
$this->response->setStatusCode($this->code);
63+
}
64+
}
65+
66+
parent::__construct($message, $code, $previous);
67+
}
68+
69+
public function getResponse(): ResponseInterface
70+
{
71+
if (null === $this->response) {
72+
$this->response = Services::response()
73+
->redirect(base_url($this->getMessage()), 'auto', $this->getCode());
74+
}
75+
76+
Services::logger()->info(
77+
'REDIRECTED ROUTE at '
78+
. ($this->response->getHeaderLine('Location') ?: substr($this->response->getHeaderLine('Refresh'), 6))
79+
);
80+
81+
return $this->response;
82+
}
2883
}

‎system/HTTP/ResponsableInterface.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?php
2+
3+
/**
4+
* This file is part of CodeIgniter 4 framework.
5+
*
6+
* (c) CodeIgniter Foundation <[email protected]>
7+
*
8+
* For the full copyright and license information, please view
9+
* the LICENSE file that was distributed with this source code.
10+
*/
11+
12+
namespace CodeIgniter\HTTP;
13+
14+
interface ResponsableInterface
15+
{
16+
public function getResponse(): ResponseInterface;
17+
}

‎tests/system/CodeIgniterTest.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,9 +441,7 @@ public function testRunForceSecure()
441441
$response = $this->getPrivateProperty($codeigniter, 'response');
442442
$this->assertNull($response->header('Location'));
443443

444-
ob_start();
445-
$codeigniter->run();
446-
ob_get_clean();
444+
$response = $codeigniter->run(null, true);
447445

448446
$this->assertSame('https://example.com/', $response->header('Location')->getValue());
449447
}

‎tests/system/CommonFunctionsTest.php‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use CodeIgniter\Config\BaseService;
1515
use CodeIgniter\Config\Factories;
1616
use CodeIgniter\HTTP\CLIRequest;
17+
use CodeIgniter\HTTP\Exceptions\RedirectException;
1718
use CodeIgniter\HTTP\IncomingRequest;
1819
use CodeIgniter\HTTP\RedirectResponse;
1920
use CodeIgniter\HTTP\Response;
@@ -35,6 +36,7 @@
3536
use Config\Routing;
3637
use Config\Services;
3738
use Config\Session as SessionConfig;
39+
use Exception;
3840
use Kint;
3941
use RuntimeException;
4042
use stdClass;
@@ -599,10 +601,23 @@ public function testViewNotSaveData()
599601
public function testForceHttpsNullRequestAndResponse()
600602
{
601603
$this->assertNull(Services::response()->header('Location'));
604+
Services::response()->setCookie('force', 'cookie');
605+
Services::response()->setHeader('Force', 'header');
606+
Services::response()->setBody('default body');
607+
608+
try {
609+
force_https();
610+
} catch (Exception $e) {
611+
$this->assertInstanceOf(RedirectException::class, $e);
612+
$this->assertSame('https://example.com/', $e->getResponse()->header('Location')->getValue());
613+
$this->assertFalse($e->getResponse()->hasCookie('force'));
614+
$this->assertSame('header', $e->getResponse()->getHeaderLine('Force'));
615+
$this->assertSame('', $e->getResponse()->getBody());
616+
$this->assertSame(307, $e->getResponse()->getStatusCode());
617+
}
602618

619+
$this->expectException(RedirectException::class);
603620
force_https();
604-
605-
$this->assertSame('https://example.com/', Services::response()->header('Location')->getValue());
606621
}
607622

608623
/**

‎tests/system/ControllerTest.php‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace CodeIgniter;
1313

1414
use CodeIgniter\Config\Factories;
15+
use CodeIgniter\HTTP\Exceptions\RedirectException;
1516
use CodeIgniter\HTTP\IncomingRequest;
1617
use CodeIgniter\HTTP\Request;
1718
use CodeIgniter\HTTP\Response;
@@ -75,10 +76,13 @@ public function testConstructorHTTPS()
7576
$original = $_SERVER;
7677
$_SERVER = ['HTTPS' => 'on'];
7778
// make sure we can instantiate one
78-
$this->controller = new class () extends Controller {
79-
protected $forceHTTPS = 1;
80-
};
81-
$this->controller->initController($this->request, $this->response, $this->logger);
79+
try {
80+
$this->controller = new class () extends Controller {
81+
protected $forceHTTPS = 1;
82+
};
83+
$this->controller->initController($this->request, $this->response, $this->logger);
84+
} catch (RedirectException $e) {
85+
}
8286

8387
$this->assertInstanceOf(Controller::class, $this->controller);
8488
$_SERVER = $original; // restore so code coverage doesn't break

‎tests/system/HTTP/RedirectExceptionTest.php‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
<?php
2+
3+
/**
4+
* This file is part of CodeIgniter 4 framework.
5+
*
6+
* (c) CodeIgniter Foundation <[email protected]>
7+
*
8+
* For the full copyright and license information, please view
9+
* the LICENSE file that was distributed with this source code.
10+
*/
11+
12+
namespace CodeIgniter\HTTP;
13+
14+
use CodeIgniter\HTTP\Exceptions\RedirectException;
15+
use CodeIgniter\Log\Logger;
16+
use CodeIgniter\Test\Mock\MockLogger as LoggerConfig;
17+
use Config\Services;
18+
use LogicException;
19+
use PHPUnit\Framework\TestCase;
20+
use Tests\Support\Log\Handlers\TestHandler;
21+
22+
/**
23+
* @internal
24+
*
25+
* @group Others
26+
*/
27+
final class RedirectExceptionTest extends TestCase
28+
{
29+
protected function setUp(): void
30+
{
31+
Services::reset();
32+
Services::injectMock('logger', new Logger(new LoggerConfig()));
33+
}
34+
35+
public function testResponse(): void
36+
{
37+
$response = (new RedirectException(
38+
Services::response()
39+
->redirect('redirect')
40+
->setCookie('cookie', 'value')
41+
->setHeader('Redirect-Header', 'value')
42+
))->getResponse();
43+
44+
$this->assertSame('redirect', $response->getHeaderLine('location'));
45+
$this->assertSame(302, $response->getStatusCode());
46+
$this->assertSame('value', $response->getHeaderLine('Redirect-Header'));
47+
$this->assertSame('value', $response->getCookie('cookie')->getValue());
48+
}
49+
50+
public function testResponseWithoutLocation(): void
51+
{
52+
$this->expectException(LogicException::class);
53+
$this->expectExceptionMessage(
54+
'The Response object passed to RedirectException does not contain a redirect address.'
55+
);
56+
57+
new RedirectException(Services::response());
58+
}
59+
60+
public function testResponseWithoutStatusCode(): void
61+
{
62+
$response = (new RedirectException(Services::response()->setHeader('Location', 'location')))->getResponse();
63+
64+
$this->assertSame('location', $response->getHeaderLine('location'));
65+
$this->assertSame(302, $response->getStatusCode());
66+
}
67+
68+
public function testLoggingLocationHeader(): void
69+
{
70+
$uri = 'http://location';
71+
$expected = 'INFO - ' . date('Y-m-d') . ' --> REDIRECTED ROUTE at ' . $uri;
72+
$response = (new RedirectException(Services::response()->redirect($uri)))->getResponse();
73+
74+
$logs = TestHandler::getLogs();
75+
76+
$this->assertSame($uri, $response->getHeaderLine('Location'));
77+
$this->assertSame('', $response->getHeaderLine('Refresh'));
78+
$this->assertSame($expected, $logs[0]);
79+
}
80+
81+
public function testLoggingRefreshHeader(): void
82+
{
83+
$uri = 'http://location';
84+
$expected = 'INFO - ' . date('Y-m-d') . ' --> REDIRECTED ROUTE at ' . $uri;
85+
$response = (new RedirectException(Services::response()->redirect($uri, 'refresh')))->getResponse();
86+
87+
$logs = TestHandler::getLogs();
88+
89+
$this->assertSame($uri, substr($response->getHeaderLine('Refresh'), 6));
90+
$this->assertSame('', $response->getHeaderLine('Location'));
91+
$this->assertSame($expected, $logs[0]);
92+
}
93+
}

‎user_guide_src/source/changelogs/v4.4.0.rst‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ Helpers and Functions
104104

105105
- **Array:** Added :php:func:`array_group_by()` helper function to group data
106106
values together. Supports dot-notation syntax.
107+
- **Common:** :php:func:`force_https()` no longer terminates the application, but throws a ``RedirectException``.
107108

108109
Others
109110
======
@@ -123,6 +124,8 @@ Others
123124
- **Request:** Added ``IncomingRequest::setValidLocales()`` method to set valid locales.
124125
- **Table:** Added ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with headings. See :ref:`table-sync-rows-with-headings` for details.
125126
- **Error Handling:** Now you can use :ref:`custom-exception-handlers`.
127+
- **RedirectException:** can also take an object that implements ResponseInterface as its first argument.
128+
- **RedirectException:** implements ResponsableInterface.
126129

127130
Message Changes
128131
***************
@@ -146,6 +149,10 @@ Changes
146149
this restriction has been removed.
147150
- **RouteCollection:** The array structure of the protected property ``$routes``
148151
has been modified for performance.
152+
- **HSTS:** Now :php:func:`force_https()` or
153+
``Config\App::$forceGlobalSecureRequests = true`` sets the HTTP status code 307,
154+
which allows the HTTP request method to be preserved after the redirect.
155+
In previous versions, it was 302.
149156

150157
Deprecations
151158
************

0 commit comments

Comments
 (0)