silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

HTMLEditorField doesn't use field assigned configuration for sanitisation #10975

Closed blueo closed 11 months ago

blueo commented 1 year ago

Affected Version

tested on 4.12 and 5

Description

Saving an HTMLEditorField always uses the 'active' HTMLEditorConfig rather than the config defined on the field when sanitising the content for saving into the database.

I would have expected the field to sanitise its content using the config set on the field.

It means it is not possible to have mor than one kind of server side validation. This probably goes un-noticed as the client will prevent adding invalid content in the JS and the default valid elements is fairly permissive. However, it is possibly to bypass the client restrictions (eg with the source button)

Steps to Reproduce

This test will reproduce the issue:

expand code sample ```php setOption('valid_elements', implode(',', $defaultValidElements)); HTMLEditorConfig::set_active_identifier('cms'); $field = HTMLEditorField::create('Standard'); $htmlValue = '

standard text

'; $expectedHtmlString = '

standard text

Header'; $field->setValue($htmlValue); $dummyObject = new class extends DataObject { }; $field->saveInto($dummyObject); $this->assertEquals($expectedHtmlString, $dummyObject->Standard); $tableValidElements = [ 'table', 'thead', 'tbody', 'tr', 'th', 'td', ]; $tableConfig = HTMLEditorConfig::get('table'); $tableConfig->setOption('valid_elements', implode(',', $tableValidElements)); // HTMLEditorConfig::set_active_identifier('table'); $tableField = HTMLEditorField::create('Standard'); $tableField->setEditorConfig($tableConfig); $htmlValue = '

standard text

Header
Header
'; $expectedHtmlString = '

standard text

' .'
Header
'; $tableField->setValue($htmlValue); $dummyObject2 = new class extends DataObject { }; $tableField->saveInto($dummyObject2); $this->assertEquals($expectedHtmlString, $dummyObject2->Standard); } } ```

if you uncomment HTMLEditorConfig::set_active_identifier('table'); then the test will pass.

Setting the active identifier works unless you have multiple different configurations within the same form - as there can only be one active config at time.

Save into could instead use the config attached to the field and fall back to the active one?

PRs

GuySartorelli commented 1 year ago

Thanks for reporting this.

Save into could instead use the config attached to the field and fall back to the active one?

Yup that sounds like the correct course of action. Would you be willing to raise a PR to implement this fix?

blueo commented 11 months ago

Thanks for reporting this.

Save into could instead use the config attached to the field and fall back to the active one?

Yup that sounds like the correct course of action. Would you be willing to raise a PR to implement this fix?

PR attached now :)

GuySartorelli commented 11 months ago

PR was merged. This will be automatically tagged by GitHub Actions.

GuySartorelli commented 6 months ago

This was reverted - see https://github.com/silverstripe/silverstripe-framework/issues/11141 for details