Skip to content

Commit

Permalink
FIX Avoid double escaping values when printing a gridfield
Browse files Browse the repository at this point in the history
  • Loading branch information
@GuySartorelli
GuySartorelli committed Feb 9, 2025
1 parent 8a2a7cc commit 5c2a259
Show file tree
Hide file tree
Showing 3 changed files with 155 additions and 6 deletions.
44 changes: 40 additions & 4 deletions src/Forms/GridField/GridFieldDataColumns.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
use InvalidArgumentException;
use LogicException;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\ORM\FieldType\DBHTMLVarchar;
use SilverStripe\View\ViewableData;

/**
Expand All @@ -31,6 +34,8 @@ class GridFieldDataColumns extends AbstractGridFieldComponent implements GridFie
*/
protected $displayFields = [];

private bool $escapeFields = true;

/**
* Modify the list of columns displayed in the table.
* See {@link GridFieldDataColumns->getDisplayFields()} and {@link GridFieldDataColumns}.
Expand Down Expand Up @@ -152,6 +157,28 @@ public function getFieldFormatting()
return $this->fieldFormatting;
}

/**
* Determines whether this component escapes strings returned from getColumnContent().
*
* This is useful because by default strings are escaped for use in HTML. This
* means there are some circumstances in which the escaping done here can result
* in double escaping those values further down the line, such as use with
* GridFieldPrintButton which temporarily sets this to false.
*/
public function setEscapeFields(bool $escape): static
{
$this->escapeFields = $escape;
return $this;
}

/**
* Get whether this component escapes strings returned from getColumnContent().
*/
public function getEscapeFields(): bool
{
return $this->escapeFields;
}

/**
* HTML for the column, content of the <td> element.
*
Expand Down Expand Up @@ -265,12 +292,21 @@ protected function castValue($gridField, $fieldName, $value)
// If the value is an object, we do one of two things
if (method_exists($value, 'Nice')) {
// If it has a "Nice" method, call that & make sure the result is safe
$value = nl2br(Convert::raw2xml($value->Nice()) ?? '');
$value = $value->Nice();
if ($this->getEscapeFields()) {
$value = nl2br(Convert::raw2xml($value));
}
} else {
// Otherwise call forTemplate - the result of this should already be safe
$value = $value->forTemplate();
$isHtml = is_a($value, DBHTMLText::class, false) || is_a($value, DBHTMLVarchar::class, false);
// For DBFields other than HTML variants, if we're not escaping values, get the raw value.
// Otherwise, check forTemplate() which is assumed to be safe.
if (!$isHtml && !$this->getEscapeFields() && is_a($value, DBField::class, false)) {
$value = $value->RAW();
} else {
$value = $value->forTemplate();
}
}
} else {
} elseif ($this->getEscapeFields()) {
// Otherwise, just treat as a text string & make sure the result is safe
$value = nl2br(Convert::raw2xml($value) ?? '');
}
Expand Down
15 changes: 13 additions & 2 deletions src/Forms/GridField/GridFieldPrintButton.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@

use LogicException;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Core\Convert;
use SilverStripe\Core\Extensible;
use SilverStripe\ORM\ArrayList;
use SilverStripe\ORM\FieldType\DBDatetime;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\ORM\FieldType\DBHTMLVarchar;
use SilverStripe\Security\Security;
use SilverStripe\View\ArrayData;
use SilverStripe\View\Requirements;
Expand Down Expand Up @@ -231,7 +231,11 @@ public function generatePrintData(GridField $gridField)
$items = $gridField->getManipulatedList();
$itemRows = new ArrayList();

// If there's a GridFieldDataColumns component, ensure it doesn't escape raw strings
// as that would result in double escaping when we render out the print template.
$gridFieldColumnsComponent = $gridField->getConfig()->getComponentByType(GridFieldDataColumns::class);
$origEscape = $gridFieldColumnsComponent?->getEscapeFields();
$gridFieldColumnsComponent?->setEscapeFields(false);

/** @var ViewableData $item */
foreach ($items->limit(null) as $item) {
Expand All @@ -244,8 +248,13 @@ public function generatePrintData(GridField $gridField)
? strip_tags($gridFieldColumnsComponent->getColumnContent($gridField, $item, $field))
: $gridField->getDataFieldValue($item, $field);

// The value is used in a template, so to prevent XSS attacks we can't allow an HTML field here.
// Getting the raw string here means it will end up being default-casted to DBText which is safe.
if (is_a($value, DBHTMLText::class, false) || is_a($value, DBHTMLVarchar::class, false)) {
$value = $value->__toString();
}
$itemRow->push(new ArrayData([
"CellString" => $value,
'CellString' => $value,
]));
}

Expand All @@ -258,6 +267,8 @@ public function generatePrintData(GridField $gridField)
}
}

$gridFieldColumnsComponent?->setEscapeFields($origEscape);

