[DrupalHtmlEngine] HTML-reserved characters (>, <, &) in <script> and <style> tag are converted to HTML entities

I think for security purpose they disabled it in CKEditor5. Please try with CKEditor4.

What does your text format & text editor configuration look like? Please export both pieces of config and post/upload them here 🙏

I have also encountered this issue. It is not a CKEditor 5 base issue, or at least does not always happen with base CKEditor 5. A minimal CKEditor 5 configuration with source editing does not have the issue, but a minimal Drupal CKEditor 5 configuration does. I'm attaching minimal editor/format configs with the issue and an equivalent minimal vanilla CKEditor 5 setup.

To reproduce the issue in Drupal:

1. Import the attached editor.editor.source_editing.yml and filter.format.source_editing.yml config files to a Drupal site with CKE5 enabled
2. Visit a page with WYSIWYG editing and switch to the filter
3. Toggle the source editing mode
4. Enter content like: <style>span > a { }</style>
5. Toggle the source editing mode off and then on again
6. You will see that the editor has converted the > to >

To see the issue does not happen in vanilla CKEditor 5:

1. Extract the attached cke5_minimal_source_editing.tar.gz archive
2. Visit the index.html page contained within
3. Toggle the source editing mode
4. Enter content like: <style>span > a { }</style>
5. Toggle the source editing mode off and then on again
6. You will see that the editor retains the > instead of converting it to >

I've pushed up an MR and attached a patch with a simple fix which uses the raw textContent for script and style tags. There's probably room for improvement (e.g. there might be other tags affected, or maybe a less naive check than just checking the parent tag name should be used and possibly there should be a test), but this fixes the issue for me.

Definitely something that could benefit from a test case.

#7++

This patch/MR needs to expand core/modules/ckeditor5/tests/src/Nightwatch/Tests/drupalHtmlBuilderTest.js to become committable.

Also, it needs to first land in 11.x. A patch may be simpler in this case, so that you can test against all supported branches at the same time.

Would allowing the "style" tag to be rendered as raw, without standard filtering, cause security problems? Shouldn't we limit the patch here to just the "script" tag?

Jut filter the SCRIPT tag, leave the STYLE tag as-is.

FYI we ran into this on a client's site where they were pasting HTML into a text format that did not have the "Limit allowed HTML" filter enabled.

Moving back to "needs work" as it needs tests and definitive steps to reproduce.

#10 works for me. Drupal 10.1.3.

Would allowing the "style" tag to be rendered as raw, without standard filtering, cause security problems?

Based on \Drupal\Component\Utility\Xss::filterAdmin(): yes.

Because this is security-adjacent, this absolutely needs test coverage before it can be considered close to ready.

Tried to fix CCF in #10.

Re-roll the patch to 9.5.x

Running into similar issue that may help with knowing the scope...which is definitely beyond the script tag. When I add items with twig such it gets changed by CKEditor before it is even saved and before the twig filter executes:

Steps to reproduce: edit a node, view source and paste the following into the textarea.

<p>
    The item you requested in the catalog (<strong>{{ drupal_token('superglobals:_REQUEST:TITLE')|e('html')}}</strong>) is currently checked out. You have two options. You can either {{'<a href="/howto/recall?TITLE='~ drupal_token('superglobals:_REQUEST:TITLE')|e('url') ~ '&AUTHOR=' ~ drupal_token('superglobals:_REQUEST:AUTHOR')|e('url') ~ '&PUB=' ~ drupal_token('superglobals:_REQUEST:PUB')|e('url') ~ '&PERM=' ~ drupal_token('superglobals:_REQUEST:PERM')|e('url') ~ '&CALL=' ~ drupal_token('superglobals:_REQUEST:CALL')|e('url') ~ '&BIB=' ~ drupal_token('superglobals:_REQUEST:BIB')|e('url') ~ '&LOC=' ~ drupal_token('superglobals:_REQUEST:LOC')|e('url') ~ '&STATUS=' ~ drupal_token('superglobals:_REQUEST:STATUS')|e('url') ~ '&CAT=' ~ drupal_token('superglobals:_REQUEST:CAT')|e('url') ~ '">recall the item</a>' }} or you can request it from another participating.
</p>

Switch from view source to normal editing view and then back to view source and the code now looks like this:

