Improvement/12/ci code rules superglobals

.github/workflows/code-rules.yml

@@ -0,0 +1,62 @@
+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: scripts/PHPStan/.cache/code-rules
+          key: phpstan-code-rules-${{ github.run_id }}
+          restore-keys: phpstan-code-rules-
+
+      - name: ILIAS Code Rules
+        # Hard gate: the branch that introduced these rules also brought the count to
+        # zero, so any new violation has to be fixed or exempted in the pull request
+        # that adds it.
+        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
+/scripts/PHPStan/.cache
 /components/ILIAS/PHPUnit/config/cfg.phpunit.php
 /nbproject
 /templates/default/delos.css.map

components/ILIAS/Calendar/classes/class.ilCalendarRemoteAccessHandler.php

@@ -20,6 +20,7 @@
 
 use ILIAS\Refinery\Factory as Refinery;
 use ILIAS\HTTP\Services as HTTPServices;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * @classDescription Handles requests from external calendar applications
@@ -49,6 +50,7 @@ public function getTokenHandler(): ?ilCalendarAuthenticationToken
     /**
      * Fetch client id, the chosen calendar...
      */
+    #[AllowSuperglobalWrite('resolves the client id for the remote calendar entry point before ilInitialisation runs, so no HTTP service exists yet.', 12)]
     public function parseRequest(): void
     {
         // before initialization: $_GET and $_COOKIE is required is unavoidable

components/ILIAS/CmiXapi/classes/XapiProxy/DataService.php

@@ -20,8 +20,11 @@
 
 namespace XapiProxy;
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 class DataService
 {
+    #[AllowSuperglobalWrite('the client id has to be visible to ilInitialisation, which runs before the HTTP service exists.', 12)]
     public static function initIlias(string $client_id): void
     {
         define("CLIENT_ID", $client_id);

components/ILIAS/CmiXapi/resources/xapitoken.php

@@ -49,8 +49,10 @@
         ilCmiXapiAuthToken::OPENSSL_IV
     ), true);
 
+    // @phpstan-ignore ilias.superglobalWrite.v12 (restores session and client id from the signed token before ilInitialisation runs, so no HTTP service exists yet)
     $_COOKIE[session_name()] = $param[session_name()];
 
+    // @phpstan-ignore ilias.superglobalWrite.v12 (restores session and client id from the signed token before ilInitialisation runs, so no HTTP service exists yet)
     $_COOKIE['ilClientId'] = $param['ilClientId'];
     $objId = $param['obj_id'];
     $refId = $param['ref_id'];

components/ILIAS/DataCollection/classes/Content/class.ilDclRecordEditGUI.php

@@ -18,6 +18,8 @@
 
 declare(strict_types=1);
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 class ilDclRecordEditGUI
 {
     /**
@@ -486,6 +488,10 @@ public function saveConfirmation(ilDclBaseRecordModel $record_obj, string $fileh
     /**
      * Save record
      */
+    #[AllowSuperglobalWrite(
+        'adds a $_FILES entry for every file input the form expects, because the legacy file inputs read their state from there. The placeholders come from the request, so the shape of the added entries is client-controlled; narrowing this to the field names the form knows is open work.',
+        12
+    )]
     public function save(): void
     {
         global $DIC;

components/ILIAS/DataCollection/classes/Helpers/class.ilDclPropertyFormGUI.php

@@ -18,6 +18,8 @@
 
 declare(strict_types=1);
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 /**
  * Class ilDclPropertyFormGUI
  * @ilCtrl_Calls ilDclPropertyFormGUI: ilFormPropertyDispatchGUI
@@ -71,6 +73,7 @@ public static function getTempFilename(
     /**
      * @throws ilDclException
      */
+    #[AllowSuperglobalWrite('re-injects files uploaded in an earlier step of the same form, because the file inputs read them from $_FILES.', 12)]
     public static function rebuildTempFileByHash(string $hash): void
     {
         $temp_path = ilFileUtils::getDataDir() . "/temp";

components/ILIAS/Form/classes/class.ilFileInputGUI.php

@@ -21,6 +21,7 @@
 use ILIAS\FileUpload\Exception\IllegalStateException;
 use ILIAS\FileUpload\FileUpload;
 use ILIAS\UI\Implementation\Component\Input\UploadLimitResolver;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * This class represents a file property in a property form.
@@ -161,6 +162,7 @@ public function getALlowDeletion(): bool
         return $this->allow_deletion;
     }
 
+    #[AllowSuperglobalWrite('sanitises the uploaded file name in place, because getInput() hands $_FILES[postVar] on to every caller.', 12)]
     public function checkInput(): bool
     {
         if (!$this->upload_service->hasBeenProcessed()) {

components/ILIAS/Form/classes/class.ilFileWizardInputGUI.php

@@ -19,6 +19,7 @@
 declare(strict_types=1);
 
 use ILIAS\Filesystem\Util;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * This class represents a file wizard property in a property form.
@@ -79,6 +80,7 @@ public function getAllowMove(): bool
         return $this->allowMove;
     }
 
+    #[AllowSuperglobalWrite('sanitises the uploaded file names in place, because the inherited getInput() hands $_FILES[postVar] on to every caller.', 12)]
     public function checkInput(): bool
     {
         $lng = $this->lng;

components/ILIAS/Form/classes/class.ilPropertyFormGUI.php

@@ -20,6 +20,7 @@
 
 use ILIAS\HTTP;
 use ILIAS\Refinery;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * This class represents a property form user interface
@@ -1012,6 +1013,7 @@ public function moveFileUpload(
         return "";
     }
 
+    #[AllowSuperglobalWrite('re-injects files uploaded in an earlier step of the same form, because the file inputs read them from $_FILES.', 12)]
     protected function rebuildUploadedFiles(): void
     {
         $file_hash = (string) $this->getFileHash();

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

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

components/ILIAS/Init/resources/sso/index.php

@@ -41,6 +41,7 @@
     }
 
     setcookie('ilClientId', $_GET['client_id'], 0, $cookie_path, '');
+    // @phpstan-ignore ilias.superglobalWrite.v12 (the client id has to be visible to ilInitialisation, which runs before the HTTP service exists)
     $_COOKIE['ilClientId'] = $_GET['client_id'];
 }
 

components/ILIAS/Init/src/ErrorHandling/Infrastructure/Whoops/DelegatingHandler.php

@@ -23,6 +23,7 @@
 use ILIAS\Init\ErrorHandling\Application\ContextErrorHandlerProvider;
 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
@@ -34,6 +35,7 @@
  * This class is not ment to be extended, as the definition of error handlers should be handled in one place
  * in ilErrorHandling, so this class acts rather dump and asks ilErrorHandling for a handler.
  */
+#[AllowSuperglobalWrite('The error handler needs to write to SuperGlobals to remove secret data.', 12)]
 final class DelegatingHandler extends Handler
 {
     private ?HandlerInterface $current_handler = null;

components/ILIAS/LTIProvider/classes/InternalProvider/class.ilLTITool.php

@@ -23,6 +23,7 @@
 use ceLTIc\LTI\Tool;
 use ceLTIc\LTI\User;
 use ILIAS\HTTP\Wrapper\ArrayBasedRequestWrapper;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
  * LTI provider for LTI launch
@@ -87,6 +88,7 @@ public function parsePostBody(ArrayBasedRequestWrapper $postData): array
         return $res;
     }
 
+    #[AllowSuperglobalWrite('the celtic/lti library reads the request from the superglobals, so the parsed PSR-7 request has to be written back into them.', 12)]
     public function handleRequest(?bool $strictMode = null, bool $disableCookieCheck = false, bool $generateWarnings = false): void
     {
         global $DIC;

components/ILIAS/LTIProvider/resources/lti.php

@@ -19,7 +19,9 @@
 declare(strict_types=1);
 
 
+// @phpstan-ignore ilias.superglobalWrite.v12 (pins the command for the LTI entry point before ilInitialisation runs, so no HTTP service exists yet)
 $_GET['cmd'] = 'post';
+// @phpstan-ignore ilias.superglobalWrite.v12 (pins the command for the LTI entry point before ilInitialisation runs, so no HTTP service exists yet)
 $_POST['cmd'] = 'doLTIAuthentication';
 
 require_once '../vendor/composer/vendor/autoload.php';

components/ILIAS/Test/classes/class.ilObjTestGUI.php

@@ -2576,9 +2576,8 @@ public function createQuestionPoolAndCopyObject()
         }
 
         $question_pool = $this->createQuestionPool($title, $this->testrequest->raw('description'));
-        $_REQUEST['sel_qpl'] = $question_pool->getRefId();
 
-        $this->copyAndLinkQuestionsToPoolObject();
+        $this->copyAndLinkQuestionsToPoolObject($question_pool->getRefId());
     }
 
     /**

components/ILIAS/TestQuestionPool/classes/class.ilKprimChoiceWizardInputGUI.php

@@ -16,6 +16,8 @@
  *
  *********************************************************************/
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 /**
  * @author		Björn Heyser <bheyser@databay.de>
  * @version		$Id$
@@ -307,6 +309,7 @@ public function insert(ilTemplate $a_tpl): void
         $this->tpl->addJavascript('assets/js/ilAssKprimChoice.js');
     }
 
+    #[AllowSuperglobalWrite('flags a rejected image by writing a custom upload error into $_FILES, which the inherited getInput() then reports to the caller.', 12)]
     public function checkUploads($foundvalues): bool
     {
         if (is_array($_FILES) && count($_FILES) && $this->getSingleline()) {

components/ILIAS/TestQuestionPool/classes/forms/class.ilIdentifiedMultiFilesJsPositionIndexRemover.php

@@ -16,6 +16,8 @@
  *
  *********************************************************************/
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 /**
  * @author        Björn Heyser <bheyser@databay.de>
  */
@@ -65,6 +67,7 @@ protected function isFileSubmitAvailable(): bool
         return true;
     }
 
