diff --git a/Classes/Factory/PolicyFactory.php b/Classes/Factory/PolicyFactory.php index dafec31..bdab27d 100644 --- a/Classes/Factory/PolicyFactory.php +++ b/Classes/Factory/PolicyFactory.php @@ -33,15 +33,15 @@ class PolicyFactory */ /** - * @param array> $defaultDirectives - * @param array> $customDirectives + * @param array|null> $defaultDirectives + * @param array|null> $customDirectives * @throws InvalidDirectiveException * @throws DirectivesNormalizerException */ public function create(Nonce $nonce, array $defaultDirectives, array $customDirectives): Policy { - $normalizedDefaultDirectives = DirectivesNormalizer::normalize($defaultDirectives, $this->logger); - $normalizedCustomDirectives = DirectivesNormalizer::normalize($customDirectives, $this->logger); + $normalizedDefaultDirectives = DirectivesNormalizer::normalize($defaultDirectives); + $normalizedCustomDirectives = DirectivesNormalizer::normalize($customDirectives); $resultDirectives = $normalizedDefaultDirectives; foreach ($normalizedCustomDirectives as $key => $customDirective) { diff --git a/Classes/Helpers/DirectivesNormalizer.php b/Classes/Helpers/DirectivesNormalizer.php index 61ebbcb..470f836 100644 --- a/Classes/Helpers/DirectivesNormalizer.php +++ b/Classes/Helpers/DirectivesNormalizer.php @@ -5,70 +5,46 @@ namespace Flowpack\ContentSecurityPolicy\Helpers; use Flowpack\ContentSecurityPolicy\Exceptions\DirectivesNormalizerException; -use Psr\Log\LoggerInterface; /** - * Helper to support normalization of directives from different formats. - * The old format supported yaml lists. Now key-value pairs should be used for directives. - * In the future we will deprecate the list format! - * - * We also cleanup of empty directives and entries here before further processing. + * Normalizes CSP directives from yaml key-value pairs (e.g. example.com: true) to string arrays. + * Also removes empty directives and entries before further processing. */ final class DirectivesNormalizer { /** - * @param array> $directives + * @param array|null> $directives * @return string[][] * @throws DirectivesNormalizerException */ - public static function normalize(array $directives, LoggerInterface $logger): array + public static function normalize(array $directives): array { $result = []; - // directives e.g. script-src: foreach ($directives as $directive => $values) { if (!is_array($values) || count($values) === 0) { continue; } $normalizedValues = []; - $firstKeyType = null; - // values e.g. 'self', 'unsafe-inline' OR key-value pairs e.g. example.com: true foreach ($values as $key => $value) { - if ($firstKeyType === null) { - $firstKeyType = gettype($key); - } else { - if (gettype($key) !== $firstKeyType) { - // we do not allow mixed key types -> this should be marked as an error in the IDE as well - // as Flow should throw an exception here. But just to be sure, we add this check. - throw new DirectivesNormalizerException( - 'Directives must be defined as a list OR an object.' - ); - } - } - - if (is_int($key) && is_string($value) && trim($value) !== '') { - // old configuration format using list - $normalizedValues[] = $value; - $logger->warning( - 'Using list format for CSP directives is deprecated and will be removed in future versions. Please use key-value pairs with boolean values instead.' + if (!is_string($key)) { + throw new DirectivesNormalizerException( + 'Directives must be defined as an object with string keys and boolean values.' ); - } elseif (is_string($key)) { - // new configuration format using key-value pairs - if (is_bool($value)) { - if ($value === true && trim($key) !== '') { - $normalizedValues[] = $key; - } - continue; - } + } - // We chose a format similar to NodeType constraints yaml configuration. + if (!is_bool($value)) { throw new DirectivesNormalizerException( 'When using keys in your yaml, the values must be boolean.' ); } + + if ($value === true && trim($key) !== '') { + $normalizedValues[] = $key; + } } + if ($normalizedValues !== []) { - // we also clean up empty directives here $result[$directive] = $normalizedValues; } } diff --git a/README.md b/README.md index a2751a4..caf4056 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,6 @@ * [Flowpack.ContentSecurityPolicy](#flowpackcontentsecuritypolicy) * [Introduction](#introduction) * [Usage](#usage) - * [Deprecated Configuration](#deprecated-configuration) * [Custom directives and values](#custom-directives-and-values) * [Show CSP configuration](#show-csp-configuration) * [Disable or report only](#disable-or-report-only) @@ -70,24 +69,6 @@ Flowpack: Now only resources from the same origin are allowed for the most common directives. It is enabled by default and the report-only mode is disabled. -## Deprecated Configuration - -Make sure to change any old configuration to the new object format. **Support for the old list format will be removed in future versions.** - -The new config allows to merge configurations from different packages/yaml files. - -```yaml -frame-src: - # Deprecated list format - # - 'https://www.youtube.com': - # - 'https://staticxx.facebook.com': - - # New object format - 'https://www.youtube.com': true - 'https://staticxx.facebook.com': true -``` - - ## Custom directives and values If you want to override the default config don't forget to add this package as a dependency in the composer.json file diff --git a/Tests/Unit/Factory/PolicyFactoryTest.php b/Tests/Unit/Factory/PolicyFactoryTest.php index 093b899..0eb8698 100644 --- a/Tests/Unit/Factory/PolicyFactoryTest.php +++ b/Tests/Unit/Factory/PolicyFactoryTest.php @@ -49,32 +49,19 @@ public function testCreateShouldReturnPolicyAndMergeCustomWithDefaultDirective() $nonceMock = $this->createMock(Nonce::class); $defaultDirective = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'test.com', - ], + 'base-uri' => ['test.com' => true], + 'script-src' => ['test.com' => true], ]; $customDirective = [ - 'script-src' => [ - 'custom.com', - ], - ]; - - $expected = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'test.com', - 'custom.com', - ], + 'script-src' => ['custom.com' => true], ]; $result = $this->policyFactory->create($nonceMock, $defaultDirective, $customDirective); - self::assertSame($expected, $result->getDirectives()); + self::assertSame([ + 'base-uri' => ['test.com'], + 'script-src' => ['test.com', 'custom.com'], + ], $result->getDirectives()); } public function testCreateShouldReturnPolicyAndHandleSpecialDirectives(): void @@ -83,22 +70,17 @@ public function testCreateShouldReturnPolicyAndHandleSpecialDirectives(): void $defaultDirective = [ 'script-src' => [ - '{nonce}', - 'self', + '{nonce}' => true, + 'self' => true, ], ]; $customDirective = []; - $expected = [ - 'script-src' => [ - "'nonce-'", - "'self'", - ], - ]; - $result = $this->policyFactory->create($nonceMock, $defaultDirective, $customDirective); - self::assertSame($expected, $result->getDirectives()); + self::assertSame([ + 'script-src' => ["'nonce-'", "'self'"], + ], $result->getDirectives()); } public function testCreateShouldFailWithInvalidDirective(): void @@ -106,12 +88,8 @@ public function testCreateShouldFailWithInvalidDirective(): void $nonceMock = $this->createMock(Nonce::class); $defaultDirective = [ - 'invalid' => [ - 'self', - ], - 'script-src' => [ - 'self', - ], + 'invalid' => ['self' => true], + 'script-src' => ['self' => true], ]; $customDirective = []; @@ -128,12 +106,8 @@ public function testCreateShouldLogInvalidDirectiveInProduction(): void ); $defaultDirective = [ - 'invalid' => [ - 'self', - ], - 'script-src' => [ - 'self', - ], + 'invalid' => ['self' => true], + 'script-src' => ['self' => true], ]; $customDirective = []; @@ -151,35 +125,20 @@ public function testCreateShouldReturnPolicyWithUniqueValues(): void $nonceMock = $this->createMock(Nonce::class); $defaultDirective = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'test.com', - ], + 'base-uri' => ['test.com' => true], + 'script-src' => ['test.com' => true], ]; $customDirective = [ - 'base-uri' => [ - 'test.com', - 'test.com', - ], - 'script-src' => [ - 'test.com', - ], - ]; - - $expected = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'test.com', - ], + 'base-uri' => ['test.com' => true], + 'script-src' => ['test.com' => true], ]; $result = $this->policyFactory->create($nonceMock, $defaultDirective, $customDirective); - self::assertSame($expected, $result->getDirectives()); + self::assertSame([ + 'base-uri' => ['test.com'], + 'script-src' => ['test.com'], + ], $result->getDirectives()); } public function testCreateShouldAddDirectiveWhichIsPresentInCustomButNotDefaultConfiguration(): void @@ -187,33 +146,19 @@ public function testCreateShouldAddDirectiveWhichIsPresentInCustomButNotDefaultC $nonceMock = $this->createMock(Nonce::class); $defaultDirective = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'test.com', - ], + 'base-uri' => ['test.com' => true], + 'script-src' => ['test.com' => true], ]; $customDirective = [ - 'worker-src' => [ - 'test.com', - ], - ]; - - $expected = [ - 'base-uri' => [ - "test.com", - ], - 'script-src' => [ - "test.com", - ], - 'worker-src' => [ - "test.com", - ], + 'worker-src' => ['test.com' => true], ]; $result = $this->policyFactory->create($nonceMock, $defaultDirective, $customDirective); - self::assertSame($expected, $result->getDirectives()); + self::assertSame([ + 'base-uri' => ['test.com'], + 'script-src' => ['test.com'], + 'worker-src' => ['test.com'], + ], $result->getDirectives()); } } diff --git a/Tests/Unit/Helpers/DirectivesNormalizerTest.php b/Tests/Unit/Helpers/DirectivesNormalizerTest.php index 7e30e4f..e44e4f6 100644 --- a/Tests/Unit/Helpers/DirectivesNormalizerTest.php +++ b/Tests/Unit/Helpers/DirectivesNormalizerTest.php @@ -7,105 +7,13 @@ use Flowpack\ContentSecurityPolicy\Exceptions\DirectivesNormalizerException; use Flowpack\ContentSecurityPolicy\Helpers\DirectivesNormalizer; use PHPUnit\Framework\Attributes\CoversClass; -use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use Psr\Log\LoggerInterface; #[CoversClass(DirectivesNormalizer::class)] #[CoversClass(DirectivesNormalizerException::class)] class DirectivesNormalizerTest extends TestCase { - private readonly LoggerInterface&MockObject $loggerMock; - - protected function setUp(): void - { - parent::setUp(); - $this->loggerMock = $this->createMock(LoggerInterface::class); - } - - public function testDeprecatedConfiguration(): void - { - // deprecated list configuration should still work but log deprecation warnings - $this->loggerMock->expects($this->atLeast(4))->method('warning'); - - $actual = DirectivesNormalizer::normalize([ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ], $this->loggerMock); - $expected = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ]; - self::assertSame($expected, $actual); - - // empty directives should be removed - $actual = DirectivesNormalizer::normalize([ - 'base-uri' => [], - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ], $this->loggerMock); - $expected = [ - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ]; - self::assertSame($expected, $actual); - - // empty directive entries should be removed - // empty directives should be removed - $actual = DirectivesNormalizer::normalize([ - 'base-uri' => [ - '', - ], - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ], $this->loggerMock); - $expected = [ - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ]; - self::assertSame($expected, $actual); - - // empty directives should be removed - $actual = DirectivesNormalizer::normalize([ - 'base-uri' => [], - 'script-src' => [], - ], $this->loggerMock); - $expected = []; - - self::assertSame($expected, $actual); - } - - public function testConfiguration(): void + public function testNormalizesKeyValueDirectives(): void { $actual = DirectivesNormalizer::normalize([ 'base-uri' => [ @@ -117,108 +25,92 @@ public function testConfiguration(): void 'example.com' => true, 'another-example.com' => true, ], - ], $this->loggerMock); - $expected = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', - ], - ]; - self::assertSame($expected, $actual); + ]); + self::assertSame([ + 'base-uri' => ['test.com'], + 'script-src' => ['self', 'unsafe-inline', 'example.com', 'another-example.com'], + ], $actual); + } - // empty directive entries should be removed - // empty directives should be removed + public function testExcludesFalseValues(): void + { $actual = DirectivesNormalizer::normalize([ 'base-uri' => [ - '' => true, + 'test.com' => true, + 'another-example.com' => false, ], 'script-src' => [ 'self' => true, - 'unsafe-inline' => true, + 'unsafe-inline' => false, 'example.com' => true, - 'another-example.com' => true, - ], - ], $this->loggerMock); - $expected = [ - 'script-src' => [ - 'self', - 'unsafe-inline', - 'example.com', - 'another-example.com', + 'another-example.com' => false, ], - ]; - self::assertSame($expected, $actual); + ]); + self::assertSame([ + 'base-uri' => ['test.com'], + 'script-src' => ['self', 'example.com'], + ], $actual); + } + + public function testRemovesDirectiveWithEmptyStringKey(): void + { + $actual = DirectivesNormalizer::normalize([ + 'base-uri' => ['' => true], + 'script-src' => ['self' => true], + ]); + self::assertSame(['script-src' => ['self']], $actual); + } + public function testRemovesEmptyDirectiveArray(): void + { $actual = DirectivesNormalizer::normalize([ 'base-uri' => [], 'script-src' => [], - ], $this->loggerMock); - $expected = [ - ]; - self::assertSame($expected, $actual); + ]); + self::assertSame([], $actual); + } + public function testRemovesNullDirective(): void + { $actual = DirectivesNormalizer::normalize([ 'base-uri' => null, - 'script-src' => [], - ], $this->loggerMock); - - self::assertSame($expected, $actual); + 'script-src' => ['self' => true], + ]); + self::assertSame(['script-src' => ['self']], $actual); + } + public function testSkipsTopLevelIntKeyedDirective(): void + { // @phpstan-ignore argument.type $actual = DirectivesNormalizer::normalize([ 'base-uri', 'script-src' => [], - ], $this->loggerMock); - - self::assertSame($expected, $actual); + ]); + self::assertSame([], $actual); } - public function testConfigurationWithDirectivesUsingFalse(): void + public function testDeprecatedListFormatThrowsException(): void { - $actual = DirectivesNormalizer::normalize([ - 'base-uri' => [ - 'test.com' => true, - 'another-example.com' => false, - ], - 'script-src' => [ - 'self' => true, - 'unsafe-inline' => false, - 'example.com' => true, - 'another-example.com' => false, - ], - ], $this->loggerMock); - $expected = [ - 'base-uri' => [ - 'test.com', - ], - 'script-src' => [ - 'self', - 'example.com', - ], - ]; - self::assertSame($expected, $actual); + $this->expectException(DirectivesNormalizerException::class); + DirectivesNormalizer::normalize([ + 'script-src' => ['self', 'unsafe-inline'], + ]); } - public function testInvalidDirectiveExceptionForMixedConfig(): void + public function testMixedIntAndStringKeysThrowException(): void { $this->expectException(DirectivesNormalizerException::class); - $this->expectExceptionMessageMatches("/must be defined as a list OR an object/"); + $this->expectExceptionMessageMatches("/must be defined as an object with string keys and boolean values/"); DirectivesNormalizer::normalize([ 'script-src' => [ 'self' => true, - 'unsafe-inline' => true, 'example.com', 'another-example.com' => true, ], - ], $this->loggerMock); + ]); } - public function testInvalidDirectiveExceptionForWrongKeyValue(): void + public function testNonBooleanValueThrowsException(): void { $this->expectException(DirectivesNormalizerException::class); $this->expectExceptionMessageMatches("/the values must be boolean/"); @@ -226,9 +118,7 @@ public function testInvalidDirectiveExceptionForWrongKeyValue(): void 'script-src' => [ 'self' => true, 'unsafe-inline' => 'foo bar', - 'example.com' => true, - 'another-example.com' => true, ], - ], $this->loggerMock); + ]); } }