diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..64b4ec1 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,26 @@ +name: CI-coverage + +on: [ push ] + +jobs: + build-test: + runs-on: ubuntu-latest + strategy: + matrix: + php-version: [ 8.2, 8.3 ] + + steps: + - uses: actions/checkout@v3 + + - uses: php-actions/composer@v6 + + - name: PHPUnit Tests + uses: php-actions/phpunit@v3 + with: + php_version: ${{ matrix.php-version }} + php_extensions: xdebug + bootstrap: vendor/autoload.php + configuration: phpunit.xml + coverage_text: true + env: + XDEBUG_MODE: coverage diff --git a/.gitignore b/.gitignore index 6a7a4d1..c81800e 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ composer.lock vendor Packages +.phpunit.cache diff --git a/Classes/Factory/PolicyFactory.php b/Classes/Factory/PolicyFactory.php index e1a4781..d5997df 100644 --- a/Classes/Factory/PolicyFactory.php +++ b/Classes/Factory/PolicyFactory.php @@ -12,6 +12,8 @@ class PolicyFactory { /** * @throws InvalidDirectiveException + * @param string[][] $defaultDirective + * @param string[][] $customDirective */ public static function create(Nonce $nonce, array $defaultDirective, array $customDirective): Policy { diff --git a/Classes/Helpers/TagHelper.php b/Classes/Helpers/TagHelper.php index 82a4f79..146b17d 100644 --- a/Classes/Helpers/TagHelper.php +++ b/Classes/Helpers/TagHelper.php @@ -6,8 +6,6 @@ class TagHelper { - public const NONCE = 'nonce'; - public static function tagHasAttribute( string $tag, string $name, @@ -18,15 +16,15 @@ public static function tagHasAttribute( self::buildMatchAttributeNameReqex($name), $tag ); - } else { - return ! ! preg_match( - self::buildMatchAttributeNameWithSpecificValueReqex( - $name, - $value - ), - $tag - ); } + + return ! ! preg_match( + self::buildMatchAttributeNameWithSpecificValueReqex( + $name, + $value + ), + $tag + ); } public static function tagChangeAttributeValue( @@ -63,9 +61,9 @@ function ($hits) use ($name, $value) { $value. '"'. $hits["end"]; - } else { - return $hits["start"].' '.$name.$hits["end"]; } + + return $hits["start"].' '.$name.$hits["end"]; }, $tag ); @@ -82,9 +80,8 @@ private static function buildMatchEndOfOpeningTagReqex(): string return '/(?<start><[a-z]+.*?)(?<end>>|\/>)/'; } - private static function buildMatchAttributeNameWithAnyValueReqex( - string $name - ): string { + private static function buildMatchAttributeNameWithAnyValueReqex(string $name): string + { $nameQuoted = self::escapeReqexCharsInString($name); return '/(?<pre><.*? )(?<name>'. diff --git a/Classes/Http/CspHeaderMiddleware.php b/Classes/Http/CspHeaderMiddleware.php index 66b5245..daa1cb9 100644 --- a/Classes/Http/CspHeaderMiddleware.php +++ b/Classes/Http/CspHeaderMiddleware.php @@ -20,6 +20,8 @@ class CspHeaderMiddleware implements MiddlewareInterface { + private const NONCE = 'nonce'; + /** * @Flow\InjectConfiguration(path="enabled") */ @@ -101,11 +103,11 @@ private function addNonceToTags(string $markup): string return $this->checkTagAndReplaceUsingACallback($tagNames, $markup, function ( $tagMarkup, ) { - if (TagHelper::tagHasAttribute($tagMarkup, TagHelper::NONCE)) { - return TagHelper::tagChangeAttributeValue($tagMarkup, TagHelper::NONCE, $this->nonce->getValue()); + if (TagHelper::tagHasAttribute($tagMarkup, self::NONCE)) { + return TagHelper::tagChangeAttributeValue($tagMarkup, self::NONCE, $this->nonce->getValue()); } - return TagHelper::tagAddAttribute($tagMarkup, TagHelper::NONCE, $this->nonce->getValue()); + return TagHelper::tagAddAttribute($tagMarkup, self::NONCE, $this->nonce->getValue()); }); } diff --git a/Classes/Model/Nonce.php b/Classes/Model/Nonce.php index 988d7e5..fd8816f 100644 --- a/Classes/Model/Nonce.php +++ b/Classes/Model/Nonce.php @@ -29,9 +29,9 @@ public function getValue(): string return $this->value; } - public function __toString() + public function __toString(): string { - return $this->value; + return $this->getValue(); } /** diff --git a/Classes/Model/Policy.php b/Classes/Model/Policy.php index 16ace5d..f7fe65b 100644 --- a/Classes/Model/Policy.php +++ b/Classes/Model/Policy.php @@ -40,14 +40,9 @@ public function setNonce(Nonce $nonce): Policy return $this; } - public function isReportOnly(): bool - { - return $this->reportOnly; - } - public function getSecurityHeaderKey(): string { - if ($this->isReportOnly()) { + if ($this->reportOnly) { return self::SECURITY_HEADER_KEY_REPORT_ONLY; } @@ -59,10 +54,16 @@ public function getDirectives(): array return $this->directives; } + public function hasNonceDirectiveValue(): bool + { + return $this->hasNonceDirectiveValue; + } + /** + * @param string[] $values * @throws InvalidDirectiveException */ - public function addDirective(string $directive, $values): self + public function addDirective(string $directive, array $values): self { if (! Directive::isValidDirective($directive)) { throw new InvalidDirectiveException($directive); @@ -74,16 +75,6 @@ public function addDirective(string $directive, $values): self return $this; } - public function getNonce(): Nonce - { - return $this->nonce; - } - - public function hasNonceDirectiveValue(): bool - { - return $this->hasNonceDirectiveValue; - } - public function __toString(): string { $directives = $this->getDirectives(); @@ -95,26 +86,21 @@ public function __toString(): string return "$directive $value"; }, $directives, $keys); - return implode(';', $items).';'; + return implode('; ', $items).';'; } private function sanitizeValue(string $value): string { - if ($this->isSpecialValue($value)) { + if (in_array($value, self::SPECIAL_DIRECTIVES)) { return "'$value'"; } if ($value === '{nonce}') { $this->hasNonceDirectiveValue = true; - return "'nonce-".$this->getNonce()->getValue()."'"; + return "'nonce-".$this->nonce->getValue()."'"; } return $value; } - - private function isSpecialValue(string $directive): bool - { - return in_array($directive, self::SPECIAL_DIRECTIVES); - } } diff --git a/Tests/Unit/Factory/PolicyFactoryTest.php b/Tests/Unit/Factory/PolicyFactoryTest.php new file mode 100644 index 0000000..a5232a3 --- /dev/null +++ b/Tests/Unit/Factory/PolicyFactoryTest.php @@ -0,0 +1,52 @@ +<?php + +declare(strict_types=1); + +namespace Unit\Factory; + +use Flowpack\ContentSecurityPolicy\Factory\PolicyFactory; +use Flowpack\ContentSecurityPolicy\Model\Directive; +use Flowpack\ContentSecurityPolicy\Model\Nonce; +use Flowpack\ContentSecurityPolicy\Model\Policy; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\UsesClass; +use PHPUnit\Framework\TestCase; + +#[CoversClass(PolicyFactory::class)] +#[UsesClass(Policy::class)] +#[UsesClass(Directive::class)] +class PolicyFactoryTest extends TestCase +{ + public function testCreateShouldReturnPolicy(): void + { + $nonceMock = $this->createMock(Nonce::class); + + $defaultDirective = [ + 'base-uri' => [ + 'self', + ], + 'script-src' => [ + 'self', + ], + ]; + $customDirective = [ + 'script-src' => [ + '{nonce}', + ], + ]; + + $expected = [ + 'base-uri' => [ + "'self'", + ], + 'script-src' => [ + "'self'", + "'nonce-'", + ], + ]; + + $result = PolicyFactory::create($nonceMock, $defaultDirective, $customDirective); + + self::assertSame($expected, $result->getDirectives()); + } +} diff --git a/Tests/Unit/Helpers/TagHelperTest.php b/Tests/Unit/Helpers/TagHelperTest.php new file mode 100644 index 0000000..5de0288 --- /dev/null +++ b/Tests/Unit/Helpers/TagHelperTest.php @@ -0,0 +1,77 @@ +<?php + +declare(strict_types=1); + +namespace Unit\Helpers; + +use Flowpack\ContentSecurityPolicy\Helpers\TagHelper; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +#[CoversClass(TagHelper::class)] +class TagHelperTest extends TestCase +{ + public function testTagHasAttributeWithoutValueShouldReturnTrue(): void + { + $tag = '<script src="https://google.com"></script>'; + self::assertTrue(TagHelper::tagHasAttribute($tag, 'src')); + } + + public function testTagHasAttributeWithoutValueShouldReturnFalse(): void + { + $tag = '<script src="https://google.com"></script>'; + self::assertFalse(TagHelper::tagHasAttribute($tag, 'bar')); + } + + public function testTagHasAttributeWithValueShouldReturnTrue(): void + { + $tag = '<script src="https://google.com"></script>'; + self::assertTrue(TagHelper::tagHasAttribute($tag, 'src', 'https://google.com')); + } + + public function testTagHasAttributeWithValueShouldReturnFalse(): void + { + $tag = '<script src="https://google.com"></script>'; + self::assertFalse(TagHelper::tagHasAttribute($tag, 'src', 'another value')); + } + + public function testTagChangeAttributeValueShouldChangeValue(): void + { + $tag = '<script src="https://google.com"></script>'; + + self::assertSame( + '<script src="https://test.com"></script>', + TagHelper::tagChangeAttributeValue($tag, 'src', 'https://test.com') + ); + } + + public function testTagChangeAttributeValueShouldDoNothingIfAttributeDoesntExist(): void + { + $tag = '<script src="https://google.com"></script>'; + + self::assertSame( + '<script src="https://google.com"></script>', + TagHelper::tagChangeAttributeValue($tag, 'nonce', 'da65sf1g') + ); + } + + public function testTagAddAttributeShouldAddAttributeWithValue(): void + { + $tag = '<script src="https://google.com"></script>'; + + self::assertSame( + '<script src="https://google.com" nonce="da65sf1g"></script>', + TagHelper::tagAddAttribute($tag, 'nonce', 'da65sf1g') + ); + } + + public function testTagAddAttributeShouldAddAttributeWithoutValue(): void + { + $tag = '<script src="https://google.com"></script>'; + + self::assertSame( + '<script src="https://google.com" defer></script>', + TagHelper::tagAddAttribute($tag, 'defer') + ); + } +} diff --git a/Tests/Unit/Http/CspHeaderMiddlewareTest.php b/Tests/Unit/Http/CspHeaderMiddlewareTest.php new file mode 100644 index 0000000..46bbb92 --- /dev/null +++ b/Tests/Unit/Http/CspHeaderMiddlewareTest.php @@ -0,0 +1,61 @@ +<?php + +declare(strict_types=1); + +namespace Unit\Http; + +use Exception; +use Flowpack\ContentSecurityPolicy\Http\CspHeaderMiddleware; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ServerRequestInterface; +use Psr\Http\Server\RequestHandlerInterface; +use Psr\Log\LoggerInterface; +use ReflectionClass; +use Throwable; + +#[CoversClass(CspHeaderMiddleware::class)] +class CspHeaderMiddlewareTest extends TestCase +{ + private readonly CspHeaderMiddleware $middleware; + private readonly ReflectionClass $middlewareReflection; + + private readonly ServerRequestInterface|MockObject $requestMock; + private readonly RequestHandlerInterface|MockObject $requestHandlerMock; + private readonly ResponseInterface|MockObject $responseMock; + private readonly LoggerInterface|MockObject $loggerMock; + + /** + * @throws Throwable + */ + protected function setUp(): void + { + parent::setUp(); + + $this->middleware = new CspHeaderMiddleware(); + + $this->requestMock = $this->createMock(ServerRequestInterface::class); + $this->requestHandlerMock = $this->createMock(RequestHandlerInterface::class); + $this->responseMock = $this->createMock(ResponseInterface::class); + $this->loggerMock = $this->createMock(LoggerInterface::class); + + $this->middlewareReflection = new ReflectionClass($this->middleware); + $reflectionProperty = $this->middlewareReflection->getProperty('logger'); + $reflectionProperty->setValue($this->middleware, $this->loggerMock); + + $reflectionProperty = $this->middlewareReflection->getProperty('enabled'); + $reflectionProperty->setValue($this->middleware, true); + } + + public function testProcessShouldDoNothingIfIsDisabled(): void + { + $reflectionProperty = $this->middlewareReflection->getProperty('enabled'); + $reflectionProperty->setValue($this->middleware, false); + + $this->requestHandlerMock->expects($this->once())->method('handle')->willReturn($this->responseMock); + + $this->middleware->process($this->requestMock, $this->requestHandlerMock); + } +} diff --git a/Tests/Unit/Model/DirectiveTest.php b/Tests/Unit/Model/DirectiveTest.php new file mode 100644 index 0000000..36f8425 --- /dev/null +++ b/Tests/Unit/Model/DirectiveTest.php @@ -0,0 +1,23 @@ +<?php + +declare(strict_types=1); + +namespace Unit\Model; + +use Flowpack\ContentSecurityPolicy\Model\Directive; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +#[CoversClass(Directive::class)] +class DirectiveTest extends TestCase +{ + public function testIsValidDirectiveShouldReturnTrue(): void + { + self::assertTrue(Directive::isValidDirective('media-src')); + } + + public function testIsValidDirectiveShouldReturnFalse(): void + { + self::assertFalse(Directive::isValidDirective('bar')); + } +} diff --git a/Tests/Unit/Model/NonceTest.php b/Tests/Unit/Model/NonceTest.php new file mode 100644 index 0000000..9b48a25 --- /dev/null +++ b/Tests/Unit/Model/NonceTest.php @@ -0,0 +1,29 @@ +<?php + +declare(strict_types=1); + +namespace Unit\Model; + +use Flowpack\ContentSecurityPolicy\Model\Nonce; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +#[CoversClass(Nonce::class)] +class NonceTest extends TestCase +{ + public function testGetValueShouldReturnRandomNonceValues(): void + { + $testRunCount = 10000; + $randomStrings = []; + + for ($i = 0; $i < $testRunCount; $i++) { + $nonce = new Nonce(); + $randomString = (string)$nonce; + + self::assertSame(16, strlen($randomString)); + $randomStrings[] = $randomString; + } + + self::assertTrue(array_unique($randomStrings) === $randomStrings, 'Random generator not sufficient'); + } +} diff --git a/Tests/Unit/Model/PolicyTest.php b/Tests/Unit/Model/PolicyTest.php new file mode 100644 index 0000000..b41536b --- /dev/null +++ b/Tests/Unit/Model/PolicyTest.php @@ -0,0 +1,109 @@ +<?php + +declare(strict_types=1); + +namespace Unit\Model; + +use Flowpack\ContentSecurityPolicy\Exceptions\InvalidDirectiveException; +use Flowpack\ContentSecurityPolicy\Model\Directive; +use Flowpack\ContentSecurityPolicy\Model\Nonce; +use Flowpack\ContentSecurityPolicy\Model\Policy; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\UsesClass; +use PHPUnit\Framework\TestCase; +use ReflectionClass; + +#[CoversClass(Policy::class)] +#[CoversClass(InvalidDirectiveException::class)] +#[UsesClass(Directive::class)] +class PolicyTest extends TestCase +{ + private readonly Policy $policy; + private readonly ReflectionClass $policyReflection; + + protected function setUp(): void + { + parent::setUp(); + + $this->policy = new Policy(); + $nonceMock = $this->createMock(Nonce::class); + $this->policy->setNonce($nonceMock); + + $this->policyReflection = new ReflectionClass($this->policy); + $this->policyReflection->getProperty('reportOnly')->setValue($this->policy, false); + } + + public function testGetSecurityHeaderKeyWithReportOnlyEnabled(): void + { + $this->policyReflection->getProperty('reportOnly')->setValue($this->policy, true); + + self::assertSame( + 'Content-Security-Policy-Report-Only', + $this->policy->getSecurityHeaderKey() + ); + } + + public function testGetSecurityHeaderKeyWithReportOnlyDisabled(): void + { + self::assertSame( + 'Content-Security-Policy', + $this->policy->getSecurityHeaderKey() + ); + } + + public function testAddDirectiveShouldFailWithInvalidDirective(): void + { + $this->expectException(InvalidDirectiveException::class); + + $this->policy->addDirective('invalid-directive', ['bar']); + } + + public function testAddDirectiveShouldAddSpecialDirective(): void + { + $this->policy->addDirective('script-src', ['self',]); + + self::assertSame( + [ + 'script-src' => ["'self'"], + ], + $this->policy->getDirectives() + ); + } + + public function testAddDirectiveShouldDetectNonceDirective(): void + { + $this->policy->addDirective('script-src', ['self', '{nonce}']); + + self::assertSame( + [ + 'script-src' => ["'self'", "'nonce-'"], + ], + $this->policy->getDirectives() + ); + + self::assertTrue($this->policy->hasNonceDirectiveValue()); + } + + public function testAddDirectiveShouldAddNonSpecialDirective(): void + { + $this->policy->addDirective('script-src', ['https://foo.bar']); + + self::assertSame( + [ + 'script-src' => ['https://foo.bar'], + ], + $this->policy->getDirectives() + ); + } + + public function testToString(): void + { + $this->policy->addDirective('script-src', ['https://foo.bar']); + $this->policy->addDirective('style-src', ['https://foo.bar']); + + self::assertSame( + 'script-src https://foo.bar; style-src https://foo.bar;', + (string)$this->policy + ); + } +} diff --git a/composer.json b/composer.json index d06837b..39c105e 100644 --- a/composer.json +++ b/composer.json @@ -14,6 +14,11 @@ "Flowpack\\ContentSecurityPolicy\\": "Classes/" } }, + "autoload-dev": { + "psr-4": { + "Flowpack\\ContentSecurityPolicy\\Tests\\": "Tests/" + } + }, "extra": { "neos": { "package-key": "Flowpack.ContentSecurityPolicy" @@ -23,5 +28,8 @@ "allow-plugins": { "neos/composer-plugin": true } + }, + "require-dev": { + "phpunit/phpunit": "^11.4" } } diff --git a/phpunit.xml b/phpunit.xml new file mode 100644 index 0000000..bca437a --- /dev/null +++ b/phpunit.xml @@ -0,0 +1,26 @@ +<?xml version="1.0" encoding="UTF-8"?> +<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd" + bootstrap="vendor/autoload.php" + cacheDirectory=".phpunit.cache" + executionOrder="depends,defects" + shortenArraysForExportThreshold="10" + requireCoverageMetadata="true" + beStrictAboutCoverageMetadata="true" + beStrictAboutOutputDuringTests="true" + displayDetailsOnPhpunitDeprecations="true" + failOnPhpunitDeprecation="true" + failOnRisky="true" + failOnWarning="true"> + <testsuites> + <testsuite name="default"> + <directory>Tests</directory> + </testsuite> + </testsuites> + + <source ignoreIndirectDeprecations="true" restrictNotices="true" restrictWarnings="true"> + <include> + <directory>Classes</directory> + </include> + </source> +</phpunit>