From 9095770ee02c6adfdf9b66b4a173179a086aebeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Tue, 4 Feb 2025 08:09:49 +0100 Subject: [PATCH 1/6] Add PHPStan rules to forbid some PHP functions --- composer.json | 1 + composer.lock | 2 +- phpstan.neon.dist | 2 + tools/src/PHPStan/ForbidExitRule.php | 69 ++++++++++++++++++ .../PHPStan/ForbidHttpResponseCodeRule.php | 70 +++++++++++++++++++ 5 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 tools/src/PHPStan/ForbidExitRule.php create mode 100644 tools/src/PHPStan/ForbidHttpResponseCodeRule.php diff --git a/composer.json b/composer.json index 69e0f06fbbe..4857ffafadf 100644 --- a/composer.json +++ b/composer.json @@ -108,6 +108,7 @@ "friendsoftwig/twigcs": "^6.4", "glpi-project/tools": "^0.7", "mikey179/vfsstream": "^1.6", + "nikic/php-parser": "^5.4", "php-parallel-lint/php-parallel-lint": "^1.4", "phpstan/extension-installer": "^1.4", "phpstan/phpstan": "^2.1", diff --git a/composer.lock b/composer.lock index 498a7179262..13be0749b9d 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "a37d68bd366ad1ec5ff38377678492b3", + "content-hash": "5f20fe84913c193f5cb2f435b4ba3011", "packages": [ { "name": "apereo/phpcas", diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 39668387eda..4d8c366c557 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -68,6 +68,8 @@ parameters: message: '~/(downstream\.php|config_db\.php|config_db_slave\.php)" is not a file or it does not exist.~' reportUnmatched: false rules: + - Glpi\Tools\PHPStan\ForbidExitRule + - Glpi\Tools\PHPStan\ForbidHttpResponseCodeRule - GlpiProject\Tools\PHPStan\Rules\GlobalVarTypeRule # Copy and uncomment this content into a "phpstan.neon" file to add your own config values diff --git a/tools/src/PHPStan/ForbidExitRule.php b/tools/src/PHPStan/ForbidExitRule.php new file mode 100644 index 00000000000..ebe89fc9cf1 --- /dev/null +++ b/tools/src/PHPStan/ForbidExitRule.php @@ -0,0 +1,69 @@ +. + * + * --------------------------------------------------------------------- + */ + +namespace Glpi\Tools\PHPStan; + +use PhpParser\Node; +use PhpParser\Node\Expr\Exit_; +use PHPStan\Analyser\Scope; +use PHPStan\Rules\Rule; +use PHPStan\Rules\RuleErrorBuilder; + +class ForbidExitRule implements Rule +{ + public function getNodeType(): string + { + return Exit_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $name = match ($node->getAttribute('kind')) { + Exit_::KIND_DIE => 'die', + Exit_::KIND_EXIT => 'exit', + default => 'die/exit', + }; + + return [ + RuleErrorBuilder::message( + sprintf( + 'You should not use the `%s` function. It prevents the execution of post-request/post-command routines.', + $name + ) + ) + ->identifier('glpi.forbidExit') + ->build() + ]; + } +} diff --git a/tools/src/PHPStan/ForbidHttpResponseCodeRule.php b/tools/src/PHPStan/ForbidHttpResponseCodeRule.php new file mode 100644 index 00000000000..418b6850c19 --- /dev/null +++ b/tools/src/PHPStan/ForbidHttpResponseCodeRule.php @@ -0,0 +1,70 @@ +. + * + * --------------------------------------------------------------------- + */ + +namespace Glpi\Tools\PHPStan; + +use PhpParser\Node; +use PhpParser\Node\Expr\FuncCall; +use PHPStan\Analyser\Scope; +use PhpParser\Node\Name; +use PHPStan\Rules\Rule; +use PHPStan\Rules\RuleErrorBuilder; + +class ForbidHttpResponseCodeRule implements Rule +{ + public function getNodeType(): string + { + return FuncCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if ( + $node instanceof FuncCall + && $node->name instanceof Name + && $node->name->toString() === 'http_response_code' + && count($node->getRawArgs()) > 0 // `http_response_code()` used without args is a setter and does not cause issues + ) { + return [ + RuleErrorBuilder::message( + 'You should not use the `http_response_code` function to change the response code. Due to a PHP bug, it may not provide the expected result (see https://bugs.php.net/bug.php?id=81451).', + ) + ->identifier('glpi.forbidHttpResponseCode') + ->build() + ]; + } + + return []; + } +} From 5867a9df18d0c1bad3e4cbecf4a8e26ac8a7ae46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 28 Apr 2025 15:55:42 +0200 Subject: [PATCH 2/6] Ignore legitimate `exit()` usages in pre-boot sequence --- front/_check_webserver_config.php | 5 ++--- src/Glpi/Application/ResourcesChecker.php | 4 ++-- src/Glpi/Kernel/Kernel.php | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/front/_check_webserver_config.php b/front/_check_webserver_config.php index 759ae0176ff..84ccbe97fb6 100644 --- a/front/_check_webserver_config.php +++ b/front/_check_webserver_config.php @@ -8,7 +8,6 @@ * http://glpi-project.org * * @copyright 2015-2025 Teclib' and contributors. - * @copyright 2003-2014 by the INDEPNET Development Team. * @licence https://www.gnu.org/licenses/gpl-3.0.html * * --------------------------------------------------------------------- @@ -36,7 +35,7 @@ if (!class_exists('Glpi\\Kernel\\Kernel', autoload: false)) { // `Glpi\Kernel\Kernel` class will exists if the request was processed by the `/public/index.php` file, // and will not be found otherwise. - http_response_code(404); + header('HTTP/1.1 404 Not Found'); readfile(__DIR__ . '/../index.html'); - exit(); + exit(); // @phpstan-ignore glpi.forbidExit (Script execution should be stopped to prevent further errors) } diff --git a/src/Glpi/Application/ResourcesChecker.php b/src/Glpi/Application/ResourcesChecker.php index 9575ba8c2b8..9db7c485537 100644 --- a/src/Glpi/Application/ResourcesChecker.php +++ b/src/Glpi/Application/ResourcesChecker.php @@ -53,12 +53,12 @@ public function checkResources(): void if (!$this->areDependenciesUpToDate()) { echo 'Application dependencies are not up to date.' . PHP_EOL; echo 'Run "php bin/console dependencies install" in the glpi tree to fix this.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (Script execution should be stopped to prevent further errors) } if (!$this->areLocalesUpToDate()) { echo 'Application locales have to be compiled.' . PHP_EOL; echo 'Run "php bin/console locales:compile" in the glpi tree to fix this.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (Script execution should be stopped to prevent further errors) } } diff --git a/src/Glpi/Kernel/Kernel.php b/src/Glpi/Kernel/Kernel.php index 2e8e4f91f1b..a955627bc69 100644 --- a/src/Glpi/Kernel/Kernel.php +++ b/src/Glpi/Kernel/Kernel.php @@ -161,7 +161,7 @@ protected function buildContainer(): ContainerBuilder echo sprintf('Unable to write in the `%s` directory.', $relative_path) . PHP_EOL; echo 'Files ACL must be fixed.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (Script execution should be stopped to prevent further errors) } } From 38f23f911ba38c434f0c44c5dbee6eee1e08e6ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 28 Apr 2025 15:56:28 +0200 Subject: [PATCH 3/6] Ignore legitimate `exit()` usages in CLI/cron context --- front/cron.php | 14 +++++++------- src/CronTask.php | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/front/cron.php b/front/cron.php index 391f7d46753..e5ff6db63ae 100644 --- a/front/cron.php +++ b/front/cron.php @@ -53,7 +53,7 @@ echo "\t" . 'You should run the script as the same user that your web server runs as to avoid file permissions being ruined.' . "\n"; if (!in_array('--allow-superuser', $_SERVER['argv'], true)) { echo "\t" . 'Use --allow-superuser option to bypass this limitation.' . "\n"; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } } @@ -79,7 +79,7 @@ if ($CFG_GLPI['maintenance_mode'] ?? false) { echo 'Service is down for maintenance. It will be back shortly.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } if (!($DB instanceof DBmysql)) { @@ -87,28 +87,28 @@ 'ERROR: The database configuration file "%s" is missing or is corrupted. You have to either restart the install process, or restore this file.', GLPI_CONFIG_DIR . '/config_db.php' ) . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } if (!$DB->connected) { echo 'ERROR: The connection to the SQL server could not be established. Please check your configuration.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } if (!Config::isLegacyConfigurationLoaded()) { echo 'ERROR: Unable to load the GLPI configuration from the database.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } if (Update::isUpdateMandatory()) { echo 'The GLPI codebase has been updated. The update of the GLPI database is necessary.' . PHP_EOL; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } if (!is_writable(GLPI_LOCK_DIR)) { echo "\t" . sprintf('ERROR: %s is not writable.' . "\n", GLPI_LOCK_DIR); echo "\t" . 'Run the script as the same user that your web server runs as.' . "\n"; - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (CLI context) } if (isset($_SERVER['argc']) && ($_SERVER['argc'] > 1)) { diff --git a/src/CronTask.php b/src/CronTask.php index e812f5a54bd..07dee97d3c4 100644 --- a/src/CronTask.php +++ b/src/CronTask.php @@ -213,7 +213,7 @@ public function signal($signo) $_SESSION["glpicronuserrunning"] = ''; self::release_lock(); Toolbox::logInFile('cron', __('Action aborted') . "\n"); - exit(); + exit(); // @phpstan-ignore glpi.forbidExit (CLI context) } } From 400462824c8afa0aa557afda8b315ebc31163179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 28 Apr 2025 15:57:08 +0200 Subject: [PATCH 4/6] Ignore `exit()` usages in deprecated method --- src/Glpi/Http/Response.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Glpi/Http/Response.php b/src/Glpi/Http/Response.php index 53c27197f13..417d0201a7f 100644 --- a/src/Glpi/Http/Response.php +++ b/src/Glpi/Http/Response.php @@ -88,7 +88,7 @@ public static function sendError(int $code, string $message, string $content_typ Toolbox::logDebug($message); echo($output); - exit(1); + exit(1); // @phpstan-ignore glpi.forbidExit (Deprecated scope) } public function sendHeaders(): Response From 37a68a1efe56b6c026a443195b4d4dfebdf45214 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 28 Apr 2025 15:58:02 +0200 Subject: [PATCH 5/6] Use `return` or exceptions instead of `exi()` in massive actions --- src/CommonITILObject.php | 11 ++++++++++- src/MassiveAction.php | 24 ++++++++++-------------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/CommonITILObject.php b/src/CommonITILObject.php index 43ef3b8e4fd..b7b50babe36 100644 --- a/src/CommonITILObject.php +++ b/src/CommonITILObject.php @@ -4167,7 +4167,16 @@ public static function showMassiveActionsSubForm(MassiveAction $ma) switch ($ma->getAction()) { case 'add_task': - $itemtype = $ma->getItemtype(true); + $itemtype_or_selector = $ma->getItemtype(true); + + if (is_bool($itemtype_or_selector)) { + // MassiveAction::getItemtype() will return a boolean if the itemtype selector needs to be displayed. + return $itemtype_or_selector; + } + + // MassiveAction::getItemtype() will return a classname if the selector does not need to be displayed. + $itemtype = $itemtype_or_selector; + $tasktype = $itemtype . 'Task'; if ($ttype = getItemForItemtype($tasktype)) { /** @var CommonITILTask $ttype */ diff --git a/src/MassiveAction.php b/src/MassiveAction.php index f947b42dcad..be5a607c05c 100644 --- a/src/MassiveAction.php +++ b/src/MassiveAction.php @@ -246,8 +246,8 @@ public function __construct(array $POST, array $GET, $stage, ?int $items_id = nu throw new \Exception(__('Implementation error!')); } if ($POST['action'] == -1) { - // Case when no action is choosen - exit(); + // Case when no action is choosen + throw new \RuntimeException(); } if (isset($POST['actions'])) { // First, get the name of current action ! @@ -550,11 +550,11 @@ public function getCheckItem($POST) if ($this->check_item === null && isset($POST['check_itemtype'])) { if (!($this->check_item = getItemForItemtype($POST['check_itemtype']))) { - exit(); + throw new \RuntimeException(); } if (isset($POST['check_items_id'])) { if (!$this->check_item->getFromDB($POST['check_items_id'])) { - exit(); + throw new \RuntimeException(); } else { $this->check_item->getEmpty(); } @@ -595,7 +595,7 @@ public function addHiddenFields() * * @param boolean $display_selector can we display the itemtype selector ? * - * @return string|boolean the itemtype or false if we cannot define it (and we cannot display the selector) + * @return string|boolean the itemtype, or true if the selector is displayed, or false if we cannot define the itemtype nor display the selector **/ public function getItemtype($display_selector) { @@ -629,7 +629,7 @@ public function getItemtype($display_selector) ); echo " "; - exit(); + return true; } return false; @@ -1083,15 +1083,11 @@ public static function showMassiveActionsSubForm(MassiveAction $ma) ); } // Only display the form for this stage - exit(); + return; } if (!isset($ma->POST['common_options'])) { - echo "
" . - __s(

"; - echo "" . __s('Implementation error!') . "
"; - echo "
"; - exit(); + throw new \RuntimeException('Implementation error!'); } if ($ma->POST['common_options'] == 'false') { @@ -1124,7 +1120,7 @@ public static function showMassiveActionsSubForm(MassiveAction $ma) $itemtype_search_options = SearchOption::getOptionsForItemtype($so_itemtype); if (!isset($itemtype_search_options[$so_index])) { - exit(); + throw new \RuntimeException(); } $item = $so_item; @@ -1133,7 +1129,7 @@ public static function showMassiveActionsSubForm(MassiveAction $ma) } if ($item === null) { - exit(); + throw new \RuntimeException(); } $plugdisplay = false; From 8bc8f184406fff521df58271a6f52edd4f1b017d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 28 Apr 2025 16:06:33 +0200 Subject: [PATCH 6/6] Add remaining errors into the baseline, they will be fixed later --- .phpstan-baseline.php | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/.phpstan-baseline.php b/.phpstan-baseline.php index 979d041c7a1..96579e13194 100644 --- a/.phpstan-baseline.php +++ b/.phpstan-baseline.php @@ -1555,6 +1555,18 @@ 'count' => 1, 'path' => __DIR__ . '/src/Glpi/Api/API.php', ]; +$ignoreErrors[] = [ + 'message' => '#^You should not use the `exit` function\\. It prevents the execution of post\\-request/post\\-command routines\\.$#', + 'identifier' => 'glpi.forbidExit', + 'count' => 3, + 'path' => __DIR__ . '/src/Glpi/Api/API.php', +]; +$ignoreErrors[] = [ + 'message' => '#^You should not use the `http_response_code` function to change the response code\\. Due to a PHP bug, it may not provide the expected result \\(see https\\://bugs\\.php\\.net/bug\\.php\\?id\\=81451\\)\\.$#', + 'identifier' => 'glpi.forbidHttpResponseCode', + 'count' => 1, + 'path' => __DIR__ . '/src/Glpi/Api/API.php', +]; $ignoreErrors[] = [ 'message' => '#^Method Glpi\\\\Api\\\\APIRest\\:\\:parseIncomingParams\\(\\) with return type void returns string but should not return anything\\.$#', 'identifier' => 'return.void', @@ -1579,6 +1591,18 @@ 'count' => 1, 'path' => __DIR__ . '/src/Glpi/Api/APIRest.php', ]; +$ignoreErrors[] = [ + 'message' => '#^You should not use the `exit` function\\. It prevents the execution of post\\-request/post\\-command routines\\.$#', + 'identifier' => 'glpi.forbidExit', + 'count' => 3, + 'path' => __DIR__ . '/src/Glpi/Api/APIRest.php', +]; +$ignoreErrors[] = [ + 'message' => '#^You should not use the `http_response_code` function to change the response code\\. Due to a PHP bug, it may not provide the expected result \\(see https\\://bugs\\.php\\.net/bug\\.php\\?id\\=81451\\)\\.$#', + 'identifier' => 'glpi.forbidHttpResponseCode', + 'count' => 1, + 'path' => __DIR__ . '/src/Glpi/Api/APIRest.php', +]; $ignoreErrors[] = [ 'message' => '#^Call to function is_array\\(\\) with array will always evaluate to true\\.$#', 'identifier' => 'function.alreadyNarrowedType', @@ -2173,6 +2197,12 @@ 'count' => 1, 'path' => __DIR__ . '/src/Glpi/Dropdown/Dropdown.php', ]; +$ignoreErrors[] = [ + 'message' => '#^You should not use the `http_response_code` function to change the response code\\. Due to a PHP bug, it may not provide the expected result \\(see https\\://bugs\\.php\\.net/bug\\.php\\?id\\=81451\\)\\.$#', + 'identifier' => 'glpi.forbidHttpResponseCode', + 'count' => 1, + 'path' => __DIR__ . '/src/Glpi/Http/Response.php', +]; $ignoreErrors[] = [ 'message' => '#^Negated boolean expression is always false\\.$#', 'identifier' => 'booleanNot.alwaysFalse',