<p>
    The item you requested in the catalog (<strong>{{ drupal_token('superglobals:_REQUEST:TITLE')|e('html')}}</strong>) is currently checked out. You have two options. You can either {{'<a href="/howto/recall?TITLE=&apos;~ drupal_token(&apos;superglobals:_REQUEST:TITLE&apos;)|e(&apos;url&apos;) ~ &apos;&amp;AUTHOR=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:AUTHOR&apos;)|e(&apos;url&apos;) ~ &apos;&amp;PUB=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:PUB&apos;)|e(&apos;url&apos;) ~ &apos;&amp;PERM=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:PERM&apos;)|e(&apos;url&apos;) ~ &apos;&amp;CALL=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:CALL&apos;)|e(&apos;url&apos;) ~ &apos;&amp;BIB=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:BIB&apos;)|e(&apos;url&apos;) ~ &apos;&amp;LOC=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:LOC&apos;)|e(&apos;url&apos;) ~ &apos;&amp;STATUS=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:STATUS&apos;)|e(&apos;url&apos;) ~ &apos;&amp;CAT=&apos; ~ drupal_token(&apos;superglobals:_REQUEST:CAT&apos;)|e(&apos;url&apos;) ~ &apos;">recall the item</a>' }} or you can request it from another participating.
</p>

Note that the apostrophe is turned into &apos; in some areas, but not all, the first line seems untouched but the rest are munged.

Updated the patch #15 to include the updated library version for aggregation for 10.1.x

Closed #3398013: CKEditor 5 + Full HTML prevents the child combinator CSS selector to be used in <style> as a duplicate of this — thanks @ramprassad for connecting the dots!

Clarifying this is a bug in Drupal's DrupalHtmlEngine plugin.

@Wim

There is a related issue I have created based on one of the reported issues in our project. Can you please check? https://www.drupal.org/project/drupal/issues/3408656

Regards,
Ramprassad

Hello,
I have the same issue when trying to add

The patch no longer applies under D10.2.1

Adding the patch for latest D10.2.x for allowing only script tag

Adding the patch for latest D10.2.x for allowing script and style tags as per our requirement.

Confirming #25 works for me...thanks!

One more patch for 11.x for allowing script and style tags

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fix the patch applied issue for 11.x
Please Review.

Please look at the tags for items to address before putting in review.

Still needs steps to reproduce added to the issue summary, actually needs a full issue summary update

Needs test.

Not ready for review yet.

Added a MR with the relevant test, and requires review.

Hiding patches for clarity.

Added missing sections to issue summary, proposed solution will need to be filled in

Leaving in review for additional eyes.

Only 1 of the 3 test cases fails in the test-only CI job. For the style-only test coverage that's because there's no > anywhere in the CSS 😄 Easily fixed.

I'd like to see all 3 new test cases fail 🙏 Then I'll happily RTBC! Having looked at the code, I withdraw the Needs security review that I added 9 months ago in #3 — that was because the title/report was alarming at the time, now it is not :)

@Wim Leers good spot on the test-only changes! I switched to the @dataProvider last minute and didn't really contemplate the test cases properly.

Those relevant features have been implemented, and awaiting review

Seems all 3 tests are now failing with feedback addressed.

Added a comment to the MR. I'm not sure that doing 4 reinstalls just to use a text editor is necessary... see comment on the MR.

Implemented the feedback, however it makes testing all the test cases via the test only pipeline a little harder.

Appears feedback has been addressed to not use a dataprovider.

it makes testing all the test cases via the test only pipeline a little harder.

Indeed, but that's a fine trade-off.

In #37 I wrote that all 3 tests should fail. But now there are six test cases.

I saw the 3 failures in https://git.drupalcode.org/issue/drupal-3364884/-/jobs/1119730 … which means the other 3 passed. These 3 did not fail:

1. script like tag
2. script to escape
3. unescaped script tag

Why is that not a problem? 🤔 I feel like I'm overlooking something? 🙈 Sorry!

@Wim Leers

Why is that not a problem? 🤔 I feel like I'm overlooking something? 🙈 Sorry!

