From 1b0b3e82d859d678384cad101ed0e6c821f0f6e4 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Thu, 18 Jul 2024 13:56:19 +0200 Subject: [PATCH] Add a PHPStan rule to detect invalid patterns passed to composer/pcre methods --- extension.neon | 1 + phpstan-baseline.neon | 90 +++++++++++ phpstan.neon.dist | 1 + src/PHPStan/InvalidRegexPatternRule.php | 142 ++++++++++++++++++ .../InvalidRegexPatternRuleTest.php | 71 +++++++++ .../UnsafeStrictGroupsCallRuleTest.php | 2 +- .../fixtures/invalid-patterns.php | 22 +++ tests/PHPStanTests/nsrt/preg-match.php | 24 +-- 8 files changed, 340 insertions(+), 13 deletions(-) create mode 100644 src/PHPStan/InvalidRegexPatternRule.php create mode 100644 tests/PHPStanTests/InvalidRegexPatternRuleTest.php create mode 100644 tests/PHPStanTests/fixtures/invalid-patterns.php diff --git a/extension.neon b/extension.neon index 2147431..3a81d30 100644 --- a/extension.neon +++ b/extension.neon @@ -19,3 +19,4 @@ services: rules: - Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule + - Composer\Pcre\PHPStan\InvalidRegexPatternRule diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index b38221e..2ec840a 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -24,3 +24,93 @@ parameters: message: "#^Parameter &\\$matches @param\\-out type of method Composer\\\\Pcre\\\\Preg\\:\\:matchWithOffsets\\(\\) expects array\\\\}\\>, array\\ given\\.$#" count: 1 path: src/Preg.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/GrepTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/IsMatchAllTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/IsMatchAllWithOffsetsTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/IsMatchTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/IsMatchWithOffsetsTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/MatchAllTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/MatchTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/ReplaceCallbackArrayTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/ReplaceCallbackTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/ReplaceTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/SplitTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/PregTests/SplitWithOffsetsTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/RegexTests/IsMatchTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/RegexTests/MatchAllTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/RegexTests/MatchTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/RegexTests/ReplaceCallbackArrayTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/RegexTests/ReplaceCallbackTest.php + + - + message: "#^Regex pattern is invalid\\: No ending matching delimiter '\\}' found$#" + count: 2 + path: tests/RegexTests/ReplaceTest.php diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 890f3bc..d40b6ad 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -12,6 +12,7 @@ parameters: excludePaths: - tests/PHPStanTests/nsrt/* + - tests/PHPStanTests/fixtures/* includes: - extension.neon diff --git a/src/PHPStan/InvalidRegexPatternRule.php b/src/PHPStan/InvalidRegexPatternRule.php new file mode 100644 index 0000000..8a05fb2 --- /dev/null +++ b/src/PHPStan/InvalidRegexPatternRule.php @@ -0,0 +1,142 @@ + + */ +class InvalidRegexPatternRule implements Rule +{ + public function getNodeType(): string + { + return StaticCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $patterns = $this->extractPatterns($node, $scope); + + $errors = []; + foreach ($patterns as $pattern) { + $errorMessage = $this->validatePattern($pattern); + if ($errorMessage === null) { + continue; + } + + $errors[] = RuleErrorBuilder::message(sprintf('Regex pattern is invalid: %s', $errorMessage))->identifier('regexp.pattern')->build(); + } + + return $errors; + } + + /** + * @return string[] + */ + private function extractPatterns(StaticCall $node, Scope $scope): array + { + if (!$node->class instanceof FullyQualified) { + return []; + } + $isRegex = $node->class->toString() === Regex::class; + $isPreg = $node->class->toString() === Preg::class; + if (!$isRegex && !$isPreg) { + return []; + } + if (!$node->name instanceof Node\Identifier || !Preg::isMatch('{^(match|isMatch|grep|replace|split)}', $node->name->name)) { + return []; + } + + $functionName = $node->name->name; + if (!isset($node->getArgs()[0])) { + return []; + } + + $patternNode = $node->getArgs()[0]->value; + $patternType = $scope->getType($patternNode); + + $patternStrings = []; + + foreach ($patternType->getConstantStrings() as $constantStringType) { + if ($functionName === 'replaceCallbackArray') { + continue; + } + + $patternStrings[] = $constantStringType->getValue(); + } + + foreach ($patternType->getConstantArrays() as $constantArrayType) { + if ( + in_array($functionName, [ + 'replace', + 'replaceCallback', + ], true) + ) { + foreach ($constantArrayType->getValueTypes() as $arrayKeyType) { + foreach ($arrayKeyType->getConstantStrings() as $constantString) { + $patternStrings[] = $constantString->getValue(); + } + } + } + + if ($functionName !== 'replaceCallbackArray') { + continue; + } + + foreach ($constantArrayType->getKeyTypes() as $arrayKeyType) { + foreach ($arrayKeyType->getConstantStrings() as $constantString) { + $patternStrings[] = $constantString->getValue(); + } + } + } + + return $patternStrings; + } + + private function validatePattern(string $pattern): ?string + { + try { + $msg = null; + $prev = set_error_handler(function (int $severity, string $message, string $file) use (&$msg): bool { + $msg = preg_replace("#^preg_match(_all)?\\(.*?\\): #", '', $message); + + return true; + }); + + if ($pattern === '') { + return 'Empty string is not a valid regular expression'; + } + + Preg::match($pattern, ''); + if ($msg !== null) { + return $msg; + } + } catch (PcreException $e) { + if ($e->getCode() === PREG_INTERNAL_ERROR && $msg !== null) { + return $msg; + } + + return preg_replace('{.*? failed executing ".*": }', '', $e->getMessage()); + } finally { + restore_error_handler(); + } + + return null; + } + +} diff --git a/tests/PHPStanTests/InvalidRegexPatternRuleTest.php b/tests/PHPStanTests/InvalidRegexPatternRuleTest.php new file mode 100644 index 0000000..86c3334 --- /dev/null +++ b/tests/PHPStanTests/InvalidRegexPatternRuleTest.php @@ -0,0 +1,71 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace Composer\Pcre\PHPStanTests; + +use PHPStan\Testing\RuleTestCase; +use Composer\Pcre\PHPStan\InvalidRegexPatternRule; +use PHPStan\Type\Php\RegexArrayShapeMatcher; + +/** + * Run with "vendor/bin/phpunit --testsuite phpstan" + * + * This is excluded by default to avoid side effects with the library tests + * + * @extends RuleTestCase + */ +class InvalidRegexPatternRuleTest extends RuleTestCase +{ + protected function getRule(): \PHPStan\Rules\Rule + { + return new InvalidRegexPatternRule(); + } + + public function testRule(): void + { + $missing = PHP_VERSION_ID < 70300 ? ')' : 'closing parenthesis'; + + $this->analyse([__DIR__ . '/fixtures/invalid-patterns.php'], [ + [ + 'Regex pattern is invalid: Compilation failed: missing '.$missing.' at offset 1', + 11, + ], + [ + 'Regex pattern is invalid: Compilation failed: missing '.$missing.' at offset 1', + 13, + ], + [ + 'Regex pattern is invalid: Compilation failed: missing '.$missing.' at offset 1', + 15, + ], + [ + 'Regex pattern is invalid: Compilation failed: missing '.$missing.' at offset 1', + 17, + ], + [ + 'Regex pattern is invalid: Compilation failed: missing '.$missing.' at offset 1', + 19, + ], + [ + 'Regex pattern is invalid: Compilation failed: missing '.$missing.' at offset 1', + 21, + ], + ]); + } + + public static function getAdditionalConfigFiles(): array + { + return [ + 'phar://' . __DIR__ . '/../../vendor/phpstan/phpstan/phpstan.phar/conf/bleedingEdge.neon', + __DIR__ . '/../../extension.neon', + ]; + } +} diff --git a/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php b/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php index 74eb9e0..2264efd 100644 --- a/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php +++ b/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php @@ -22,7 +22,7 @@ * * @extends RuleTestCase */ -class UnsafeStrictGruopsCallRuleTest extends RuleTestCase +class UnsafeStrictGroupsCallRuleTest extends RuleTestCase { protected function getRule(): \PHPStan\Rules\Rule { diff --git a/tests/PHPStanTests/fixtures/invalid-patterns.php b/tests/PHPStanTests/fixtures/invalid-patterns.php new file mode 100644 index 0000000..fdb84ed --- /dev/null +++ b/tests/PHPStanTests/fixtures/invalid-patterns.php @@ -0,0 +1,22 @@ + function ($match) { return ''; }], $s); +} diff --git a/tests/PHPStanTests/nsrt/preg-match.php b/tests/PHPStanTests/nsrt/preg-match.php index 5dd23db..922da89 100644 --- a/tests/PHPStanTests/nsrt/preg-match.php +++ b/tests/PHPStanTests/nsrt/preg-match.php @@ -16,33 +16,33 @@ function doMatch(string $s): void assertType('array{}|array{string}', $matches); if (Preg::match('/Price: (£|€)\d+/', $s, $matches)) { - assertType('array{string, string}', $matches); + assertType('array{string, non-empty-string}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{string, string}', $matches); + assertType('array{}|array{string, non-empty-string}', $matches); if (Preg::match('/Price: (£|€)?\d+/', $s, $matches)) { - assertType('array{string, string|null}', $matches); + assertType('array{string, non-empty-string|null}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{string, string|null}', $matches); + assertType('array{}|array{string, non-empty-string|null}', $matches); // passing the PREG_UNMATCHED_AS_NULL should change nothing compared to above as it is always set if (Preg::match('/Price: (£|€)?\d+/', $s, $matches, PREG_UNMATCHED_AS_NULL)) { - assertType('array{string, string|null}', $matches); + assertType('array{string, non-empty-string|null}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{string, string|null}', $matches); + assertType('array{}|array{string, non-empty-string|null}', $matches); if (Preg::isMatch('/Price: (?£|€)\d+/', $s, $matches)) { - assertType('array{0: string, currency: string, 1: string}', $matches); + assertType('array{0: string, currency: non-empty-string, 1: non-empty-string}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{0: string, currency: string, 1: string}', $matches); + assertType('array{}|array{0: string, currency: non-empty-string, 1: non-empty-string}', $matches); } function doMatchStrictGroups(string $s): void @@ -55,18 +55,18 @@ function doMatchStrictGroups(string $s): void assertType('array{}|array{string}', $matches); if (Preg::matchStrictGroups('/Price: (£|€)\d+/', $s, $matches)) { - assertType('array{string, string}', $matches); + assertType('array{string, non-empty-string}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{string, string}', $matches); + assertType('array{}|array{string, non-empty-string}', $matches); if (Preg::isMatchStrictGroups('/Price: (?£|€)\d+/', $s, $matches)) { - assertType('array{0: string, test: string, 1: string}', $matches); + assertType('array{0: string, test: non-empty-string, 1: non-empty-string}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{0: string, test: string, 1: string}', $matches); + assertType('array{}|array{0: string, test: non-empty-string, 1: non-empty-string}', $matches); } function doMatchStrictGroupsUnsafe(string $s): void