Skip to content

Commit 3e0666d

Browse files
btrybtry
committed
fix(formanswer, textfield, textareafield): escaping
1 parent f9bcfab commit 3e0666d

10 files changed

+141
-17
lines changed

‎inc/abstracttarget.class.php

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ public function post_updateItem($history = 1) {
190190
* @param string $template
191191
* @param PluginFormcreatorFormAnswer $formAnswer form answer to render
192192
* @param bool $richText Disable rich text output
193-
* @return string
193+
* @return string without sql or html escaping
194194
*/
195195
protected function prepareTemplate($template, PluginFormcreatorFormAnswer $formAnswer, $richText = false) {
196196
if (strpos($template, '##FULLFORM##') !== false) {
@@ -208,7 +208,6 @@ protected function prepareTemplate($template, PluginFormcreatorFormAnswer $formA
208208

209209
if ($richText) {
210210
$template = str_replace(['<p>', '</p>'], ['<div>', '</div>'], $template);
211-
$template = Sanitizer::sanitize($template);
212211
}
213212

214213
return $template;

‎inc/field/textfield.class.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,11 @@ public function parseAnswerValues($input, $nonDestructive = false): bool {
202202
return false;
203203
}
204204

205+
// input is sanitized (it comes from $_POST),
206+
// but unsanitize ignores pair count of consecutive backslashes
207+
// when nothing else must be unsanitized
208+
// We need to force it
209+
$this->value = stripslashes($input[$key]);
205210
$this->value = Sanitizer::unsanitize($input[$key]);
206211
return true;
207212
}

‎inc/fieldinterface.class.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ public function hasInput($input): bool;
156156

157157
/**
158158
* Read the value of the field from answers
159-
* @param array $input answers of all questions of the form
159+
* @param array $input answers of all questions of the form, sanitized
160160
* @param bool $nonDestructive for File field, ensure that the file uploads imported as document
161161
*
162162
* @return boolean true on sucess, false otherwise

‎inc/formanswer.class.php

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1268,7 +1268,7 @@ public function post_purgeItem() {
12681268
* @param string $content String to be parsed
12691269
* @param PluginFormcreatorTargetInterface $target Target for which output is being generated
12701270
* @param boolean $richText Disable rich text mode for field rendering
1271-
* @return string Parsed string with tags replaced by form values
1271+
* @return string Parsed string with tags replaced by form values. Not SQL nor HTML escaped
12721272
*/
12731273
public function parseTags(string $content, PluginFormcreatorTargetInterface $target = null, $richText = false): string {
12741274
// Prepare all fields of the form
@@ -1289,7 +1289,8 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
12891289
$value = $this->questionFields[$questionId]->getValueForTargetText($domain, $richText);
12901290
}
12911291

1292-
$content = str_replace('##question_' . $questionId . '##', Sanitizer::sanitize($name), $content);
1292+
// $content = str_replace('##question_' . $questionId . '##', Sanitizer::sanitize($name), $content);
1293+
$content = str_replace('##question_' . $questionId . '##', $name, $content);
12931294
if ($question->fields['fieldtype'] === 'file') {
12941295
if (strpos($content, '##answer_' . $questionId . '##') !== false) {
12951296
if ($target !== null && $target instanceof PluginFormcreatorAbstractItilTarget) {
@@ -1299,7 +1300,8 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
12991300
}
13001301
}
13011302
}
1302-
$content = str_replace('##answer_' . $questionId . '##', Sanitizer::sanitize($value ?? ''), $content);
1303+
// $content = str_replace('##answer_' . $questionId . '##', Sanitizer::sanitize($value ?? ''), $content);
1304+
$content = str_replace('##answer_' . $questionId . '##', $value ?? '', $content);
13031305

13041306
if ($this->questionFields[$questionId] instanceof DropdownField) {
13051307
$content = $this->questionFields[$questionId]->parseObjectProperties($field->getValueForDesign(), $content);
@@ -1310,6 +1312,8 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
13101312
// convert sanitization from old style GLPI ( up to 9.5 to modern style)
13111313
$content = Sanitizer::unsanitize($content);
13121314
$content = Sanitizer::sanitize($content);
1315+
} else {
1316+
$content = Sanitizer::sanitize($content);
13131317
}
13141318

13151319
$hook_data = Plugin::doHookFunction('formcreator_parse_extra_tags', [

‎inc/targetchange.class.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -654,7 +654,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
654654
$formanswer,
655655
true
656656
);
657-
$data['name'] = Toolbox::addslashes_deep($data['name']);
658657
$data['name'] = $formanswer->parseTags($data['name'], $this);
659658

660659
$changeFields = [

‎inc/targetproblem.class.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
175175
$formanswer,
176176
true
177177
);
178-
$data['name'] = Toolbox::addslashes_deep($data['name']);
179178
$data['name'] = $formanswer->parseTags($data['name'], $this);
180179

181180
$problemFields = [

‎inc/targetticket.class.php

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -820,7 +820,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
820820
$formanswer,
821821
false
822822
);
823-
$data['name'] = Toolbox::addslashes_deep($data['name']);
824823
$data['name'] = $formanswer->parseTags($data['name'], $this);
825824
$data['date'] = $_SESSION['glpi_currenttime'];
826825

@@ -830,7 +829,6 @@ public function save(PluginFormcreatorFormAnswer $formanswer): ?CommonDBTM {
830829
$richText
831830
);
832831

833-
// $data['content'] = Toolbox::addslashes_deep($data['content']);
834832
$data['content'] = $formanswer->parseTags($data['content'], $this, $richText);
835833

836834
$data['_tickettemplates_id'] = $this->fields['tickettemplates_id'];

‎tests/3-unit/GlpiPlugin/Formcreator/Field/DatetimeField.php

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,12 +144,41 @@ public function testParseAnswerValues($question, $value, $expected, $expectedVal
144144
'formcreator_field_' . $question->getID() => $value
145145
]);
146146
$this->boolean($output)->isEqualTo($expected);
147+
}
148+
149+
public function providerGetValueForTargetText() {
150+
return [
151+
[
152+
'question' => $this->getQuestion(),
153+
'value' => '',
154+
'expected' => true,
155+
'expectedValue' => ' ',
156+
],
157+
[
158+
'question' => $this->getQuestion(),
159+
'value' => '2018-12-25 23:00:00',
160+
'expected' => true,
161+
'expectedValue' => '2018-12-25 23:00',
162+
],
163+
];
164+
}
165+
166+
/**
167+
* @dataProvider providerGetValueForTargetText
168+
*
169+
* @return void
170+
*/
171+
public function testGetValueForTargetText($question, $value, $expected, $expectedValue) {
172+
$instance = $this->newTestedInstance($question);
173+
$output = $instance->parseAnswerValues([
174+
'formcreator_field_' . $question->getID() => $value
175+
]);
147176

148-
$outputValue = $instance->getValueForTargetText('', false);
177+
$output = $instance->getValueForTargetText('', false);
149178
if ($expected === false) {
150-
$this->variable($outputValue)->isNull();
179+
$this->variable($output)->isNull();
151180
} else {
152-
$this->string($outputValue)
181+
$this->string($output)
153182
->isEqualTo($expectedValue);
154183
}
155184
}

‎tests/3-unit/GlpiPlugin/Formcreator/Field/TextField.php

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,4 +330,95 @@ public function testGetValueForApi($input, $expected) {
330330
$output = $instance->getValueForApi();
331331
$this->string($output)->isEqualTo($expected);
332332
}
333+
334+
public function providerParseAnswerValues() {
335+
return [
336+
[
337+
'question' => $this->getQuestion(),
338+
'value' => '',
339+
'expected' => true,
340+
'expectedValue' => '',
341+
],
342+
[
343+
'question' => $this->getQuestion(),
344+
'value' => 'foo',
345+
'expected' => true,
346+
'expectedValue' => 'foo',
347+
],
348+
[
349+
'question' => $this->getQuestion(),
350+
'value' => 'foo\\bar',
351+
'expected' => true,
352+
'expectedValue' => 'foo\\bar',
353+
],
354+
[
355+
'question' => $this->getQuestion(),
356+
'value' => 'foo\\\\bar',
357+
'expected' => true,
358+
'expectedValue' => 'foo\\bar',
359+
],
360+
];
361+
}
362+
363+
/**
364+
* @dataProvider providerParseAnswerValues
365+
*/
366+
public function testParseAnswerValues($question, $value, $expected, $expectedValue) {
367+
$instance = $this->newTestedInstance($question);
368+
$output = $instance->parseAnswerValues([
369+
'formcreator_field_' . $question->getID() => $value
370+
]);
371+
$this->boolean($output)->isEqualTo($expected);
372+
}
373+
374+
public function providerGetValueForTargetText() {
375+
return [
376+
[
377+
'question' => $this->getQuestion(),
378+
'value' => '',
379+
'expected' => true,
380+
'expectedValue' => '',
381+
],
382+
[
383+
'question' => $this->getQuestion(),
384+
'value' => 'foo',
385+
'expected' => true,
386+
'expectedValue' => 'foo',
387+
],
388+
[
389+
'question' => $this->getQuestion(),
390+
'value' => 'foo\\bar',
391+
'expected' => true,
392+
'expectedValue' => 'foo\\bar',
393+
],
394+
[
395+
'question' => $this->getQuestion(),
396+
'value' => 'foo\\\\bar',
397+
'expected' => true,
398+
'expectedValue' => 'foo\\bar',
399+
],
400+
];
401+
}
402+
403+
/**
404+
* @dataProvider providerGetValueForTargetText
405+
*
406+
* @return void
407+
*/
408+
public function testGetValueForTargetText($question, $value, $expected, $expectedValue) {
409+
$instance = $this->newTestedInstance($question);
410+
$output = $instance->parseAnswerValues([
411+
'formcreator_field_' . $question->getID() => $value
412+
]);
413+
414+
$output = $instance->getValueForTargetText('', false);
415+
if ($expected === false) {
416+
$this->variable($output)->isNull();
417+
} else {
418+
$this->string($output)
419+
->isEqualTo($expectedValue);
420+
}
421+
}
422+
423+
333424
}

‎tests/3-unit/PluginFormcreatorTargetTicket.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -636,12 +636,12 @@ public function providerPrepareTemplate() {
636636
0 => 'Form data' . $eolSimple
637637
. '=================' . $eolSimple
638638
. $eolSimple
639-
. $eolSimple . Toolbox::addslashes_deep($sectionName) . $eolSimple
639+
. $eolSimple . $sectionName . $eolSimple
640640
. '---------------------------------' . $eolSimple
641641
. '1) ' . $questionTag . ' : ' . $answerTag . $eolSimple . $eolSimple,
642-
1 => '&#60;h1&#62;Form data&#60;/h1&#62;'
643-
. '&#60;h2&#62;' . Toolbox::addslashes_deep($sectionName) . '&#60;/h2&#62;'
644-
. '&#60;div&#62;&#60;b&#62;1) ' . $questionTag . ' : &#60;/b&#62;' . $answerTag . '&#60;/div&#62;',
642+
1 => '<h1>Form data</h1>'
643+
. '<h2>' . $sectionName . '</h2>'
644+
. '<div><b>1) ' . $questionTag . ' : </b>' . $answerTag . '</div>',
645645
],
646646
],
647647
];

0 commit comments

Comments
 (0)