Fix wysiwyg sanitisation

[GuySartorelli]

Description

There are multiple commits here since we're doing several related but separate changes:

1. Reimplement the original bugfix (just cherry-picked the commit directly)
2. Refactoring a sub-optimal test to use dataproviders and to test valid_elements and extended_valid_elements separately
3. Don't skip validation of HTML - without this, an empty valid_elements set would result in skipping validation entirely.
4. Set the default valid_elements settings for any new TinyMCE config instances
5. Wasn't in the ACs but seems prudent - make the default TinyMCE settings configurable at a global level.

Issues

• [Bug] TinyMCEConfig breaks when using option extended_valid_elements on a custom Config. #11141

[GuySartorelli]

I've gone with default_options as the name for this configuration because you usually interact with these by calling setOption() or setOptions()

[emteknetnz]

Seems good, have tested locally. Having the img tag on valid_elements seems fine, I think part of why it was originally on extended_valid_elements along with iframe rather than valid_elements was that (if my memory serves me correctly), earlier versions of silverstripe would output img tags in some instances when 'Inserting from file', though more recent versions always use an image shortcode

iframe is still used when inserting external media i.e. youtube, so makes sense to keep that the extended elements list

[emteknetnz]

Suggested change

	. "-ul[class],-li[class],br,img[id|dir|longdesc|usemap|class|src|border|alt=|title|hspace|vspace|width|height|align|name|data*],"
	. "-ul[class],-li[class],br,img[id|dir|longdesc|usemap|class|src|border|alt|title|hspace|vspace|width|height|align|name|data*],"

Is that a typo with the = for alt?

Do we want name, hspace, space for the old extended_valid_elements from admin that's being deleted?

[GuySartorelli]

Is that a typo with the = for alt?

The = is correct, as per https://www.tiny.cloud/docs/tinymce/latest/content-filtering/#valid_elements
Basically, it's defaulting to including a blank alt property for all images which don't have a specific alt text defined for them, which is correct for accessibility.

Do we want name, hspace, space for the old extended_valid_elements from admin that's being deleted?

It's only being deleted because I've moved those here. They seem safe, so I'd be inclined to keep them here. If you have a specific reason not to, I can move them back to extended_valid_elements in the "cms" config where they have previously lived.

[emteknetnz]

Do we want name, hspace, space for the old extended_valid_elements from admin that's being deleted?

Sorry I'm blind, somehow I managed to not noticed they're copied over. Yes keep them as is in this PR

[emteknetnz]

Just retesting this not working as expected, I feel like I'm doing something wrong here

<?php

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\HTMLEditor\HTMLEditorField;

class Page extends SiteTree
{
    private static $db = [
        'SomeContent' => 'HTMLText'
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();

        # doesn't allow any tags except p,ul,li
        $h = $fields->dataFieldByName('Content');
        $c = $h->getEditorConfig();
        $c->setOption('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|width|height|align|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]");
        $h->setEditorConfig($c);

        # works correctly
        $h2 = HTMLEditorField::create('SomeContent');
        $c2 = $h2->getEditorConfig();
        $c2->setOption('valid_elements', 'p,ul,li');
        $h2->setEditorConfig($c2);

        $fields->addFieldsToTab('Root.Main', [$h2]);
        return $fields;
    }
}

[GuySartorelli]

Looks like you've uncovered a completely new bug 🥳

To test, in a fresh installation without any of these PRs and without your configuration above, add a link to a page and save. Then, add only the second config you have there (i.e. add $h2 and $c2, leaving the original "Content" field as it was) and refresh the page. TinyMCE will have stripped the link out of your original content.

If you can reproduce that with a fresh installation, it's a separate bug and should be handled in a separate card (and probably fixed in 5.2)

[emteknetnz]

Yup validated it's an existing issue, have raised card