Skip to content

Commit

Permalink
fix(textfield): Unescaped HTML when displaying a form answer
Browse files Browse the repository at this point in the history
  • Loading branch information
btry committed May 31, 2023
1 parent af40ec9 commit 6ce71f9
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 5 deletions.
8 changes: 6 additions & 2 deletions inc/field/textfield.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
Expand Down Expand Up @@ -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() {
Expand Down
1 change: 0 additions & 1 deletion inc/formanswer.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
16 changes: 15 additions & 1 deletion tests/3-unit/GlpiPlugin/Formcreator/Field/TextField.php
Original file line number Diff line number Diff line change
Expand Up @@ -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>',
],
];
}

Expand All @@ -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>');
}
}
12 changes: 12 additions & 0 deletions tests/3-unit/GlpiPlugin/Formcreator/Field/TextareaField.php
Original file line number Diff line number Diff line change
Expand Up @@ -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>');
}
}
44 changes: 43 additions & 1 deletion tests/src/CommonTargetTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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;');
}
}

0 comments on commit 6ce71f9

Please sign in to comment.