WIP: Improvement/12/ci code rules superglobals

.github/workflows/code-rules.yml

@@ -0,0 +1,63 @@
+name: code-rules
+on: [pull_request, push]
+permissions:
+  contents: read
+  pull-requests: read
+jobs:
+  code-rules:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 1
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '8.4'
+          extensions: dom, curl, libxml, mbstring, zip, gd, json, readline, xsl, imagick
+          tools: composer:v2
+          coverage: none
+
+      - name: Cache Composer dependencies
+        uses: actions/cache@v4
+        with:
+          path: vendor/composer/vendor
+          key: composer-${{ hashFiles('composer.lock') }}
+          restore-keys: composer-
+
+      - name: Prepare Customizing directory
+        # The composer classmap includes ./public/Customizing/global/plugins; --no-scripts
+        # skips the pre-install-cmd that would create it, so create it here — otherwise
+        # autoload generation aborts ("Could not scan for classes inside ...").
+        run: mkdir -p public/Customizing/global/plugins
+
+      - name: Install Composer packages
+        # --no-scripts skips the post-autoload-dump build (cli/setup.php build), which
+        # copies front-end assets from node_modules. PHPStan only needs the autoloader
+        # and classmap (still generated), so neither the build nor Node.js is required.
+        run: composer install --no-interaction --no-progress --no-scripts
+
+      - name: Cache PHPStan result cache
+        uses: actions/cache@v4
+        with:
+          path: tmp/phpstan-code-rules
+          key: phpstan-code-rules-${{ github.run_id }}
+          restore-keys: phpstan-code-rules-
+
+      - name: ILIAS Code Rules
+        # Non-blocking for now: the gate reports violations (annotations + report) but
+        # does not fail the job while the existing findings are being fixed / exempted.
+        # Remove continue-on-error to make it a hard gate once the count reaches zero.
+        continue-on-error: true
+        run: scripts/PHPStan/run_code_rules.sh
+        env:
+          GHRUN: "yes"
+          ERROR_FORMAT: github
+
+      - name: Code Rules Summary
+        if: always()
+        run: ERROR_FORMAT=stepSummary scripts/PHPStan/run_code_rules.sh >> "$GITHUB_STEP_SUMMARY" || true
+        env:
+          GHRUN: "yes"

.gitignore

@@ -9,6 +9,7 @@
 /.settings
 /errors
 /extern
+/tmp
 /components/ILIAS/PHPUnit/config/cfg.phpunit.php
 /nbproject
 /templates/default/delos.css.map

components/ILIAS/Init/classes/ErrorHandling/class.ilDelegatingHandler.php

@@ -20,6 +20,7 @@
 
 use Whoops\Handler\Handler;
 use Whoops\Handler\HandlerInterface;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * A Whoops error handler that delegates calls on it self to another handler that is created only in the
@@ -32,6 +33,7 @@
  * in ilErrorHandling, so this class acts rather dump and asks ilErrorHandling for a handler.
  * @author Richard Klees <richard.klees@concepts-and-training.de>
  */
