diff --git a/src/Control/HTTPResponse.php b/src/Control/HTTPResponse.php
index 15eb80520..5f7dc5b4b 100644
--- a/src/Control/HTTPResponse.php
+++ b/src/Control/HTTPResponse.php
@@ -267,7 +267,7 @@ class HTTPResponse
public function addHeader($header, $value)
{
$header = strtolower($header ?? '');
- $this->headers[$header] = $value;
+ $this->headers[$header] = $this->sanitiseHeader($value);
return $this;
}
@@ -310,6 +310,14 @@ class HTTPResponse
return $this;
}
+ /**
+ * Sanitise header values to avoid possible XSS vectors
+ */
+ private function sanitiseHeader(string $value): string
+ {
+ return preg_replace('/\v/', '', $value);
+ }
+
/**
* @param string $dest
* @param int $code
diff --git a/src/Forms/GridField/GridFieldSortableHeader.php b/src/Forms/GridField/GridFieldSortableHeader.php
index e6943f348..8a72b30dc 100644
--- a/src/Forms/GridField/GridFieldSortableHeader.php
+++ b/src/Forms/GridField/GridFieldSortableHeader.php
@@ -11,6 +11,7 @@ use SilverStripe\ORM\DataObject;
use SilverStripe\View\ArrayData;
use SilverStripe\View\SSViewer;
use LogicException;
+use SilverStripe\Core\Injector\Injector;
/**
* GridFieldSortableHeader adds column headers to a {@link GridField} that can
@@ -271,6 +272,16 @@ class GridFieldSortableHeader extends AbstractGridFieldComponent implements Grid
return $dataList;
}
+ // Prevent SQL Injection by validating that SortColumn exists
+ /** @var GridFieldDataColumns $columns */
+ $columns = $gridField->getConfig()->getComponentByType(GridFieldDataColumns::class);
+ $fields = $columns->getDisplayFields($gridField);
+ if (!array_key_exists($state->SortColumn, $fields) &&
+ !in_array($state->SortColumn, $this->getFieldSorting())
+ ) {
+ throw new LogicException('Invalid SortColumn: ' . $state->SortColumn);
+ }
+
return $dataList->sort($state->SortColumn, $state->SortDirection('asc'));
}
diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php
index 09ff3e8a4..a075d98fa 100644
--- a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php
+++ b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php
@@ -347,9 +347,9 @@ class HTMLEditorSanitiser
}
// Matches "javascript:" with any arbitrary linebreaks inbetween the characters.
- $regex = '/^\s*' . implode('\v*', str_split('javascript:')) . '/';
+ $regex = '/^\s*' . implode('\s*', str_split('javascript:')) . '/i';
// Strip out javascript execution in href or src attributes.
- foreach (['src', 'href'] as $dangerAttribute) {
+ foreach (['src', 'href', 'data'] as $dangerAttribute) {
if ($el->hasAttribute($dangerAttribute)) {
if (preg_match($regex, $el->getAttribute($dangerAttribute))) {
$el->removeAttribute($dangerAttribute);
diff --git a/src/ORM/RelatedData/StandardRelatedDataService.php b/src/ORM/RelatedData/StandardRelatedDataService.php
index 2c3eab624..04501fc23 100644
--- a/src/ORM/RelatedData/StandardRelatedDataService.php
+++ b/src/ORM/RelatedData/StandardRelatedDataService.php
@@ -162,7 +162,7 @@ class StandardRelatedDataService implements RelatedDataService
$tableName = $this->dataObjectSchema->tableName($candidateClass);
break;
}
- $candidateClass = get_parent_class($class ?? '');
+ $candidateClass = get_parent_class($candidateClass ?? '');
}
return $tableName;
}
diff --git a/src/View/Shortcodes/EmbedShortcodeProvider.php b/src/View/Shortcodes/EmbedShortcodeProvider.php
index 7b844ba60..18744f6b7 100644
--- a/src/View/Shortcodes/EmbedShortcodeProvider.php
+++ b/src/View/Shortcodes/EmbedShortcodeProvider.php
@@ -16,6 +16,7 @@ use SilverStripe\View\HTML;
use SilverStripe\View\Parsers\ShortcodeHandler;
use SilverStripe\View\Parsers\ShortcodeParser;
use SilverStripe\Control\Director;
+use SilverStripe\Core\Config\Configurable;
use SilverStripe\Dev\Deprecation;
use SilverStripe\View\Embed\EmbedContainer;
@@ -26,6 +27,23 @@ use SilverStripe\View\Embed\EmbedContainer;
*/
class EmbedShortcodeProvider implements ShortcodeHandler
{
+ use Configurable;
+
+ /**
+ * A whitelist of shortcode attributes which are allowed in the resultant markup.
+ * Note that the tinymce plugin restricts attributes on the client-side separately.
+ *
+ * @config
+ * @deprecated 4.12.0 Removed without equivalent functionality to replace it
+ */
+ private static array $attribute_whitelist = [
+ 'url',
+ 'thumbnail',
+ 'class',
+ 'width',
+ 'height',
+ 'caption',
+ ];
/**
* Gets the list of shortcodes provided by this handler
@@ -207,9 +225,17 @@ class EmbedShortcodeProvider implements ShortcodeHandler
}
}
+ $attributes = static::buildAttributeListFromArguments($arguments, ['width', 'height', 'url', 'caption']);
+ if (array_key_exists('style', $arguments)) {
+ $attributes->push(ArrayData::create([
+ 'Name' => 'style',
+ 'Value' => Convert::raw2att($arguments['style']),
+ ]));
+ }
+
$data = [
'Arguments' => $arguments,
- 'Attributes' => static::buildAttributeListFromArguments($arguments, ['width', 'height', 'url', 'caption']),
+ 'Attributes' => $attributes,
'Content' => DBField::create_field('HTMLFragment', $content)
];
@@ -263,6 +289,12 @@ class EmbedShortcodeProvider implements ShortcodeHandler
*/
private static function buildAttributeListFromArguments(array $arguments, array $exclude = []): ArrayList
{
+ // Clean out any empty arguments and anything not whitelisted
+ $whitelist = static::config()->get('attribute_whitelist');
+ $arguments = array_filter($arguments, function ($value, $key) use ($whitelist) {
+ return in_array($key, $whitelist) && strlen(trim($value ?? ''));
+ }, ARRAY_FILTER_USE_BOTH);
+
$attributes = ArrayList::create();
foreach ($arguments as $key => $value) {
if (in_array($key, $exclude ?? [])) {
diff --git a/tests/php/Control/HTTPResponseTest.php b/tests/php/Control/HTTPResponseTest.php
index 18469ec62..88c1aaeeb 100644
--- a/tests/php/Control/HTTPResponseTest.php
+++ b/tests/php/Control/HTTPResponseTest.php
@@ -45,6 +45,26 @@ class HTTPResponseTest extends SapphireTest
$this->assertEmpty($response->getHeader('X-Animal'));
}
+ public function providerSanitiseHeaders()
+ {
+ return [
+ 'plain text is retained' => ['some arbitrary value1', 'some arbitrary value1'],
+ 'special chars are retained' => ['`~!@#$%^&*()_+-=,./<>?;\':"[]{}\\|', '`~!@#$%^&*()_+-=,./<>?;\':"[]{}\\|'],
+ 'line breaks are removed' => ['no line breaks', "n\ro line \nbreaks\r\n"],
+ ];
+ }
+
+ /**
+ * @dataProvider providerSanitiseHeaders
+ */
+ public function testSanitiseHeaders(string $expected, string $value)
+ {
+ $response = new HTTPResponse();
+
+ $response->addHeader('X-Sanitised', $value);
+ $this->assertSame($expected, $response->getHeader('X-Sanitised'));
+ }
+
public function providerTestValidStatusCodes()
{
return [
diff --git a/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php b/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php
index b3c5cb8aa..0f4567cde 100644
--- a/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php
+++ b/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php
@@ -71,13 +71,14 @@ class GridFieldSortableHeaderTest extends SapphireTest
$list = Team::get()->filter([ 'ClassName' => Team::class ]);
$config = new GridFieldConfig_RecordEditor();
$gridField = new GridField('testfield', 'testfield', $list, $config);
+ $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class);
// Test normal sorting
+ $component->setFieldSorting(['Name' => 'City']);
$state = $gridField->State->GridFieldSortableHeader;
$state->SortColumn = 'City';
$state->SortDirection = 'asc';
- $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class);
$listA = $component->getManipulatedData($gridField, $list);
$state->SortDirection = 'desc';
@@ -93,6 +94,7 @@ class GridFieldSortableHeaderTest extends SapphireTest
);
// Test one relation 'deep'
+ $component->setFieldSorting(['Name' => 'Cheerleader.Name']);
$state->SortColumn = 'Cheerleader.Name';
$state->SortDirection = 'asc';
$relationListA = $component->getManipulatedData($gridField, $list);
@@ -110,6 +112,7 @@ class GridFieldSortableHeaderTest extends SapphireTest
);
// Test two relations 'deep'
+ $component->setFieldSorting(['Name' => 'Cheerleader.Hat.Colour']);
$state->SortColumn = 'Cheerleader.Hat.Colour';
$state->SortDirection = 'asc';
$relationListC = $component->getManipulatedData($gridField, $list);
@@ -139,6 +142,7 @@ class GridFieldSortableHeaderTest extends SapphireTest
$component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class);
// Test that inherited dataobjects will work correctly
+ $component->setFieldSorting(['Name' => 'Cheerleader.Hat.Colour']);
$state->SortColumn = 'Cheerleader.Hat.Colour';
$state->SortDirection = 'asc';
$relationListA = $component->getManipulatedData($gridField, $list);
@@ -179,6 +183,7 @@ class GridFieldSortableHeaderTest extends SapphireTest
);
// Test subclasses of tables
+ $component->setFieldSorting(['Name' => 'CheerleadersMom.Hat.Colour']);
$state->SortColumn = 'CheerleadersMom.Hat.Colour';
$state->SortDirection = 'asc';
$relationListB = $component->getManipulatedData($gridField, $list);
@@ -229,4 +234,21 @@ class GridFieldSortableHeaderTest extends SapphireTest
$relationListBdesc->column('City')
);
}
+
+ public function testSortColumnValidation()
+ {
+ $this->expectException(\LogicException::class);
+ $this->expectExceptionMessage('Invalid SortColumn: INVALID');
+
+ $list = Team::get()->filter([ 'ClassName' => Team::class ]);
+ $config = new GridFieldConfig_RecordEditor();
+ $gridField = new GridField('testfield', 'testfield', $list, $config);
+ $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class);
+
+ $state = $gridField->State->GridFieldSortableHeader;
+ $state->SortColumn = 'INVALID';
+ $state->SortDirection = 'asc';
+
+ $component->getManipulatedData($gridField, $list);
+ }
}
diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php
index 4b3695274..3d5c3d5c6 100644
--- a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php
+++ b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php
@@ -98,6 +98,36 @@ class HTMLEditorSanitiserTest extends FunctionalTest
'',
'Javascript in the src attribute of an iframe is completely removed'
],
+ [
+ 'iframe[src]',
+ '',
+ '',
+ 'Mixed case javascript in the src attribute of an iframe is completely removed'
+ ],
+ [
+ 'iframe[src]',
+ "",
+ '',
+ 'Javascript with tab elements the src attribute of an iframe is completely removed'
+ ],
+ [
+ 'object[data]',
+ '',
+ '',
+ 'Object with OK content in the data attribute is retained'
+ ],
+ [
+ 'object[data]',
+ '