From 73ccba79f4afb6cd84b29838b616b16c502aa84a Mon Sep 17 00:00:00 2001 From: Timon Heuser Date: Wed, 23 Oct 2024 10:54:04 +0200 Subject: [PATCH] TASK: add static code analysis --- .github/workflows/ci.yml | 14 ++++++++++--- .../Command/CspConfigCommandController.php | 2 +- Classes/Helpers/TagHelper.php | 9 ++++---- Classes/Http/CspHeaderMiddleware.php | 15 ++++--------- Classes/Model/Directive.php | 2 +- Classes/Model/Policy.php | 8 +++++-- Tests/Unit/Http/CspHeaderMiddlewareTest.php | 21 ++++++++++--------- composer.json | 9 ++++++-- phpstan-baseline.neon | 16 ++++++++++++++ phpstan.neon | 14 +++++++++++++ 10 files changed, 76 insertions(+), 34 deletions(-) create mode 100644 phpstan-baseline.neon create mode 100644 phpstan.neon diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30811e8..7312a27 100644 --- 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 index 2bc80bf..9811c3f 100644 --- 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 index 146b17d..7f0ba69 100644 --- 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 index 38b4dda..65fdfbe 100644 --- 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 index 28ce135..9b9d84a 100644 --- 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 index f7fe65b..f4fb9fe 100644 --- 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 index f8972c1..06d6517 100644 --- 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 index 39c105e..9eea1da 100644 --- 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 new file mode 100644 index 0000000..8113ead --- /dev/null +++ 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 new file mode 100644 index 0000000..63c9f0c --- /dev/null +++ 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