Skip to content

Commit 1126fb4

Browse files
SeldaekSeldaek
committed
Implement new rule to warn about unsafe strict groups calls
1 parent 4875f9f commit 1126fb4

File tree

10 files changed

+207
-15
lines changed

10 files changed

+207
-15
lines changed

‎extension.neon

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,14 @@ conditionalTags:
88
phpstan.staticMethodParameterOutTypeExtension: %featureToggles.narrowPregMatches%
99
Composer\Pcre\PHPStan\PregMatchTypeSpecifyingExtension:
1010
phpstan.typeSpecifier.staticMethodTypeSpecifyingExtension: %featureToggles.narrowPregMatches%
11+
Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule:
12+
phpstan.rules.rule: %featureToggles.narrowPregMatches%
1113

1214
services:
1315
-
1416
class: Composer\Pcre\PHPStan\PregMatchParameterOutTypeExtension
1517
-
1618
class: Composer\Pcre\PHPStan\PregMatchTypeSpecifyingExtension
19+
20+
rules:
21+
- Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule

‎src/PHPStan/PregMatchTypeSpecifyingExtension.php

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,15 @@ public function specifyTypes(MethodReflection $methodReflection, StaticCall $nod
7878
&& count($matchedType->getConstantArrays()) === 1
7979
) {
8080
$matchedType = $matchedType->getConstantArrays()[0];
81-
foreach ($matchedType as $type) {
82-
if ($type->containsNull()) {
83-
// TODO trigger an error here that $methodReflection->getName() should not be used, or the pattern needs to make sure that $type->?? get key name is not nullable
84-
}
85-
}
81+
$matchedType = new ConstantArrayType(
82+
$matchedType->getKeyTypes(),
83+
array_map(static function (Type $valueType): Type {
84+
return TypeCombinator::removeNull($valueType);
85+
}, $matchedType->getValueTypes()),
86+
$matchedType->getNextAutoIndexes(),
87+
[],
88+
$matchedType->isList()
89+
);
8690
}
8791

8892
$overwrite = false;

‎src/PHPStan/UnsafeStrictGroupsCallRule.php

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace Composer\Pcre\PHPStan;
4+
5+
use Composer\Pcre\Preg;
6+
use Composer\Pcre\Regex;
7+
use PhpParser\Node;
8+
use PhpParser\Node\Expr\StaticCall;
9+
use PhpParser\Node\Name\FullyQualified;
10+
use PHPStan\Analyser\Scope;
11+
use PHPStan\Analyser\SpecifiedTypes;
12+
use PHPStan\Rules\Rule;
13+
use PHPStan\Rules\RuleErrorBuilder;
14+
use PHPStan\TrinaryLogic;
15+
use PHPStan\Type\ObjectType;
16+
use PHPStan\Type\Type;
17+
use PHPStan\Type\TypeCombinator;
18+
use PHPStan\Type\Php\RegexArrayShapeMatcher;
19+
use function sprintf;
20+
21+
/**
22+
* @implements Rule<StaticCall>
23+
*/
24+
final class UnsafeStrictGroupsCallRule implements Rule
25+
{
26+
/**
27+
* @var RegexArrayShapeMatcher
28+
*/
29+
private $regexShapeMatcher;
30+
31+
public function __construct(RegexArrayShapeMatcher $regexShapeMatcher)
32+
{
33+
$this->regexShapeMatcher = $regexShapeMatcher;
34+
}
35+
36+
public function getNodeType(): string
37+
{
38+
return StaticCall::class;
39+
}
40+
41+
public function processNode(Node $node, Scope $scope): array
42+
{
43+
if (!$node->class instanceof FullyQualified) {
44+
return [];
45+
}
46+
$isRegex = $node->class->toString() === Regex::class;
47+
$isPreg = $node->class->toString() === Preg::class;
48+
if (!$isRegex && !$isPreg) {
49+
return [];
50+
}
51+
if (!$node->name instanceof Node\Identifier || !in_array($node->name->name, ['matchStrictGroups', 'isMatchStrictGroups', 'matchAllStrictGroups', 'isMatchAllStrictGroups'], true)) {
52+
return [];
53+
}
54+
55+
$args = $node->getArgs();
56+
if (!isset($args[0])) {
57+
return [];
58+
}
59+
60+
$patternArg = $args[0] ?? null;
61+
if ($isPreg) {
62+
if (!isset($args[2])) { // no matches set, skip as the matches won't be used anyway
63+
return [];
64+
}
65+
$flagsArg = $args[3] ?? null;
66+
} else {
67+
$flagsArg = $args[2] ?? null;
68+
}
69+
70+
if ($patternArg === null) {
71+
return [];
72+
}
73+
74+
$flagsType = PregMatchFlags::getType($flagsArg, $scope);
75+
if ($flagsType === null) {
76+
return [];
77+
}
78+
$patternType = $scope->getType($patternArg->value);
79+
80+
$matchedType = $this->regexShapeMatcher->matchType($patternType, $flagsType, TrinaryLogic::createFromBoolean(true));
81+
if ($matchedType === null) {
82+
return [
83+
RuleErrorBuilder::message(sprintf('The %s call is potentially unsafe as $matches\' type could not be inferred.', $node->name->name))
84+
->identifier('composerPcre.maybeUnsafeStrictGroups')
85+
->build(),
86+
];
87+
}
88+
89+
if (count($matchedType->getConstantArrays()) === 1) {
90+
$matchedType = $matchedType->getConstantArrays()[0];
91+
$nullableGroups = [];
92+
foreach ($matchedType->getValueTypes() as $index => $type) {
93+
if (TypeCombinator::containsNull($type)) {
94+
$nullableGroups[] = $matchedType->getKeyTypes()[$index]->getValue();
95+
}
96+
}
97+
98+
if (\count($nullableGroups) > 0) {
99+
return [
100+
RuleErrorBuilder::message(sprintf(
101+
'The %s call is unsafe as match group%s "%s" %s optional and may be null.',
102+
$node->name->name,
103+
\count($nullableGroups) > 1 ? 's' : '',
104+
implode('", "', $nullableGroups),
105+
\count($nullableGroups) > 1 ? 'are' : 'is',
106+
))->identifier('composerPcre.unsafeStrictGroups')->build(),
107+
];
108+
}
109+
}
110+
111+
return [];
112+
}
113+
}

