Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
davejtoews opened this issue Feb 14, 2024 · 26 comments
Closed
2 tasks done

Comments

@davejtoews
Copy link

davejtoews commented Feb 14, 2024

Module version(s) affected

4.13.30-4.13.42

Description

Upon upgrading a site from 4.13.29 to 4.13.41 a bug was introduced in our TinyMCE editor. Nearly all formatting is being stripped on save. It appears as though only html elements listed in extended_valid_elements are being allowed, as if the list is being used to replace the list of valid elements rather than amend it.

How to reproduce

Define an extended valid elements value for a custom TinyMCE config in _config.php.

TinyMCEConfig::get('CustomConfig')
    ->setOptions(['extended_valid_elements' => 'small']);

Add a custom field using this config in Page.php

        private static $db = [
            'RichTextField' => 'HTMLText'
        ];

        public function getCMSFields()
        {
            $fields = parent::getCMSFields();
            $fields->addFieldToTab('Root.Main', HTMLEditorField::create('RichTextField', 'RichTextField', $this->RichTextField, 'CustomConfig'));
            return $fields;
        }

Open a TinyMCE editor in the CMS and enter paragraph breaks, links or any other formatting. On save, all such formatting is stripped.

If the extended_valid_elements option is removed, or set to an empty string, the issue does not occur.

If you set the value to 'h3', you will be able to save h3 elements, but nothing else.

Possible Solution

This bug does not appear in 4.13.29, but it appears in all versions after 4.13.30

The change in 4.13.30 appears to cause custom configs to be built from an empty config, instead of bootstrapping the defaults as it did previously.

e5eb98c#diff-816b9855682fea334353bdfeb9b733211672205ec14351283df7dc1289f89037L148

Reverting this change would fix my issue, but I'm not clear what problem it was trying to solve. In any case this was a breaking change in config behaviour.

Additional Context

No response

Validations

  • Check that there isn't already an issue that reports the same bug
  • Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

Acceptance criteria for minor release

  • this bugfix is reimplemented
  • sanitisation for HTML content is not skipped even when no valid elements are declared (see this code)
  • A default valid_elements is defined for TinyMCEConfig in the settings property
  • For all TinyMCEConfig instances, a combination of all valid_elements and extended_valid_elements declarations are used for sanitisation, and are always respected even if one or both of these settings are empty
  • This change is highlighted in the changelog, with a call to action to check custom TinyMCEConfig and update extended_valid_elements if necessary.

PRs

@davejtoews
Copy link
Author

Would appreciate any insight from maintainers here on what the intended behaviour is here. Have I been exploiting a bug that I took for a feature and which 4.13.30 fixed, or did 4.13.30 introduce a bug?

@davejtoews
Copy link
Author

If anyone else is seeing this bug, we were able to fix the particular issue with extended_valid_elements by doing this:

$valid_elements = TinyMCEConfig::get('cms')->getOption('valid_elements');
$valid_elements .= ',small';

TinyMCEConfig::get('CustomConfig')
    ->setOptions(['extended_valid_elements' => $valid_elements]);

The above fix however assumes that only the valid element list is being wiped out. If we assume that any number of config options could be being affected, we need to programatically clone and pass along all of them. See below.

$default_options = json_decode(TinyMCEConfig::get('cms')->getAttributes()['data-config'], true);

$createConfig = function ($name) use ($default_options) {
    $config =  TinyMCEConfig::get($name);
    foreach ($default_options as $key => $value) {
        if ($key === 'external_plugins') {
            $config->enablePlugins($value);
            continue;
        }
        if ($key === 'toolbar') {
            foreach ($value as $line => $buttons) {
                $config->setButtonsForLine($line + 1, $buttons);
            }
            continue;
        }
        $config->setOption($key, $value);
    }
    return $config;
};

$createConfig('CustomConfig')
    ->setOptions(['extended_valid_elements' => 'small']);

@GuySartorelli
Copy link
Member

Expected behaviour is that the configuration set for a given HTMLEditorConfig instance is used for validation.

If extended_valid_elements and valid_elements are set for a given config instance, then that is what should be used to validate the HTML.

