Merge branch '5' into 6

Fix wysiwyg sanitisation

src/Core/Manifest/VersionProvider.php

@@ -189,6 +189,9 @@ class VersionProvider
     {
         $versions = [];
         foreach ($modules as $module) {
+            if (!InstalledVersions::isInstalled($module)) {
+                continue;
+            }
             $versions[$module] = InstalledVersions::getPrettyVersion($module);
         }
         return $versions;

src/Forms/HTMLEditor/HTMLEditorField.php

@@ -138,7 +138,8 @@ class HTMLEditorField extends TextareaField
         // Sanitise if requested
         $htmlValue = HTMLValue::create($this->Value());
         if (HTMLEditorField::config()->sanitise_server_side) {
-            $santiser = HTMLEditorSanitiser::create(HTMLEditorConfig::get_active());
+            $config = $this->getEditorConfig();
+            $santiser = HTMLEditorSanitiser::create($config);
             $santiser->sanitise($htmlValue);
         }
 

src/Forms/HTMLEditor/HTMLEditorSanitiser.php

@@ -287,10 +287,6 @@ class HTMLEditorSanitiser
      */
     public function sanitise(HTMLValue $html)
     {
-        if (!$this->elements && !$this->elementPatterns) {
-            return;
-        }
-
         $linkRelValue = $this->config()->get('link_rel_value');
         $doc = $html->getDocument();
 

src/Forms/HTMLEditor/TinyMCEConfig.php

@@ -250,13 +250,11 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider
     private static $image_size_presets = [ ];
 
     /**
-     * TinyMCE JS settings
+     * Default TinyMCE JS options which apply to all new configurations.
      *
      * @link https://www.tiny.cloud/docs/tinymce/6/tinydrive-getting-started/#configure-the-required-tinymce-options
-     *
-     * @var array
      */
-    protected $settings = [
+    private static array $default_options = [
         'fix_list_elements' => true, // https://www.tiny.cloud/docs/tinymce/6/content-filtering/#fix_list_elements
         'formats' => [
             'alignleft' => [
@@ -311,8 +309,24 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider
         'promotion' => false,
         'upload_folder_id' => null, // Set folder ID for insert media dialog
         'link_default_target' => '_blank', // https://www.tiny.cloud/docs/tinymce/6/autolink/#example-using-link_default_target
+        // Default set of valid_elements which apply for all new configurations
+        'valid_elements' => "@[id|class|style|title],a[id|rel|rev|dir|tabindex|accesskey|type|name|href|target|title"
+        . "|class],-strong/-b[class],-em/-i[class],-strike[class],-u[class],#p[id|dir|class|align|style],-ol[class],"
+        . "-ul[class],-li[class],br,img[id|dir|longdesc|usemap|class|src|border|alt=|title|hspace|vspace|width|height|align|name|data*],"
+        . "-sub[class],-sup[class],-blockquote[dir|class],-cite[dir|class|id|title],"
+        . "-table[cellspacing|cellpadding|width|height|class|align|summary|dir|id|style],"
+        . "-tr[id|dir|class|rowspan|width|height|align|valign|bgcolor|background|bordercolor|style],"
+        . "tbody[id|class|style],thead[id|class|style],tfoot[id|class|style],"
+        . "#td[id|dir|class|colspan|rowspan|width|height|align|valign|scope|style],"
+        . "-th[id|dir|class|colspan|rowspan|width|height|align|valign|scope|style],caption[id|dir|class],"
+        . "-div[id|dir|class|align|style],-span[class|align|style],-pre[class|align],address[class|align],"
+        . "-h1[id|dir|class|align|style],-h2[id|dir|class|align|style],-h3[id|dir|class|align|style],"
+        . "-h4[id|dir|class|align|style],-h5[id|dir|class|align|style],-h6[id|dir|class|align|style],hr[class],"
+        . "dd[id|class|title|dir],dl[id|class|title|dir],dt[id|class|title|dir],"
     ];
 
+    protected $settings = [];
+
     /**
      * Holder list of enabled plugins
      *
@@ -337,6 +351,11 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider
      */
     protected $theme = 'silver';
 
+    public function __construct()
+    {
+        $this->settings = static::config()->get('default_options');
+    }
+
     /**
      * Get the theme
      *

src/ORM/DataList.php

@@ -1129,18 +1129,18 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
                 // $parentData represents a record in this DataList
                 $hasOneID = $parentData[$hasOneIDField];
                 $fetchedIDs[] = $hasOneID;
-                $addTo[$hasOneID] = $parentData['ID'];
+                $addTo[$hasOneID][] = $parentData['ID'];
             } elseif ($parentData instanceof DataObject) {
                 // $parentData represents another has_one record
                 $hasOneID = $parentData->$hasOneIDField;
                 $fetchedIDs[] = $hasOneID;
-                $addTo[$hasOneID] = $parentData;
+                $addTo[$hasOneID][] = $parentData;
             } elseif ($parentData instanceof EagerLoadedList) {
                 // $parentData represents a has_many or many_many relation
                 foreach ($parentData->getRows() as $parentRow) {
                     $hasOneID = $parentRow[$hasOneIDField];
                     $fetchedIDs[] = $hasOneID;
-                    $addTo[$hasOneID] = ['ID' => $parentRow['ID'], 'list' => $parentData];
+                    $addTo[$hasOneID][] = ['ID' => $parentRow['ID'], 'list' => $parentData];
                 }
             } else {
                 throw new LogicException("Invalid parent for eager loading $relationType relation $relationName");
@@ -1151,10 +1151,8 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
 
         // Add each fetched record to the appropriate place
         foreach ($fetchedRecords as $fetched) {
-            $fetchedID = $fetched->ID;
-            $added = false;
-            foreach ($addTo as $matchID => $addHere) {
-                if ($matchID === $fetchedID) {
+            if (isset($addTo[$fetched->ID])) {
+                foreach ($addTo[$fetched->ID] as $addHere) {
                     if ($addHere instanceof DataObject) {
                         $addHere->setEagerLoadedData($relationName, $fetched);
                     } elseif (is_array($addHere)) {
@@ -1162,11 +1160,8 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
                     } else {
                         $this->eagerLoadedData[$relationChain][$addHere][$relationName] = $fetched;
                     }
-                    $added = true;
-                    break;
                 }
-            }
-            if (!$added) {
+            } else {
                 throw new LogicException("Couldn't find parent for record $fetchedID on $relationType relation $relationName");
             }
         }

tests/php/Core/Manifest/VersionProviderTest.php

@@ -102,6 +102,27 @@ class VersionProviderTest extends SapphireTest
         $this->assertStringNotContainsString('Framework: 1.2.3', $result);
     }
 
+    public function testGetModuleVersionWhenPackageMayNotBeInstalled()
+    {
+        if (!class_exists(VersionParser::class)) {
+            $this->markTestSkipped('This test requires composer/semver to be installed');
+        }
+        $provider = $this->getProvider();
+        // VersionProvider::getModuleVersion() will loop over the modules defined in the "modules" config value, which
+        // may sometimes include packages that are optional (e.g. recipe-core). This tests that the version can still
+        // be found even if non-existent modules are encountered
+        Config::modify()->set(VersionProvider::class, 'modules', [
+            'this/module/cannot/possibly/exist' => 'Oopsies',
+            'silverstripe/framework' => 'Framework',
+            'silverstripe/another-module-that-does-not-exist' => 'Sapphire',
+        ]);
+        $moduleVersion = $provider->getModuleVersion('silverstripe/framework');
+        $parser = new VersionParser();
+        $this->assertIsString($parser->normalize($moduleVersion), "Expected a valid semver but got $moduleVersion");
+        $result = $provider->getVersion();
+        $this->assertStringNotContainsString('Framework: 1.2.3', $result);
+    }
+
     private function clearCache()
     {
         $cache = Injector::inst()->get(CacheInterface::class . '.VersionProvider');

tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php

@@ -12,6 +12,7 @@ use SilverStripe\Control\Director;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\CSSContentParser;
 use SilverStripe\Dev\FunctionalTest;
+use SilverStripe\Forms\HTMLEditor\HTMLEditorConfig;
 use SilverStripe\Forms\HTMLEditor\HTMLEditorField;
 use SilverStripe\Forms\HTMLEditor\TinyMCEConfig;
 use SilverStripe\Forms\HTMLReadonlyField;
@@ -278,4 +279,41 @@ EOS
         $this->assertEquals("auto", $data_config->height, 'Config height is not set');
         $this->assertEquals("60px", $data_config->row_height, 'Config row_height is not set');
     }
+
+    public function testFieldConfigSanitization()
+    {
+        $obj = TestObject::create();
+        $editor = HTMLEditorField::create('Content');
+        $defaultValidElements = [
+            '@[id|class|style|title|data*]',
+            'a[id|rel|dir|tabindex|accesskey|type|name|href|target|title|class]',
+            '-strong/-b[class]',
+            '-em/-i[class]',
+            '-ol[class]',
+            '#p[id|dir|class|align|style]',
+            '-li[class]',
+            'br',
+            '-span[class|align|style]',
+            '-ul[class]',
+            '-h3[id|dir|class|align|style]',
+            '-h2[id|dir|class|align|style]',
+            'hr[class]',
+        ];
+        $restrictedConfig = HTMLEditorConfig::get('restricted');
+        $restrictedConfig->setOption('valid_elements', implode(',', $defaultValidElements));
+        $editor->setEditorConfig($restrictedConfig);
+
+        $expectedHtmlString = '<p>standard text</p>Header';
+        $htmlValue = '<p>standard text</p><table><th><tr><td>Header</td></tr></th><tbody></tbody></table>';
+        $editor->setValue($htmlValue);
+        $editor->saveInto($obj);
+        $this->assertEquals($expectedHtmlString, $obj->Content, 'Table is not removed');
+
+        $defaultConfig = HTMLEditorConfig::get('default');
+        $editor->setEditorConfig($defaultConfig);
+
+        $editor->setValue($htmlValue);
+        $editor->saveInto($obj);
+        $this->assertEquals($htmlValue, $obj->Content, 'Table is removed');
+    }
 }

tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php

@@ -11,9 +11,9 @@ use SilverStripe\View\Parsers\HTMLValue;
 class HTMLEditorSanitiserTest extends FunctionalTest
 {
 
-    public function testSanitisation()
+    public function provideSanitise(): array
     {
-        $tests = [
+        return [
             [
                 'p,strong',
                 '<p>Leave Alone</p><div>Strip parent<strong>But keep children</strong> in order</div>',
@@ -129,13 +129,20 @@ class HTMLEditorSanitiserTest extends FunctionalTest
                 'XSS vulnerable attributes starting with on or style are removed via configuration'
             ],
         ];
+    }
 
-        $config = HTMLEditorConfig::get('htmleditorsanitisertest');
-
-        foreach ($tests as $test) {
-            list($validElements, $input, $output, $desc) = $test;
-
-            $config->setOptions(['valid_elements' => $validElements]);
+    /**
+     * @dataProvider provideSanitise
+     */
+    public function testSanitisation(string $validElements, string $input, string $output, string $desc): void
+    {
+        foreach (['valid_elements', 'extended_valid_elements'] as $configType) {
+            $config = HTMLEditorConfig::get('htmleditorsanitisertest_' . $configType);
+            $config->setOptions([$configType => $validElements]);
+            // Remove default valid elements if we're testing extended valid elements
+            if ($configType !== 'valid_elements') {
+                $config->setOptions(['valid_elements' => '']);
+            }
             $sanitiser = new HtmlEditorSanitiser($config);
 
             $value = 'noopener noreferrer';
@@ -144,12 +151,30 @@ class HTMLEditorSanitiserTest extends FunctionalTest
             } elseif (strpos($desc ?? '', 'link_rel_value is null') !== false) {
                 $value = null;
             }
-            Config::inst()->set(HTMLEditorSanitiser::class, 'link_rel_value', $value);
+
+            HTMLEditorSanitiser::config()->set('link_rel_value', $value);
 
             $htmlValue = HTMLValue::create($input);
             $sanitiser->sanitise($htmlValue);
 
-            $this->assertEquals($output, $htmlValue->getContent(), $desc);
+            $this->assertEquals($output, $htmlValue->getContent(), "{$desc} - using config type: {$configType}");
         }
     }
+
+    /**
+     * Ensure that when there are no valid elements at all for a configuration set,
+     * nothing is allowed.
+     */
+    public function testSanitiseNoValidElements(): void
+    {
+        $config = HTMLEditorConfig::get('htmleditorsanitisertest');
+        $config->setOptions(['valid_elements' => '']);
+        $config->setOptions(['extended_valid_elements' => '']);
+        $sanitiser = new HtmlEditorSanitiser($config);
+
+        $htmlValue = HTMLValue::create('<p>standard text</p><table><tbody><tr><th><a href="some-link">text</a></th></tr><tr><td>Header</td></tr></tbody></table>');
+        $sanitiser->sanitise($htmlValue);
+
+        $this->assertEquals('standard texttextHeader', $htmlValue->getContent());
+    }
 }

tests/php/ORM/DataListEagerLoadingTest.php

@@ -1588,4 +1588,72 @@ class DataListEagerLoadingTest extends SapphireTest
             }
         }
     }
