From 3e0666d4d63a539ec517f2004198bce061322bc3 Mon Sep 17 00:00:00 2001 From: Thierry Bugier Date: Tue, 10 Jan 2023 15:05:19 +0100 Subject: [PATCH] fix(formanswer, textfield, textareafield): escaping --- inc/abstracttarget.class.php | 3 +- inc/field/textfield.class.php | 5 + inc/fieldinterface.class.php | 2 +- inc/formanswer.class.php | 10 +- inc/targetchange.class.php | 1 - inc/targetproblem.class.php | 1 - inc/targetticket.class.php | 2 - .../Formcreator/Field/DatetimeField.php | 35 ++++++- .../Formcreator/Field/TextField.php | 91 +++++++++++++++++++ .../3-unit/PluginFormcreatorTargetTicket.php | 8 +- 10 files changed, 141 insertions(+), 17 deletions(-) diff --git a/inc/abstracttarget.class.php b/inc/abstracttarget.class.php index eab223bc5..c7962ac33 100644 --- 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(['

', '

'], ['
', '
'], $template); - $template = Sanitizer::sanitize($template); } return $template; diff --git a/inc/field/textfield.class.php b/inc/field/textfield.class.php index 713737032..aa28d6b38 100644 --- 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 index 1a63515ff..4197efb84 100644 --- 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 index e2e9fa4e8..d8072dace 100644 --- 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 index dc8f38cba..3fc4937ae 100644 --- 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 index c22e13e2f..3cadf536e 100644 --- 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 index b186370c6..d5f7ba2ed 100644 --- 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 index 9fe70dac6..1fd0c79a9 100644 --- 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 index 1f569d618..8f15350f3 100644 --- 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 index 1d60113de..476139c1f 100644 --- 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 => '<h1>Form data</h1>' - . '<h2>' . Toolbox::addslashes_deep($sectionName) . '</h2>' - . '<div><b>1) ' . $questionTag . ' : </b>' . $answerTag . '</div>', + 1 => '

Form data

' + . '

' . $sectionName . '

' + . '
1) ' . $questionTag . ' : ' . $answerTag . '
', ], ], ];