Removal of newlines in CDATA breaks inline scripts

[jeremyfelt]

As of 4.2.3, depending on how a CDATA block is used, the stripping of new lines in this content may break when displayed on the front end, even when published with unfiltered_html capabilities.

Intended content:

<script>// <![CDATA[
_my_function('data');
// ]]>
</script>

In 4.2.2, this content is left as is and _my_function() fires as expected. In 4.2.3, the content is manipulated as such:

<script>// <![CDATA[ _my_function('data'); // ]]></script>

This results in the script being commented out by the // and it will not fire.

A workaround for this is to use /* for commenting.

<script> /* <![CDATA[ */ _my_function('data'); /* ]]> */ </script>

[miqrogroove]

Attached patch is a rough draft and does not work. Just brainstorming.

[iseulde]

Duplicate? #33099

[chriscct7]

With 33106-testcase.html

Tested in latest Chrome, FireFox, and Opera by hand, and then emulated 101 other browser/browser version/OS combos.

The ones that failed:

• Konqueror 4.4
• Konqueror 4.8
• Konqueror 4.9
• Opera 10.60
• Opera 10.53

None of major browser/browser version/os combos failed emulation testing.

[miqrogroove]

Excuse the format; I wrote this in Notepad at a hotel.

[dd32]

Seemingly related is #33099 which uses a similar format:

<script><!--
my_function();
\\ --> </script>

[chriscct7]

Do we care about nested CDATA entries like: <![CDATA[]]]]><![CDATA[>]]>

[miqrogroove]

Replying to chriscct7:

Do we care about nested CDATA entries like: <![CDATA[]]]]><![CDATA[>]]>

No problems there.

33106.2.patch should be very close to what we need now.

[azaozz]

Alt patch for autoembed

[azaozz]

33106.3.patch is an alternative for autoembed(). Instead of removing line breaks it doesn't look inside of HTML tags (including comments and CDATA). It changes the behavior a bit and fixes #25387.

[chriscct7]

#33099 was marked as a duplicate.

[miqrogroove]

33106.4.patch works.

[miqrogroove]

33106.5.patch Adds a few unit tests for wpautop only. The tests/media.php file was returning nothing but false positives so I didn't add anything in there. The oEmbed system calls get_post and I wasn't sure how to initialize it or why none of the other tests are doing it.

[kitchin]

Results with only the tests in 33106.5.patch and 33106.tests.media.patch:

phpunit tests_phpunit_tests_formatting_Autop
Tests: 14, Assertions: 19, Failures: 5.

phpunit tests_phpunit_tests_media
Tests: 32, Assertions: 82, Failures: 2.

Results with all of 33106.5.patch and 33106.tests.media.patch:

phpunit tests_phpunit_tests_formatting_Autop
OK (14 tests, 19 assertions)

phpunit tests_phpunit_tests_media
OK (32 tests, 82 assertions)

[kitchin]

Fixed, 33106.tests.media.2.patch has same results as previous.
It tests the original CDATA case in this bug and the case mention in comment:4, <!-- ... //-->.

[miqrogroove]

kitchin, I'm not sure if those autoembed tests are valid. When I add a bare URL as a baseline test, the autoembed function still does nothing in the tests. It runs the regexp but the ::shortcode() is returning input with no changes. Could you try that?

[kitchin]

Appears that ->autoembed does not do the embedding, instead it canonicalizes the text for later work by the oEmbed service, and for the [embed] shortcode.

I'm attaching my patch with a third test case, using apply_filters('the_content', ....) to get a bigger picture:

• If youtube link: any [embed] is removed from around the link.
• If youtube link: canonicalized from http://.../embed/... to https://.../watch?v=....
• Other links: [embed] becomes <a href=....
• Other links without [embed]: no change.

Not sure the third test should be checked in, it's not targeted to a single issue.

All 3 tests I've written fail before your patch.5 and succeed after.

[kitchin]

Maybe too much. Third additional test to show the_content application.

[azaozz]

The autoembed() part of the 33106.5.patch doesn't work here. It replaces embeddable URLs that are wrapped in a link. These should not be embedded. To reproduce:

1. Paste this in the Text editor:

<a href="https://www.youtube.com/watch?v=BZL3CSUwoTc">https://www.youtube.com/watch?v=BZL3CSUwoTc</a>

2. Preview the post. You should see a link to that video, instead it is embedded (and the <a> tag is broken).

To fix that we either need to revert to removing line breaks in autoembed() or do something like 33106.3.patch.

[azaozz]

In 33106.6.patch: revert how autoembed() works. Replace line breaks with placeholders, search for embeddable URLs on their own line, then put the line breaks back (similarly to how wpautop() works).

[kitchin]

