fix(formanswer, textfield, textareafield): escaping

pluginsGLPI · Jan 13, 2023 · 3e0666d · 3e0666d
1 parent f9bcfab
commit 3e0666d
Show file tree

Hide file tree

Showing 10 changed files with 141 additions and 17 deletions.
diff --git a/inc/abstracttarget.class.php b/inc/abstracttarget.class.php
@@ -190,7 +190,7 @@ public function post_updateItem($history = 1) {
     * @param string $template
     * @param PluginFormcreatorFormAnswer $formAnswer form answer to render
     * @param bool $richText Disable rich text output
-    * @return string
+    * @return string without sql or html escaping
     */
    protected function prepareTemplate($template, PluginFormcreatorFormAnswer $formAnswer, $richText = false) {
       if (strpos($template, '##FULLFORM##') !== false) {
@@ -208,7 +208,6 @@ protected function prepareTemplate($template, PluginFormcreatorFormAnswer $formA
 
       if ($richText) {
          $template = str_replace(['<p>', '</p>'], ['<div>', '</div>'], $template);
-         $template = Sanitizer::sanitize($template);
       }
 
       return $template;

diff --git a/inc/field/textfield.class.php b/inc/field/textfield.class.php
@@ -202,6 +202,11 @@ public function parseAnswerValues($input, $nonDestructive = false): bool {
          return false;
       }
 
+      // input is sanitized (it comes from $_POST),
+      // but unsanitize ignores pair count of consecutive backslashes
+      // when nothing else must be unsanitized
+      // We need to force it
+      $this->value = stripslashes($input[$key]);
       $this->value = Sanitizer::unsanitize($input[$key]);
       return true;
    }

diff --git a/inc/fieldinterface.class.php b/inc/fieldinterface.class.php
@@ -156,7 +156,7 @@ public function hasInput($input): bool;
 
    /**
     * Read the value of the field from answers
-    * @param array $input answers of all questions of the form
+    * @param array $input answers of all questions of the form, sanitized
     * @param bool $nonDestructive for File field, ensure that the file uploads imported as document
     *
     * @return boolean true on sucess, false otherwise

diff --git a/inc/formanswer.class.php b/inc/formanswer.class.php
@@ -1268,7 +1268,7 @@ public function post_purgeItem() {
     * @param  string $content                            String to be parsed
     * @param  PluginFormcreatorTargetInterface $target   Target for which output is being generated
     * @param  boolean $richText                          Disable rich text mode for field rendering
-    * @return string                                     Parsed string with tags replaced by form values
+    * @return string                                     Parsed string with tags replaced by form values. Not SQL nor HTML escaped
     */
    public function parseTags(string $content, PluginFormcreatorTargetInterface $target = null, $richText = false): string {
       // Prepare all fields of the form
@@ -1289,7 +1289,8 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
             $value = $this->questionFields[$questionId]->getValueForTargetText($domain, $richText);
          }
 
-         $content = str_replace('##question_' . $questionId . '##', Sanitizer::sanitize($name), $content);
+         // $content = str_replace('##question_' . $questionId . '##', Sanitizer::sanitize($name), $content);
+         $content = str_replace('##question_' . $questionId . '##', $name, $content);
          if ($question->fields['fieldtype'] === 'file') {
             if (strpos($content, '##answer_' . $questionId . '##') !== false) {
                if ($target !== null && $target instanceof PluginFormcreatorAbstractItilTarget) {
@@ -1299,7 +1300,8 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
                }
             }
          }
-         $content = str_replace('##answer_' . $questionId . '##', Sanitizer::sanitize($value ?? ''), $content);
+         // $content = str_replace('##answer_' . $questionId . '##', Sanitizer::sanitize($value ?? ''), $content);
+         $content = str_replace('##answer_' . $questionId . '##', $value ?? '', $content);
 
          if ($this->questionFields[$questionId] instanceof DropdownField) {
             $content = $this->questionFields[$questionId]->parseObjectProperties($field->getValueForDesign(), $content);
@@ -1310,6 +1312,8 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
          // convert sanitization from old style GLPI ( up to 9.5 to modern style)
          $content = Sanitizer::unsanitize($content);
          $content = Sanitizer::sanitize($content);
+      } else {
+         $content = Sanitizer::sanitize($content);
       }
 
       $hook_data = Plugin::doHookFunction('formcreator_parse_extra_tags', [

diff --git a/inc/targetchange.class.php b/inc/targetchange.class.php
@@ -654,7 +654,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
          $formanswer,
          true
       );
-      $data['name'] = Toolbox::addslashes_deep($data['name']);
       $data['name'] = $formanswer->parseTags($data['name'], $this);
 
       $changeFields = [

diff --git a/inc/targetproblem.class.php b/inc/targetproblem.class.php
@@ -175,7 +175,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
          $formanswer,
          true
       );
-      $data['name'] = Toolbox::addslashes_deep($data['name']);
       $data['name'] = $formanswer->parseTags($data['name'], $this);
 
       $problemFields = [

diff --git a/inc/targetticket.class.php b/inc/targetticket.class.php
@@ -820,7 +820,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
          $formanswer,
          false
       );
-      $data['name'] = Toolbox::addslashes_deep($data['name']);
       $data['name'] = $formanswer->parseTags($data['name'], $this);
       $data['date'] = $_SESSION['glpi_currenttime'];
 
@@ -830,7 +829,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
          $richText
       );
 
-      // $data['content'] = Toolbox::addslashes_deep($data['content']);
       $data['content'] = $formanswer->parseTags($data['content'], $this, $richText);
 
       $data['_tickettemplates_id'] = $this->fields['tickettemplates_id'];

diff --git a/tests/3-unit/GlpiPlugin/Formcreator/Field/DatetimeField.php b/tests/3-unit/GlpiPlugin/Formcreator/Field/DatetimeField.php
@@ -144,12 +144,41 @@ public function testParseAnswerValues($question, $value, $expected, $expectedVal
          'formcreator_field_' . $question->getID() => $value
       ]);
       $this->boolean($output)->isEqualTo($expected);
+   }
+
+   public function providerGetValueForTargetText() {
+      return [
+         [
+            'question' => $this->getQuestion(),
+            'value' => '',
+            'expected' => true,
+            'expectedValue' => ' ',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => '2018-12-25 23:00:00',
+            'expected' => true,
+            'expectedValue' => '2018-12-25 23:00',
+         ],
+      ];
+   }
+
+   /**
+    * @dataProvider providerGetValueForTargetText
+    *
+    * @return void
+    */
+   public function testGetValueForTargetText($question, $value, $expected, $expectedValue) {
+      $instance = $this->newTestedInstance($question);
+      $output = $instance->parseAnswerValues([
+         'formcreator_field_' . $question->getID() => $value
+      ]);
 
-      $outputValue = $instance->getValueForTargetText('', false);
+      $output = $instance->getValueForTargetText('', false);
       if ($expected === false) {
-         $this->variable($outputValue)->isNull();
+         $this->variable($output)->isNull();
       } else {
-         $this->string($outputValue)
+         $this->string($output)
             ->isEqualTo($expectedValue);
       }
    }

diff --git a/tests/3-unit/GlpiPlugin/Formcreator/Field/TextField.php b/tests/3-unit/GlpiPlugin/Formcreator/Field/TextField.php
@@ -330,4 +330,95 @@ public function testGetValueForApi($input, $expected) {
       $output = $instance->getValueForApi();
       $this->string($output)->isEqualTo($expected);
    }
+
+   public function providerParseAnswerValues() {
+      return [
+         [
+            'question' => $this->getQuestion(),
+            'value' => '',
+            'expected' => true,
+            'expectedValue' => '',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => 'foo',
+            'expected' => true,
+            'expectedValue' => 'foo',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => 'foo\\bar',
+            'expected' => true,
+            'expectedValue' => 'foo\\bar',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => 'foo\\\\bar',
+            'expected' => true,
+            'expectedValue' => 'foo\\bar',
+         ],
+      ];
+   }
+
+   /**
+    * @dataProvider providerParseAnswerValues
+    */
+   public function testParseAnswerValues($question, $value, $expected, $expectedValue) {
+      $instance = $this->newTestedInstance($question);
+      $output = $instance->parseAnswerValues([
+         'formcreator_field_' . $question->getID() => $value
+      ]);
+      $this->boolean($output)->isEqualTo($expected);
+   }
+
+   public function providerGetValueForTargetText() {
+      return [
+         [
+            'question' => $this->getQuestion(),
+            'value' => '',
+            'expected' => true,
+            'expectedValue' => '',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => 'foo',
+            'expected' => true,
+            'expectedValue' => 'foo',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => 'foo\\bar',
+            'expected' => true,
+            'expectedValue' => 'foo\\bar',
+         ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => 'foo\\\\bar',
+            'expected' => true,
+            'expectedValue' => 'foo\\bar',
+         ],
+      ];
+   }
+
+   /**
+    * @dataProvider providerGetValueForTargetText
+    *
+    * @return void
+    */
+   public function testGetValueForTargetText($question, $value, $expected, $expectedValue) {
+      $instance = $this->newTestedInstance($question);
+      $output = $instance->parseAnswerValues([
+         'formcreator_field_' . $question->getID() => $value
+      ]);
+
+      $output = $instance->getValueForTargetText('', false);
+      if ($expected === false) {
+         $this->variable($output)->isNull();
+      } else {
+         $this->string($output)
+            ->isEqualTo($expectedValue);
+      }
+   }
+
+
 }
diff --git a/tests/3-unit/PluginFormcreatorTargetTicket.php b/tests/3-unit/PluginFormcreatorTargetTicket.php
@@ -636,12 +636,12 @@ public function providerPrepareTemplate() {
                0 => 'Form data' . $eolSimple
                   . '=================' . $eolSimple
                   . $eolSimple
-                  . $eolSimple . Toolbox::addslashes_deep($sectionName) . $eolSimple
+                  . $eolSimple . $sectionName . $eolSimple
                   . '---------------------------------' . $eolSimple
                   . '1) ' . $questionTag . ' : ' . $answerTag . $eolSimple . $eolSimple,
-               1 => '&#60;h1&#62;Form data&#60;/h1&#62;'
-                  . '&#60;h2&#62;' . Toolbox::addslashes_deep($sectionName) . '&#60;/h2&#62;'
-                  . '&#60;div&#62;&#60;b&#62;1) ' . $questionTag . ' : &#60;/b&#62;' . $answerTag . '&#60;/div&#62;',
+               1 => '<h1>Form data</h1>'
+                  . '<h2>' . $sectionName . '</h2>'
+                  . '<div><b>1) ' . $questionTag . ' : </b>' . $answerTag . '</div>',
             ],
          ],
       ];