fix(textfield): Unescaped HTML when displaying a form answer

pluginsGLPI · May 31, 2023 · 6ce71f9 · 6ce71f9
1 parent af40ec9
commit 6ce71f9
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 5 deletions.
diff --git a/inc/field/textfield.class.php b/inc/field/textfield.class.php
@@ -63,7 +63,7 @@ public function showForm(array $options): void {
 
    public function getRenderedHtml($domain, $canEdit = true): string {
       if (!$canEdit) {
-         return $this->value;
+         return Sanitizer::sanitize($this->value, false);
       }
 
       $html         = '';
@@ -108,7 +108,11 @@ public function getValueForDesign(): string {
    }
 
    public function getValueForTargetText($domain, $richText): ?string {
-      return Sanitizer::unsanitize($this->value);
+      if ($richText) {
+         return Sanitizer::sanitize($this->value, false);
+      }
+
+      return $this->value;
    }
 
    public function moveUploads() {

diff --git a/inc/formanswer.class.php b/inc/formanswer.class.php
@@ -1353,7 +1353,6 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
                }
             }
          }
-         // $content = str_replace('##answer_' . $questionId . '##', Sanitizer::sanitize($value ?? ''), $content);
          $content = str_replace('##answer_' . $questionId . '##', $value ?? '', $content);
 
          if ($this->questionFields[$questionId] instanceof DropdownField) {

diff --git a/tests/3-unit/GlpiPlugin/Formcreator/Field/TextField.php b/tests/3-unit/GlpiPlugin/Formcreator/Field/TextField.php
@@ -397,6 +397,12 @@ public function providerGetValueForTargetText() {
             'expected' => true,
             'expectedValue' => 'foo\\bar',
          ],
+         [
+            'question' => $this->getQuestion(),
+            'value' => '"><img src=x onerror="alert(1337)" x=x>',
+            'expected' => true,
+            'expectedValue' => '"><img src=x onerror="alert(1337)" x=x>',
+         ],
       ];
    }
 
@@ -420,5 +426,13 @@ public function testGetValueForTargetText($question, $value, $expected, $expecte
       }
    }
 
-
+   public function testGetRenderedHtml() {
+      // XSS check
+      $instance = $this->newTestedInstance($this->getQuestion());
+      $instance->deserializeValue('"><img src=x onerror="alert(1337)" x=x>');
+      $output = $instance->getRenderedHtml('no_domain', false);
+      $this->string($output)->isEqualTo('"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;');
+      $output = $instance->getRenderedHtml('no_domain', true);
+      $this->string($output)->contains('&quot;><img src=x onerror=&quot;alert(1337)&quot; x=x>');
+   }
 }
diff --git a/tests/3-unit/GlpiPlugin/Formcreator/Field/TextareaField.php b/tests/3-unit/GlpiPlugin/Formcreator/Field/TextareaField.php
@@ -313,4 +313,16 @@ public function testGetValueForApi($input, $expected) {
       $output = $instance->getValueForApi();
       $this->string($output)->isEqualTo($expected);
    }
+
+   public function testGetRenderedHtml() {
+      // XSS check
+      $formAnswer = new PluginFormcreatorFormAnswer();
+      $instance = $this->newTestedInstance($this->getQuestion());
+      $instance->setFormAnswer($formAnswer);
+      $instance->deserializeValue('"><img src=x onerror="alert(1337)" x=x>');
+      $output = $instance->getRenderedHtml('no_domain', false);
+      $this->string($output)->isEqualTo('"&gt;<img src="x" alt="image"  loading="lazy">');
+      $output = $instance->getRenderedHtml('no_domain', true);
+      $this->string($output)->contains('"><img src=x onerror="alert(1337)" x=x>');
+   }
 }
diff --git a/tests/src/CommonTargetTestCase.php b/tests/src/CommonTargetTestCase.php
@@ -2,9 +2,20 @@
 
 namespace GlpiPlugin\Formcreator\Tests;
 use PluginFormcreatorForm;
+use PluginFormcreatorFormAnswer;
+use PluginFormcreatorSection;
 
 abstract class CommonTargetTestCase extends CommonTestCase
 {
+   public function beforeTestMethod($method) {
+      parent::beforeTestMethod($method);
+      switch ($method) {
+         case 'testXSS':
+            $this->login('glpi', 'glpi');
+            break;
+      }
+   }
+
    /**
     * Test handling of uuid when adding an item
     */
@@ -56,4 +67,35 @@ public function testPrepareInputForUpdate_uuid() {
        $this->array($output)->HasKey('uuid');
        $this->string($output['uuid'])->isEqualTo('foo');
    }
-}
+
+   public function testXSS() {
+      $question = $this->getQuestion([
+         'fieldtype' => 'text',
+      ]);
+      $section = new PluginFormcreatorSection();
+      $section->update([
+            'id' => $question->fields['plugin_formcreator_sections_id'],
+            'name' => 'section',
+      ]);
+      $form = PluginFormcreatorForm::getByItem($question);
+      $testedClassName = $this->getTestedClassName();
+      $target = new $testedClassName();
+      $target->add([
+         'name' => $this->getUniqueString(),
+         'plugin_formcreator_forms_id' => $form->getID(),
+         'target_name' => '##answer_' . $question->getID() . '##',
+         'content'     => '##FULLFORM##',
+      ]);
+      $formAnswer = new PluginFormcreatorFormAnswer();
+      $formAnswer->add([
+         'plugin_formcreator_forms_id' => $form->getID(),
+         'formcreator_field_' . $question->getID() => '"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;"',
+      ]);
+      $generated = $formAnswer->targetList[0] ?? null;
+      $this->object($generated);
+      $this->string($generated->fields['name'])
+         ->isEqualTo('"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;"');
+      $this->string($generated->fields['content'])
+         ->isEqualTo('&#60;h1&#62;Form data&#60;/h1&#62;&#60;h2&#62;section&#60;/h2&#62;&#60;div&#62;&#60;b&#62;1) question : &#60;/b&#62;"&#38;#62;&#38;#60;img src=x onerror="alert(1337)" x=x&#38;#62;"&#60;/div&#62;');
+   }
+}