$ret = new ArrayData([
"Title" => $this->getTitle($gridField),
"Header" => $header,
Expand Down
102 changes: 102 additions & 0 deletions tests/php/Forms/GridField/GridFieldPrintButtonTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use ReflectionMethod;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Control\Controller;
use SilverStripe\Dev\CSSContentParser;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\Form;
use SilverStripe\Forms\GridField\GridFieldPrintButton;
Expand All @@ -15,6 +16,10 @@
use SilverStripe\Forms\GridField\GridFieldDataColumns;
use SilverStripe\Forms\Tests\GridField\GridFieldPrintButtonTest\TestObject;
use SilverStripe\ORM\ArrayList;
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\ORM\FieldType\DBHTMLVarchar;
use SilverStripe\ORM\FieldType\DBText;
use SilverStripe\View\ArrayData;

class GridFieldPrintButtonTest extends SapphireTest
Expand Down Expand Up @@ -93,6 +98,103 @@ public function testGeneratePrintData()
$this->assertSame($names, $foundNames);
}

public function provideHandlePrintEscaping(): array
{
$scenarios = [
'raw string pre-escaped' => [
'value' => 'before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;',
'useGridFieldDataColumns' => false,
'expected' => 'before&amp;lt;script&amp;gt;alert("hehehe");&amp;lt;/script&amp;gt;after&amp;amp;',
],
'raw string as HTML' => [
'value' => 'before<script>alert("hehehe");</script>after&amp;',
'useGridFieldDataColumns' => false,
'expected' => 'before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;amp;',
],
'DBText pre-escaped' => [
'value' => (new DBText('field'))->setValue('before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;'),
'useGridFieldDataColumns' => false,
'expected' => 'before&amp;lt;script&amp;gt;alert("hehehe");&amp;lt;/script&amp;gt;after&amp;amp;',
],
'DBText as HTML' => [
'value' => (new DBText('field'))->setValue('before<script>alert("hehehe");</script>after&amp;'),
'useGridFieldDataColumns' => false,
'expected' => 'before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;amp;',
],
'DBHTMLText pre-escaped' => [
'value' => (new DBHTMLText('field'))->setValue('before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;'),
'useGridFieldDataColumns' => false,
'expected' => 'before&amp;lt;script&amp;gt;alert("hehehe");&amp;lt;/script&amp;gt;after&amp;amp;',
],
'DBHTMLText as HTML' => [
'value' => (new DBHTMLText('field'))->setValue('before<script>alert("hehehe");</script>after&amp;'),
'useGridFieldDataColumns' => false,
'expected' => 'before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;amp;',
],
'DBHTMLVarchar pre-escaped' => [
'value' => (new DBHTMLVarchar('field'))->setValue('before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;'),
'useGridFieldDataColumns' => false,
'expected' => 'before&amp;lt;script&amp;gt;alert("hehehe");&amp;lt;/script&amp;gt;after&amp;amp;',
],
'DBHTMLVarchar as HTML' => [
'value' => (new DBHTMLVarchar('field'))->setValue('before<script>alert("hehehe");</script>after&amp;'),
'useGridFieldDataColumns' => false,
'expected' => 'before&lt;script&gt;alert("hehehe");&lt;/script&gt;after&amp;amp;',
],
];
// The GridFieldDataColumn changes the code path used to fetch the value
// so we need to test for that as well. The expected value should always
// be the same regardless of whether that component is added or not.
foreach ($scenarios as $name => $scenario) {
$scenario['useGridFieldDataColumns'] = true;
$scenarios[$name . ' with datacolumns'] = $scenario;
}
return $scenarios;
}

/**
* Explicitly tests that the following are both true:
* - XML entities are not double-escaped
* - XSS attack vectors are not introduced
*
* @dataProvider provideHandlePrintEscaping
*/
public function testHandlePrintEscaping(string|DBField $value, bool $useGridFieldDataColumns, string $expected): void
{
$component = new GridFieldPrintButton();
$component->getPrintColumns();

$list = new ArrayList([new ArrayData(['Name' => $value])]);

$button = new GridFieldPrintButton();
$button->setPrintColumns(['Name' => 'My Name']);

// Get paginated gridfield config
$config = GridFieldConfig::create()
->addComponent(new GridFieldPaginator(10))
->addComponent($button);
if ($useGridFieldDataColumns) {
// If this component is present, GridFieldPrintButton uses it to get the value,
// and that includes some transformation of the value including escaping.
// So we need to check both with and without the component to ensure both scenarios
// present sane results.
$columns = new GridFieldDataColumns();
$columns->setDisplayFields(['Name' => 'My Name']);
$config->addComponent($columns);
}
$gridField = new GridField('testfield', 'testfield', $list, $config);
new Form(Controller::curr(), 'Form', new FieldList($gridField), new FieldList());

// Printed data should ignore pagination limit
$result = $button->handlePrint($gridField);

$parser = new CSSContentParser($result->__toString());
$cellContent = $parser->getBySelector('td');

$this->assertCount(1, $cellContent);
$this->assertSame("<td>{$expected}</td>", $cellContent[0]->asXML());
}

public function testGetPrintColumnsForGridFieldThrowsException()
{
$component = new GridFieldPrintButton();
Expand Down

0 comments on commit 5c2a259

Please sign in to comment.