From d71f6491ca99c27bff070a99615aa2b668602e42 Mon Sep 17 00:00:00 2001 From: Timon Heuser Date: Wed, 23 Oct 2024 10:18:40 +0200 Subject: [PATCH] TASK: add tests --- .github/workflows/ci.yml | 22 +++ .gitignore | 1 + .../Command/CspConfigCommandController.php | 9 +- Classes/Factory/PolicyFactory.php | 8 +- Classes/Helpers/TagHelper.php | 27 ++-- Classes/Http/CspHeaderMiddleware.php | 19 ++- Classes/Model/Nonce.php | 4 +- Classes/Model/Policy.php | 36 ++--- Tests/Unit/Factory/PolicyFactoryTest.php | 53 +++++++ Tests/Unit/Helpers/TagHelperTest.php | 77 ++++++++++ Tests/Unit/Http/CspHeaderMiddlewareTest.php | 137 ++++++++++++++++++ Tests/Unit/Model/DirectiveTest.php | 23 +++ Tests/Unit/Model/NonceTest.php | 29 ++++ Tests/Unit/Model/PolicyTest.php | 109 ++++++++++++++ composer.json | 8 + phpunit.xml | 26 ++++ 16 files changed, 537 insertions(+), 51 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 Tests/Unit/Factory/PolicyFactoryTest.php create mode 100644 Tests/Unit/Helpers/TagHelperTest.php create mode 100644 Tests/Unit/Http/CspHeaderMiddlewareTest.php create mode 100644 Tests/Unit/Model/DirectiveTest.php create mode 100644 Tests/Unit/Model/NonceTest.php create mode 100644 Tests/Unit/Model/PolicyTest.php create mode 100644 phpunit.xml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..30811e8 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,22 @@ +name: CI + +on: [ push ] + +jobs: + phpunit: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + + - uses: php-actions/composer@v6 + + - name: PHPUnit Tests + uses: php-actions/phpunit@v4 + with: + 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/Command/CspConfigCommandController.php b/Classes/Command/CspConfigCommandController.php index 1408fb3..2bc80bf 100644 --- a/Classes/Command/CspConfigCommandController.php +++ b/Classes/Command/CspConfigCommandController.php @@ -24,6 +24,11 @@ class CspConfigCommandController extends CommandController */ protected Nonce $nonce; + /** + * @Flow\Inject + */ + protected PolicyFactory $policyFactory; + /** * @Flow\InjectConfiguration(path="content-security-policy") * @var mixed[] @@ -39,13 +44,13 @@ class CspConfigCommandController extends CommandController public function showCommand(): void { try { - $backendPolicy = PolicyFactory::create( + $backendPolicy = $this->policyFactory->create( $this->nonce, $this->configuration['backend'], $this->configuration['custom-backend'] ); - $frontendPolicy = PolicyFactory::create( + $frontendPolicy = $this->policyFactory->create( $this->nonce, $this->configuration['default'], $this->configuration['custom'] diff --git a/Classes/Factory/PolicyFactory.php b/Classes/Factory/PolicyFactory.php index e1a4781..d441eda 100644 --- a/Classes/Factory/PolicyFactory.php +++ b/Classes/Factory/PolicyFactory.php @@ -7,13 +7,19 @@ use Flowpack\ContentSecurityPolicy\Exceptions\InvalidDirectiveException; use Flowpack\ContentSecurityPolicy\Model\Nonce; use Flowpack\ContentSecurityPolicy\Model\Policy; +use Neos\Flow\Annotations as Flow; +/** + * @Flow\Scope("singleton") + */ class PolicyFactory { /** + * @param string[][] $defaultDirective + * @param string[][] $customDirective * @throws InvalidDirectiveException */ - public static function create(Nonce $nonce, array $defaultDirective, array $customDirective): Policy + public function create(Nonce $nonce, array $defaultDirective, array $customDirective): Policy { $directiveCollections = [$defaultDirective, $customDirective]; $defaultDirective = array_shift($directiveCollections); 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 '/(?<[a-z]+.*?)(?>|\/>)/'; } - private static function buildMatchAttributeNameWithAnyValueReqex( - string $name - ): string { + private static function buildMatchAttributeNameWithAnyValueReqex(string $name): string + { $nameQuoted = self::escapeReqexCharsInString($name); return '/(?
<.*? )(?'.
diff --git a/Classes/Http/CspHeaderMiddleware.php b/Classes/Http/CspHeaderMiddleware.php
index 66b5245..38b4dda 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")
      */
@@ -35,6 +37,11 @@ class CspHeaderMiddleware implements MiddlewareInterface
      */
     protected Nonce $nonce;
 
+    /**
+     * @Flow\Inject
+     */
+    protected PolicyFactory $policyFactory;
+
     /**
      * @Flow\InjectConfiguration(path="content-security-policy")
      * @var mixed[]
@@ -47,7 +54,7 @@ class CspHeaderMiddleware implements MiddlewareInterface
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
         $response = $handler->handle($request);
-        if (!$this->enabled) {
+        if (! $this->enabled) {
             return $response;
         }
 
@@ -79,14 +86,14 @@ private function getPolicyByCurrentContext(ServerRequestInterface $request): Pol
          * Neos\Neos\Domain\Service\ContentContext at this point as it throws an error.
          */
         if (str_starts_with($request->getUri()->getPath(), '/neos')) {
-            return PolicyFactory::create(
+            return $this->policyFactory->create(
                 $this->nonce,
                 $this->configuration['backend'],
                 $this->configuration['custom-backend']
             );
         }
 
-        return PolicyFactory::create(
+        return $this->policyFactory->create(
             $this->nonce,
             $this->configuration['default'],
             $this->configuration['custom']
@@ -101,11 +108,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..5b0cbf6
--- /dev/null
+++ b/Tests/Unit/Factory/PolicyFactoryTest.php
@@ -0,0 +1,53 @@
+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 @@
+';
+        self::assertTrue(TagHelper::tagHasAttribute($tag, 'src'));
+    }
+
+    public function testTagHasAttributeWithoutValueShouldReturnFalse(): void
+    {
+        $tag = '';
+        self::assertFalse(TagHelper::tagHasAttribute($tag, 'bar'));
+    }
+
+    public function testTagHasAttributeWithValueShouldReturnTrue(): void
+    {
+        $tag = '';
+        self::assertTrue(TagHelper::tagHasAttribute($tag, 'src', 'https://google.com'));
+    }
+
+    public function testTagHasAttributeWithValueShouldReturnFalse(): void
+    {
+        $tag = '';
+        self::assertFalse(TagHelper::tagHasAttribute($tag, 'src', 'another value'));
+    }
+
+    public function testTagChangeAttributeValueShouldChangeValue(): void
+    {
+        $tag = '';
+
+        self::assertSame(
+            '',
+            TagHelper::tagChangeAttributeValue($tag, 'src', 'https://test.com')
+        );
+    }
+
+    public function testTagChangeAttributeValueShouldDoNothingIfAttributeDoesntExist(): void
+    {
+        $tag = '';
+
+        self::assertSame(
+            '',
+            TagHelper::tagChangeAttributeValue($tag, 'nonce', 'da65sf1g')
+        );
+    }
+
+    public function testTagAddAttributeShouldAddAttributeWithValue(): void
+    {
+        $tag = '';
+
+        self::assertSame(
+            '',
+            TagHelper::tagAddAttribute($tag, 'nonce', 'da65sf1g')
+        );
+    }
+
+    public function testTagAddAttributeShouldAddAttributeWithoutValue(): void
+    {
+        $tag = '';
+
+        self::assertSame(
+            '',
+            TagHelper::tagAddAttribute($tag, 'defer')
+        );
+    }
+}
diff --git a/Tests/Unit/Http/CspHeaderMiddlewareTest.php b/Tests/Unit/Http/CspHeaderMiddlewareTest.php
new file mode 100644
index 0000000..f8972c1
--- /dev/null
+++ b/Tests/Unit/Http/CspHeaderMiddlewareTest.php
@@ -0,0 +1,137 @@
+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->nonceMock = $this->createMock(Nonce::class);
+        $this->uriMock = $this->createMock(UriInterface::class);
+        $this->policyFactoryMock = $this->createMock(PolicyFactory::class);
+        $this->policyMock = $this->createMock(Policy::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);
+
+        $reflectionProperty = $this->middlewareReflection->getProperty('nonce');
+        $reflectionProperty->setValue($this->middleware, $this->nonceMock);
+
+        $reflectionProperty = $this->middlewareReflection->getProperty('policyFactory');
+        $reflectionProperty->setValue($this->middleware, $this->policyFactoryMock);
+
+        $reflectionProperty = $this->middlewareReflection->getProperty('configuration');
+        $reflectionProperty->setValue(
+            $this->middleware,
+            ['backend' => [], 'custom-backend' => [], 'default' => [], 'custom' => [],]
+        );
+
+        $this->requestHandlerMock->expects($this->once())->method('handle')->willReturn($this->responseMock);
+    }
+
+    public function testProcessShouldDoNothingIfIsDisabled(): void
+    {
+        $reflectionProperty = $this->middlewareReflection->getProperty('enabled');
+        $reflectionProperty->setValue($this->middleware, false);
+
+        $this->responseMock->expects($this->never())->method('withAddedHeader');
+
+        $this->middleware->process($this->requestMock, $this->requestHandlerMock);
+    }
+
+    public function testProcessShouldDoNothingIfPolicyIsInvalid(): void
+    {
+        $this->requestMock->expects($this->once())->method('getUri')->willReturn($this->uriMock);
+        $this->uriMock->expects($this->once())->method('getPath')->willReturn('/');
+
+        $this->policyFactoryMock->expects($this->once())->method('create')->willThrowException(
+            new InvalidPolicyException()
+        );
+
+        $this->responseMock->expects($this->never())->method('withAddedHeader');
+
+        $this->middleware->process($this->requestMock, $this->requestHandlerMock);
+    }
+
+    public function testProcessShouldAddHeadersToResponse(): void
+    {
+        $this->requestMock->expects($this->once())->method('getUri')->willReturn($this->uriMock);
+        $this->uriMock->expects($this->once())->method('getPath')->willReturn('/neos');
+
+        $this->policyFactoryMock->expects($this->once())->method('create')->willReturn($this->policyMock);
+        $this->policyMock->expects($this->once())->method('hasNonceDirectiveValue')->willReturn(false);
+
+        $this->responseMock->expects($this->once())->method('withAddedHeader')->willReturnSelf();
+
+        $this->middleware->process($this->requestMock, $this->requestHandlerMock);
+    }
+
+    public function testProcessShouldAddHeadersToResponseAndReplaceBody(): void
+    {
+        $this->requestMock->expects($this->once())->method('getUri')->willReturn($this->uriMock);
+        $this->uriMock->expects($this->once())->method('getPath')->willReturn('/neos');
+
+        $this->policyFactoryMock->expects($this->once())->method('create')->willReturn($this->policyMock);
+
+        $this->policyMock->expects($this->once())->method('hasNonceDirectiveValue')->willReturn(true);
+        $this->responseMock->expects(once())->method('getBody')->willReturn(
+            'Test'
+        );
+
+        $this->responseMock->expects($this->once())->method('withBody')->willReturnSelf();
+        $this->responseMock->expects($this->once())->method('withAddedHeader')->willReturnSelf();
+
+        $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 @@
+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 @@
+
+
+    
+        
+            Tests
+        
+    
+
+    
+        
+            Classes
+        
+    
+