fix/ escape SQL values#627
Conversation
There was a problem hiding this comment.
It would be cleaner to refactor the code to replace doQuery() with the query builder.
$result = $DB->request($criteria);
| ), | ||
| ]; | ||
| } | ||
| unset($_FILES['filename']); |
There was a problem hiding this comment.
Why ?
I'm not sure if this is directly related to the topic of this PR?!
There was a problem hiding this comment.
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.
| //Add additional parameters specific to this itemtype (or function checkPresent exists) | ||
| if (method_exists($injectionClass, 'checkPresent')) { | ||
| $where .= $injectionClass->checkPresent($this->values, $options); | ||
| $extra = trim((string) $injectionClass->checkPresent($this->values, $options)); |
There was a problem hiding this comment.
The three implementations of checkPresent() (softwareversion, softwarelicense, networkport) still return raw SQL with unescaped name values.
QueryExpression continues to receive potentially dangerous values.
These methods should either be updated to return an array of criteria, or at the very least, $DB->escape() should be added to the string fields.
| $extra = $injectionClass->checkPresent($this->values, $options); | ||
| if (is_array($extra) && count($extra) > 0) { | ||
| $where = array_merge($where, $extra); | ||
| } |
There was a problem hiding this comment.
An exception should be thrown or a warning logged if $extra is not an array and is not empty
There was a problem hiding this comment.
The tests are not being run by the CI because the configuration is missing:
diff --git a/.gitignore b/.gitignore
index f033123..508e8ab 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,3 +3,5 @@ vendor/
.gh_token
*.min.*
+
+var/phpunit
diff --git a/phpunit.xml b/phpunit.xml
new file mode 100644
index 0000000..b827cdf
--- /dev/null
+++ b/phpunit.xml
@@ -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>
diff --git a/tests/bootstrap.php b/tests/bootstrap.php
new file mode 100644
index 0000000..7438cd6
--- /dev/null
+++ b/tests/bootstrap.php
@@ -0,0 +1,10 @@
+<?php
+
+$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");
+}
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
3 participants
Checklist before requesting a review
Please delete options that are not relevant.
Description