From 4d5659aca5358fc71cfb2c0d5fa34db0c5591e6b Mon Sep 17 00:00:00 2001 From: Stanislas Kita <7335054+stonebuzz@users.noreply.github.com> Date: Tue, 8 Sep 2026 09:28:40 +0200 Subject: [PATCH] Fix(core):Fix model selector validation and access control --- CHANGELOG.md | 1 + front/mapping.form.php | 19 ++++-- front/popup.php | 10 ++- inc/model.class.php | 72 ++++++++++---------- inc/modelcsv.class.php | 20 +++--- tests/unit/ModelCheckRightTest.php | 105 +++++++++++++++++++++++++++++ tests/unit/ModelCsvLookupTest.php | 80 ++++++++++++++++++++++ 7 files changed, 255 insertions(+), 52 deletions(-) create mode 100644 tests/unit/ModelCheckRightTest.php create mode 100644 tests/unit/ModelCsvLookupTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 6466ca03..569b6594 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Move to iterator - Fix various minor bugs in the import/export workflow - Fix an issue where data are not formatted when coming from a field plugin's custom field. +- Fix model selector validation and access control ## [2.15.10] - 2026-08-07 diff --git a/front/mapping.form.php b/front/mapping.form.php index 8e99af94..d6ffb671 100644 --- a/front/mapping.form.php +++ b/front/mapping.form.php @@ -34,9 +34,21 @@ if (isset($_POST["update"])) { $at_least_one_mandatory = false; $mapping = new PluginDatainjectionMapping(); + $existing = new PluginDatainjectionMapping(); + + $models_id = (int) ($_POST['models_id'] ?? 0); + $model = new PluginDatainjectionModel(); + $model->check($models_id, UPDATE); foreach ($_POST['data'] as $id => $mapping_infos) { - $mapping_infos['id'] = $id; + if ( + !$existing->getFromDB((int) $id) + || (int) $existing->fields['models_id'] !== $models_id + ) { + continue; + } + + $mapping_infos['id'] = (int) $id; //If no field selected, reset other values if ($mapping_infos['value'] == PluginDatainjectionInjectionType::NO_VALUE) { @@ -64,12 +76,9 @@ true, ); } else { - $model = new PluginDatainjectionModel(); - $model->getFromDB($_POST['models_id']); - if ($model->fields['step'] != PluginDatainjectionModel::READY_TO_USE_STEP) { PluginDatainjectionModel::changeStep( - $_POST['models_id'], + $models_id, PluginDatainjectionModel::OTHERS_STEP, ); Session::setActiveTab('PluginDatainjectionModel', 'PluginDatainjectionModel$5'); diff --git a/front/popup.php b/front/popup.php index 63999660..4b6747b9 100644 --- a/front/popup.php +++ b/front/popup.php @@ -32,16 +32,20 @@ switch ($_GET["popup"]) { case "preview": + $models_id = (int) ($_GET['models_id'] ?? 0); $model = new PluginDatainjectionModel(); - $model->check($_GET['models_id'], READ); + $model->check($models_id, READ); Html::popHeader(__('See the file', 'datainjection'), $_SERVER['PHP_SELF']); - PluginDatainjectionModel::showPreviewMappings($_GET['models_id']); + PluginDatainjectionModel::showPreviewMappings($models_id); Html::popFooter(); break; case "log": + $models_id = (int) ($_GET['models_id'] ?? 0); + $model = new PluginDatainjectionModel(); + $model->check($models_id, READ); Html::popHeader(__('Data injection report', 'datainjection'), $_SERVER['PHP_SELF']); - PluginDatainjectionModel::showLogResults($_GET['models_id']); + PluginDatainjectionModel::showLogResults($models_id); Html::popFooter(); break; } diff --git a/inc/model.class.php b/inc/model.class.php index 8fbbfd0e..983e130c 100644 --- a/inc/model.class.php +++ b/inc/model.class.php @@ -137,7 +137,7 @@ public function canViewItem(): bool return false; } - return self::checkRightOnModel($this->fields['id']); + return self::checkRightOnModel((int) ($this->fields['id'] ?? 0)); } @@ -158,7 +158,7 @@ public function canCreateItem(): bool return false; } - return self::checkRightOnModel($this->fields['id']); + return self::checkRightOnModel((int) ($this->fields['id'] ?? 0)); } @@ -351,10 +351,10 @@ public static function dropdown($options = []) if ($model['entities_id'] == -1) { echo "\n"; } else { - echo "\n'; + )) . '">'; } $prev = $model['entities_id']; @@ -362,8 +362,8 @@ public static function dropdown($options = []) $selected = $model['id'] == $value ? "selected" : ""; - $comment = $model['comment'] ? "title='" . htmlentities((string) $model['comment'], ENT_QUOTES, 'UTF-8') . "'" : ""; - echo "\n"; + $comment = $model['comment'] ? "title='" . htmlescape((string) $model['comment']) . "'" : ""; + echo "\n"; } if ($prev >= -1) { @@ -436,7 +436,7 @@ public static function getModels($user_id, $order = "name", $entity = -1, $all = foreach ($DB->request($query) as $data) { if ( - self::checkRightOnModel($data['id']) + self::checkRightOnModel((int) $data['id']) && class_exists($data['itemtype']) ) { $models[] = $data; @@ -870,7 +870,7 @@ public function prepareInputForAdd($input) return false; } - if (!$input['behavior_add'] && !$input['behavior_update']) { + if (!($input['behavior_add'] ?? 0) && !($input['behavior_update'] ?? 0)) { Session::addMessageAfterRedirect( __s( 'Your model should allow import and/or update of data', @@ -1251,40 +1251,44 @@ public function populateSeveraltimesMappedFields() } - /** - * @param int $models_id - **/ - public static function checkRightOnModel($models_id) + public static function checkRightOnModel(int $models_id): bool { /** @var DBmysql $DB */ global $DB; - $continue = true; - $model = new self(); - if ($model->getFromDB($models_id)) { - $query = "(SELECT `itemtype` - FROM `glpi_plugin_datainjection_models` - WHERE `id` = '" . $models_id . "') - UNION (SELECT DISTINCT `itemtype` - FROM `glpi_plugin_datainjection_mappings` - WHERE `models_id` = '" . $models_id . "') - UNION (SELECT DISTINCT `itemtype` - FROM `glpi_plugin_datainjection_infos` - WHERE `models_id` = '" . $models_id . "')"; - foreach ($DB->doQuery($query) as $data) { - if ($data['itemtype'] != PluginDatainjectionInjectionType::NO_VALUE && is_a($data['itemtype'], CommonDBTM::class, true)) { - $item = new $data['itemtype'](); - $item->fields['itemtype'] = $model->fields['itemtype']; - if (!($item instanceof CommonDBRelation) && !$item->canCreate()) { - $continue = false; - break; - } - } + if (!$model->getFromDB($models_id)) { + //New model being created: no injected itemtype to check yet + return true; + } + + $itemtypes = [$model->fields['itemtype']]; + + foreach (['glpi_plugin_datainjection_mappings', 'glpi_plugin_datainjection_infos'] as $table) { + $iterator = $DB->request([ + 'SELECT' => 'itemtype', + 'DISTINCT' => true, + 'FROM' => $table, + 'WHERE' => ['models_id' => $models_id], + ]); + + foreach ($iterator as $data) { + $itemtypes[] = $data['itemtype']; } } - return $continue; + foreach (array_unique($itemtypes) as $itemtype) { + if ($itemtype == PluginDatainjectionInjectionType::NO_VALUE || !is_a($itemtype, CommonDBTM::class, true)) { + continue; + } + + $item = new $itemtype(); + if (!$item->canCreate()) { + return false; + } + } + + return true; } diff --git a/inc/modelcsv.class.php b/inc/modelcsv.class.php index c643bf00..8666a76b 100644 --- a/inc/modelcsv.class.php +++ b/inc/modelcsv.class.php @@ -160,26 +160,26 @@ public function checkFileName($filename) * * @return int the ID of the row in glpi_plugin_datainjection_modelcsv **/ - public function getFromDBByModelID($models_id) + public function getFromDBByModelID(int $models_id): int { /** @var DBmysql $DB */ global $DB; - $query = "SELECT `id` - FROM `" . $this->getTable() . "` - WHERE `models_id` = '" . $models_id . "'"; + $iterator = $DB->request([ + 'SELECT' => 'id', + 'FROM' => $this->getTable(), + 'WHERE' => ['models_id' => $models_id], + 'LIMIT' => 1, + ]); - $results = $DB->doQuery($query); - $id = 0; - - if ($DB->numrows($results) > 0) { - $id = $DB->result($results, 0, 'id'); + if (count($iterator) > 0) { + $id = (int) $iterator->current()['id']; $this->getFromDB($id); } else { $this->getEmpty(); $tmp = $this->fields; $tmp['models_id'] = $models_id; - $id = $this->add($tmp); + $id = (int) $this->add($tmp); $this->getFromDB($id); } diff --git a/tests/unit/ModelCheckRightTest.php b/tests/unit/ModelCheckRightTest.php new file mode 100644 index 00000000..33c3f31a --- /dev/null +++ b/tests/unit/ModelCheckRightTest.php @@ -0,0 +1,105 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by DataInjection plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/datainjection + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Datainjection\Tests\Unit; + +use Computer; +use Group; +use Group_User; +use PluginDatainjectionMapping; +use PluginDatainjectionModel; +use Session; +use User; +use Glpi\Tests\DbTestCase; + +final class ModelCheckRightTest extends DbTestCase +{ + private function createModel(string $itemtype = Computer::class): int + { + $model = new PluginDatainjectionModel(); + $models_id = $model->add([ + 'name' => 'Test_Model_CheckRight_' . $itemtype . '_' . random_int(1, PHP_INT_MAX), + 'itemtype' => $itemtype, + 'filetype' => 'csv', + 'entities_id' => 0, + 'is_private' => 0, + 'behavior_add' => 1, + 'behavior_update' => 0, + 'users_id' => Session::getLoginUserID(), + ]); + $this->assertGreaterThan(0, $models_id); + + return (int) $models_id; + } + + public function testUnknownModelIsAllowed(): void + { + $this->login(); + + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel(999999)); + } + + public function testCreationPathReturnsBooleanOnEmptyModel(): void + { + $this->login(); + + $model = new PluginDatainjectionModel(); + $model->getEmpty(); + + $this->assertIsBool($model->canCreateItem()); + } + + public function testMappedRelationItemtypeWithoutRightsIsDenied(): void + { + $this->login(); + + $models_id = $this->createModel(); + $control_id = $this->createModel(); + + $mapping = new PluginDatainjectionMapping(); + $this->assertGreaterThan(0, $mapping->add([ + 'models_id' => $models_id, + 'itemtype' => Group_User::class, + 'rank' => 0, + 'name' => 'groups_id', + 'value' => 'groups_id', + 'is_mandatory' => 0, + ])); + + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($models_id)); + + $_SESSION['glpiactiveprofile'][User::$rightname] = 0; + $_SESSION['glpiactiveprofile'][Group::$rightname] = 0; + + //Control model keeps its own granted itemtype: only the mapped relation may deny + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($control_id)); + $this->assertFalse(PluginDatainjectionModel::checkRightOnModel($models_id)); + } +} diff --git a/tests/unit/ModelCsvLookupTest.php b/tests/unit/ModelCsvLookupTest.php new file mode 100644 index 00000000..fd5fa47a --- /dev/null +++ b/tests/unit/ModelCsvLookupTest.php @@ -0,0 +1,80 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by DataInjection plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/datainjection + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Datainjection\Tests\Unit; + +use Session; +use Glpi\Tests\DbTestCase; +use PluginDatainjectionModel; +use PluginDatainjectionModelCsv; +use TypeError; + +final class ModelCsvLookupTest extends DbTestCase +{ + private function createModel(): int + { + $model = new PluginDatainjectionModel(); + $models_id = $model->add([ + 'name' => 'Test_ModelCsv_Lookup', + 'itemtype' => 'Computer', + 'filetype' => 'csv', + 'entities_id' => 0, + 'is_private' => 0, + 'behavior_add' => 1, + 'behavior_update' => 0, + 'users_id' => Session::getLoginUserID(), + ]); + $this->assertGreaterThan(0, $models_id); + + return (int) $models_id; + } + + public function testRowIsCreatedThenReusedForSameModel(): void + { + $models_id = $this->createModel(); + + $csv = new PluginDatainjectionModelCsv(); + $first = $csv->getFromDBByModelID($models_id); + + $this->assertGreaterThan(0, $first); + $this->assertSame($models_id, (int) $csv->fields['models_id']); + + $second = (new PluginDatainjectionModelCsv())->getFromDBByModelID($models_id); + $this->assertSame($first, $second); + } + + public function testNonNumericModelIdIsRejected(): void + { + $csv = new PluginDatainjectionModelCsv(); + + $this->expectException(TypeError::class); + $csv->getFromDBByModelID("id' AND SLEEP(5)-- "); + } +}