33106.6.patch passes my three unit tests.

[cooperator_JR]

Same problem Here: ​https://wordpress.org/support/topic/simple-javascript-to-show-daymonth-stop-working-after-423-update-1

ORIGINAL CODE:

Altered CODE "automaticaly", after 4.2.3 UPDATE:

The 2 codes works equally on an external test environment. ( not posts or Wordpress)
Well, lets spend an overnight to remove ...

// <![CDATA[

and then this, right before the final close script tag

// ]]>

...from 175 posts that use the code...

It´s the life! :-)
Happy to know that you´re working to solve.

[miqrogroove]

Replying to kitchin:

Appears that ->autoembed does not do the embedding, instead it canonicalizes the text for later work by the oEmbed service, and for the [embed] shortcode.

So how do we test the oEmbed service? How do we make sure to not break this in the future?

[miqrogroove]

33106.7.patch expands the wp_html_split() idea, making $regex static and implementing the new function in do_shortcode().

[schtief]

Hi Wordpress-Team,

please just replace

// <![CDATA[
by
/* <![CDATA[ */

and

// ]]>
by
/* ]]> */

With this CDATA-definition also inline JS are supported:

/* <![CDATA[ */document.write('hello');/* ]]> */

As a work around you can also define a filter in the functions.php for this task, but it would be great if native WP would support "one-line" (minified) javascript:

<?php
// somewhere in the functions.php
function my_filter_cdata( $content ) {
  $content = str_replace( '// <![CDATA[', '/* <![CDATA[ */', $content );
  $content = str_replace( '// ]]>', '/* ]]> */', $content );
  return $content;
}
add_filter( 'content_save_pre', 'my_filter_cdata', 9, 1 );

Thank you

[kitchin]

Replying to miqrogroove:

Replying to kitchin:

Appears that ->autoembed does not do the embedding, instead it canonicalizes the text for later work by the oEmbed service, and for the [embed] shortcode.

So how do we test the oEmbed service? How do we make sure to not break this in the future?

Grepping I do not see a test harness for the service. Do you know if round-tripping TinyMCE is also not covered? I mean, emulating a user throwing some html into TinyMCE.

[kitchin]

New tests coming up. These tests require a new hook in WP_Embed::shortcode() so they may not be accepted. The new tests detect the bug in patch.5 found by azaozz in comment:17.

Three patches coming up. Only use two of them.

• 33106.tests.media.mod-hook.4.patch
  The new tests. They require one of the following two patches to trunk:

• 33106.5.mod-hook.patch
  Same as 33106.5.patch, plus the new hook in shortcode().
  Will get one failure in:
  $ phpunit tests_phpunit_tests_media

• 33106.7.mod-hook.patch
  Same as 33106.7.patch, plus the new hook in shortcode().
  Will get no failures in:
  $ phpunit tests_phpunit_tests_media

[miqrogroove]

Cool ideas. Thank you kitchin. Just need to add one or two tests where a CDATA block contains a URL on its own line. The idea is to make sure we are not now or in the future going to autoembed anywhere in the middle of an inline script. Then combine the tests with 33106.7.mod-hook.patch so we have it all in one patch.

[miqrogroove]

Tests from kitchin added with some tweaks. Fixes from azaozz still included.

[miqrogroove]

Latest patch adds all the tests we came up with. While it doesn't fully process oEmbed, I am confident that it now covers relevant changes in WP_Embed::autoembed().

[stevenkword]

Replying to chriscct7:

Do we care about nested CDATA entries like: <![CDATA[]]]]><![CDATA[>]]>

I believe nested CDATA like this breaks RSS, but that's a little outside the point of the ticket.

[markjaquith]

#33159 was marked as a duplicate.

[millertimesites]

Looks like this CDATA problem with 4.2.3 is affecting all Google Adsense code.
If the code below was my prior code, what should I change it to in order to get it working again?

<script src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js" async=""></script><!-- 336x280 Content --> <ins class="adsbygoogle" style="display: inline; float: left; margin: 5px; width: 336px; height: 280px;" data-ad-client="ca-pub-3788205869703165" data-ad-slot="5395550042"></ins><script>// <![CDATA[
(adsbygoogle = window.adsbygoogle || []).push({});
// ]]></script>

Thanks

[miqrogroove]

Hi @millertimesites , until the patch is installed, one of the workarounds for this bug is to replace // <![CDATA[ with /* <![CDATA[ */

[millertimesites]

I tried that by changing

<script src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js" async=""></script><!-- 336x280 Content --> <ins class="adsbygoogle" style="display: inline; float: left; margin: 5px; width: 336px; height: 280px;" data-ad-client="ca-pub-3788205869703165" data-ad-slot="5395550042"></ins><script>// <![CDATA[
(adsbygoogle = window.adsbygoogle || []).push({});
// ]]></script>

