From 61a384d04efde5e10124f356017fc6cb972835c9 Mon Sep 17 00:00:00 2001
From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>
Date: Wed, 12 Feb 2025 11:36:24 +1300
Subject: [PATCH] FIX Avoid double escaping values when printing a gridfield
(#11598)
---
src/Forms/GridField/GridFieldDataColumns.php | 47 +++++-
src/Forms/GridField/GridFieldPrintButton.php | 15 +-
.../GridField/GridFieldPrintButtonTest.php | 139 ++++++++++++++++++
3 files changed, 195 insertions(+), 6 deletions(-)
diff --git a/src/Forms/GridField/GridFieldDataColumns.php b/src/Forms/GridField/GridFieldDataColumns.php
index 1bcb2ea4a93..7edb3488f57 100644
--- a/src/Forms/GridField/GridFieldDataColumns.php
+++ b/src/Forms/GridField/GridFieldDataColumns.php
@@ -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;
/**
@@ -31,6 +34,8 @@ class GridFieldDataColumns extends AbstractGridFieldComponent implements GridFie
*/
protected $displayFields = [];
+ private bool $doEscapeFields = true;
+
/**
* Modify the list of columns displayed in the table.
* See {@link GridFieldDataColumns->getDisplayFields()} and {@link GridFieldDataColumns}.
@@ -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 setDoEscapeFields(bool $doEscapeFields): static
+ {
+ $this->doEscapeFields = $doEscapeFields;
+ return $this;
+ }
+
+ /**
+ * Get whether this component escapes strings returned from getColumnContent().
+ */
+ public function getDoEscapeFields(): bool
+ {
+ return $this->doEscapeFields;
+ }
+
/**
* HTML for the column, content of the | element.
*
@@ -265,12 +292,24 @@ 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->getDoEscapeFields()) {
+ $value = nl2br(Convert::raw2xml($value));
+ }
} else {
- // Otherwise call forTemplate - the result of this should already be safe
- $value = $value->forTemplate();
+ if (!$this->getDoEscapeFields()
+ && is_a($value, DBField::class, false)
+ && !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.
+ $value = $value->RAW();
+ } else {
+ // Otherwise, check forTemplate() which is assumed to be safe.
+ $value = $value->forTemplate();
+ }
}
- } else {
+ } elseif ($this->getDoEscapeFields()) {
// Otherwise, just treat as a text string & make sure the result is safe
$value = nl2br(Convert::raw2xml($value) ?? '');
}
diff --git a/src/Forms/GridField/GridFieldPrintButton.php b/src/Forms/GridField/GridFieldPrintButton.php
index 96a47508859..d56e621f879 100644
--- a/src/Forms/GridField/GridFieldPrintButton.php
+++ b/src/Forms/GridField/GridFieldPrintButton.php
@@ -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;
@@ -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);
+ $origDoEscapeFields = $gridFieldColumnsComponent?->getDoEscapeFields();
+ $gridFieldColumnsComponent?->setDoEscapeFields(false);
/** @var ViewableData $item */
foreach ($items->limit(null) as $item) {
@@ -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,
]));
}
@@ -258,6 +267,8 @@ public function generatePrintData(GridField $gridField)
}
}
+ $gridFieldColumnsComponent?->setDoEscapeFields($origDoEscapeFields);
+
$ret = new ArrayData([
"Title" => $this->getTitle($gridField),
"Header" => $header,
diff --git a/tests/php/Forms/GridField/GridFieldPrintButtonTest.php b/tests/php/Forms/GridField/GridFieldPrintButtonTest.php
index 8b9b2f61d2b..e0369eb2f36 100644
--- a/tests/php/Forms/GridField/GridFieldPrintButtonTest.php
+++ b/tests/php/Forms/GridField/GridFieldPrintButtonTest.php
@@ -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;
@@ -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
@@ -93,6 +98,140 @@ public function testGeneratePrintData()
$this->assertSame($names, $foundNames);
}
+ public function provideHandlePrintEscaping(): array
+ {
+ return [
+ // Without data columns component
+ 'raw string pre-escaped' => [
+ 'value' => 'before<script>alert("hehehe");</script>after&',
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'raw string as HTML' => [
+ 'value' => 'beforeafter&',
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBText pre-escaped' => [
+ 'value' => (new DBText('field'))->setValue('before<script>alert("hehehe");</script>after&'),
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBText as HTML' => [
+ 'value' => (new DBText('field'))->setValue('beforeafter&'),
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBHTMLText pre-escaped' => [
+ 'value' => (new DBHTMLText('field'))->setValue('before<script>alert("hehehe");</script>after&'),
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBHTMLText as HTML' => [
+ 'value' => (new DBHTMLText('field'))->setValue('beforeafter&'),
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBHTMLVarchar pre-escaped' => [
+ 'value' => (new DBHTMLVarchar('field'))->setValue('before<script>alert("hehehe");</script>after&'),
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBHTMLVarchar as HTML' => [
+ 'value' => (new DBHTMLVarchar('field'))->setValue('beforeafter&'),
+ 'useGridFieldDataColumns' => false,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ // With data columns component
+ 'raw string pre-escaped with datacolumns' => [
+ 'value' => 'before<script>alert("hehehe");</script>after&',
+ 'useGridFieldDataColumns' => true,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'raw string pre-escaped with datacolumns' => [
+ 'value' => 'beforeafter&',
+ 'useGridFieldDataColumns' => true,
+ 'expected' => 'beforealert("hehehe");after&',
+ ],
+ 'DBText pre-escaped with datacolumns' => [
+ 'value' => (new DBText('field'))->setValue('before<script>alert("hehehe");</script>after&'),
+ 'useGridFieldDataColumns' => true,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBText as HTML with datacolumns' => [
+ 'value' => (new DBText('field'))->setValue('beforeafter&'),
+ 'useGridFieldDataColumns' => true,
+ // Note stripped tags here
+ 'expected' => 'beforealert("hehehe");after&',
+ ],
+ 'DBHTMLText pre-escaped with datacolumns' => [
+ 'value' => (new DBHTMLText('field'))->setValue('before<script>alert("hehehe");</script>after&'),
+ 'useGridFieldDataColumns' => true,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBHTMLText as HTML with datacolumns' => [
+ 'value' => (new DBHTMLText('field'))->setValue('beforeafter&'),
+ 'useGridFieldDataColumns' => true,
+ // Note stripped tags here
+ 'expected' => 'beforealert("hehehe");after&',
+ ],
+ 'DBHTMLVarchar pre-escaped with datacolumns' => [
+ 'value' => (new DBHTMLVarchar('field'))->setValue('before<script>alert("hehehe");</script>after&'),
+ 'useGridFieldDataColumns' => true,
+ 'expected' => 'before<script>alert("hehehe");</script>after&',
+ ],
+ 'DBHTMLVarchar as HTML with datacolumns' => [
+ 'value' => (new DBHTMLVarchar('field'))->setValue('beforeafter&'),
+ 'useGridFieldDataColumns' => true,
+ // Note stripped tags here
+ 'expected' => 'beforealert("hehehe");after&',
+ ],
+ ];
+ }
+
+ /**
+ * 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(" | {$expected} | ", $cellContent[0]->asXML());
+ }
+
public function testGetPrintColumnsForGridFieldThrowsException()
{
$component = new GridFieldPrintButton();