From 70a6a0f6b03d8fa770ada3c364d6dfca4adefb42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=A0pa=C4=8Dek?= Date: Thu, 9 Jan 2025 21:57:45 +0100 Subject: [PATCH] Detect callables in constructors Follow-up to #281 Ref #275 --- src/Calls/NewCalls.php | 25 ++- .../DisallowedCallableParameterRuleErrors.php | 34 +++- .../Calls/NewCallsCallableParametersTest.php | 184 ++++++++++++++++++ tests/Calls/NewCallsDefinedInTest.php | 2 + tests/Calls/NewCallsTest.php | 2 + tests/src/disallowed/callableParameters.php | 28 +++ 6 files changed, 265 insertions(+), 10 deletions(-) create mode 100644 tests/Calls/NewCallsCallableParametersTest.php diff --git a/src/Calls/NewCalls.php b/src/Calls/NewCalls.php index 4c1951d..d036389 100644 --- a/src/Calls/NewCalls.php +++ b/src/Calls/NewCalls.php @@ -13,6 +13,7 @@ use PHPStan\ShouldNotHappenException; use Spaze\PHPStan\Rules\Disallowed\DisallowedCall; use Spaze\PHPStan\Rules\Disallowed\DisallowedCallFactory; +use Spaze\PHPStan\Rules\Disallowed\RuleErrors\DisallowedCallableParameterRuleErrors; use Spaze\PHPStan\Rules\Disallowed\RuleErrors\DisallowedCallsRuleErrors; use Spaze\PHPStan\Rules\Disallowed\RuleErrors\ErrorIdentifiers; @@ -24,25 +25,33 @@ */ class NewCalls implements Rule { - private const CONSTRUCT = '::__construct'; + public const CONSTRUCT = '__construct'; private DisallowedCallsRuleErrors $disallowedCallsRuleErrors; + private DisallowedCallableParameterRuleErrors $disallowedCallableParameterRuleErrors; + /** @var list */ private array $disallowedCalls; /** * @param DisallowedCallsRuleErrors $disallowedCallsRuleErrors + * @param DisallowedCallableParameterRuleErrors $disallowedCallableParameterRuleErrors * @param DisallowedCallFactory $disallowedCallFactory * @param array $forbiddenCalls * @phpstan-param ForbiddenCallsConfig $forbiddenCalls * @noinspection PhpUndefinedClassInspection ForbiddenCallsConfig is a type alias defined in PHPStan config * @throws ShouldNotHappenException */ - public function __construct(DisallowedCallsRuleErrors $disallowedCallsRuleErrors, DisallowedCallFactory $disallowedCallFactory, array $forbiddenCalls) - { + public function __construct( + DisallowedCallsRuleErrors $disallowedCallsRuleErrors, + DisallowedCallableParameterRuleErrors $disallowedCallableParameterRuleErrors, + DisallowedCallFactory $disallowedCallFactory, + array $forbiddenCalls + ) { $this->disallowedCallsRuleErrors = $disallowedCallsRuleErrors; + $this->disallowedCallableParameterRuleErrors = $disallowedCallableParameterRuleErrors; $this->disallowedCalls = $disallowedCallFactory->createFromConfig($forbiddenCalls); } @@ -89,11 +98,11 @@ public function processNode(Node $node, Scope $scope): array $definedIn = $reflection ? $reflection->getFileName() : null; foreach ($names as $name) { - $name .= self::CONSTRUCT; - $errors = array_merge( - $errors, - $this->disallowedCallsRuleErrors->get($node, $scope, $name, $type->getClassName() . self::CONSTRUCT, $definedIn, $this->disallowedCalls, ErrorIdentifiers::DISALLOWED_NEW) - ); + $ruleErrors = $this->disallowedCallsRuleErrors->get($node, $scope, $name . '::' . self::CONSTRUCT, $type->getClassName() . '::' . self::CONSTRUCT, $definedIn, $this->disallowedCalls, ErrorIdentifiers::DISALLOWED_NEW); + $paramErrors = $this->disallowedCallableParameterRuleErrors->getForConstructor(new Name($name), $node, $scope); + if ($errors || $ruleErrors || $paramErrors) { + $errors = array_merge($errors, $ruleErrors, $paramErrors); + } } } diff --git a/src/RuleErrors/DisallowedCallableParameterRuleErrors.php b/src/RuleErrors/DisallowedCallableParameterRuleErrors.php index f20c336..a3bdf8f 100644 --- a/src/RuleErrors/DisallowedCallableParameterRuleErrors.php +++ b/src/RuleErrors/DisallowedCallableParameterRuleErrors.php @@ -7,6 +7,7 @@ use PhpParser\Node\Expr\CallLike; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\New_; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Name; use PHPStan\Analyser\ArgumentsNormalizer; @@ -18,6 +19,7 @@ use PHPStan\Rules\IdentifierRuleError; use PHPStan\ShouldNotHappenException; use PHPStan\Type\TypeCombinator; +use Spaze\PHPStan\Rules\Disallowed\Calls\NewCalls; use Spaze\PHPStan\Rules\Disallowed\DisallowedCall; use Spaze\PHPStan\Rules\Disallowed\DisallowedCallFactory; use Spaze\PHPStan\Rules\Disallowed\PHPStan1Compatibility; @@ -106,6 +108,34 @@ public function getForFunction(FuncCall $node, Scope $scope): array * @throws ShouldNotHappenException */ public function getForMethod($class, CallLike $node, Scope $scope): array + { + $names = $this->typeResolver->getNamesFromCall($node, $scope); + return $this->getForMethods($class, $names, $node, $scope); + } + + + /** + * @param Name|Expr $class + * @param New_ $node + * @param Scope $scope + * @return list + * @throws ShouldNotHappenException + */ + public function getForConstructor($class, New_ $node, Scope $scope): array + { + return $this->getForMethods($class, [new Name(NewCalls::CONSTRUCT)], $node, $scope); + } + + + /** + * @param Name|Expr $class + * @param list $names + * @param CallLike $node + * @param Scope $scope + * @return list + * @throws ShouldNotHappenException + */ + public function getForMethods($class, array $names, CallLike $node, Scope $scope): array { $ruleErrors = []; $classType = $this->typeResolver->getType($class, $scope); @@ -117,7 +147,7 @@ public function getForMethod($class, CallLike $node, Scope $scope): array continue; } $classReflection = $this->reflectionProvider->getClass($className); - foreach ($this->typeResolver->getNamesFromCall($node, $scope) as $name) { + foreach ($names as $name) { if (!$classReflection->hasMethod($name->toString())) { continue; } @@ -134,7 +164,7 @@ public function getForMethod($class, CallLike $node, Scope $scope): array /** * @param Scope $scope - * @param FuncCall|MethodCall|StaticCall $node + * @param CallLike $node * @param ExtendedMethodReflection|FunctionReflection $reflection * @return list * @throws ShouldNotHappenException diff --git a/tests/Calls/NewCallsCallableParametersTest.php b/tests/Calls/NewCallsCallableParametersTest.php new file mode 100644 index 0000000..8852f6a --- /dev/null +++ b/tests/Calls/NewCallsCallableParametersTest.php @@ -0,0 +1,184 @@ +getByType(TypeResolver::class), + $container->getByType(DisallowedFunctionRuleErrors::class), + $container->getByType(DisallowedMethodRuleErrors::class), + $container->getByType(DisallowedCallFactory::class), + $container->getByType(ReflectionProvider::class), + [ + [ + 'function' => 'var_dump()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + ], + [ + [ + 'method' => 'Callbacks::call()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + [ + 'method' => 'CallbacksInterface::interfaceCall()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + [ + 'method' => 'CallbacksTrait::traitCall()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + ], + [ + [ + 'method' => 'Callbacks::staticCall()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + [ + 'method' => 'CallbacksInterface::interfaceStaticCall()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + [ + 'method' => 'CallbacksTrait::traitStaticCall()', + 'allowIn' => [ + __DIR__ . '/../src/disallowed-allow/*.php', + __DIR__ . '/../src/*-allow/*.*', + ], + ], + ], + ); + return new NewCalls( + $container->getByType(DisallowedCallsRuleErrors::class), + $disallowedCallableParameterRuleErrors, + $container->getByType(DisallowedCallFactory::class), + [], + ); + } + + + public function testRule(): void + { + // Based on the configuration above, in this file: + $this->analyse([__DIR__ . '/../src/disallowed/callableParameters.php'], [ + [ + // expect this error message: + 'Calling var_dump() is forbidden.', + // on this line: + 176, + ], + [ + 'Calling var_dump() is forbidden.', + 177, + ], + [ + 'Calling var_dump() is forbidden.', + 178, + ], + [ + 'Calling Callbacks::call() is forbidden.', + 179, + ], + [ + 'Calling Callbacks::call() is forbidden.', + 180, + ], + [ + 'Calling Callbacks::call() is forbidden.', + 181, + ], + [ + 'Calling Callbacks::call() (as Callbacks2::call()) is forbidden.', + 182, + ], + [ + 'Calling Callbacks::call() (as Callbacks2::call()) is forbidden.', + 183, + ], + [ + 'Calling Callbacks::call() (as Callbacks2::call()) is forbidden.', + 184, + ], + [ + 'Calling Callbacks::staticCall() is forbidden.', + 185, + ], + [ + 'Calling Callbacks::staticCall() is forbidden.', + 186, + ], + [ + 'Calling Callbacks::staticCall() is forbidden.', + 187, + ], + [ + 'Calling Callbacks::staticCall() is forbidden.', + 188, + ], + [ + 'Calling Callbacks::staticCall() (as Callbacks2::staticCall()) is forbidden.', + 189, + ], + [ + 'Calling Callbacks::staticCall() (as Callbacks2::staticCall()) is forbidden.', + 190, + ], + [ + 'Calling Callbacks::staticCall() (as Callbacks2::staticCall()) is forbidden.', + 191, + ], + [ + 'Calling Callbacks::staticCall() (as Callbacks2::staticCall()) is forbidden.', + 192, + ], + ]); + // Based on the configuration above, no errors in this file: + $this->analyse([__DIR__ . '/../src/disallowed-allow/callableParameters.php'], []); + } + + + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../../extension.neon', + ]; + } + +} diff --git a/tests/Calls/NewCallsDefinedInTest.php b/tests/Calls/NewCallsDefinedInTest.php index 7c71467..cdb98d5 100644 --- a/tests/Calls/NewCallsDefinedInTest.php +++ b/tests/Calls/NewCallsDefinedInTest.php @@ -7,6 +7,7 @@ use PHPStan\ShouldNotHappenException; use PHPStan\Testing\RuleTestCase; use Spaze\PHPStan\Rules\Disallowed\DisallowedCallFactory; +use Spaze\PHPStan\Rules\Disallowed\RuleErrors\DisallowedCallableParameterRuleErrors; use Spaze\PHPStan\Rules\Disallowed\RuleErrors\DisallowedCallsRuleErrors; class NewCallsDefinedInTest extends RuleTestCase @@ -20,6 +21,7 @@ protected function getRule(): Rule $container = self::getContainer(); return new NewCalls( $container->getByType(DisallowedCallsRuleErrors::class), + $container->getByType(DisallowedCallableParameterRuleErrors::class), $container->getByType(DisallowedCallFactory::class), [ [ diff --git a/tests/Calls/NewCallsTest.php b/tests/Calls/NewCallsTest.php index 5f579f0..00b92be 100644 --- a/tests/Calls/NewCallsTest.php +++ b/tests/Calls/NewCallsTest.php @@ -7,6 +7,7 @@ use PHPStan\ShouldNotHappenException; use PHPStan\Testing\RuleTestCase; use Spaze\PHPStan\Rules\Disallowed\DisallowedCallFactory; +use Spaze\PHPStan\Rules\Disallowed\RuleErrors\DisallowedCallableParameterRuleErrors; use Spaze\PHPStan\Rules\Disallowed\RuleErrors\DisallowedCallsRuleErrors; class NewCallsTest extends RuleTestCase @@ -20,6 +21,7 @@ protected function getRule(): Rule $container = self::getContainer(); return new NewCalls( $container->getByType(DisallowedCallsRuleErrors::class), + $container->getByType(DisallowedCallableParameterRuleErrors::class), $container->getByType(DisallowedCallFactory::class), [ [ diff --git a/tests/src/disallowed/callableParameters.php b/tests/src/disallowed/callableParameters.php index 8fa04b2..452569c 100644 --- a/tests/src/disallowed/callableParameters.php +++ b/tests/src/disallowed/callableParameters.php @@ -166,3 +166,31 @@ public static function interfaceStaticCall(): void { echo __METHOD__; } CallableParams::staticCall([$callbacksPlusPlus, 'interfaceStaticCall']); CallableParams::staticCall([$callbacksPlusPlus, 'traitCall']); CallableParams::staticCall([CallbacksPlusPlus::class, 'traitStaticCall']); + +class ConstructorCallable +{ + public function __construct(?callable $callback, ?string $string = null) {} +} + +// disallowed callable params in constructors +new ConstructorCallable('var_dump'); +new ConstructorCallable($varDump); +new ConstructorCallable(string: null, callback: $varDump); +new ConstructorCallable([$callbacks, 'call']); +new ConstructorCallable([$callbacks, $call]); +new ConstructorCallable([new Callbacks, $call]); +new ConstructorCallable([$callbacks2, 'call']); +new ConstructorCallable([$callbacks2, $call]); +new ConstructorCallable([new Callbacks2, $call]); +new ConstructorCallable(['Callbacks', 'staticCall']); +new ConstructorCallable(['Callbacks', $staticCall]); +new ConstructorCallable([Callbacks::class, 'staticCall']); +new ConstructorCallable([Callbacks::class, $staticCall]); +new ConstructorCallable(['Callbacks2', 'staticCall']); +new ConstructorCallable(['Callbacks2', $staticCall]); +new ConstructorCallable([Callbacks2::class, 'staticCall']); +new ConstructorCallable([Callbacks2::class, $staticCall]); + +// not a callable param +new ConstructorCallable(null, 'var_dump'); +new ConstructorCallable(string: $varDump, callback: null);