To check:

  1. In 4.13.29 was the set of valid elements being validated against actually set on the given HTMLEditorConfig instance?
    • Check by calling getOption('extended_valid_elements') and getOption('valid_elements')
  2. After 4.13.30 is the set of valid elements being validated against set on the HTMLEditorConfig instance (i.e. did the change in 4.13.30 actually change the config itself?

@GuySartorelli GuySartorelli self-assigned this Feb 19, 2024
@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 19, 2024

Looks like this is acting as expected.

use SilverStripe\Forms\HTMLEditor\TinyMCEConfig;
$config = TinyMCEConfig::get('CustomConfig');
$config->setOptions(['extended_valid_elements' => 'small']);
var_dump([
    'valid_elements' => $config->getOption('valid_elements'),
    'extended_valid_elements' => $config->getOption('extended_valid_elements'),
]);

This outputs

array(2) {
  ["valid_elements"]=>
  NULL
  ["extended_valid_elements"]=>
  string(5) "small"
}

In otherwords, previously your config wasn't being validated correctly - but now it is. Any elements that your config do not say are valid.... well, they're not valid.

In hindsight if we'd known this would be a problem we would have held off releasing this change until a minor release in CMS 5, but now that it's out there reverting it would just cause problems for people who have already updated and are relying on the now-correct behaviour.

@GuySartorelli GuySartorelli closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2024
@davejtoews
Copy link
Author

davejtoews commented Feb 20, 2024

@GuySartorelli this output does not explain the TinyMCE behaviour.

Stripping of tags only occurs when extended_valid_elements is set. If you do the following:

use SilverStripe\Forms\HTMLEditor\TinyMCEConfig;
$config = TinyMCEConfig::get('CustomConfig');
var_dump([
    'valid_elements' => $config->getOption('valid_elements'),
    'extended_valid_elements' => $config->getOption('extended_valid_elements'),
]);

This outputs

array(2) {
  ["valid_elements"]=>
  NULL
  ["extended_valid_elements"]=>
  NULL
}

The above is true in both 4.13.29 and 4.13.30-42. And in all versions you are able to save <p> tags, <h2> tags etc.

If the validator is intended to remove any elements not in the list then it should not be possible to save any such markup after 4.13.30. It is however very much possible until you set extended_valid_elements. The issue here is that setting extended_valid_elements seems to actually decrease the number of valid elements the editor will accept after 4.13.30.

It would seem very odd to me if this was the intended behaviour.

@GuySartorelli GuySartorelli reopened this Feb 20, 2024
@GuySartorelli
Copy link
Member

Stripping of tags only occurs when extended_valid_elements is set

Ahh okay thank you, I must have skipped over that part of the original description, that's good context to have. I'll take a look at that side of it.

@davejtoews
Copy link
Author

Probably also worth noting that a new TinyMCEConfig, w/ no modifications applied will load on the frontend w/ a number of buttons in the toolbar to generate various HTML tags. Headings, lists, tables, etc. If the idea is to wipe the valid_elements list clean unless specified, I would also thing the button rows should be wiped clean so we don't have the interface to add elements that will not be allowed.

@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 20, 2024

My current approach (and I passed this by @blueo who implemented the fix you linked to in the original issue description) is to explicitly set some default valid_elements for all new config (which gets overridden of course if you explicitly set valid_elements yourself).

This retains the current behaviour for new configs that don't have any explicit valid_elements or extended_valid_elements config defined for it, but it doesn't cause unexpected behaviour when all you set is extended_valid_elements, because the valid_elements is still set to a sane default.

So it keeps the good part of the current behaviour, and removes the unexpected element. Hopefully.
Does that sound about right to you? If so I'll whip up a PR along those lines.

@GuySartorelli
Copy link
Member

Or if you'd like to amend yours to be along those lines, that works too 🥳

@davejtoews
Copy link
Author

Yes, I think there should be a default set of valid_elements, presumably set in TinyMCEConfig::$settings?

I guess my question would then be what are the default valid elements? Willing to work on a PR but I'm not sure how such defaults have been, or should be arrived at.

There is a list defined in silverstripe/admin/_config.php for the 'cms' config. Should that be the default? If so presumably we want a single source of truth rather than copy pasting? Why does the 'cms' config include both valid_elements and extended_valid_elements? I would have assumed it would all be in valid_elements and extended_valid_elements would be left blank for developers to be able to extend the list without overriding items from the existing list. Perhaps the docs could/should make it clear the best way to modify this part of the config?

The 'cms' config defines quite a few allowed attributes, not just the element type. How was this list arrived at?

Presumably we want to align the default valid elements list with the default set of buttons and plugins? Is there any way we can derive the default elements from these other options or otherwise ensure that a change in one is reflected by a change in the other?

$config = TinyMCEConfig::get('Custom');
print_r([
    'buttons' => $config->getButtons(),
    'plugins' => $config->getPlugins()
]);

Output

Array
(
    [buttons] => Array
        (
            [1] => Array
                (
                    [0] => bold
                    [1] => italic
                    [2] => underline
                    [3] => removeformat
                    [4] => |
                    [5] => alignleft
                    [6] => aligncenter
                    [7] => alignright
                    [8] => alignjustify
                    [9] => |
                    [10] => bullist
                    [11] => numlist
                    [12] => outdent
                    [13] => indent
                )

            [2] => Array
                (
                    [0] => formatselect
                    [1] => |
                    [2] => paste
                    [3] => pastetext
                    [4] => |
                    [5] => table
                    [6] => sslink
                    [7] => unlink
                    [8] => |
                    [9] => code
                )

        )

    [plugins] => Array
        (
            [table] => 
            [emoticons] => 
            [paste] => 
            [code] => 
            [importcss] => 
            [lists] => 
        )

Front-end
Screenshot 2024-02-20 at 4 56 28 PM

@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 21, 2024

Most of those questions are about past decisions, and I can't answer any of those since I don't have that context.

The one I can answer (sort of) is what the default valid elements should be. And that's simply "whatever is currently allowed when you try to sanitise a new config which hasn't had its valid elements defined"

We shouldn't change what buttons or plugins are available. The goal here is to just remove the footgun that setting extended_valid_elements reduces the set of valid elements, instead of increasing it. Very narrow scope bug fix here.

There is definitely scope to clarify things in docs, though, to your question about that.

@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 21, 2024

Looks like the set of valid elements that TinyMCE uses by default (which is what's applied in that scenario since the PHP sanitiser is effectively skipped) was nice and clearly defined in TinyMCE 3, but in 4+ they don't hard code it.
That said, it can be derived from tinymce.get()[0].parser.schema.elements - so I'm gonna explore that avenue tomorrow.

@davejtoews
Copy link
Author

Attempting to generate the string we need then.

Object.keys(tinymce.get()[0].parser.schema.elements).reduce(function (previous, key) { return (previous ? previous + "," : '') + key + '[' + Object.keys(tinymce.get()[0].parser.schema.elements[key].attributes).join('|') + ']'; }, "");

Output

acronym[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],applet[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|codebase|archive|code|object|alt|name|width|height|align|hspace|vspace],basefont[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|size|color|face],big[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],font[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|size|color|face],strike[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],tt[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],center[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],dir[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|compact],isindex[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|prompt],noframes[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],html[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|manifest],head[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],br[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|clear],noscript[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],hr[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align|noshade|size|width],title[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],base[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|href|target],link[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|href|rel|media|hreflang|type|sizes|crossorigin],meta[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|name|http-equiv|content|charset],style[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|media|type|scoped|xml:space],body[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|onafterprint|onbeforeprint|onbeforeunload|onblur|onerror|onfocus|onhashchange|onload|onmessage|onoffline|ononline|onpagehide|onpageshow|onpopstate|onresize|onscroll|onstorage|onunload|background|bgcolor|text|link|vlink|alink],caption[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],div[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],dd[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],dt[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],address[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],dfn[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],cite[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],s[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],small[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],strong[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],em[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],legend[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],span[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],bdo[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],u[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],b[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],i[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],sup[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],sub[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],kbd[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],samp[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],var[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],code[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],abbr[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],p[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],pre[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|width|xml:space],h6[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],h5[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],h4[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],h3[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],h2[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],h1[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align],blockquote[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|cite],ol[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|reversed|start|type|compact],ul[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|type|compact],li[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|value|type],dl[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|compact],a[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|href|target|rel|media|hreflang|type|charset|name|rev|shape|coords|download],q[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|cite],del[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|cite|datetime],ins[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|cite|datetime],img[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|src|sizes|srcset|alt|usemap|ismap|width|height|name|longdesc|align|border|hspace|vspace|crossorigin],iframe[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|src|name|width|height|longdesc|frameborder|marginwidth|marginheight|scrolling|align|sandbox|seamless|allowfullscreen],embed[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|src|type|width|height|align|name|hspace|vspace],object[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|data|type|typemustmatch|name|usemap|form|width|height|declare|classid|code|codebase|codetype|archive|standby|align|border|hspace|vspace],param[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|name|value|valuetype|type],map[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|name],area[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|alt|coords|shape|href|target|rel|media|hreflang|type|nohref],table[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|border|summary|width|frame|rules|cellspacing|cellpadding|align|bgcolor],colgroup[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|span|width|align|char|charoff|valign],col[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|span|width|align|char|charoff|valign],tfoot[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align|char|charoff|valign],thead[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align|char|charoff|valign],tbody[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align|char|charoff|valign],tr[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|align|char|charoff|valign|bgcolor],td[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|colspan|rowspan|headers|abbr|axis|scope|align|char|charoff|valign|nowrap|bgcolor|width|height],th[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|colspan|rowspan|headers|scope|abbr|axis|align|char|charoff|valign|nowrap|bgcolor|width|height],form[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|accept-charset|action|autocomplete|enctype|method|name|novalidate|target|accept],fieldset[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|disabled|form|name],label[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|form|for],input[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|accept|alt|autocomplete|checked|dirname|disabled|form|formaction|formenctype|formmethod|formnovalidate|formtarget|height|list|max|maxlength|min|multiple|name|pattern|readonly|required|size|src|step|type|value|width|usemap|align|autofocus|placeholder],button[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|disabled|form|formaction|formenctype|formmethod|formnovalidate|formtarget|name|type|value|autofocus],select[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|disabled|form|multiple|name|required|size|onchange|autofocus],optgroup[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|disabled|label],option[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|disabled|label|selected|value],textarea[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|cols|dirname|disabled|form|maxlength|name|readonly|required|rows|wrap|autofocus|placeholder],menu[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|type|label|compact],wbr[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],ruby[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],figcaption[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],bdi[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],summary[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],rp[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],rt[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],mark[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],canvas[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|width|height],video[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|src|crossorigin|poster|preload|autoplay|mediagroup|loop|muted|controls|width|height|buffered],audio[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|src|crossorigin|preload|autoplay|mediagroup|loop|muted|controls|buffered|volume],picture[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],source[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|src|srcset|type|media|sizes],track[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|kind|src|srclang|label|default],datalist[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],footer[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],header[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],main[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],aside[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],nav[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],section[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],article[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],hgroup[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],figure[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang],time[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|datetime],dialog[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|open],command[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|type|label|icon|disabled|checked|radiogroup|command],output[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|for|form|name],progress[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|value|max],meter[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|value|min|max|low|high|optimum],details[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|open],keygen[id|accesskey|class|dir|lang|style|tabindex|title|role|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|xml:lang|autofocus|challenge|disabled|form|keytype|name]

@davejtoews
Copy link
Author

davejtoews commented Feb 21, 2024

Applying the above string w/ $config->setOption('valid_elements, $string) and then checking tinymce.get()[0].parser.schema.elements in the browser results in similar, but not identical results to just running tinymce.get()[0].parser.schema.elements for an unmodified config.

Unmodified version includes this for a.attributes.xml

        "xml": {
            "forcedValue": "lang"
        }

But in the one where we've passed in the string via PHP we get

        "xml:lang": {},

Unmodified has an attributesForced value w/ elements.a

    "attributesForced": [
        {
            "name": "xml",
            "value": "lang"
        }
    ]

Configured w/ php there's no attributes forced but there is

        "removeEmpty": true

Not sure if these make a big enough difference to be concerned about, nor do I know how to pass in a string that would make the results identical.

@blueo
Copy link
Contributor

blueo commented Feb 21, 2024

just a thought here - the CMS config is arguably what most people will expect as the 'default' - so whilst tinyMCE might have its own defaults perhaps it would be more consistent to stay with the CMS settings (perhaps without the funny extended valid elements)? The generated default above appears very permissive and maybe we should err on the side of 'sensible defaults' for a CMS product?

@davejtoews
Copy link
Author

That's the direction I was going with some of my questions but if the goal here is to restore the functionality that broke in v4.13.30 and not disrupt anyone else's configuration which may for some reason depend on the permissive settings the TinyMCE defaults make sense. Restoring what broke is a different task than revisiting the expectations, I suppose.

@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 21, 2024

The generated default above appears very permissive and maybe we should err on the side of 'sensible defaults' for a CMS product?

How do you decide which defaults are "sensible"? What do we do when we change to a less permissive default set and it breaks someone's site because they're relying on the permissive set that we currently inherit from TinyMCE?

That's definitely something we can look at doing in a major release where our opinion of what is sensible is allowed to be a breaking change, but for now we just want to not break things when extended_valid_elements is defined. It's a very narrow scope bug fix.

@GuySartorelli
Copy link
Member

GuySartorelli commented Feb 21, 2024

Although... actually. Now I'm thinking (which is always dangerous)... in 4.13.29 it wouldn't have been the full TinyMCE set would it? It would have been the "current config" set I think. So maybe we can just take the set that'd defined on cms and call it a default?

Thoughts on that?

@blueo
Copy link
Contributor

blueo commented Feb 21, 2024

Although... actually. Now I'm thinking (which is always dangerous)... in 4.13.29 it wouldn't have been the full TinyMCE set would it? It would have been the "current config" set I think. So maybe we can just take the set that'd defined on cms and call it a default?

Thoughts on that?

Hmm yeah I think it was essentially defaulting to the cms config for all sanitisation as it would get set here: https://github.com/silverstripe/silverstripe-admin/blob/2/code/LeftAndMain.php#L688
Well that assumes that the user would generally have CMS as their config

@GuySartorelli
Copy link
Member

Well that assumes that the user would generally have CMS as their config

That's a good point.

@GuySartorelli
Copy link
Member

All this discussion makes it pretty clear this issue needs a bit more thought put into how we choose to solve it - I've put it into our internal refinement column so we can discuss the options internally

@davejtoews
Copy link
Author

There's an underlying issue here of why setting extended_valid_elements changes the behaviour of valid_elements. Is that a TinyMCE issue or something to do with Silverstripe's particular use of it? Kind of seems like we're working out how to patch around this quirk without identifying why this quirk happens.

@GuySartorelli
Copy link
Member

There's an underlying issue here of why setting extended_valid_elements changes the behaviour of valid_elements.

it doesn't. It's just the difference between "something is set and I need to validate" vs "nothing is set so there's nothing to validate". At some point in the past someone made the decision that "nothing set" means "validate nothing" instead of "allow nothing".

if (!$this->elements && !$this->elementPatterns) {
return;
}

That just wasn't happening until now because the wrong config was being used for sanitisation, so there was something set (it just wasn't what the developer had intentionally set)

@GuySartorelli
Copy link
Member

So we definitely need to rip that condition out to get the valid elements respected properly. But we also have to add some default valid elements so we're not just stripping everyone's HTML out of their not-quite-configured-enough WYSIWYGs

@GuySartorelli
Copy link
Member

GuySartorelli commented Mar 13, 2024

FYI we have discussed this internally and added some acceptance criteria to the card.
@davejtoews if you're keen to try update your PR to meet these criteria please feel free - if not, one of the CMS Squad will tackle this in the near future.

The decision was ultimately:

  1. Revert the bugfix that caused this bug in patch releases. That's less likely to cause disruption than trying to fix this bug in a patch release. See Revert TinyMCE config bugfix prior to CMS 5.2 release #11179
  2. In a future minor release (it'll be in 5.3 unfortunately, we missed the window for 5.2), use the current "cms" set of elements as a default set of valid_elements since it is the behaviour most people will have been seeing prior to that original bugfix (assuming most people didn't alter the "cms" config and didn't set a different "default" set)

@emteknetnz
Copy link
Member

Linked PRs have been merged and will be released as part of the Silverstripe 5.3 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants