Merge branch '4.11' into 4.12-release

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

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'));
     }
 

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);

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;
     }

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 ?? [])) {

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 [

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);
+    }
 }

tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php

@@ -98,6 +98,36 @@ class HTMLEditorSanitiserTest extends FunctionalTest
                 '<iframe></iframe>',
                 'Javascript in the src attribute of an iframe is completely removed'
             ],
+            [
+                'iframe[src]',
+                '<iframe src="jAvAsCrIpT:alert(0);"></iframe>',
+                '<iframe></iframe>',
+                'Mixed case javascript in the src attribute of an iframe is completely removed'
+            ],
+            [
+                'iframe[src]',
+                "<iframe src=\"java\tscript:alert(0);\"></iframe>",
+                '<iframe></iframe>',
+                'Javascript with tab elements the src attribute of an iframe is completely removed'
+            ],
+            [
+                'object[data]',
+                '<object data="OK"></object>',
+                '<object data="OK"></object>',
+                'Object with OK content in the data attribute is retained'
+            ],
+            [
+                'object[data]',
+                '<object data=javascript:alert()>',
+                '<object></object>',
+                'Object with dangerous content in data attribute is completely removed'
+            ],
+            [
+                'img[src]',
+                '<img src="https://owasp.org/myimage.jpg" style="url:xss" onerror="alert(1)">',
+                '<img src="https://owasp.org/myimage.jpg">',
+                'XSS vulnerable attributes starting with on or style are removed via configuration'
+            ],
         ];
 
         $config = HTMLEditorConfig::get('htmleditorsanitisertest');

tests/php/Security/SecurityDefaultAdminTest.php

@@ -7,6 +7,7 @@ use SilverStripe\Security\Member;
 use SilverStripe\Security\PasswordEncryptor;
 use SilverStripe\Security\Permission;
 use SilverStripe\Security\DefaultAdminService;
+use SilverStripe\Security\Security;
 
 class SecurityDefaultAdminTest extends SapphireTest
 {
@@ -35,6 +36,7 @@ class SecurityDefaultAdminTest extends SapphireTest
             $this->defaultUsername = null;
             $this->defaultPassword = null;
         }
+        Security::config()->set('password_encryption_algorithm', 'blowfish');
         DefaultAdminService::setDefaultAdmin('admin', 'password');
         Permission::reset();
     }

tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\View\Tests\Shortcodes;
 
 use Psr\SimpleCache\CacheInterface;
+use SilverStripe\Core\Config\Config;
 use SilverStripe\View\Parsers\ShortcodeParser;
 use SilverStripe\View\Shortcodes\EmbedShortcodeProvider;
 use SilverStripe\Dev\SapphireTest;
@@ -186,4 +187,67 @@ class EmbedShortcodeProviderTest extends EmbedUnitTest
         EmbedShortcodeProvider::flushCachedShortcodes($parser, $content);
         $this->assertFalse($cache->has($key));
     }
+
+    public function testOnlyWhitelistedAttributesAllowed()
+    {
+        $url = 'https://www.youtube.com/watch?v=dM15HfUYwF0';
+        $html = $this->getShortcodeHtml(
+            $url,
+            $url,
+            <<<EOT
+            <link rel="alternate" type="application/json+oembed" href="https://www.youtube.com/oembed?format=json&amp;url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da2tDOYkFCYo" title="The flying car completes first ever inter-city flight (Official Video)">
+            EOT,
+            <<<EOT
+            {"title":"The flying car completes first ever inter-city flight (Official Video)","author_name":"KleinVision","author_url":"https://www.youtube.com/channel/UCCHAHvcO7KSNmgXVRIJLNkw","type":"video","height":113,"width":200,"version":"1.0","provider_name":"YouTube","provider_url":"https://www.youtube.com/","thumbnail_height":360,"thumbnail_width":480,"thumbnail_url":"https://i.ytimg.com/vi/a2tDOYkFCYo/hqdefault.jpg","html":"\u003ciframe width=\u0022200\u0022 height=\u0022113\u0022 src=\u0022https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed\u0022 frameborder=\u00220\u0022 allow=\u0022accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture\u0022 allowfullscreen\u003e\u003c/iframe\u003e"}
+            EOT,
+            [
+                'url' => $url,
+                'caption' => 'A nice video',
+                'width' => 778,
+                'height' => 437,
+                'data-some-value' => 'my-data',
+                'onmouseover' => 'alert(2)',
+                'style' => 'background-color:red;',
+            ],
+        );
+        $this->assertEqualIgnoringWhitespace(
+            <<<EOT
+            <div style="width:778px;"><iframe width="778" height="437" src="https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed" frameborder="0" allow="accelerometer;autoplay;clipboard-write;encrypted-media;gyroscope;picture-in-picture" allowfullscreen></iframe><p class="caption">A nice video</p></div>
+            EOT,
+            $html
+        );
+    }
+
+    public function testWhitelistIsConfigurable()
+    {
+        // Allow new whitelisted attribute
+        Config::modify()->merge(EmbedShortcodeProvider::class, 'attribute_whitelist', ['data-some-value']);
+
+        $url = 'https://www.youtube.com/watch?v=dM15HfUYwF0';
+        $html = $this->getShortcodeHtml(
+            $url,
+            $url,
+            <<<EOT
+            <link rel="alternate" type="application/json+oembed" href="https://www.youtube.com/oembed?format=json&amp;url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da2tDOYkFCYo" title="The flying car completes first ever inter-city flight (Official Video)">
+            EOT,
+            <<<EOT
+            {"title":"The flying car completes first ever inter-city flight (Official Video)","author_name":"KleinVision","author_url":"https://www.youtube.com/channel/UCCHAHvcO7KSNmgXVRIJLNkw","type":"video","height":113,"width":200,"version":"1.0","provider_name":"YouTube","provider_url":"https://www.youtube.com/","thumbnail_height":360,"thumbnail_width":480,"thumbnail_url":"https://i.ytimg.com/vi/a2tDOYkFCYo/hqdefault.jpg","html":"\u003ciframe width=\u0022200\u0022 height=\u0022113\u0022 src=\u0022https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed\u0022 frameborder=\u00220\u0022 allow=\u0022accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture\u0022 allowfullscreen\u003e\u003c/iframe\u003e"}
+            EOT,
+            [
+                'url' => $url,
+                'caption' => 'A nice video',
+                'width' => 779,
+                'height' => 437,
+                'data-some-value' => 'my-data',
+                'onmouseover' => 'alert(2)',
+                'style' => 'background-color:red;',
+            ],
+        );
+        $this->assertEqualIgnoringWhitespace(
+            <<<EOT
+            <div data-some-value="my-data" style="width:779px;"><iframe width="779" height="437" src="https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed" frameborder="0" allow="accelerometer;autoplay;clipboard-write;encrypted-media;gyroscope;picture-in-picture" allowfullscreen></iframe><p class="caption">A nice video</p></div>
+            EOT,
+            $html
+        );
+    }
 }