Skip to content

Commit 615ef4d

Browse files
patel-vanshpatel-vansh
authored
fix: Hardcoded CSP Nonce Tags in ResponseTrait (#9937)
* Created a method to clear nonce placeholders. * Added tests * cs-fix * Fix static analysis errors * Fixed `Argument #1 ($text) must be of type string, null given` error * Apply code suggestions Co-authored-by: John Paul E. Balandan, CPA <paulbalandan@gmail.com> * Fix the failing test case * Remove unused function * Updated docs * Addressing the review on tests. * Addressing the case when both $CSPEnabled and $autoNonce are false. * Applying suggested fix. * Simplifying code according to review. * Used Services::injectMock instead of ReflectionClass
1 parent 9215c8c commit 615ef4d

File tree

4 files changed

+161
-8
lines changed

4 files changed

+161
-8
lines changed

‎system/HTTP/ContentSecurityPolicy.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -427,9 +427,7 @@ public function getScriptNonce(): string
427427
*/
428428
public function finalize(ResponseInterface $response)
429429
{
430-
if ($this->autoNonce) {
431-
$this->generateNonces($response);
432-
}
430+
$this->generateNonces($response);
433431

434432
$this->buildHeaders($response);
435433
}
@@ -892,6 +890,10 @@ protected function addOption($options, string $target, ?bool $explicitReporting
892890
*/
893891
protected function generateNonces(ResponseInterface $response)
894892
{
893+
if ($this->enabled() && ! $this->autoNonce) {
894+
return;
895+
}
896+
895897
$body = (string) $response->getBody();
896898

897899
if ($body === '') {
@@ -905,6 +907,10 @@ protected function generateNonces(ResponseInterface $response)
905907
$pattern = sprintf('/(%s|%s)/', preg_quote($this->styleNonceTag, '/'), preg_quote($this->scriptNonceTag, '/'));
906908

907909
$body = preg_replace_callback($pattern, function ($match) use ($jsonEscape): string {
910+
if (! $this->enabled()) {
911+
return '';
912+
}
913+
908914
$nonce = $match[0] === $this->styleNonceTag ? $this->getStyleNonce() : $this->getScriptNonce();
909915
$attr = 'nonce="' . $nonce . '"';
910916

@@ -923,6 +929,10 @@ protected function generateNonces(ResponseInterface $response)
923929
*/
924930
protected function buildHeaders(ResponseInterface $response)
925931
{
932+
if (! $this->enabled()) {
933+
return;
934+
}
935+
926936
$response->setHeader('Content-Security-Policy', []);
927937
$response->setHeader('Content-Security-Policy-Report-Only', []);
928938
$response->setHeader('Reporting-Endpoints', []);

‎system/HTTP/ResponseTrait.php‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -367,11 +367,7 @@ public function send()
367367
{
368368
// If we're enforcing a Content Security Policy,
369369
// we need to give it a chance to build out it's headers.
370-
if ($this->CSP->enabled()) {
371-
$this->CSP->finalize($this);
372-
} else {
373-
$this->body = str_replace(['{csp-style-nonce}', '{csp-script-nonce}'], '', $this->body ?? '');
374-
}
370+
$this->CSP->finalize($this);
375371

376372
$this->sendHeaders();
377373
$this->sendCookies();

‎tests/system/HTTP/ResponseTest.php‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,4 +577,150 @@ public function testPretendOutput(): void
577577

578578
$this->assertSame('Happy days', $actual);
579579
}
580+
581+
public function testSendRemovesDefaultNoncePlaceholdersWhenCSPDisabled(): void
582+
{
583+
$config = new App();
584+
$config->CSPEnabled = false;
585+
586+
$response = new Response($config);
587+
$response->pretend(true);
588+
589+
$body = '<html><script {csp-script-nonce}>console.log("test")</script><style {csp-style-nonce}>.test{}</style></html>';
590+
$response->setBody($body);
591+
592+
ob_start();
593+
$response->send();
594+
$actual = ob_get_clean();
595+
596+
// Nonce placeholders should be removed when CSP is disabled
597+
$this->assertIsString($actual);
598+
$this->assertStringNotContainsString('{csp-script-nonce}', $actual);
599+
$this->assertStringNotContainsString('{csp-style-nonce}', $actual);
600+
$this->assertStringContainsString('<script >console.log("test")</script>', $actual);
601+
$this->assertStringContainsString('<style >.test{}</style>', $actual);
602+
}
603+
604+
public function testSendRemovesCustomNoncePlaceholdersWhenCSPDisabled(): void
605+
{
606+
$appConfig = new App();
607+
$appConfig->CSPEnabled = false;
608+
609+
// Create custom CSP config with custom nonce tags
610+
$cspConfig = new \Config\ContentSecurityPolicy();
611+
$cspConfig->scriptNonceTag = '{custom-script-tag}';
612+
$cspConfig->styleNonceTag = '{custom-style-tag}';
613+
614+
Services::injectMock('csp', new ContentSecurityPolicy($cspConfig));
615+
616+
$response = new Response($appConfig);
617+
$response->pretend(true);
618+
619+
$body = '<html><script {custom-script-tag}>test()</script><style {custom-style-tag}>.x{}</style></html>';
620+
$response->setBody($body);
621+
622+
ob_start();
623+
$response->send();
624+
$actual = ob_get_clean();
625+
626+
// Custom nonce placeholders should be removed when CSP is disabled
627+
$this->assertIsString($actual);
628+
$this->assertStringNotContainsString('{custom-script-tag}', $actual);
629+
$this->assertStringNotContainsString('{custom-style-tag}', $actual);
630+
$this->assertStringContainsString('<script >test()</script>', $actual);
631+
$this->assertStringContainsString('<style >.x{}</style>', $actual);
632+
}
633+
634+
public function testSendNoEffectWhenBodyEmptyAndCSPDisabled(): void
635+
{
636+
$config = new App();
637+
$config->CSPEnabled = false;
638+
639+
$response = new Response($config);
640+
$response->pretend(true);
641+
642+
$body = '';
643+
$response->setBody($body);
644+
645+
ob_start();
646+
$response->send();
647+
$actual = ob_get_clean();
648+
649+
$this->assertIsString($actual);
650+
$this->assertSame('', $actual);
651+
}
652+
653+
public function testSendNoEffectWithNoPlaceholdersAndCSPDisabled(): void
654+
{
655+
$config = new App();
656+
$config->CSPEnabled = false;
657+
658+
$response = new Response($config);
659+
$response->pretend(true);
660+
661+
$body = '<html><head><title>Test</title></head><body><p>No placeholders here</p></body></html>';
662+
$response->setBody($body);
663+
664+
ob_start();
665+
$response->send();
666+
$actual = ob_get_clean();
667+
668+
// Body should be unchanged when there are no placeholders and CSP is disabled
669+
$this->assertIsString($actual);
670+
$this->assertSame($body, $actual);
671+
}
672+
673+
public function testSendRemovesMultiplePlaceholdersWhenCSPDisabled(): void
674+
{
675+
$config = new App();
676+
$config->CSPEnabled = false;
677+
678+
$response = new Response($config);
679+
$response->pretend(true);
680+
681+
$body = '<html><script {csp-script-nonce}>console.log("test")</script><script {csp-script-nonce}>console.log("test2")</script><style {csp-style-nonce}>.test{}</style><style {csp-style-nonce}>.test2{}</style></html>';
682+
$response->setBody($body);
683+
684+
ob_start();
685+
$response->send();
686+
$actual = ob_get_clean();
687+
688+
// All nonce placeholders should be removed when CSP is disabled
689+
$this->assertIsString($actual);
690+
$this->assertStringNotContainsString('{csp-script-nonce}', $actual);
691+
$this->assertStringNotContainsString('{csp-style-nonce}', $actual);
692+
$this->assertStringContainsString('<script >console.log("test")</script>', $actual);
693+
$this->assertStringContainsString('<script >console.log("test2")</script>', $actual);
694+
$this->assertStringContainsString('<style >.test{}</style>', $actual);
695+
$this->assertStringContainsString('<style >.test2{}</style>', $actual);
696+
}
697+
698+
public function testSendRemovesPlaceholdersWhenBothCSPAndAutoNonceAreDisabled(): void
699+
{
700+
$appConfig = new App();
701+
$appConfig->CSPEnabled = false;
702+
703+
// Create custom CSP config with custom nonce tags
704+
$cspConfig = new \Config\ContentSecurityPolicy();
705+
$cspConfig->autoNonce = false;
706+
707+
Services::injectMock('csp', new ContentSecurityPolicy($cspConfig));
708+
709+
$response = new Response($appConfig);
710+
$response->pretend(true);
711+
712+
$body = '<html><script {csp-script-nonce}>test()</script><style {csp-style-nonce}>.x{}</style></html>';
713+
$response->setBody($body);
714+
715+
ob_start();
716+
$response->send();
717+
$actual = ob_get_clean();
718+
719+
// Custom nonce placeholders should be removed when CSP is disabled
720+
$this->assertIsString($actual);
721+
$this->assertStringNotContainsString('{csp-script-nonce}', $actual);
722+
$this->assertStringNotContainsString('{csp-style-nonce}', $actual);
723+
$this->assertStringContainsString('<script >test()</script>', $actual);
724+
$this->assertStringContainsString('<style >.x{}</style>', $actual);
725+
}
580726
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ Deprecations
3232
Bugs Fixed
3333
**********
3434

35+
- **ContentSecurityPolicy:** Fixed a bug where custom CSP tags were not removed from generated HTML when CSP was disabled. The method now ensures that all custom CSP tags are removed from the generated HTML.
3536
- **ContentSecurityPolicy:** Fixed a bug where ``generateNonces()`` produces corrupted JSON responses by replacing CSP nonce placeholders with unescaped double quotes. The method now automatically JSON-escapes nonce attributes when the response Content-Type is JSON.
3637
- **Model:** Fixed a bug where ``BaseModel::updateBatch()`` threw an exception when ``updateOnlyChanged`` was ``true`` and the index field value did not change.
3738
- **Session:** Fixed a bug in ``MemcachedHandler`` where the constructor incorrectly threw an exception when ``savePath`` was not empty.

0 commit comments

Comments
 (0)