Skip to content

fix: normalize string values and keys in ContentSecurityPolicy::addOption() #10636

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.

Sign up for GitHub

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

Open
gr8man wants to merge 1 commit into
base: develop
Choose a base branch
from
+273 −3
Open

fix: normalize string values and keys in ContentSecurityPolicy::addOption() #10636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 41 additions & 3 deletions system/HTTP/ContentSecurityPolicy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, bool>
*/
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
Expand Down Expand Up @@ -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-')
Expand All @@ -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);
}
}
Expand Down
231 changes: 231 additions & 0 deletions tests/system/HTTP/ContentSecurityPolicyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions user_guide_src/source/changelogs/v4.7.6.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading