Add AccessGlobalVariableWithinContextRule (#9)

* Add AccessGlobalVariableWithinContextRule * Each TestAsset in a subfolder to avoid namespace conflicts * Added AccessGlobalVariableWithinContextRule confi for Yii * Updated REAMDE.md * How to test this?
Slamdunk · May 14, 2019 · c98f0ba · c98f0ba
1 parent 67447a8
commit c98f0ba
Show file tree

Hide file tree

Showing 19 changed files with 298 additions and 67 deletions.
diff --git a/README.md b/README.md
@@ -36,6 +36,8 @@ includes:
     1. `@covers`
     1. `@coversDefaultClass`
     1. `@uses`
+1.  `SlamPhpStan\AccessGlobalVariableWithinContextRule`: inhibit the access to globals within
+classes that extend or implement a certain class/interface
 1.  `SlamPhpStan\AccessStaticPropertyWithinModelContextRule`: inhibit the access to static attributes of a class within
 classes that extend or implement a certain class/interface, useful to prohibit usage of singletons in models
 

diff --git a/conf/yii-rules.neon b/conf/yii-rules.neon
@@ -13,3 +13,15 @@ services:
         arguments:
             modelBaseClassOrInterface: yii\db\ActiveQueryInterface
             singletonAccessor: yii\BaseYii
+    -
+        class: SlamPhpStan\AccessGlobalVariableWithinContextRule
+        tags:
+            - phpstan.rules.rule
+        arguments:
+            contextBaseClassOrInterface: yii\db\ActiveRecordInterface
+    -
+        class: SlamPhpStan\AccessGlobalVariableWithinContextRule
+        tags:
+            - phpstan.rules.rule
+        arguments:
+            contextBaseClassOrInterface: yii\db\ActiveQueryInterface
diff --git a/lib/AccessGlobalVariableWithinContextRule.php b/lib/AccessGlobalVariableWithinContextRule.php
@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SlamPhpStan;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\Variable;
+use PHPStan\Analyser\Scope;
+use PHPStan\Broker\Broker;
+use PHPStan\Rules\Rule;
+
+final class AccessGlobalVariableWithinContextRule implements Rule
+{
+    /**
+     * @var Broker
+     */
+    private $broker;
+
+    /**
+     * @var string
+     */
+    private $contextBaseClassOrInterface;
+
+    private $globals = [
+        'GLOBALS'  => true,
+        '_SERVER'  => true,
+        '_GET'     => true,
+        '_POST'    => true,
+        '_FILES'   => true,
+        '_REQUEST' => true,
+        '_SESSION' => true,
+        '_ENV'     => true,
+        '_COOKIE'  => true,
+        'argc'     => true,
+        'argv'     => true,
+    ];
+
+    public function __construct(Broker $broker, string $contextBaseClassOrInterface)
+    {
+        $this->broker                       = $broker;
+        $this->contextBaseClassOrInterface  = $contextBaseClassOrInterface;
+    }
+
+    public function getNodeType(): string
+    {
+        return Variable::class;
+    }
+
+    /**
+     * @param \PhpParser\Node\Expr\Variable $node
+     * @param \PHPStan\Analyser\Scope       $scope
+     *
+     * @return string[] errors
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        if (! \is_string($node->name)) {
+            return [];
+        }
+
+        if (! isset($this->globals[$node->name])) {
+            return [];
+        }
+
+        if (! $scope->isInClass()) {
+            return [];
+        }
+
+        $classReflection = $scope->getClassReflection();
+        if (! $classReflection->isSubclassOf($this->contextBaseClassOrInterface)) {
+            return [];
+        }
+
+        $modelBaseClassOrInterface = $this->broker->getClass($this->contextBaseClassOrInterface);
+
+        return [\sprintf('Class %s %s %s and uses $%s: accessing globals in this context is considered an anti-pattern',
+            $classReflection->getDisplayName(),
+            $modelBaseClassOrInterface->isInterface() ? 'implements' : 'extends',
+            $this->contextBaseClassOrInterface,
+            $node->name
+        )];
+    }
+}
diff --git a/tests/AccessGlobalVariableWithinContextRuleTest.php b/tests/AccessGlobalVariableWithinContextRuleTest.php
@@ -0,0 +1,66 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SlamPhpStan\Tests;
+
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+use SlamPhpStan\AccessGlobalVariableWithinContextRule;
+
+/**
+ * @covers \SlamPhpStan\AccessGlobalVariableWithinContextRule
+ */
+final class AccessGlobalVariableWithinContextRuleTest extends RuleTestCase
+{
+    /**
+     * @var string
+     */
+    private $contextBaseClassOrInterface;
+
+    public function __construct($name = null, array $data = [], $dataName = '')
+    {
+        $this->contextBaseClassOrInterface = TestAsset\AccessGlobalVariableWithinContextRule\YiiAlikeActiveRecordInterface::class;
+
+        parent::__construct($name, $data, $dataName);
+    }
+
+    protected function getRule(): Rule
+    {
+        $broker = $this->createBroker();
+
+        return new AccessGlobalVariableWithinContextRule($broker, $this->contextBaseClassOrInterface);
+    }
+
+    public function testRule()
+    {
+        $this->analyse(
+            [
+                __DIR__ . '/TestAsset/AccessGlobalVariableWithinContextRule/fixture.php',
+            ],
+            [
+                [
+                    \sprintf('Class %s implements %s and uses $_POST: accessing globals in this context is considered an anti-pattern',
+                        TestAsset\AccessGlobalVariableWithinContextRule\ModelAccessingGlobalVariable::class,
+                        $this->contextBaseClassOrInterface
+                    ),
+                    8,
+                ],
+                [
+                    \sprintf('Class %s implements %s and uses $GLOBALS: accessing globals in this context is considered an anti-pattern',
+                        TestAsset\AccessGlobalVariableWithinContextRule\ModelAccessingGlobalVariable::class,
+                        $this->contextBaseClassOrInterface
+                    ),
+                    9,
+                ],
+                [
+                    \sprintf('Class %s implements %s and uses $argc: accessing globals in this context is considered an anti-pattern',
+                        TestAsset\AccessGlobalVariableWithinContextRule\ModelAccessingGlobalVariable::class,
+                        $this->contextBaseClassOrInterface
+                    ),
+                    10,
+                ],
+            ]
+        );
+    }
+}
diff --git a/tests/AccessStaticPropertyWithinModelContextRuleTest.php b/tests/AccessStaticPropertyWithinModelContextRuleTest.php
@@ -25,8 +25,8 @@ final class AccessStaticPropertyWithinModelContextRuleTest extends RuleTestCase
 
     public function __construct($name = null, array $data = [], $dataName = '')
     {
-        $this->modelBaseClassOrInterface = TestAsset\YiiAlikeActiveRecordInterface::class;
-        $this->singletonAccessor         = TestAsset\YiiAlikeBaseYii::class;
+        $this->modelBaseClassOrInterface = TestAsset\AccessStaticPropertyWithinModelContextRule\YiiAlikeActiveRecordInterface::class;
+        $this->singletonAccessor         = TestAsset\AccessStaticPropertyWithinModelContextRule\YiiAlikeBaseYii::class;
 
         parent::__construct($name, $data, $dataName);
     }
@@ -46,20 +46,20 @@ public function testRule()
     {
         $this->analyse(
             [
-                __DIR__ . '/TestAsset/AccessStaticPropertyWithinModelContextRule.php',
+                __DIR__ . '/TestAsset/AccessStaticPropertyWithinModelContextRule/fixture.php',
             ],
             [
                 [
                     \sprintf('Class %s implements %s and uses %s::$app: accessing a singleton in this context is considered an anti-pattern',
-                        TestAsset\ModelAccessingYiiAppSingletons::class,
+                        TestAsset\AccessStaticPropertyWithinModelContextRule\ModelAccessingYiiAppSingletons::class,
                         $this->modelBaseClassOrInterface,
                         $this->singletonAccessor
                     ),
                     8,
                 ],
                 [
                     \sprintf('Class %s implements %s and uses %s::$app: accessing a singleton in this context is considered an anti-pattern',
-                        TestAsset\ModelAccessingYiiAppSingletons::class,
+                        TestAsset\AccessStaticPropertyWithinModelContextRule\ModelAccessingYiiAppSingletons::class,
                         $this->modelBaseClassOrInterface,
                         $this->singletonAccessor
                     ),

diff --git a/tests/ClassNotationRuleTest.php b/tests/ClassNotationRuleTest.php
@@ -24,27 +24,27 @@ public function testClassConstant()
     {
         $this->analyse(
             [
-                __DIR__ . '/TestAsset/ClassNotation.php',
+                __DIR__ . '/TestAsset/ClassNotation/fixture.php',
             ],
             [
                 [
-                    'Interface SlamPhpStan\Tests\TestAsset\A should end with "Interface" suffix.',
+                    \sprintf('Interface %s should end with "Interface" suffix.', TestAsset\ClassNotation\A::class),
                     5,
                 ],
                 [
-                    'Abstract class SlamPhpStan\Tests\TestAsset\B should start with "Abstract" prefix.',
+                    \sprintf('Abstract class %s should start with "Abstract" prefix.', TestAsset\ClassNotation\B::class),
                     9,
                 ],
                 [
-                    'Abstract class SlamPhpStan\Tests\TestAsset\Abstract_B should start with "Abstract" prefix.',
+                    \sprintf('Abstract class %s should start with "Abstract" prefix.', TestAsset\ClassNotation\Abstract_B::class),
                     13,
                 ],
                 [
-                    'Trait SlamPhpStan\Tests\TestAsset\C should end with "Trait" suffix.',
+                    \sprintf('Trait %s should end with "Trait" suffix.', TestAsset\ClassNotation\C::class),
                     17,
                 ],
                 [
-                    'Exception class SlamPhpStan\Tests\TestAsset\E should end with "Exception" suffix.',
+                    \sprintf('Exception class %s should end with "Exception" suffix.', TestAsset\ClassNotation\E::class),
                     20,
                 ],
             ]

diff --git a/tests/GotoRuleTest.php b/tests/GotoRuleTest.php
@@ -22,7 +22,7 @@ public function testClassConstant()
     {
         $this->analyse(
             [
-                __DIR__ . '/TestAsset/Goto.php',
+                __DIR__ . '/TestAsset/GotoRule/fixture.php',
             ],
             [
                 [

diff --git a/tests/PhpUnitFqcnAnnotationRuleTest.php b/tests/PhpUnitFqcnAnnotationRuleTest.php
@@ -24,7 +24,7 @@ public function testClassConstant()
     {
         $this->analyse(
             [
-                __DIR__ . '/TestAsset/PhpUnitFqcnAnnotation.php',
+                __DIR__ . '/TestAsset/PhpUnitFqcnAnnotation/fixture.php',
             ],
             [
                 [

diff --git a/tests/StringToClassRuleTest.php b/tests/StringToClassRuleTest.php
@@ -24,15 +24,15 @@ public function testClassConstant()
     {
         $this->analyse(
             [
-                __DIR__ . '/TestAsset/FooStringToClass.php',
+                __DIR__ . '/TestAsset/StringToClassRule/fixture.php',
             ],
             [
                 [
-                    'Class SlamPhpStan\Tests\TestAsset\FooStringToClass should be written with ::class notation, string found.',
+                    \sprintf('Class %s should be written with ::class notation, string found.', TestAsset\StringToClassRule\FooStringToClass::class),
                     12,
                 ],
                 [
-                    'Class SlamPhpStan\Tests\TestAsset\FooStringToClass should be written with ::class notation, string found.',
+                    \sprintf('Class %s should be written with ::class notation, string found.', TestAsset\StringToClassRule\FooStringToClass::class),
                     13,
                 ],
                 [
@@ -44,11 +44,11 @@ public function testClassConstant()
                     15,
                 ],
                 [
-                    'Class SlamPhpStan\Tests\TestAsset\FooStringToClass should be written with ::class notation, string found.',
+                    \sprintf('Class %s should be written with ::class notation, string found.', TestAsset\StringToClassRule\FooStringToClass::class),
                     17,
                 ],
                 [
-                    'Class SlamPhpStan\Tests\TestAsset\FooStringToClass should be written with ::class notation, string found.',
+                    \sprintf('Class %s should be written with ::class notation, string found.', TestAsset\StringToClassRule\FooStringToClass::class),
                     18,
                 ],
                 [

diff --git a/tests/TestAsset/AccessGlobalVariableWithinContextRule/fixture.php b/tests/TestAsset/AccessGlobalVariableWithinContextRule/fixture.php
@@ -0,0 +1,67 @@
+<?php
+
+namespace SlamPhpStan\Tests\TestAsset\AccessGlobalVariableWithinContextRule;
+
+class ModelAccessingGlobalVariable implements YiiAlikeActiveRecordInterface {
+    public function foo() {
+        // $bar0 = $_POST;
+        $bar1 = $_POST;
+        $bar2 = $GLOBALS;
+        $argc = [];
+
+        $baz1 = $POST ?? null;
+        $baz2 = $argcount ?? null;
+
+        $var = 'foo';
+        $$var = 1;
+        ${$var} = 2;
+    }
+}
+
+class AnyOtherClass {
+    public function foo() {
+        // $bar0 = $_POST;
+        $bar1 = $_POST;
+        $bar2 = $GLOBALS;
+        $argc = [];
+
+        $baz1 = $POST ?? null;
+        $baz2 = $argcount ?? null;
+
+        $var = 'foo';
+        $$var = 1;
+        ${$var} = 2;
+    }
+}
+
+trait YiiAppSingletonCallRuleTrait {
+    public function foo() {
+        // $bar0 = $_POST;
+        $bar1 = $_POST;
+        $bar2 = $GLOBALS;
+        $argc = [];
+
+        $baz1 = $POST ?? null;
+        $baz2 = $argcount ?? null;
+
+        $var = 'foo';
+        $$var = 1;
+        ${$var} = 2;
+    }
+}
+
+function YiiAppSingletonCallRuleFunction() {
+    // $bar0 = $_POST;
+    $bar1 = $_POST;
+    $bar2 = $GLOBALS;
+    $argc = [];
+
+    $baz1 = $POST ?? null;
+    $baz2 = $argcount ?? null;
+
+    $var = 'foo';
+    $$var = 1;
+    ${$var} = 2;
+}
+
+interface YiiAlikeActiveRecordInterface {}
diff --git a/...sStaticPropertyWithinModelContextRule.php → ...ropertyWithinModelContextRule/fixture.php b/...sStaticPropertyWithinModelContextRule.php → ...ropertyWithinModelContextRule/fixture.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace SlamPhpStan\Tests\TestAsset;
+namespace SlamPhpStan\Tests\TestAsset\AccessStaticPropertyWithinModelContextRule;
 
 class ModelAccessingYiiAppSingletons implements YiiAlikeActiveRecordInterface {
     public function foo() {

diff --git a/tests/TestAsset/ClassNotation.php → tests/TestAsset/ClassNotation/fixture.php b/tests/TestAsset/ClassNotation.php → tests/TestAsset/ClassNotation/fixture.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace SlamPhpStan\Tests\TestAsset;
+namespace SlamPhpStan\Tests\TestAsset\ClassNotation;
 
 interface A {}
 interface AInterface {}