diff --git a/.gitignore b/.gitignore index b2e56b0..11532bd 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ dist/ vendor/ .gh_token -*.min.* \ No newline at end of file +*.min.* +.phpunit.result.cache \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d1546a..0f64b3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +### Fixed + +- Enforce item rights and sanitize inputs in tree loading + ## [1.20.0] - 2025-09-16 ### Added diff --git a/composer.json b/composer.json index af26759..87e2634 100644 --- a/composer.json +++ b/composer.json @@ -8,6 +8,11 @@ "php-parallel-lint/php-parallel-lint": "^1.4", "phpstan/phpstan": "^2.1" }, + "autoload": { + "psr-4": { + "GlpiPlugin\\Treeview\\Tests\\": "tests" + } + }, "config": { "optimize-autoloader": true, "platform": { diff --git a/front/config.form.php b/front/config.form.php index 12ed308..65ff079 100644 --- a/front/config.form.php +++ b/front/config.form.php @@ -30,10 +30,25 @@ include('../../../inc/includes.php'); -Session::checkRight('treeview', UPDATE); +Session::checkRight(PluginTreeviewConfig::$rightname, UPDATE); $config = new PluginTreeviewConfig(); if (isset($_POST['update'])) { + if (isset($_POST['target']) && !in_array($_POST['target'], ['_blank', 'right'], true)) { + Session::addMessageAfterRedirect( + sprintf(__s('Invalid target value (%s) ignored', 'treeview'), htmlspecialchars((string) $_POST['target'], ENT_QUOTES, 'UTF-8')), + false, + ERROR, + ); + unset($_POST['target']); + } + + foreach (['folderLinks', 'useSelection', 'useLines', 'useIcons', 'closeSameLevel', 'itemName', 'locationName'] as $field) { + if (isset($_POST[$field])) { + $_POST[$field] = (int) $_POST[$field]; + } + } + $config->update($_POST); Html::back(); } else { diff --git a/front/preference.form.php b/front/preference.form.php index e343954..f6d7e58 100644 --- a/front/preference.form.php +++ b/front/preference.form.php @@ -36,6 +36,14 @@ //Save user preferences if (isset($_POST['plugin_treeview_user_preferences_save'])) { - $pref->update($_POST); + if (!($own_id = $pref->checkIfPreferenceExists(Session::getLoginUserID()))) { + $own_id = $pref->addDefaultPreference(Session::getLoginUserID()); + if (!$own_id) { + Session::addMessageAfterRedirect(__s('Unable to save preferences', 'treeview'), false, ERROR); + Html::back(); + } + } + + $pref->update(['id' => $own_id, 'show_on_load' => (int) ($_POST['show_on_load'] ?? 0)]); Html::back(); } diff --git a/inc/config.class.php b/inc/config.class.php index dd575fc..bffa831 100644 --- a/inc/config.class.php +++ b/inc/config.class.php @@ -51,6 +51,8 @@ class PluginTreeviewConfig extends CommonDBTM 'PassiveDCEquipment', ]; + public static $rightname = 'config'; + /** * Display name of itemtype * @@ -306,7 +308,7 @@ public function getNodesFromDb() $closeSameLevel = $this->fields['closeSameLevel']; // Load the settings in JavaSript so that dTree script can apply them - echo "d.config.target = '" . $target . "';\n"; + echo "d.config.target = " . json_encode($target) . ";\n"; echo 'd.config.folderLinks = ' . $folderLinks . ";\n"; echo 'd.config.useSelection = ' . $useSelection . ";\n"; echo 'd.config.useLines = ' . $useLines . ";\n"; @@ -333,7 +335,7 @@ public function getNodesFromDb() // Is this the first time we load the page? if (isset($_GET['nodes']) && $_GET['nodes'] != '') { // If no then get all the nodes requested by the client - $nodes = array_reverse(explode('.', $_GET['nodes'])); + $nodes = array_map('intval', array_reverse(explode('.', $_GET['nodes']))); } else { // If yes then get only the root node $nodes[0] = 0; @@ -389,7 +391,7 @@ public function getNodesFromDb() "\", true, -1,'');\n"; $dontLoad = 'true'; // Then add aloso its items - foreach (self::$types as $type) { + foreach (self::getTypes() as $type) { $item = new $type(); $itemtable = getTableForItemType($type); @@ -408,7 +410,7 @@ public function getNodesFromDb() $criteria['WHERE']['is_deleted'] = 0; } - if ($this->isEntityAssign()) { + if ($item->isEntityAssign()) { $criteria['WHERE']['entities_id'] = $_SESSION['glpiactive_entity']; } @@ -517,9 +519,9 @@ public function getNodesFromDb() // Open the tree to the desired node if ($openedType != -1) { - echo 'd.openTo(' . $openedType . ");\n"; + echo 'd.openTo(' . (int) $openedType . ");\n"; } else { - echo 'd.openTo(' . $nodes[count($nodes) - 1] . ");\n"; + echo 'd.openTo(' . (int) $nodes[count($nodes) - 1] . ");\n"; } } diff --git a/inc/preference.class.php b/inc/preference.class.php index 2140f4a..744a288 100644 --- a/inc/preference.class.php +++ b/inc/preference.class.php @@ -65,8 +65,7 @@ public function showFormUserPreference($target, $id) echo ""; echo ""; - echo ""; + _sx('button', 'Post') . "' class='submit'>"; echo ""; echo "" . __('Warning: If there are more than one plugin which be loaded at startup, then only the first will be used', 'treeview'); diff --git a/index.php b/index.php index bc45949..3fb3307 100644 --- a/index.php +++ b/index.php @@ -30,6 +30,12 @@ include('../../inc/includes.php'); +Session::checkLoginUser(); + +if (empty($_SESSION['glpi_plugin_treeview_profile']['treeview'])) { + Html::displayRightError(); +} + Plugin::load('treeview', true); $_SESSION['glpi_plugin_treeview_loaded'] = 1; diff --git a/left.php b/left.php index 013f9ca..3afa579 100644 --- a/left.php +++ b/left.php @@ -32,6 +32,10 @@ Session::checkLoginUser(); +if (empty($_SESSION['glpi_plugin_treeview_profile']['treeview'])) { + Html::displayRightError(); +} + $treeview_url = Plugin::getWebDir('treeview'); Html::includeHeader('TreeView'); diff --git a/phpunit.xml b/phpunit.xml new file mode 100644 index 0000000..3023d8c --- /dev/null +++ b/phpunit.xml @@ -0,0 +1,7 @@ + + + + tests + + + diff --git a/tests/TreeviewTestCase.php b/tests/TreeviewTestCase.php new file mode 100644 index 0000000..5e58be4 --- /dev/null +++ b/tests/TreeviewTestCase.php @@ -0,0 +1,69 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by Teclib'. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/treeview + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Treeview\Tests; + +use DbTestCase; +use PluginTreeviewConfig; + +abstract class TreeviewTestCase extends DbTestCase +{ + /** + * Capture the JavaScript tree markup generated by + * PluginTreeviewConfig::getNodesFromDb() for the current session + * (active entity, current user rights). + * + * @param int|null $opened_locations_id A top-level location id to expand, + * so its items get loaded too (the + * tree only loads items for nodes + * present in the "nodes" path, same + * as a client expanding a node). + */ + protected function getTreeOutput(?int $opened_locations_id = null): string + { + $previous_nodes = $_GET['nodes'] ?? null; + if ($opened_locations_id !== null) { + $_GET['nodes'] = $opened_locations_id . '.0'; + } + + $config = new PluginTreeviewConfig(); + ob_start(); + $config->getNodesFromDb(); + $output = ob_get_clean(); + + if ($previous_nodes === null) { + unset($_GET['nodes']); + } else { + $_GET['nodes'] = $previous_nodes; + } + + return $output; + } +} diff --git a/tests/Units/ConfigTest.php b/tests/Units/ConfigTest.php new file mode 100644 index 0000000..8168e1b --- /dev/null +++ b/tests/Units/ConfigTest.php @@ -0,0 +1,120 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by Teclib'. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/treeview + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Treeview\Tests\Units; + +use Computer; +use Entity; +use GlpiPlugin\Treeview\Tests\TreeviewTestCase; +use Location; +use Profile; +use ProfileRight; + +final class ConfigTest extends TreeviewTestCase +{ + public function testGetNodesFromDbOnlyShowsActiveEntityData(): void + { + $this->login(); + $root_id = $this->getTestRootEntity(true); + + $entity_a = $this->createItem(Entity::class, [ + 'name' => 'treeview_entity_a_' . $this->getUniqueString(), + 'entities_id' => $root_id, + ]); + $entity_b = $this->createItem(Entity::class, [ + 'name' => 'treeview_entity_b_' . $this->getUniqueString(), + 'entities_id' => $root_id, + ]); + + $location_a = $this->createItem(Location::class, [ + 'name' => 'treeview_loc_a_' . $this->getUniqueString(), + 'entities_id' => $entity_a->getID(), + ]); + $location_b = $this->createItem(Location::class, [ + 'name' => 'treeview_loc_b_' . $this->getUniqueString(), + 'entities_id' => $entity_b->getID(), + ]); + + $computer_a = $this->createItem(Computer::class, [ + 'name' => 'treeview_computer_a_' . $this->getUniqueString(), + 'entities_id' => $entity_a->getID(), + 'locations_id' => $location_a->getID(), + ]); + $computer_b = $this->createItem(Computer::class, [ + 'name' => 'treeview_computer_b_' . $this->getUniqueString(), + 'entities_id' => $entity_b->getID(), + 'locations_id' => $location_b->getID(), + ]); + + // Switch active entity A only (not recursive). + $this->setEntity($entity_a->getID(), false); + + $output = $this->getTreeOutput($location_a->getID()); + + $this->assertStringContainsString($computer_a->fields['name'], $output); + $this->assertStringNotContainsString($computer_b->fields['name'], $output); + $this->assertStringNotContainsString($location_b->fields['name'], $output); + } + + public function testGetNodesFromDbHidesItemtypeWithoutViewRight(): void + { + $this->login(); + $entity_id = $this->getTestRootEntity(true); + + $location = $this->createItem(Location::class, [ + 'name' => 'treeview_loc_' . $this->getUniqueString(), + 'entities_id' => $entity_id, + ]); + $computer = $this->createItem(Computer::class, [ + 'name' => 'treeview_computer_' . $this->getUniqueString(), + 'entities_id' => $entity_id, + 'locations_id' => $location->getID(), + ]); + + $super_admin_id = getItemByTypeName(Profile::class, 'Super-Admin', true); + $original_rights = ProfileRight::getProfileRights($super_admin_id, [Computer::$rightname]); + + // remove all rights to view computers + ProfileRight::updateProfileRights($super_admin_id, [ + Computer::$rightname => $original_rights[Computer::$rightname] & ~(READ), + ]); + try { + $this->login('glpi'); + $this->setEntity($entity_id, false); + $output = $this->getTreeOutput($location->getID()); + + $this->assertStringNotContainsString($computer->fields['name'], $output); + } finally { + ProfileRight::updateProfileRights($super_admin_id, [ + Computer::$rightname => $original_rights[Computer::$rightname], + ]); + } + } +} diff --git a/tests/Units/PreferenceTest.php b/tests/Units/PreferenceTest.php new file mode 100644 index 0000000..371fc7d --- /dev/null +++ b/tests/Units/PreferenceTest.php @@ -0,0 +1,82 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by Teclib'. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/treeview + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Treeview\Tests\Units; + +use GlpiPlugin\Treeview\Tests\TreeviewTestCase; +use PluginTreeviewPreference; +use Session; + +final class PreferenceTest extends TreeviewTestCase +{ + public function testCheckIfPreferenceExistsIsScopedToOwner(): void + { + $this->login(); + $user_a_id = (int) Session::getLoginUserID(); + $pref_a = new PluginTreeviewPreference(); + $pref_a_id = $pref_a->addDefaultPreference($user_a_id); + + $this->login('normal', 'normal'); + $user_b_id = (int) Session::getLoginUserID(); + $pref_b = new PluginTreeviewPreference(); + $pref_b_id = $pref_b->addDefaultPreference($user_b_id); + + $this->assertNotEquals($pref_a_id, $pref_b_id); + + // Even if an attacker-controlled value referenced user A's record, + // resolving "own_id" must always come back to the logged-in user. + $own_id = $pref_b->checkIfPreferenceExists($user_b_id); + $this->assertEquals($pref_b_id, $own_id); + $this->assertNotEquals($pref_a_id, $own_id); + } + + public function testUpdateScopedToOwnPreferenceDoesNotAffectOtherUser(): void + { + $this->login(); + $pref_a = new PluginTreeviewPreference(); + $pref_a_id = $pref_a->addDefaultPreference((int) Session::getLoginUserID()); + + $this->login('normal', 'normal'); + $pref_b = new PluginTreeviewPreference(); + $pref_b_id = $pref_b->addDefaultPreference((int) Session::getLoginUserID()); + + // Reproduces the fixed front/preference.form.php flow: the id used + // for the update is resolved server-side from the session user, + // never taken from client-supplied data. + $own_id = $pref_b->checkIfPreferenceExists(Session::getLoginUserID()); + $this->assertTrue($pref_b->update(['id' => $own_id, 'show_on_load' => 1])); + + $this->assertTrue($pref_a->getFromDB($pref_a_id)); + $this->assertEquals(0, $pref_a->fields['show_on_load']); + + $this->assertTrue($pref_b->getFromDB($pref_b_id)); + $this->assertEquals(1, $pref_b->fields['show_on_load']); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php new file mode 100644 index 0000000..00b6650 --- /dev/null +++ b/tests/bootstrap.php @@ -0,0 +1,58 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by Teclib'. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/treeview + * ------------------------------------------------------------------------- + */ + +global $CFG_GLPI, $PLUGIN_HOOKS; + +define('GLPI_ROOT', __DIR__ . '/../../../'); +define('GLPI_LOG_DIR', __DIR__ . '/files/_logs'); + +define('TU_USER', 'glpi'); +define('TU_PASS', 'glpi'); +define('GLPI_LOG_LVL', 'DEBUG'); + +require GLPI_ROOT . '/inc/includes.php'; +include_once GLPI_ROOT . '/phpunit/GLPITestCase.php'; +include_once GLPI_ROOT . '/phpunit/DbTestCase.php'; +require_once __DIR__ . '/../vendor/autoload.php'; +require_once __DIR__ . '/../setup.php'; + +if (!Plugin::isPluginActive("treeview")) { + throw new RuntimeException("Plugin treeview is not active in the test database"); +} + +if (!file_exists(GLPI_LOG_DIR . '/php-errors.log')) { + file_put_contents(GLPI_LOG_DIR . '/php-errors.log', ''); +} + +if (!file_exists(GLPI_LOG_DIR . '/sql-errors.log')) { + file_put_contents(GLPI_LOG_DIR . '/sql-errors.log', ''); +} + +plugin_init_treeview();