-
Notifications
You must be signed in to change notification settings - Fork 62
fix/ escape SQL values #627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
f00b224
acde442
e980f99
db8d43a
2d3b54d
02e3ff2
f45c2d0
bcf08dd
818a32c
6a138c9
5d7202d
2d55a5d
7fbec49
96bdc49
0ad46d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,3 +3,5 @@ vendor/ | |
| .gh_token | ||
| *.min.* | ||
|
|
||
|
|
||
| var/phpunit | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ | |
| * @link https://github.com/pluginsGLPI/datainjection | ||
| * ------------------------------------------------------------------------- | ||
| */ | ||
|
|
||
| use Glpi\DBAL\QuerySubQuery; | ||
| use Glpi\Exception\Http\HttpException; | ||
| use Glpi\Features\AssignableItem; | ||
|
|
||
|
|
@@ -1865,7 +1867,6 @@ private function dataAlreadyInDB($injectionClass, $itemtype) | |
| /** @var DBmysql $DB */ | ||
| global $DB; | ||
|
|
||
| $where = ""; | ||
| $continue = true; | ||
|
|
||
| $injectionClass->getOptions($this->primary_type); | ||
|
|
@@ -1893,17 +1894,15 @@ private function dataAlreadyInDB($injectionClass, $itemtype) | |
| if (!$continue) { | ||
| $this->values[$itemtype]['id'] = self::ITEM_NOT_FOUND; | ||
| } else { | ||
| $sql = "SELECT * | ||
| FROM `" . $injectionClass->getTable() . "`"; | ||
|
|
||
| if (!is_a($itemtype, CommonDBTM::class, true)) { | ||
| throw new HttpException(500, 'Class ' . $itemtype . ' is not a valid class'); | ||
| } | ||
| $item = new $itemtype(); | ||
| $item = new $itemtype(); | ||
| $where = []; | ||
|
|
||
| //If it's a computer device | ||
| if ($item instanceof CommonDevice) { | ||
| $sql .= " WHERE `designation` = '" . | ||
| $this->getValueByItemtypeAndName($itemtype, 'designation') . "'"; | ||
| $where['designation'] = $this->getValueByItemtypeAndName($itemtype, 'designation'); | ||
| } elseif ($item instanceof CommonDBRelation) { | ||
| //Type is a relation : check it this relation still exists | ||
| //Define the side of the relation to use | ||
|
|
@@ -1919,94 +1918,101 @@ private function dataAlreadyInDB($injectionClass, $itemtype) | |
| $source_itemtype = $item::$itemtype_1; | ||
| $destination_itemtype = $item::$itemtype_2; | ||
| } | ||
| $where .= " AND `$source_id`='" . | ||
| $this->getValueByItemtypeAndName($itemtype, $source_id) . "'"; | ||
| $where[$source_id] = $this->getValueByItemtypeAndName($itemtype, $source_id); | ||
| $where[$destination_id] = $this->getValueByItemtypeAndName($itemtype, $destination_id); | ||
| if ($item->isField('itemtype')) { | ||
| $where .= " AND `$source_itemtype`='" . | ||
| $this->getValueByItemtypeAndName($itemtype, $source_itemtype) . "'"; | ||
| $where[$source_itemtype] = $this->getValueByItemtypeAndName($itemtype, $source_itemtype); | ||
| } | ||
| $where .= " AND `" . $destination_id . "`='" . | ||
| $this->getValueByItemtypeAndName($itemtype, $destination_id) . "'"; | ||
| $sql .= " WHERE 1 " . $where; | ||
| } else { | ||
| //Type is not a relation | ||
|
|
||
| //Type can be deleted | ||
| if ($injectionClass->maybeDeleted()) { | ||
| $where .= " AND `is_deleted` = '0' "; | ||
| $where['is_deleted'] = 0; | ||
| } | ||
|
|
||
| //Type can be a template | ||
| if ($injectionClass->maybeTemplate()) { | ||
| $where .= " AND `is_template` = '0' "; | ||
| $where['is_template'] = 0; | ||
| } | ||
|
|
||
| //Type can be assigned to an entity | ||
| if ($injectionClass->isEntityAssign()) { | ||
| //Type can be recursive | ||
| if ($injectionClass->maybeRecursive()) { | ||
| $where_entity = getEntitiesRestrictRequest( | ||
| " AND", | ||
| $injectionClass->getTable(), | ||
| "entities_id", | ||
| $this->getValueByItemtypeAndName( | ||
| $itemtype, | ||
| $where = array_merge( | ||
| $where, | ||
| getEntitiesRestrictCriteria( | ||
| $injectionClass->getTable(), | ||
| 'entities_id', | ||
| $this->getValueByItemtypeAndName($itemtype, 'entities_id'), | ||
| true, | ||
| ), | ||
| true, | ||
| ); | ||
| } else { | ||
| //Type cannot be recursive | ||
| $where_entity = " AND `entities_id` = '" . | ||
| $this->getValueByItemtypeAndName($itemtype, 'entities_id') . "'"; | ||
| $where['entities_id'] = $this->getValueByItemtypeAndName($itemtype, 'entities_id'); | ||
| } | ||
| } else { //If no entity assignment for this itemtype | ||
| $where_entity = ""; | ||
| } | ||
|
|
||
| //Add mandatory fields to the query only if it's the primary_type to be injected | ||
| if ($itemtype == $this->primary_type) { | ||
| foreach ($this->mandatory_fields[$itemtype] as $field => $is_mandatory) { | ||
| if ($is_mandatory) { | ||
| if ($item instanceof User && $field == "useremails_id") { | ||
| $email = $DB->escape($this->getValueByItemtypeAndName($itemtype, $field)); | ||
| $where .= " AND `id` IN (SELECT `users_id` FROM glpi_useremails WHERE `email` = '$email') "; | ||
| $where['id'] = new QuerySubQuery([ | ||
| 'SELECT' => 'users_id', | ||
| 'FROM' => 'glpi_useremails', | ||
| 'WHERE' => ['email' => $this->getValueByItemtypeAndName($itemtype, $field)], | ||
| ]); | ||
| } else { | ||
| $where .= " AND `" . $field . "`='" . (string) $this->getValueByItemtypeAndName($itemtype, $field) . "'"; | ||
| $where[$field] = $this->getValueByItemtypeAndName($itemtype, $field); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| //Table contains an itemtype field | ||
| if ($injectionClass->isField('itemtype')) { | ||
| $where .= " AND `itemtype` = '" . $this->getValueByItemtypeAndName( | ||
| $itemtype, | ||
| 'itemtype', | ||
| ) . "'"; | ||
| $where['itemtype'] = $this->getValueByItemtypeAndName($itemtype, 'itemtype'); | ||
| } | ||
|
|
||
| //Table contains an items_id field | ||
| if ($injectionClass->isField('items_id')) { | ||
| $where .= " AND `items_id` = '" . $this->getValueByItemtypeAndName( | ||
| $itemtype, | ||
| 'items_id', | ||
| ) . "'"; | ||
| $where['items_id'] = $this->getValueByItemtypeAndName($itemtype, 'items_id'); | ||
| } | ||
| } | ||
|
|
||
| //Add additional parameters specific to this itemtype (or function checkPresent exists) | ||
| if (method_exists($injectionClass, 'checkPresent')) { | ||
| $where .= $injectionClass->checkPresent($this->values, $options); | ||
| $extra = $injectionClass->checkPresent($this->values, $options); | ||
| if (is_array($extra)) { | ||
| if (count($extra) > 0) { | ||
| $where = array_merge($where, $extra); | ||
| } | ||
| } elseif (!empty($extra)) { | ||
| trigger_error( | ||
| sprintf( | ||
| '%s::checkPresent() must return an array, %s returned instead.', | ||
| get_class($injectionClass), | ||
| gettype($extra), | ||
| ), | ||
| E_USER_WARNING, | ||
| ); | ||
| } | ||
|
Comment on lines
+1987
to
+2001
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An exception should be thrown or a warning logged if |
||
| } | ||
| $sql .= " WHERE 1 " . $where_entity . " " . $where; | ||
| } | ||
| $result = $DB->doQuery($sql); | ||
| if ($DB->numrows($result) > 0) { | ||
| $db_fields = $DB->fetchAssoc($result); | ||
|
|
||
| $result = $DB->request([ | ||
| 'FROM' => $injectionClass->getTable(), | ||
| 'WHERE' => $where, | ||
| ]); | ||
|
|
||
| if (count($result) > 0) { | ||
| $db_fields = $result->current(); | ||
| foreach ($db_fields as $key => $value) { | ||
| $this->setValueForItemtype($itemtype, $key, $value, true); | ||
| } | ||
| $this->setValueForItemtype($itemtype, 'id', $DB->result($result, 0, 'id')); | ||
| $this->setValueForItemtype($itemtype, 'id', $db_fields['id']); | ||
| } else { | ||
| $this->setValueForItemtype($itemtype, 'id', self::ITEM_NOT_FOUND); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -993,6 +993,7 @@ public function readUploadedFile($options = []) | |
| ), | ||
| ]; | ||
| } | ||
| unset($_FILES['filename']); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After move_uploaded_file(), the temporary file is deleted, but $_FILES['filename'] still contains its path. Later, when rendering the page, Html::footer() calls createFromGlobals(). An UploadedFile instance is created for each entry, and it checks whether the temporary file still exists. Since the file has already been moved and no longer exists, an exception is thrown. unset() clears the corresponding entry from $_FILES after the file has been moved. |
||
| } | ||
|
|
||
| //If file has not the right extension, reject it and delete if | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <phpunit | ||
| bootstrap="tests/bootstrap.php" | ||
| colors="true" | ||
| testdox="true" | ||
| cacheDirectory="var/phpunit" | ||
| > | ||
| <source> | ||
| <include> | ||
| <directory>src</directory> | ||
| </include> | ||
| </source> | ||
|
|
||
| <testsuites> | ||
| <testsuite name="Tests"> | ||
| <directory suffix="Test.php">tests</directory> | ||
| </testsuite> | ||
| </testsuites> | ||
| </phpunit> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * ------------------------------------------------------------------------- | ||
| * DataInjection plugin for GLPI | ||
| * ------------------------------------------------------------------------- | ||
| * | ||
| * LICENSE | ||
| * | ||
| * This file is part of DataInjection. | ||
| * | ||
| * DataInjection is free software; you can redistribute it and/or modify | ||
| * it under the terms of the GNU General Public License as published by | ||
| * the Free Software Foundation; either version 2 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * DataInjection is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with DataInjection. If not, see <http://www.gnu.org/licenses/>. | ||
| * ------------------------------------------------------------------------- | ||
| * @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 | ||
| * ------------------------------------------------------------------------- | ||
| */ | ||
|
|
||
| $current_plugin_folder = basename(realpath(__DIR__ . '/../')); | ||
|
|
||
| require __DIR__ . '/../../../tests/bootstrap.php'; | ||
| require dirname(__DIR__) . '/vendor/autoload.php'; | ||
|
|
||
| if (!Plugin::isPluginActive($current_plugin_folder)) { | ||
| throw new RuntimeException("Plugin $current_plugin_folder is not active in the test database"); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.