+    #[AllowSuperglobalWrite('normalises the shape of $_FILES[postVar], because the inherited getInput() reads it back from there.', 12)]
     protected function prepareFileSubmit(): void
     {
         $_FILES[$this->getPostVar()] = $this->prepareMultiFilesSubmitValues(

components/ILIAS/TestQuestionPool/classes/forms/class.ilImagemapFileInputGUI.php

@@ -21,6 +21,7 @@
 use ILIAS\UI\Renderer;
 use ILIAS\UI\Component\Button\Factory as ButtonFactory;
 use ILIAS\UI\Component\Symbol\Glyph\Factory as GlyphFactory;
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
 
 /**
 * This class represents an image map file property in a property form.
@@ -177,6 +178,7 @@ private function getPostBody(): array
     * Check input, strip slashes etc. set alert, if input is not ok.
     * @return	boolean		Input ok, true/false
     */
+    #[AllowSuperglobalWrite('sanitises the uploaded file name in place, because the inherited getInput() hands $_FILES[postVar] on to every caller.', 12)]
     public function checkInput(): bool
     {
         $lng = $this->lng;

components/ILIAS/TestQuestionPool/classes/forms/class.ilMultiFilesSubmitRecursiveSlashesStripper.php

@@ -16,6 +16,8 @@
  *
  *********************************************************************/
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 /**
  * @author        Björn Heyser <bheyser@databay.de>
  */
@@ -64,6 +66,7 @@ public function manipulateFormSubmitValues(array $submitValues): array
     /**
      * perform the strip slashing on files submit
      */
+    #[AllowSuperglobalWrite('strips slashes in $_FILES[postVar], because the inherited getInput() reads it back from there.', 12)]
     protected function manipulateFileSubmitValues(): void
     {
         if ($_FILES) {

components/ILIAS/TestQuestionPool/classes/forms/class.ilMultipleImagesAdditionalIndexLevelRemover.php

@@ -16,6 +16,8 @@
  *
  *********************************************************************/
 
+use ILIAS\Scripts\PHPStan\Attributes\AllowSuperglobalWrite;
+
 /**
  * @author        Björn Heyser <bheyser@databay.de>
  */
@@ -46,6 +48,7 @@ public function manipulateFormInputValues(array $inputValues): array
         return $inputValues;
     }
 
+    #[AllowSuperglobalWrite('normalises the shape of $_FILES[postVar], because the inherited getInput() reads it back from there.', 12)]
     public function manipulateFormSubmitValues(array $submitValues): array
     {
         $submitValues = $this->removeAdditionalSubFieldsLevelFromSubmitValues($submitValues);

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.', 12)]
     public function setUp(): void
     {
         //This avoids various index not set warnings, which are only relevant in test context.