Skip to content

Commit 6ce71f9

Browse files
btrybtry
committed
fix(textfield): Unescaped HTML when displaying a form answer
1 parent af40ec9 commit 6ce71f9

File tree

5 files changed

+76
-5
lines changed

5 files changed

+76
-5
lines changed

‎inc/field/textfield.class.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public function showForm(array $options): void {
6363

6464
public function getRenderedHtml($domain, $canEdit = true): string {
6565
if (!$canEdit) {
66-
return $this->value;
66+
return Sanitizer::sanitize($this->value, false);
6767
}
6868

6969
$html = '';
@@ -108,7 +108,11 @@ public function getValueForDesign(): string {
108108
}
109109

110110
public function getValueForTargetText($domain, $richText): ?string {
111-
return Sanitizer::unsanitize($this->value);
111+
if ($richText) {
112+
return Sanitizer::sanitize($this->value, false);
113+
}
114+
115+
return $this->value;
112116
}
113117

114118
public function moveUploads() {

‎inc/formanswer.class.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1353,7 +1353,6 @@ public function parseTags(string $content, PluginFormcreatorTargetInterface $tar
13531353
}
13541354
}
13551355
}
1356-
// $content = str_replace('##answer_' . $questionId . '##', Sanitizer::sanitize($value ?? ''), $content);
13571356
$content = str_replace('##answer_' . $questionId . '##', $value ?? '', $content);
13581357

13591358
if ($this->questionFields[$questionId] instanceof DropdownField) {

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,12 @@ public function providerGetValueForTargetText() {
397397
'expected' => true,
398398
'expectedValue' => 'foo\\bar',
399399
],
400+
[
401+
'question' => $this->getQuestion(),
402+
'value' => '"><img src=x onerror="alert(1337)" x=x>',
403+
'expected' => true,
404+
'expectedValue' => '"><img src=x onerror="alert(1337)" x=x>',
405+
],
400406
];
401407
}
402408

@@ -420,5 +426,13 @@ public function testGetValueForTargetText($question, $value, $expected, $expecte
420426
}
421427
}
422428

423-
429+
public function testGetRenderedHtml() {
430+
// XSS check
431+
$instance = $this->newTestedInstance($this->getQuestion());
432+
$instance->deserializeValue('"><img src=x onerror="alert(1337)" x=x>');
433+
$output = $instance->getRenderedHtml('no_domain', false);
434+
$this->string($output)->isEqualTo('"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;');
435+
$output = $instance->getRenderedHtml('no_domain', true);
436+
$this->string($output)->contains('&quot;><img src=x onerror=&quot;alert(1337)&quot; x=x>');
437+
}
424438
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,4 +313,16 @@ public function testGetValueForApi($input, $expected) {
313313
$output = $instance->getValueForApi();
314314
$this->string($output)->isEqualTo($expected);
315315
}
316+
317+
public function testGetRenderedHtml() {
318+
// XSS check
319+
$formAnswer = new PluginFormcreatorFormAnswer();
320+
$instance = $this->newTestedInstance($this->getQuestion());
321+
$instance->setFormAnswer($formAnswer);
322+
$instance->deserializeValue('"><img src=x onerror="alert(1337)" x=x>');
323+
$output = $instance->getRenderedHtml('no_domain', false);
324+
$this->string($output)->isEqualTo('"&gt;<img src="x" alt="image" loading="lazy">');
325+
$output = $instance->getRenderedHtml('no_domain', true);
326+
$this->string($output)->contains('"><img src=x onerror="alert(1337)" x=x>');
327+
}
316328
}

‎tests/src/CommonTargetTestCase.php‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,20 @@
22

33
namespace GlpiPlugin\Formcreator\Tests;
44
use PluginFormcreatorForm;
5+
use PluginFormcreatorFormAnswer;
6+
use PluginFormcreatorSection;
57

68
abstract class CommonTargetTestCase extends CommonTestCase
79
{
10+
public function beforeTestMethod($method) {
11+
parent::beforeTestMethod($method);
12+
switch ($method) {
13+
case 'testXSS':
14+
$this->login('glpi', 'glpi');
15+
break;
16+
}
17+
}
18+
819
/**
920
* Test handling of uuid when adding an item
1021
*/
@@ -56,4 +67,35 @@ public function testPrepareInputForUpdate_uuid() {
5667
$this->array($output)->HasKey('uuid');
5768
$this->string($output['uuid'])->isEqualTo('foo');
5869
}
59-
}
70+
71+
public function testXSS() {
72+
$question = $this->getQuestion([
73+
'fieldtype' => 'text',
74+
]);
75+
$section = new PluginFormcreatorSection();
76+
$section->update([
77+
'id' => $question->fields['plugin_formcreator_sections_id'],
78+
'name' => 'section',
79+
]);
80+
$form = PluginFormcreatorForm::getByItem($question);
81+
$testedClassName = $this->getTestedClassName();
82+
$target = new $testedClassName();
83+
$target->add([
84+
'name' => $this->getUniqueString(),
85+
'plugin_formcreator_forms_id' => $form->getID(),
86+
'target_name' => '##answer_' . $question->getID() . '##',
87+
'content' => '##FULLFORM##',
88+
]);
89+
$formAnswer = new PluginFormcreatorFormAnswer();
90+
$formAnswer->add([
91+
'plugin_formcreator_forms_id' => $form->getID(),
92+
'formcreator_field_' . $question->getID() => '"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;"',
93+
]);
94+
$generated = $formAnswer->targetList[0] ?? null;
95+
$this->object($generated);
96+
$this->string($generated->fields['name'])
97+
->isEqualTo('"&#62;&#60;img src=x onerror="alert(1337)" x=x&#62;"');
98+
$this->string($generated->fields['content'])
99+
->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;');
100+
}
101+
}

0 commit comments

Comments
 (0)