From 6ce71f957d6908065edf79eeaa556fa0cc615aac Mon Sep 17 00:00:00 2001 From: Thierry Bugier Date: Fri, 7 Apr 2023 16:05:40 +0200 Subject: [PATCH] fix(textfield): Unescaped HTML when displaying a form answer --- inc/field/textfield.class.php | 8 +++- inc/formanswer.class.php | 1 - .../Formcreator/Field/TextField.php | 16 ++++++- .../Formcreator/Field/TextareaField.php | 12 +++++ tests/src/CommonTargetTestCase.php | 44 ++++++++++++++++++- 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/inc/field/textfield.class.php b/inc/field/textfield.class.php index 0b31a567f..07328d407 100644 --- 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 index 337508bd9..3bda2a636 100644 --- 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 index 8f15350f3..4a1c8c40d 100644 --- 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' => '">', + 'expected' => true, + 'expectedValue' => '">', + ], ]; } @@ -420,5 +426,13 @@ public function testGetValueForTargetText($question, $value, $expected, $expecte } } - + public function testGetRenderedHtml() { + // XSS check + $instance = $this->newTestedInstance($this->getQuestion()); + $instance->deserializeValue('">'); + $output = $instance->getRenderedHtml('no_domain', false); + $this->string($output)->isEqualTo('"><img src=x onerror="alert(1337)" x=x>'); + $output = $instance->getRenderedHtml('no_domain', true); + $this->string($output)->contains('">'); + } } diff --git a/tests/3-unit/GlpiPlugin/Formcreator/Field/TextareaField.php b/tests/3-unit/GlpiPlugin/Formcreator/Field/TextareaField.php index becf1d503..e4b57b6f3 100644 --- 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('">'); + $output = $instance->getRenderedHtml('no_domain', false); + $this->string($output)->isEqualTo('">image'); + $output = $instance->getRenderedHtml('no_domain', true); + $this->string($output)->contains('">'); + } } diff --git a/tests/src/CommonTargetTestCase.php b/tests/src/CommonTargetTestCase.php index 92ad348ab..5fc97fc28 100644 --- 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'); } -} \ No newline at end of file + + 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() => '"><img src=x onerror="alert(1337)" x=x>"', + ]); + $generated = $formAnswer->targetList[0] ?? null; + $this->object($generated); + $this->string($generated->fields['name']) + ->isEqualTo('"><img src=x onerror="alert(1337)" x=x>"'); + $this->string($generated->fields['content']) + ->isEqualTo('<h1>Form data</h1><h2>section</h2><div><b>1) question : </b>"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;"</div>'); + } +}