+
+    public function testHasOneMultipleAppearance(): void
+    {
+        $this->provideHasOneObjects();
+        $this->validateMultipleAppearance(6, EagerLoadObject::get());
+        $this->validateMultipleAppearance(2, EagerLoadObject::get()->eagerLoad('HasOneEagerLoadObject'));
+    }
+
+    protected function validateMultipleAppearance(int $expected, DataList $list): void
+    {
+        try {
+            $this->startCountingSelectQueries();
+
+            /** @var EagerLoadObject $item */
+            foreach ($list as $item) {
+                $item->HasOneEagerLoadObject()->Title;
+            }
+
+            $this->assertSame($expected, $this->stopCountingSelectQueries());
+        } finally {
+            $this->resetShowQueries();
+        }
+    }
+
+    protected function provideHasOneObjects(): void
+    {
+        $subA = new HasOneEagerLoadObject();
+        $subA->Title = 'A';
+        $subA->write();
+
+        $subB = new HasOneEagerLoadObject();
+        $subB->Title = 'B';
+        $subB->write();
+
+        $subC = new HasOneEagerLoadObject();
+        $subC->Title = 'C';
+        $subC->write();
+
+        $baseA = new EagerLoadObject();
+        $baseA->Title = 'A';
+        $baseA->HasOneEagerLoadObjectID = $subA->ID;
+        $baseA->write();
+
+        $baseB = new EagerLoadObject();
+        $baseB->Title = 'B';
+        $baseB->HasOneEagerLoadObjectID = $subA->ID;
+        $baseB->write();
+
+        $baseC = new EagerLoadObject();
+        $baseC->Title = 'C';
+        $baseC->HasOneEagerLoadObjectID = $subB->ID;
+        $baseC->write();
+
+        $baseD = new EagerLoadObject();
+        $baseD->Title = 'D';
+        $baseD->HasOneEagerLoadObjectID = $subC->ID;
+        $baseD->write();
+
+        $baseE = new EagerLoadObject();
+        $baseE->Title = 'E';
+        $baseE->HasOneEagerLoadObjectID = $subB->ID;
+        $baseE->write();
+
+        $baseF = new EagerLoadObject();
+        $baseF->Title = 'F';
+        $baseF->HasOneEagerLoadObjectID = 0;
+        $baseF->write();
+    }
 }