Skip to content

Commit 1f9f809

Browse files
committed
refactor: lazily load CSP and Cookie only when needed by Response
1 parent d0607f1 commit 1f9f809

File tree

6 files changed

+170
-45
lines changed

6 files changed

+170
-45
lines changed

system/HTTP/Response.php

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,7 @@
1313

1414
namespace CodeIgniter\HTTP;
1515

16-
use CodeIgniter\Cookie\Cookie;
17-
use CodeIgniter\Cookie\CookieStore;
1816
use CodeIgniter\HTTP\Exceptions\HTTPException;
19-
use Config\Cookie as CookieConfig;
2017

2118
/**
2219
* Representation of an outgoing, server-side response.
@@ -38,7 +35,7 @@ class Response extends Message implements ResponseInterface
3835
/**
3936
* HTTP status codes
4037
*
41-
* @var array
38+
* @var array<int, string>
4239
*/
4340
protected static $statusCodes = [
4441
// 1xx: Informational
@@ -140,23 +137,12 @@ class Response extends Message implements ResponseInterface
140137
*/
141138
protected $pretend = false;
142139

140+
/**
141+
* Construct a non-caching response with a default content type of `text/html`.
142+
*/
143143
public function __construct()
144144
{
145-
// Default to a non-caching page.
146-
// Also ensures that a Cache-control header exists.
147-
$this->noCache();
148-
149-
// We need CSP object even if not enabled to avoid calls to non existing methods
150-
$this->CSP = service('csp');
151-
152-
$this->cookieStore = new CookieStore([]);
153-
154-
$cookie = config(CookieConfig::class);
155-
156-
Cookie::setDefaults($cookie);
157-
158-
// Default to an HTML Content-Type. Devs can override if needed.
159-
$this->setContentType('text/html');
145+
$this->noCache()->setContentType('text/html');
160146
}
161147

162148
/**
@@ -168,7 +154,6 @@ public function __construct()
168154
* @return $this
169155
*
170156
* @internal For testing purposes only.
171-
* @testTag only available to test code
172157
*/
173158
public function pretend(bool $pretend = true)
174159
{

system/HTTP/ResponseTrait.php

Lines changed: 92 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
namespace CodeIgniter\HTTP;
1515

16+
use CodeIgniter\Config\Services;
1617
use CodeIgniter\Cookie\Cookie;
1718
use CodeIgniter\Cookie\CookieStore;
1819
use CodeIgniter\Cookie\Exceptions\CookieException;
@@ -21,6 +22,8 @@
2122
use CodeIgniter\I18n\Time;
2223
use CodeIgniter\Pager\PagerInterface;
2324
use CodeIgniter\Security\Exceptions\SecurityException;
25+
use Config\App;
26+
use Config\ContentSecurityPolicy as ContentSecurityPolicyConfig;
2427
use Config\Cookie as CookieConfig;
2528
use DateTime;
2629
use DateTimeZone;
@@ -31,21 +34,30 @@
3134
* Additional methods to make a PSR-7 Response class
3235
* compliant with the framework's own ResponseInterface.
3336
*
37+
* @property array<int, string> $statusCodes
38+
* @property string|null $body
39+
*
3440
* @see https://github.com/php-fig/http-message/blob/master/src/ResponseInterface.php
3541
*/
3642
trait ResponseTrait
3743
{
3844
/**
39-
* Content security policy handler
45+
* Content security policy handler.
46+
*
47+
* Lazily instantiated on first use via `self::getCSP()` so that the
48+
* ContentSecurityPolicy class is not loaded on requests that do not use CSP.
4049
*
41-
* @var ContentSecurityPolicy
50+
* @var ContentSecurityPolicy|null
4251
*/
4352
protected $CSP;
4453

4554
/**
4655
* CookieStore instance.
4756
*
48-
* @var CookieStore
57+
* Lazily instantiated on first cookie-related call so that the Cookie and
58+
* CookieStore classes are not loaded on requests that do not use cookies.
59+
*
60+
* @var CookieStore|null
4961
*/
5062
protected $cookieStore;
5163

@@ -77,19 +89,17 @@ trait ResponseTrait
7789
*/
7890
public function setStatusCode(int $code, string $reason = '')
7991
{
80-
// Valid range?
8192
if ($code < 100 || $code > 599) {
8293
throw HTTPException::forInvalidStatusCode($code);
8394
}
8495

85-
// Unknown and no message?
86-
if (! array_key_exists($code, static::$statusCodes) && ($reason === '')) {
96+
if (! array_key_exists($code, static::$statusCodes) && $reason === '') {
8797
throw HTTPException::forUnkownStatusCode($code);
8898
}
8999

90100
$this->statusCode = $code;
91101

92-
$this->reason = ($reason !== '') ? $reason : static::$statusCodes[$code];
102+
$this->reason = $reason !== '' ? $reason : static::$statusCodes[$code];
93103

94104
return $this;
95105
}
@@ -366,8 +376,10 @@ public function setLastModified($date)
366376
public function send()
367377
{
368378
// If we're enforcing a Content Security Policy,
369-
// we need to give it a chance to build out it's headers.
370-
$this->CSP->finalize($this);
379+
// we need to give it a chance to build out its headers.
380+
if ($this->shouldFinalizeCsp()) {
381+
$this->getCSP()->finalize($this);
382+
}
371383

372384
$this->sendHeaders();
373385
$this->sendCookies();
@@ -376,6 +388,44 @@ public function send()
376388
return $this;
377389
}
378390

391+
/**
392+
* Decides whether {@see ContentSecurityPolicy::finalize()} should run for
393+
* this response. Keeping the CSP class unloaded on requests that do not
394+
* need it avoids the cost of constructing a 1000+ line service on every
395+
* request.
396+
*/
397+
private function shouldFinalizeCsp(): bool
398+
{
399+
// Developer already touched CSP through getCSP(); respect it.
400+
if ($this->CSP !== null) {
401+
return true;
402+
}
403+
404+
// A CSP instance has been registered (e.g., via Services::injectMock()
405+
// or any earlier service('csp') call) — reuse it instead of skipping.
406+
if (Services::has('csp')) {
407+
return true;
408+
}
409+
410+
if (config(App::class)->CSPEnabled) {
411+
return true;
412+
}
413+
414+
// Placeholders in the body still need to be stripped even when CSP
415+
// is disabled, so the body is scanned for the configured nonce tags
416+
// before committing to loading the full CSP class.
417+
$body = (string) $this->body;
418+
419+
if ($body === '') {
420+
return false;
421+
}
422+
423+
$cspConfig = config(ContentSecurityPolicyConfig::class);
424+
425+
return str_contains($body, $cspConfig->scriptNonceTag)
426+
|| str_contains($body, $cspConfig->styleNonceTag);
427+
}
428+
379429
/**
380430
* Sends the headers of this HTTP response to the browser.
381431
*
@@ -518,8 +568,10 @@ public function setCookie(
518568
$httponly = null,
519569
$samesite = null,
520570
) {
571+
$store = $this->initializeCookieStore();
572+
521573
if ($name instanceof Cookie) {
522-
$this->cookieStore = $this->cookieStore->put($name);
574+
$this->cookieStore = $store->put($name);
523575

524576
return $this;
525577
}
@@ -553,7 +605,7 @@ public function setCookie(
553605
'samesite' => $samesite ?? '',
554606
]);
555607

556-
$this->cookieStore = $this->cookieStore->put($cookie);
608+
$this->cookieStore = $store->put($cookie);
557609

558610
return $this;
559611
}
@@ -565,17 +617,18 @@ public function setCookie(
565617
*/
566618
public function getCookieStore()
567619
{
568-
return $this->cookieStore;
620+
return $this->initializeCookieStore();
569621
}
570622

571623
/**
572624
* Checks to see if the Response has a specified cookie or not.
573625
*/
574626
public function hasCookie(string $name, ?string $value = null, string $prefix = ''): bool
575627
{
628+
$store = $this->initializeCookieStore();
576629
$prefix = $prefix !== '' ? $prefix : Cookie::setDefaults()['prefix']; // to retain BC
577630

578-
return $this->cookieStore->has($name, $prefix, $value);
631+
return $store->has($name, $prefix, $value);
579632
}
580633

581634
/**
@@ -588,14 +641,16 @@ public function hasCookie(string $name, ?string $value = null, string $prefix =
588641
*/
589642
public function getCookie(?string $name = null, string $prefix = '')
590643
{
644+
$store = $this->initializeCookieStore();
645+
591646
if ((string) $name === '') {
592-
return $this->cookieStore->display();
647+
return $store->display();
593648
}
594649

595650
try {
596651
$prefix = $prefix !== '' ? $prefix : Cookie::setDefaults()['prefix']; // to retain BC
597652

598-
return $this->cookieStore->get($name, $prefix);
653+
return $store->get($name, $prefix);
599654
} catch (CookieException $e) {
600655
log_message('error', (string) $e);
601656

@@ -614,10 +669,10 @@ public function deleteCookie(string $name = '', string $domain = '', string $pat
614669
return $this;
615670
}
616671

672+
$store = $this->initializeCookieStore();
617673
$prefix = $prefix !== '' ? $prefix : Cookie::setDefaults()['prefix']; // to retain BC
618674

619675
$prefixed = $prefix . $name;
620-
$store = $this->cookieStore;
621676
$found = false;
622677

623678
/** @var Cookie $cookie */
@@ -653,6 +708,10 @@ public function deleteCookie(string $name = '', string $domain = '', string $pat
653708
*/
654709
public function getCookies()
655710
{
711+
if ($this->cookieStore === null) {
712+
return [];
713+
}
714+
656715
return $this->cookieStore->display();
657716
}
658717

@@ -663,7 +722,7 @@ public function getCookies()
663722
*/
664723
protected function sendCookies()
665724
{
666-
if ($this->pretend) {
725+
if ($this->pretend || $this->cookieStore === null) {
667726
return;
668727
}
669728

@@ -753,6 +812,21 @@ public function download(string $filename = '', $data = '', bool $setMime = fals
753812

754813
public function getCSP(): ContentSecurityPolicy
755814
{
756-
return $this->CSP;
815+
return $this->CSP ??= service('csp');
816+
}
817+
818+
/**
819+
* Lazily initializes the cookie store and the Cookie class defaults.
820+
* Called by every cookie-related method so cookie machinery is only
821+
* loaded when the developer actually interacts with cookies.
822+
*/
823+
private function initializeCookieStore(): CookieStore
824+
{
825+
if ($this->cookieStore === null) {
826+
Cookie::setDefaults(config(CookieConfig::class));
827+
$this->cookieStore = new CookieStore([]);
828+
}
829+
830+
return $this->cookieStore;
757831
}
758832
}

tests/system/HTTP/ResponseTest.php

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
use CodeIgniter\Config\Factories;
1717
use CodeIgniter\Config\Services;
18+
use CodeIgniter\Cookie\CookieStore;
1819
use CodeIgniter\HTTP\Exceptions\HTTPException;
1920
use CodeIgniter\Superglobals;
2021
use CodeIgniter\Test\CIUnitTestCase;
@@ -24,6 +25,7 @@
2425
use DateTimeZone;
2526
use PHPUnit\Framework\Attributes\DataProvider;
2627
use PHPUnit\Framework\Attributes\Group;
28+
use ReflectionClass;
2729

2830
/**
2931
* @internal
@@ -685,4 +687,70 @@ public function testSendRemovesPlaceholdersWhenBothCSPAndAutoNonceAreDisabled():
685687
$this->assertStringContainsString('<script >test()</script>', $actual);
686688
$this->assertStringContainsString('<style >.x{}</style>', $actual);
687689
}
690+
691+
/**
692+
* @see https://github.com/codeigniter4/CodeIgniter4/issues/8201
693+
*/
694+
public function testConstructorDoesNotLoadCspAndCookieClasses(): void
695+
{
696+
$response = (new ReflectionClass(Response::class))->newInstance();
697+
698+
$this->assertNull(
699+
$this->getPrivateProperty($response, 'CSP'),
700+
'CSP must be lazily instantiated, not loaded in Response::__construct().',
701+
);
702+
$this->assertNull(
703+
$this->getPrivateProperty($response, 'cookieStore'),
704+
'CookieStore must be lazily instantiated, not loaded in Response::__construct().',
705+
);
706+
}
707+
708+
public function testSendWithoutCspOrCookiesDoesNotLoadThoseClasses(): void
709+
{
710+
$response = new Response();
711+
$response->pretend(true);
712+
$response->setBody('<html>No CSP or cookies here.</html>');
713+
714+
ob_start();
715+
$response->send();
716+
ob_end_clean();
717+
718+
$this->assertNull($this->getPrivateProperty($response, 'CSP'));
719+
$this->assertNull($this->getPrivateProperty($response, 'cookieStore'));
720+
}
721+
722+
public function testGetCspLazilyInstantiatesCsp(): void
723+
{
724+
$response = new Response();
725+
726+
$this->assertNull($this->getPrivateProperty($response, 'CSP'));
727+
728+
$csp = $response->getCSP();
729+
730+
$this->assertInstanceOf(ContentSecurityPolicy::class, $csp);
731+
$this->assertSame($csp, $response->getCSP(), 'Subsequent getCSP() calls must return the same instance.');
732+
}
733+
734+
public function testSetCookieLazilyInstantiatesCookieStore(): void
735+
{
736+
$response = new Response();
737+
738+
$this->assertNull($this->getPrivateProperty($response, 'cookieStore'));
739+
740+
$response->setCookie('foo', 'bar');
741+
742+
$this->assertInstanceOf(CookieStore::class, $this->getPrivateProperty($response, 'cookieStore'));
743+
$this->assertTrue($response->hasCookie('foo'));
744+
}
745+
746+
public function testGetCookiesReturnsEmptyArrayWhenCookieStoreNotInitialized(): void
747+
{
748+
$response = new Response();
749+
750+
$this->assertSame([], $response->getCookies());
751+
$this->assertNull(
752+
$this->getPrivateProperty($response, 'cookieStore'),
753+
'getCookies() must not instantiate the store when no cookies have been set.',
754+
);
755+
}
688756
}

user_guide_src/source/changelogs/v4.8.0.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,9 @@ HTTP
230230
- Added ``SSEResponse`` class for streaming Server-Sent Events (SSE) over HTTP. See :ref:`server-sent-events`.
231231
- ``Response`` and its child classes no longer require ``Config\App`` passed to their constructors.
232232
Consequently, ``CURLRequest``'s ``$config`` parameter is unused and will be removed in a future release.
233+
- ``Response`` now lazily loads the ``ContentSecurityPolicy``, ``Cookie``, and ``CookieStore`` classes only when used,
234+
instead of instantiating them on every request in the constructor. Requests that do not touch CSP or cookies no longer
235+
incur the cost of loading these classes.
233236
- ``CLIRequest`` now supports the ``--`` separator to mean that what follows are arguments, not options. This allows you to have arguments that start with ``-`` without them being treated as options.
234237
For example: ``php index.php command -- --myarg`` will pass ``--myarg`` as an argument instead of an option.
235238
- ``CLIRequest`` now supports options with values specified using an equals sign (e.g., ``--option=value``) in addition to the existing space-separated syntax (e.g., ``--option value``).

0 commit comments

Comments
 (0)