Merge pull request #4 from Flowpack/add-static-code-analysis

TASK: add static code analysis
Flowpack · Oct 23, 2024 · dba66ce · dba66ce
2 parents 14171d7 + 73ccba7
commit dba66ce
Show file tree

Hide file tree

Showing 10 changed files with 76 additions and 34 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -7,10 +7,8 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v3
-
+      - uses: actions/checkout@v4
       - uses: php-actions/composer@v6
-
       - name: PHPUnit Tests
         uses: php-actions/phpunit@v4
         with:
@@ -20,3 +18,13 @@ jobs:
           coverage_text: true
         env:
           XDEBUG_MODE: coverage
+
+  phpstan:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: php-actions/composer@v6
+      - uses: php-actions/phpstan@v3
+        with:
+          path: Classes Tests
+          configuration: phpstan.neon
diff --git a/Classes/Command/CspConfigCommandController.php b/Classes/Command/CspConfigCommandController.php
@@ -31,7 +31,7 @@ class CspConfigCommandController extends CommandController
 
     /**
      * @Flow\InjectConfiguration(path="content-security-policy")
-     * @var mixed[]
+     * @var string[][][]
      */
     protected array $configuration;
 

diff --git a/Classes/Helpers/TagHelper.php b/Classes/Helpers/TagHelper.php
@@ -11,14 +11,15 @@ public static function tagHasAttribute(
         string $name,
         string $value = null
     ): bool {
-        if (! $value) {
-            return ! ! preg_match(
+        $value = (string)$value;
+        if ($value === '') {
+            return (bool)preg_match(
                 self::buildMatchAttributeNameReqex($name),
                 $tag
             );
         }
 
-        return ! ! preg_match(
+        return (bool)preg_match(
             self::buildMatchAttributeNameWithSpecificValueReqex(
                 $name,
                 $value
@@ -53,7 +54,7 @@ public static function tagAddAttribute(
         return preg_replace_callback(
             self::buildMatchEndOfOpeningTagReqex(),
             function ($hits) use ($name, $value) {
-                if ($value) {
+                if ((string)$value !== '') {
                     return $hits["start"].
                         ' '.
                         $name.

diff --git a/Classes/Http/CspHeaderMiddleware.php b/Classes/Http/CspHeaderMiddleware.php
@@ -44,7 +44,7 @@ class CspHeaderMiddleware implements MiddlewareInterface
 
     /**
      * @Flow\InjectConfiguration(path="content-security-policy")
-     * @var mixed[]
+     * @var string[][][]
      */
     protected array $configuration;
 
@@ -107,7 +107,7 @@ private function addNonceToTags(string $markup): string
 
         return $this->checkTagAndReplaceUsingACallback($tagNames, $markup, function (
             $tagMarkup,
-        ) {
+        ): string {
             if (TagHelper::tagHasAttribute($tagMarkup, self::NONCE)) {
                 return TagHelper::tagChangeAttributeValue($tagMarkup, self::NONCE, $this->nonce->getValue());
             }
@@ -118,9 +118,6 @@ private function addNonceToTags(string $markup): string
 
     /**
      * @param  string[]  $tagNames
-     * @param  string  $contentMarkup
-     * @param  callable  $hitCallback
-     * @return string
      */
     private function checkTagAndReplaceUsingACallback(
         array $tagNames,
@@ -131,14 +128,10 @@ private function checkTagAndReplaceUsingACallback(
 
         return preg_replace_callback(
             $regex,
-            function ($hits) use ($hitCallback, $tagNames) {
+            function ($hits) use ($hitCallback) {
                 $tagMarkup = $hits[0];
                 $tagName = $hits[1];
-
-                if (! $hitCallback) {
-                    return $tagMarkup;
-                }
-
+
                 return call_user_func(
                     $hitCallback,
                     $tagMarkup,

diff --git a/Classes/Model/Directive.php b/Classes/Model/Directive.php
@@ -39,7 +39,7 @@ class Directive
 
     public static function isValidDirective(string $directive): bool
     {
-        return in_array($directive, self::VALID_DIRECTIVES);
+        return in_array($directive, self::VALID_DIRECTIVES, true);
     }
 
 }
diff --git a/Classes/Model/Policy.php b/Classes/Model/Policy.php
@@ -27,6 +27,7 @@ class Policy
      */
     protected bool $reportOnly;
 
+    /** @var string[][] */
     private array $directives = [];
 
     private readonly Nonce $nonce;
@@ -49,6 +50,9 @@ public function getSecurityHeaderKey(): string
         return self::SECURITY_HEADER_KEY;
     }
 
+    /**
+     * @return string[][]
+     */
     public function getDirectives(): array
     {
         return $this->directives;
@@ -68,7 +72,7 @@ public function addDirective(string $directive, array $values): self
         if (! Directive::isValidDirective($directive)) {
             throw new InvalidDirectiveException($directive);
         }
-        $this->directives[$directive] = array_map(function ($value) use ($directive) {
+        $this->directives[$directive] = array_map(function ($value) {
             return $this->sanitizeValue($value);
         }, $values);
 
@@ -91,7 +95,7 @@ public function __toString(): string
 
     private function sanitizeValue(string $value): string
     {
-        if (in_array($value, self::SPECIAL_DIRECTIVES)) {
+        if (in_array($value, self::SPECIAL_DIRECTIVES, true)) {
             return "'$value'";
         }
 

diff --git a/Tests/Unit/Http/CspHeaderMiddlewareTest.php b/Tests/Unit/Http/CspHeaderMiddlewareTest.php
@@ -30,14 +30,13 @@ 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;
-    private readonly Nonce|MockObject $nonceMock;
-    private readonly UriInterface|MockObject $uriMock;
-    private readonly PolicyFactory|MockObject $policyFactoryMock;
-    private readonly Policy|MockObject $policyMock;
+    private readonly ServerRequestInterface&MockObject $requestMock;
+    private readonly RequestHandlerInterface&MockObject $requestHandlerMock;
+    private readonly ResponseInterface&MockObject $responseMock;
+    private readonly LoggerInterface&MockObject $loggerMock;
+    private readonly UriInterface&MockObject $uriMock;
+    private readonly PolicyFactory&MockObject $policyFactoryMock;
+    private readonly Policy&MockObject $policyMock;
 
     /**
      * @throws Throwable
@@ -52,7 +51,7 @@ protected function setUp(): void
         $this->requestHandlerMock = $this->createMock(RequestHandlerInterface::class);
         $this->responseMock = $this->createMock(ResponseInterface::class);
         $this->loggerMock = $this->createMock(LoggerInterface::class);
-        $this->nonceMock = $this->createMock(Nonce::class);
+        $nonceMock = $this->createMock(Nonce::class);
         $this->uriMock = $this->createMock(UriInterface::class);
         $this->policyFactoryMock = $this->createMock(PolicyFactory::class);
         $this->policyMock = $this->createMock(Policy::class);
@@ -66,7 +65,7 @@ protected function setUp(): void
         $reflectionProperty->setValue($this->middleware, true);
 
         $reflectionProperty = $this->middlewareReflection->getProperty('nonce');
-        $reflectionProperty->setValue($this->middleware, $this->nonceMock);
+        $reflectionProperty->setValue($this->middleware, $nonceMock);
 
         $reflectionProperty = $this->middlewareReflection->getProperty('policyFactory');
         $reflectionProperty->setValue($this->middleware, $this->policyFactoryMock);
@@ -99,6 +98,8 @@ public function testProcessShouldDoNothingIfPolicyIsInvalid(): void
             new InvalidPolicyException()
         );
 
+        $this->loggerMock->expects($this->once())->method('critical');
+
         $this->responseMock->expects($this->never())->method('withAddedHeader');
 
         $this->middleware->process($this->requestMock, $this->requestHandlerMock);

diff --git a/composer.json b/composer.json
@@ -26,10 +26,15 @@
     },
     "config": {
         "allow-plugins": {
-            "neos/composer-plugin": true
+            "neos/composer-plugin": true,
+            "phpstan/extension-installer": true
         }
     },
     "require-dev": {
-        "phpunit/phpunit": "^11.4"
+        "phpunit/phpunit": "^11.4",
+        "phpstan/phpstan": "^1.12",
+        "phpstan/phpstan-phpunit": "^1.4",
+        "phpstan/extension-installer": "^1.4",
+        "phpstan/phpstan-strict-rules": "^1.6"
     }
 }
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -0,0 +1,16 @@
+parameters:
+	ignoreErrors:
+		-
+			message: "#^Method Flowpack\\\\ContentSecurityPolicy\\\\Helpers\\\\TagHelper\\:\\:tagAddAttribute\\(\\) should return string but returns string\\|null\\.$#"
+			count: 1
+			path: Classes/Helpers/TagHelper.php
+
+		-
+			message: "#^Method Flowpack\\\\ContentSecurityPolicy\\\\Helpers\\\\TagHelper\\:\\:tagChangeAttributeValue\\(\\) should return string but returns string\\|null\\.$#"
+			count: 1
+			path: Classes/Helpers/TagHelper.php
+
+		-
+			message: "#^Method Flowpack\\\\ContentSecurityPolicy\\\\Http\\\\CspHeaderMiddleware\\:\\:checkTagAndReplaceUsingACallback\\(\\) should return string but returns string\\|null\\.$#"
+			count: 1
+			path: Classes/Http/CspHeaderMiddleware.php
diff --git a/phpstan.neon b/phpstan.neon
@@ -0,0 +1,14 @@
+includes:
+	- phpstan-baseline.neon
+parameters:
+    level: max
+    paths:
+        - Classes
+        - Tests
+    ignoreErrors:
+        -
+            identifier: property.uninitializedReadonly
+        -
+            identifier: property.readOnlyAssignNotInConstructor
+        -
+            identifier: missingType.generics