‎src/Regex.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ public static function match(string $pattern, string $subject, int $flags = 0, i
4343
*/
4444
public static function matchStrictGroups(string $pattern, string $subject, int $flags = 0, int $offset = 0): MatchStrictGroupsResult
4545
{
46+
// @phpstan-ignore composerPcre.maybeUnsafeStrictGroups
4647
$count = Preg::matchStrictGroups($pattern, $subject, $matches, $flags, $offset);
4748

4849
return new MatchStrictGroupsResult($count, $matches);
@@ -87,6 +88,7 @@ public static function matchAllStrictGroups(string $pattern, string $subject, in
8788
self::checkOffsetCapture($flags, 'matchAllWithOffsets');
8889
self::checkSetOrder($flags);
8990

91+
// @phpstan-ignore composerPcre.maybeUnsafeStrictGroups
9092
$count = Preg::matchAllStrictGroups($pattern, $subject, $matches, $flags, $offset);
9193

9294
return new MatchAllStrictGroupsResult($count, $matches);

‎tests/PHPStanTests/TypeInferenceTest.php

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@
1717
* Run with "vendor/bin/phpunit --testsuite phpstan"
1818
*
1919
* This is excluded by default to avoid side effects with the library tests
20-
*
21-
* @group phpstan
2220
*/
2321
class TypeInferenceTest extends TypeInferenceTestCase
2422
{

‎tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php
2+
3+
/*
4+
* This file is part of composer/pcre.
5+
*
6+
* (c) Composer <https://github.com/composer>
7+
*
8+
* For the full copyright and license information, please view
9+
* the LICENSE file that was distributed with this source code.
10+
*/
11+
12+
namespace Composer\Pcre\PHPStanTests;
13+
14+
use PHPStan\Testing\RuleTestCase;
15+
use Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule;
16+
use PHPStan\Type\Php\RegexArrayShapeMatcher;
17+
use PHPStan\Php\PhpVersion;
18+
19+
/**
20+
* Run with "vendor/bin/phpunit --testsuite phpstan"
21+
*
22+
* This is excluded by default to avoid side effects with the library tests
23+
*
24+
* @extends RuleTestCase<UnsafeStrictGroupsCallRule>
25+
*/
26+
class UnsafeStrictGruopsCallRuleTest extends RuleTestCase
27+
{
28+
protected function getRule(): \PHPStan\Rules\Rule
29+
{
30+
// @phpstan-ignore phpstanApi.constructor,phpstanApi.constructor
31+
return new UnsafeStrictGroupsCallRule(new RegexArrayShapeMatcher(new PhpVersion(PHP_VERSION_ID, PhpVersion::SOURCE_RUNTIME)));
32+
}
33+
34+
public function testRule(): void
35+
{
36+
$this->analyse([__DIR__ . '/nsrt/preg-match.php'], [
37+
[
38+
'The matchStrictGroups call is unsafe as match group "1" is optional and may be null.',
39+
80,
40+
],
41+
[
42+
'The matchAllStrictGroups call is unsafe as match groups "foo", "2" are optional and may be null.',
43+
82,
44+
],
45+
[
46+
'The isMatchStrictGroups call is potentially unsafe as $matches\' type could not be inferred.',
47+
86,
48+
],
49+
]);
50+
}
51+
52+
public static function getAdditionalConfigFiles(): array
53+
{
54+
return [
55+
'phar://' . __DIR__ . '/../../vendor/phpstan/phpstan/phpstan.phar/conf/bleedingEdge.neon',
56+
__DIR__ . '/../../extension.neon',
57+
];
58+
}
59+
}

‎tests/PHPStanTests/nsrt/preg-match.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace PregMatchShapes;
44

55
use Composer\Pcre\Preg;
6+
use Composer\Pcre\Regex;
67
use function PHPStan\Testing\assertType;
78

89
function doMatch(string $s): void
@@ -75,8 +76,15 @@ function doMatchStrictGroupsUnsafe(string $s): void
7576
assertType('array{string, string}', $matches);
7677
}
7778

78-
if (Preg::isMatchStrictGroups('{Configure Command(?: *</td><td class="v">| *=> *)(.*)?(?:</td>|$)}m', $s, $matches)) {
79-
// should error as it is unsafe due to the optional group
79+
// should error as it is unsafe due to the optional group 1
80+
Regex::matchStrictGroups('{Configure Command(?: *</td><td class="v">| *=> *)(.*)?(?:</td>|$)}m', $s);
81+
82+
if (Preg::matchAllStrictGroups('{((?<foo>.)?z)}m', $s, $matches)) {
83+
// should error as it is unsafe due to the optional group foo/2
84+
}
85+
86+
if (Preg::isMatchStrictGroups('{'.$s.'}', $s, $matches)) {
87+
// should error as it is unsafe due not being introspectable with the dynamic string
8088
}
8189
}
8290

‎tests/PregTests/MatchAllTest.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public function testFailure(): void
4444

4545
public function testSuccessStrictGroups(): void
4646
{
47-
$count = Preg::matchAllStrictGroups('{(?P<m>\d)(?<matched>a)?}', '3a', $matches);
47+
$count = Preg::matchAllStrictGroups('{(?<m>\d)(?<matched>a)}', '3a', $matches);
4848
self::assertSame(1, $count);
4949
self::assertSame(array(0 => ['3a'], 'm' => ['3'], 1 => ['3'], 'matched' => ['a'], 2 => ['a']), $matches);
5050
}
@@ -53,7 +53,8 @@ public function testFailStrictGroups(): void
5353
{
5454
self::expectException(UnexpectedNullMatchException::class);
5555
self::expectExceptionMessage('Pattern "{(?P<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use matchAll() instead.');
56-
Preg::matchAllStrictGroups('{(?P<m>\d)(?<unmatched>b)?}', '123', $matches);
56+
// @phpstan-ignore composerPcre.unsafeStrictGroups
57+
Preg::matchAllStrictGroups('{(?<m>\d)(?<unmatched>b)?}', '123', $matches);
5758
}
5859

5960
public function testBadPatternThrowsIfWarningsAreNotThrowing(): void

‎tests/PregTests/MatchTest.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public function testSuccessWithInt(): void
3838

3939
public function testSuccessStrictGroups(): void
4040
{
41-
$count = Preg::matchStrictGroups('{(?P<m>\d)(?<matched>a)?}', '3a', $matches);
41+
$count = Preg::matchStrictGroups('{(?<m>\d)(?<matched>a)}', '3a', $matches);
4242
self::assertSame(1, $count);
4343
self::assertSame(array(0 => '3a', 'm' => '3', 1 => '3', 'matched' => 'a', 2 => 'a'), $matches);
4444
}
@@ -47,7 +47,8 @@ public function testFailStrictGroups(): void
4747
{
4848
self::expectException(UnexpectedNullMatchException::class);
4949
self::expectExceptionMessage('Pattern "{(?P<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.');
50-
Preg::matchStrictGroups('{(?P<m>\d)(?<unmatched>b)?}', '123', $matches);
50+
// @phpstan-ignore composerPcre.unsafeStrictGroups
51+
Preg::matchStrictGroups('{(?<m>\d)(?<unmatched>b)?}', '123', $matches);
5152
}
5253

5354
public function testTypeErrorWithNull(): void

‎tests/RegexTests/MatchTest.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,16 @@ public function testFailure(): void
4040

4141
public function testSuccessStrictGroups(): void
4242
{
43-
$result = Regex::matchStrictGroups('{(?P<m>\d)(?<matched>a)?}', '3a');
43+
$result = Regex::matchStrictGroups('{(?<m>\d)(?<matched>a)}', '3a');
4444
self::assertSame(array(0 => '3a', 'm' => '3', 1 => '3', 'matched' => 'a', 2 => 'a'), $result->matches);
4545
}
4646

4747
public function testFailStrictGroups(): void
4848
{
4949
self::expectException(UnexpectedNullMatchException::class);
5050
self::expectExceptionMessage('Pattern "{(?P<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.');
51-
Regex::matchStrictGroups('{(?P<m>\d)(?<unmatched>b)?}', '123');
51+
// @phpstan-ignore composerPcre.unsafeStrictGroups
52+
Regex::matchStrictGroups('{(?<m>\d)(?<unmatched>b)?}', '123');
5253
}
5354

5455
public function testBadPatternThrowsIfWarningsAreNotThrowing(): void

0 commit comments

Comments
 (0)