diff --git a/system/HTTP/ContentSecurityPolicy.php b/system/HTTP/ContentSecurityPolicy.php index dfea52a62772..5eea268737c3 100644 --- a/system/HTTP/ContentSecurityPolicy.php +++ b/system/HTTP/ContentSecurityPolicy.php @@ -878,16 +878,40 @@ protected function addOption($options, string $target, ?bool $explicitReporting { // Ensure we have an array to work with... if (is_string($this->{$target})) { - $this->{$target} = [$this->{$target} => $this->reportOnly]; + $this->{$target} = $this->stringToOptions($this->{$target}); + } elseif (! is_array($this->{$target})) { + $this->{$target} = []; } $options = is_array($options) ? $options : [$options]; foreach ($options as $option) { - $this->{$target}[$option] = $explicitReporting ?? $this->reportOnly; + $this->{$target}[trim((string) $option)] = $explicitReporting ?? $this->reportOnly; } } + /** + * Converts a string directive value into an array of options. + * + * @return array + */ + private function stringToOptions(string $values): array + { + $valueStr = trim($values); + + if ($valueStr === '') { + return ['' => $this->reportOnly]; + } + + $options = []; + + foreach (preg_split('/\s+/', $valueStr) as $val) { + $options[$val] = $this->reportOnly; + } + + return $options; + } + /** * Scans the body of the request message and replaces any nonce * placeholders with actual nonces, that we'll then add to our @@ -1012,14 +1036,20 @@ protected function buildHeaders(ResponseInterface $response) */ protected function addToHeader(string $name, $values = null) { + if ($values === null) { + return; + } + if (is_string($values)) { - $values = [$values => $this->reportOnly]; + $values = $this->stringToOptions($values); } $sources = []; $reportSources = []; foreach ($values as $value => $reportOnly) { + $value = (string) $value; + if ( in_array($value, $this->validSources, true) || str_starts_with($value, 'nonce-') @@ -1038,10 +1068,18 @@ protected function addToHeader(string $name, $values = null) } if ($sources !== []) { + if (count($sources) > 1) { + $sources = array_values(array_filter($sources, static fn ($s): bool => $s !== '')); + } + $this->tempHeaders[$name] = implode(' ', $sources); } if ($reportSources !== []) { + if (count($reportSources) > 1) { + $reportSources = array_values(array_filter($reportSources, static fn ($s): bool => $s !== '')); + } + $this->reportOnlyHeaders[$name] = implode(' ', $reportSources); } } diff --git a/tests/system/HTTP/ContentSecurityPolicyTest.php b/tests/system/HTTP/ContentSecurityPolicyTest.php index 780cb99f31ea..8e02131fbac3 100644 --- a/tests/system/HTTP/ContentSecurityPolicyTest.php +++ b/tests/system/HTTP/ContentSecurityPolicyTest.php @@ -756,6 +756,237 @@ public function testSandboxFlags(): void $this->assertContains('sandbox allow-popups allow-top-navigation', $this->getCspDirectives($header)); } + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testSandboxEmptyFlagThenFlags(): void + { + $this->csp->reportOnly(false); + $this->csp->addSandbox(''); + $this->csp->addSandbox('allow-scripts'); + $this->assertTrue($this->work()); + + $header = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($header); + $this->assertContains('sandbox allow-scripts', $this->getCspDirectives($header)); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testAddOptionWithSpaceSeparatedString(): void + { + $config = new CSPConfig(); + $config->baseURI = 'self example.com'; + + $csp = new ContentSecurityPolicy($config); + + $response = new Response(config(App::class)); + $response->pretend(true); + + $csp->addBaseURI('another.com'); + $csp->finalize($response); + + $directives = $this->getCspDirectives($response->getHeaderLine('Content-Security-Policy')); + $this->assertContains("base-uri 'self' example.com another.com", $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testAddOptionWithEmptyStringTarget(): void + { + $config = new CSPConfig(); + $config->baseURI = ''; + + $csp = new ContentSecurityPolicy($config); + + $response = new Response(config(App::class)); + $response->pretend(true); + + $csp->addBaseURI('example.com'); + $csp->finalize($response); + + $directives = $this->getCspDirectives($response->getHeaderLine('Content-Security-Policy')); + $this->assertContains('base-uri example.com', $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testAddOptionNumericString(): void + { + $this->csp->addBaseURI('0'); + $this->assertTrue($this->work()); + + $header = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($header); + $this->assertContains('base-uri 0', $this->getCspDirectives($header)); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testConfiguredBareSandboxWithAddSandboxNoop(): void + { + $config = new CSPConfig(); + $config->sandbox = ''; + + $csp = new ContentSecurityPolicy($config); + + $response = new Response(config(App::class)); + $response->pretend(true); + + $csp->addSandbox([]); + $csp->finalize($response); + + $directives = $this->getCspDirectives($response->getHeaderLine('Content-Security-Policy')); + $this->assertContains('sandbox', $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testAddFrameAncestorEmptyString(): void + { + $this->csp->addFrameAncestor(''); + $this->assertTrue($this->work()); + + $header = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($header); + $this->assertContains('frame-ancestors', $this->getCspDirectives($header)); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testSandboxMixedModeEnforcedAndReportOnly(): void + { + $this->csp->addSandbox('', false); + $this->csp->addSandbox('allow-scripts', true); + $this->assertTrue($this->work()); + + $enforced = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($enforced); + $this->assertContains('sandbox', $this->getCspDirectives($enforced)); + + $reportOnly = $this->getHeaderEmitted('Content-Security-Policy-Report-Only'); + $this->assertIsString($reportOnly); + $this->assertContains('sandbox allow-scripts', $this->getCspDirectives($reportOnly)); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testOptionMovedBetweenReportingModesPreservesEmptyMarker(): void + { + $this->csp->addSandbox('', false); + $this->csp->addSandbox('allow-scripts', false); + $this->csp->addSandbox('allow-scripts', true); + + $this->csp->addFrameAncestor('', false); + $this->csp->addFrameAncestor('https://example.com', false); + $this->csp->addFrameAncestor('https://example.com', true); + + $this->assertTrue($this->work()); + + $enforced = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($enforced); + $enforcedDirectives = $this->getCspDirectives($enforced); + $this->assertContains('sandbox', $enforcedDirectives); + $this->assertContains('frame-ancestors', $enforcedDirectives); + $this->assertNotContains('sandbox allow-scripts', $enforcedDirectives); + $this->assertNotContains('frame-ancestors https://example.com', $enforcedDirectives); + + $reportOnly = $this->getHeaderEmitted('Content-Security-Policy-Report-Only'); + $this->assertIsString($reportOnly); + $reportOnlyDirectives = $this->getCspDirectives($reportOnly); + $this->assertContains('sandbox allow-scripts', $reportOnlyDirectives); + $this->assertContains('frame-ancestors https://example.com', $reportOnlyDirectives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testConfiguredEmptyDirectivesEmitted(): void + { + $config = new CSPConfig(); + $config->sandbox = ''; + $config->frameAncestors = ''; + $config->formAction = ''; + $config->fontSrc = ''; + $config->connectSrc = ''; + + $csp = new ContentSecurityPolicy($config); + + $response = new Response(config(App::class)); + $response->pretend(true); + + $csp->finalize($response); + + $directives = $this->getCspDirectives($response->getHeaderLine('Content-Security-Policy')); + $this->assertContains('sandbox', $directives); + $this->assertContains('frame-ancestors', $directives); + $this->assertContains('form-action', $directives); + $this->assertContains('font-src', $directives); + $this->assertContains('connect-src', $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testConfiguredEmptyFormActionWithAddFormActionNoop(): void + { + $config = new CSPConfig(); + $config->formAction = ''; + + $csp = new ContentSecurityPolicy($config); + + $response = new Response(config(App::class)); + $response->pretend(true); + + $csp->addFormAction([]); + $csp->finalize($response); + + $directives = $this->getCspDirectives($response->getHeaderLine('Content-Security-Policy')); + $this->assertContains('form-action', $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testClearDirectiveThenAddEmptySource(): void + { + $this->csp->clearDirective('form-action'); + $this->csp->addFormAction(''); + $this->csp->clearDirective('connect-src'); + $this->csp->addConnectSrc(''); + $this->assertTrue($this->work()); + + $header = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($header); + $directives = $this->getCspDirectives($header); + $this->assertContains('form-action', $directives); + $this->assertContains('connect-src', $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testAddEmptySourceToFontSrc(): void + { + $this->csp->addFontSrc(''); + $this->assertTrue($this->work()); + + $header = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($header); + $directives = $this->getCspDirectives($header); + $this->assertContains('font-src', $directives); + } + + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + public function testRedundantEmptySourceFilteredWhenOtherSourcesPresent(): void + { + // connectSrc defaults to 'self' + $this->csp->addConnectSrc(''); + $this->assertTrue($this->work()); + + $header = $this->getHeaderEmitted('Content-Security-Policy'); + $this->assertIsString($header); + $directives = $this->getCspDirectives($header); + $this->assertContains("connect-src 'self'", $directives); + $this->assertNotContains('connect-src', $directives); + } + #[PreserveGlobalState(false)] #[RunInSeparateProcess] public function testUpgradeInsecureRequests(): void diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 8739e9546d4c..535a302b70e7 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -38,6 +38,7 @@ Bugs Fixed is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. +- **HTTP:** Fixed a bug in ``ContentSecurityPolicy::addOption()`` where converting string directive values to array keys resulted in invalid entries for empty strings, unquoted values for space-separated lists, and ``TypeError`` for numeric strings. - **Security:** Fixed a bug where a valid JSON request body that is not an object (for example a JSON array) was corrupted after CSRF verification when the token was sent via the ``X-CSRF-TOKEN`` header.