+#[AllowSuperglobalWrite('The error handler needs to write to SuperGlobals to remove secret data.')]
 final class ilDelegatingHandler extends Handler
 {
     private ?HandlerInterface $current_handler = null;

components/ILIAS/Init/classes/class.ilInitialisation.php

@@ -36,6 +36,7 @@
 use ILIAS\ILIASObject\Properties\AdditionalProperties\Icon\Factory as CustomIconFactory;
 use ILIAS\User\PublicInterface as UserPublicInterface;
 use ILIAS\Mail\Service\MailService;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 // needed for slow queries, etc.
 if (!isset($GLOBALS['ilGlobalStartTime']) || !$GLOBALS['ilGlobalStartTime']) {
@@ -51,6 +52,7 @@
  * @version $Id$
  * @ingroup ServicesInit
  */
+#[AllowSuperglobalWrite('Remove unsafe Characters, several other legacy mechanisms...')]
 class ilInitialisation
 {
     /**

components/ILIAS/UI/tests/Examples/ExamplesTest.php

@@ -25,6 +25,7 @@
 use ILIAS\DI\Container;
 use ILIAS\UI\NotImplementedException;
 use ILIAS\FileUpload\FileUpload;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * Class ExamplesTest Checks if all examples are implemented and properly returning strings
@@ -47,6 +48,7 @@ class ExamplesTest extends ILIAS_UI_TestBase
     protected Container $dic;
     protected Crawler\ExamplesYamlParser $example_parser;
 
+    #[AllowSuperglobalWrite('Bypass Undefined index: ilfilehash for the moment. This is for examples only.')]
     public function setUp(): void
     {
         //This avoids various index not set warnings, which are only relevant in test context.

components/ILIAS/WebDAV/src/Request/RequestTranslation.php

@@ -23,6 +23,7 @@
 use Psr\Http\Message\ServerRequestInterface;
 use ILIAS\WebDAV\Config;
 use ILIAS\HTTP\Wrapper\SuperGlobalDropInReplacement;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * @internal
@@ -92,6 +93,11 @@ public function showMountPoint(): bool
         return array_key_exists($this->config->getMountInstructionsQuery(), $this->request->getQueryParams());
     }
 
+    #[AllowSuperglobalWrite(
+        'as long as we use '
+        . \ILIAS\HTTP\Wrapper\SuperGlobalDropInReplacement::class
+        . ', we must cast back to array to make SabreDAV work.'
+    )]
     public function setup(): void
     {
         $this->post = $_POST;

scripts/PHPStan/Attributes/AllowRuleViolation.php

@@ -0,0 +1,63 @@
+<?php
+
+/**
+ * This file is part of ILIAS, a powerful learning management system
+ * published by ILIAS open source e-Learning e.V.
+ *
+ * ILIAS is licensed with the GPL-3.0,
+ * see https://www.gnu.org/licenses/gpl-3.0.en.html
+ * You should have received a copy of said license along with the
+ * source code, too.
+ *
+ * If this is not the case or you just want to try ILIAS, you'll find
+ * us at:
+ * https://www.ilias.de
+ * https://github.com/ILIAS-eLearning
+ *
+ *********************************************************************/
+
+declare(strict_types=1);
+
+namespace ILIAS\Scripts\PHPStan\Attributes;
+
+use Attribute;
+
+/**
+ * Marks a class, method or function that is deliberately allowed to violate one or
+ * more ILIAS custom code rules, exempting it from those rules.
+ *
+ * This is a zero-cost marker: it is never instantiated at runtime, only read via
+ * reflection by the rules ({@see RuleViolationAllowance}). Use it only for genuinely
+ * unavoidable cases and always give a reason.
+ *
+ * The rules are identified by their PHPStan error identifier (the value shown as
+ * `🪪 <identifier>` in the analysis output, e.g. `ilias.superglobalWrite`). Pass one
+ * or more; an allowance with no identifiers exempts nothing.
+ *
+ * PHP attributes can only sit on declarations, so this exempts the whole
+ * class/method/function. For a single free-standing statement (e.g. in a resource
+ * script without an enclosing function) use an inline
+ * `// @phpstan-ignore <identifier> (reason)` comment instead.
+ *
+ * For frequently exempted rules there are convenience subclasses that already carry
+ * the identifier, e.g. {@see AllowSuperglobalWrite}. The checker matches those via
+ * `ReflectionAttribute::IS_INSTANCEOF`, so subclasses work exactly like the base
+ * attribute.
+ *
+ * Example:
+ * ```php
+ * #[AllowRuleViolation('sanitizes $_GET before the HTTP service exists', 'ilias.superglobalWrite')]
+ * public static function recursivelyRemoveUnsafeCharacters(): void { ... }
+ * ```
+ */
+#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
+readonly class AllowRuleViolation
+{
+    /** @var list<string> */
+    public array $rules;
+
+    public function __construct(public string $reason, string ...$rules)
+    {
+        $this->rules = array_values($rules);
+    }
+}

scripts/PHPStan/Attributes/AllowSuperglobalWrite.php

@@ -0,0 +1,42 @@
+<?php
+
+/**
+ * This file is part of ILIAS, a powerful learning management system
+ * published by ILIAS open source e-Learning e.V.
+ *
+ * ILIAS is licensed with the GPL-3.0,
+ * see https://www.gnu.org/licenses/gpl-3.0.en.html
+ * You should have received a copy of said license along with the
+ * source code, too.
+ *
+ * If this is not the case or you just want to try ILIAS, you'll find
+ * us at:
+ * https://www.ilias.de
+ * https://github.com/ILIAS-eLearning
+ *
+ *********************************************************************/
+
+declare(strict_types=1);
+
+namespace ILIAS\Scripts\PHPStan\Attributes;
+
+use Attribute;
+use ILIAS\Scripts\PHPStan\Rules\SuperGlobals\AbstractSuperglobalWriteRule;
+
+/**
+ * Convenience variant of {@see AllowRuleViolation} that already carries the
+ * "No Superglobal Write" rule identifier, so only a reason has to be given:
+ *
+ * ```php
+ * #[AllowSuperglobalWrite('sanitizes $_GET before the HTTP service exists')]
+ * public static function recursivelyRemoveUnsafeCharacters(): void { ... }
+ * ```
+ */
+#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
+final readonly class AllowSuperglobalWrite extends AllowRuleViolation
+{
+    public function __construct(string $reason)
+    {
+        parent::__construct($reason, AbstractSuperglobalWriteRule::IDENTIFIER);
+    }
+}

scripts/PHPStan/Attributes/RuleViolationAllowance.php

@@ -0,0 +1,82 @@
+<?php
+
+/**
+ * This file is part of ILIAS, a powerful learning management system
+ * published by ILIAS open source e-Learning e.V.
+ *
+ * ILIAS is licensed with the GPL-3.0,
+ * see https://www.gnu.org/licenses/gpl-3.0.en.html
+ * You should have received a copy of said license along with the
+ * source code, too.
+ *
+ * If this is not the case or you just want to try ILIAS, you'll find
+ * us at:
+ * https://www.ilias.de
+ * https://github.com/ILIAS-eLearning
+ *
+ *********************************************************************/
+
+declare(strict_types=1);
+
+namespace ILIAS\Scripts\PHPStan\Attributes;
+
+use PHPStan\Analyser\Scope;
+use PHPStan\Reflection\Php\PhpFunctionReflection;
+
+/**
+ * Shared helper for ILIAS custom PHPStan rules: decides whether the code position
+ * described by a {@see Scope} is exempt from a rule via an {@see AllowRuleViolation}
+ * attribute (or any subclass of it) on the enclosing class, method or function.
+ *
+ * Rules call {@see self::isAllowedIn()} with their own error identifier before
+ * emitting an error.
+ */
+final class RuleViolationAllowance
+{
+    public static function isAllowedIn(Scope $scope, string $rule_identifier): bool
+    {
+        try {
+            foreach (self::reflections($scope) as $reflection) {
+                // IS_INSTANCEOF so convenience subclasses (e.g. AllowSuperglobalWrite) match too.
+                foreach ($reflection->getAttributes(
+                    AllowRuleViolation::class,
+                    \ReflectionAttribute::IS_INSTANCEOF
+                ) as $attribute) {
+                    $allowance = $attribute->newInstance();
+                    if (in_array($rule_identifier, $allowance->rules, true)) {
+                        return true;
+                    }
+                }
+            }
+        } catch (\Throwable) {
+            // Reflection can fail for not-yet-loadable symbols; never break analysis over it.
+            return false;
+        }
+
+        return false;
+    }
+
+    /**
+     * @return iterable<\ReflectionClass<object>|\ReflectionMethod|\ReflectionFunction>
+     */
+    private static function reflections(Scope $scope): iterable
+    {
+        $class_reflection = $scope->getClassReflection();
+        if ($class_reflection !== null) {
+            $native_class = $class_reflection->getNativeReflection();
+            yield $native_class;
+
+            $function_name = $scope->getFunctionName();
+            if ($function_name !== null && $native_class->hasMethod($function_name)) {
+                yield $native_class->getMethod($function_name);
+            }
+
+            return;
+        }
+
+        $function = $scope->getFunction();
+        if ($function instanceof PhpFunctionReflection) {
+            yield $function->getNativeReflection();
+        }
+    }
+}