to

<script src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js" async=""></script><!-- 336x280 Content --> <ins class="adsbygoogle" style="display: inline; float: left; margin: 5px; width: 336px; height: 280px;" data-ad-client="ca-pub-3788205869703165" data-ad-slot="5395550042"></ins><script>/* <![CDATA[
(adsbygoogle = window.adsbygoogle || []).push({});
*/ ]]></script>

and it didn't fix anything.
I assume that the 1,000 of us using AdSense on WordPress should just stand pat and deal with it until whenever a patch is installed?

[miqrogroove]

Yeah that won't fix anything because that's the wrong code.

[millertimesites]

If it is the wrong code, then what should it be?
The code I was using always worked great until the 4.2.3 release and was from the
official AdSense code
The code from Google:

<script async src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js"></script>
<!-- 336x280 Content -->
<ins class="adsbygoogle"
     style="display: inline; float: left; margin: 5px; width: 336px; height: 280px;"
     data-ad-client="ca-pub-3788205869703165"
     data-ad-slot="5395550042"></ins>
<script>
(adsbygoogle = window.adsbygoogle || []).push({});
</script>

Is automatically changed to

<script src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js" async=""></script><!-- 336x280 Content --> <ins class="adsbygoogle" style="display: inline; float: left; margin: 5px; width: 336px; height: 280px;" data-ad-client="ca-pub-3788205869703165" data-ad-slot="5395550042"></ins><script>// <![CDATA[
(adsbygoogle = window.adsbygoogle || []).push({});
// ]]></script>

by WordPress when you save the changes

[azaozz]

@millertimesites try adding this to a plugin:

add_filter( 'the_content', '_my_fix_cdata', 20 );
function _my_fix_cdata( $content ) {
	if ( strpos( $content, '// <![CDATA[' ) !== false ) {
		$content = preg_replace( '%(<script[^>]*>)// <!\[CDATA\[(?![\r\n])%', '$1/* <![CDATA[ */', $content );
	}

	return $content;
}

[millertimesites]

@azaozz
Thanks for the suggestion.

I don't think that is going to work as I am not using a plugin or widget for AdSense and am using the raw code provided by AdSense like most AdSense users.

The code needs to be able to work inline with the content/text of each page.
Maybe there is a solution that can be added to the theme's Quick CSS?

[wonderboymusic]

In 33469:

Protect newlines inside of CDATA. This was breaking things, notably inline JS that used comments for HTML standards compat.

• Tokenize newlines in WP_Embed::autoembed() before running ->autoembed_callback()
• Tokenize newlines with placeholders in wpautop()
• Introduce wp_html_split() to DRY the RegEx from wp_replace_in_html_tags() and do_shortcodes_in_html_tags()

Adds unit tests.

Props miqrogroove, kitchin, azaozz.
Fixes #33106.

[wonderboymusic]

for 4.2.4

[azaozz]

There is no need to add a new filter in WP_Embed::shortcode() just to be able to test part of it. The embed_maybe_make_link filter can be used instead.

[azaozz]

In 33470:

Use the embed_maybe_make_link filter to test WP_Embed::autoembed().
See #33106.

[SergeyBiryukov]

Replying to millertimesites:

The code needs to be able to work inline with the content/text of each page.
Maybe there is a solution that can be added to the theme's Quick CSS?

Until 4.2.4 is out, try adding the code from comment:39 to the theme's function.php file, it should work for all posts and pages (just tested).

[azaozz]

In 33518:

Backport r33469 and r33470 to 4.2.
See #33106.

[millertimesites]

@SergeyBiryukov I added it to the function.php file of my child theme, however it did not change anything for me. None of the pages updated to make AdSense visible again.

When I put clean AdSense code back into a page I could see the ad in the backend editor, but once I clicked save it revert to a blank space again.

Jon

[millertimesites]

I actually switched to using shortcodes for each Adsense unit/style and for the first couple pages it seems to be working great. Since the code isn't handled at a page level this way it isn't broken by WordPress when you save it.
Jon

[azaozz]

In 33521:

Backport r33469 and r33470 to 4.1.
See #33106.

[azaozz]

In 33522:

Backport r33469 and r33470 to 4.0.
See #33106.

[azaozz]

In 33523:

Backport r33469 and r33470 to 3.9.
See #33106.

[azaozz]

In 33524:

Backport r33469 and r33470 to 3.8.
See #33106.

[azaozz]

In 33525:

Backport r33469 and r33470 to 3.7.
See #33106.

[miqrogroove]

Fixed by [33469] [33470] [33518] [33521] [33522] [33523] [33524] and [33525].