I introduced 3 new tests in the latest commits (which weren't part of the original test-only pipeline that was ran), which is probably the main discrepancy.

The https://git.drupalcode.org/issue/drupal-3364884/-/jobs/1119730 job was for the 490e9434 commit which still had 3 test cases not 6.

All 6 tests

Behat\Mink\Exception\ExpectationException : The string "<script>(function() { let x = 10, y = 5; if( y <--x ) { console.log("run me!"); }})()</script>" was not found anywhere in the HTML response of the current page.

Removed that one ran next 5

Behat\Mink\Exception\ExpectationException : The string "<script>(function() { let player = 5, script = 10; if (player<script) { console.log("run me!"); }})()</script>" was not found anywhere in the HTML response of the current page.

Removed that one ran next 4

Behat\Mink\Exception\ExpectationException : The string "<script>const example = 'Consider this string: <!-- <script>';</script>" was not found anywhere in the HTML response of the current page.

Removed that one ran next 3

Behat\Mink\Exception\ExpectationException : The string "<script>
  const example = 'Consider this string: <!-- <script>';
  console.log(example);
</script>
<!-- despite appearances, this is actually part of the script still! -->
<script>
  let a = 1 + 2; // this is the same script block still...
</script>" was not found anywhere in the HTML response of the current page.

Removed that one ran next 2

Behat\Mink\Exception\ExpectationException : The string "<style>
a > span {
  /* Important comment. */
  color: red !important;
}
</style>" was not found anywhere in the HTML response of the current page.

Removed that one ran last one

Behat\Mink\Exception\ExpectationException : The string "<script type="text/javascript">
let x = 10;
let y = 5;
if(y < x){
console.log('is smaller')
}
</script><style type="text/css">
:root {
  --main-bg-color: brown;
}
.sections > .section {
  background: var(--main-bg-color);
}
</style>" was not found anywhere in the HTML response of the current page.

I fixed some whitespace stuff on commit in the test to make it easier to maintain in the future.

Committed and pushed 762d5b9adf to 11.x and ad38efa765 to 10.3.x. Thanks!

I considered backporting to 10.2.x but the changes to JS gave me pause - not sure if we'd need an update function to clear cached js aggregates to left in 10.3.x

I do not get why we have this test case:

      'unescaped script tag' => [
        <<<HTML
        <script>
          const example = 'Consider this string: <!-- <script>';
          console.log(example);
        </script>
        <!-- despite appearances, this is actually part of the script still! -->
        <script>
          let a = 1 + 2; // this is the same script block still...
        </script>
        HTML,

This is not the test I added. Can someone explain to me despite appearances, this is actually part of the script still! because I do not see a reason why this should be only one script tag. For me it's 2 different script tags and should be treated as such. If it's to get around the behavior I'm seeing below it should not work that way.

I added the test case:

<script>
const example = 'Consider this string: <!-- <script>';
console.log(example);
</script>

And that is transformed into:

<script>
const example = 'Consider this string: &amp;lt;!-- &amp;lt;script&amp;gt;';
console.log(example);
&amp;lt;/script&amp;gt;
&amp;lt;/body&amp;gt;</script>

Above creates a syntax error when it's viewed on the node. The actual result should be somethings like:

<script>
const example = 'Consider this string: \x3C!-- \x3Cscript>';
console.log(example);
</script>

So while there is not an issue with our code, CKEditor upstream doesn't parse this correctly and adds a closing script and body tag too early. Having a second script tags seems to go around the problem but fact is that there is still an issue and at least a follow-up and/or upstream issue is needed to fully support <script> tags in wysiwyg.

Opened a followup for this, it's an edge case and this is less broken than it was, so not convinced we need to revert: #3437394: [DrupalHtmlEngine] Follow-up for script and style tags in CKEditor 5

Patch is failing on D10.2.6, could someone please re-roll it?

Patch for Ckeditor5 , Drupal 10.2.4

To handle the issue & tag converting to &amp use

'#value' => Markup::create($vwo)

Oooph. Let's say you've got a site with hundreds of content editors that had this bug for about a year while the site was on D10 but before it got to 10.3. You've now got hundreds of busted script and style tags. (Yes it was bad to allow these tags, but we inherited this site created by a vendor who doesn't do Drupal anymore). Now we've got to write an update to clean this up. something along the lines of:

1. Get all base fields that accept formatted text.
2. Run a query on each table to get all rows containing <script or <style
3. Put the content through a DOM parser. Process all the script and style tags. Get their text.
4. Do some regex search/replace. Maybe something like

  $count = 0;
  $clean_content = preg_replace(['/&(amp;)*lt;/i', '/&(amp;)*gt;/i'], ['<', '>'], $original_content, -1, $count);
  if ($count) {
    // Write the clean content back to the database.
  }

(the regex above doesn't yet fix corrupted &&).

Some questions:
1. Is this cleanup something that should be in Core, since this caused data corruption over a long period of time between 10.0.0 and 10.2.10 inclusive (i.e. this issue probably should've been marked as Critical, not Major)?
2. If not, should it at least be tracked here (or in a new issue) so that others can find it?

Re-rolling the patch for D10.3.x

For Drupal 9.5 - combining patch #16 with patch #28 on Issue 3300404 for PHP 8.1 compatibility.