WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#34698 closed defect (bug) (fixed)

HTML-Embed code JavaScript error.

Reported by: nendeb55 Owned by: pento
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch
Focuses: Cc:

Description

I copy a cord of HTML-Embed from a page of Embed and input in a post of WordPress4.3.
JavaScript code && is converted to the && and get an error.

I suggest that I change & to influence in wptexturize to the cord not to use in wp-embed.js.

Attachments (4)

34698.diff (1.5 KB) - added by pento 4 years ago.
34698.2.diff (1.6 KB) - added by pento 4 years ago.
34698.3.diff (1.9 KB) - added by peterwilsoncc 4 years ago.
34698.4.diff (628 bytes) - added by peterwilsoncc 3 years ago.

Download all attachments as: .zip

Change History (49)

#1 @swissspidy
4 years ago

Hey there

Did you paste the embed code in the visual editor or the text editor?

Also, you need to make sure to have the unfiltered_html capability (usually admins & editors have these on single sites).

#2 @nendeb55
4 years ago

It is a text editor.
In administrative posts.

SyntaxError: illegal character
|h)&&d.getAttribute("security")&&(a=d.clone

#3 @obenland
4 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner set to swissspidy
  • Status changed from new to assigned

#4 @pento
4 years ago

This is caused by convert_chars() and wptexturize() indiscriminately converting & to &#038;, which it shouldn't be doing to & characters within <script> tags.

@pento
4 years ago

#5 @pento
4 years ago

  • Keywords has-patch dev-feedback needs-unit-tests added

34698.diff is a first pass at fixing this.

I removed convert_chars() from the the_content filter, as it seems to be made entirely redundant by wptexturize().

Moving the &#038; replacement to inside the loop fixes this bug, but it causes some unit tests to fail:

1) Tests_Formatting_WPTexturize::test_tag_avoidance with data set #4 ('[ photos by <a href="http://example.com/?a[]=1&a[]=2"> this guy </a> ]', '[ photos by <a href="http://example.com/?a[]=1&#038;a[]=2"> this guy </a> ]')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[ photos by <a href="http://example.com/?a[]=1&#038;a[]=2"> this guy </a> ]'
+'[ photos by <a href="http://example.com/?a[]=1&a[]=2"> this guy </a> ]'

/srv/www/wordpress-develop/tests/phpunit/tests/formatting/WPTexturize.php:1248

2) Tests_Formatting_WPTexturize::test_tag_avoidance with data set #5 ('[photos by <a href="http://example.com/?a[]=1&a[]=2"> this guy </a>]', '[photos by <a href="http://example.com/?a[]=1&#038;a[]=2"> this guy </a>]')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[photos by <a href="http://example.com/?a[]=1&#038;a[]=2"> this guy </a>]'
+'[photos by <a href="http://example.com/?a[]=1&a[]=2"> this guy </a>]'

/srv/www/wordpress-develop/tests/phpunit/tests/formatting/WPTexturize.php:1248

@miqrogroove, could you please lend your expertise to this?

#6 @nendeb55
4 years ago

Thanks for the patch.

I will be reliable in this in future.
But I cannot cope in WordPress of the version that is old when it is this.
I suggest that I change a cord in wp-embed.js,wp-embed.min.js and get_post_embed_html().

Exsample in wp-embed.js

/* Only continue if link hostname matches iframe's hostname. */
if ( targetURL.host === sourceURL.host && document.activeElement === source ) {
	window.top.location.href = data.value;
}

To

/* Only continue if link hostname matches iframe's hostname. */
if ( targetURL.host === sourceURL.host ) {
	if (document.activeElement === source ) {
		window.top.location.href = data.value;
	}
}

&& It has been used three locations

#7 @pento
4 years ago

Thanks for the feedback, @nendeb55!

You make a good point, that this can still cause issues in older WordPress versions. I think that removing those instances of && that you point out would be a good solution.

@pento
4 years ago

#8 @pento
4 years ago

34698.2.diff should do the trick.

#9 @peterwilsoncc
4 years ago

In 34698.3.diff I've moved shuffled the for loop to avoid the nested if statements.

#10 follow-up: @nendeb55
4 years ago

Thanks for These patch.

Finally, please create a JavaScript-code also patches in the wp-embed.min.js and embed-functions.php of get_post_embed_html ().

#11 in reply to: ↑ 10 @peterwilsoncc
4 years ago

Replying to nendeb55:

Thanks for These patch.

Finally, please create a JavaScript-code also patches in the wp-embed.min.js and embed-functions.php of get_post_embed_html ().

The .min file and the function are created automatically during the build process from the file in the patch by grunt. Only the uncompressed file needs to be changed in this repo.

I'm looking over what uglify.js is doing during the compression process to ensure it doesn't recreate all the && that we are removing here.

#12 follow-up: @nendeb55
4 years ago

Thanks.

Does get_post_embed_html(), too?

#13 in reply to: ↑ 12 @peterwilsoncc
4 years ago

Replying to nendeb55:

Does get_post_embed_html(), too?

It does, yes.

~

I'm stuck on ensuring uglify doesn't re-introduce the && back in to the compressed code.

Most reliable method going forward seems to be to stop uglify optimising the if/for loops in this file. Reversing the logic worked in a couple of instances (!(x!=y||z!=y)) but it's not as future friendly as a custom build.

Any suggestions from the build tools team would be grand :)

#14 @nendeb55
4 years ago

Thank you for plain explanation.

#15 @pento
4 years ago

In 35708:

Embeds: Remove & characters from the inline embed JS.

Older versions of WordPress will convert those & characters to &#038;, which makes for some non-functional JS. If folks are running an older release, let's not make their lives more difficult than it already is.

Props pento, peterwilsoncc.

See #34698.

#16 @pento
4 years ago

That fixes one half of the problem, still need to figure out what the deal is with 34698.diff.

#17 @pento
4 years ago

On further reflection, the & within the href should be (and is) handled by KSES. The existing behaviour is incorrect.

#18 @pento
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 35709:

Texturize: Only convert & to &#038; within text nodes.

Previously, & would be converted everywhere, which caused problems when it was converted within a <script>, for example.

convert_chars() is now removed from the the_content filter, as it was doing the same job as wptexturize().

KSES correctly handles converting & within HTML attributes, so there's no need for wptexturize() and convert_chars() to do the same job.

Fixes #34698.

#19 @miqrogroove
4 years ago

Is there still time to review this before 4.4? Or can this be reverted? Just wondering why we are changing the output of href values without some extra consideration.

#20 @miqrogroove
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs explanations for why this is marked as a trunk defect, why the patch is committed late in the cycle, and why the unit tests are being modified.

#21 @pento
4 years ago

  • Keywords has-patch needs-unit-tests removed
  • Owner changed from swissspidy to miqrogroove
  • Status changed from reopened to assigned

It's a defect because it breaks &s inside a <script> tag.

There are a few scenarios I considered before committing this change:

  • A user without unfiltered_html posts content with an & in it. In an HTML attribute, it will be converted to &amp; by KSES, in a text node it, it will be converted to &#027; by texturize, or if it's inside a <script> tag, the tag is going to be removed.
  • A user with unfiltered_html posts content with an & in it. In an HTML attribute, KSES didn't touch it, as it shouldn't. Texturize will no longer convert it to &#027;, which in the case of an href attribute, was incorrect, anyway. In a text node, it will still be correctly converted to &#027; by texturize.
  • The HTML came from an embed. In a text node, it will still be converted to &#027;. In a <script> tag, it will now no longer be converted.

I'm happy to make tweaks if there are other scenarios you think should be considered, but the change to <script> and href behaviour should stay.

#22 @pento
4 years ago

My mistake, I don't know why I was testing with &#027; instead of &#038; in the href, the latter works correctly.

In which case, I'm not particularly fussed about what happens to an & inside an attribute for unfiltered_html and embed output (all browsers recognise an un-encoded & in an href) - the <script> change just needs to stay.

#23 @nendeb55
4 years ago

HTML-Embed Code

Error has come out in a compressed JavaScript in the get_post_embed_html().
But it works fine if you try in SCRIPT_DEBUG.
It works correctly when further compress the wp-embed.js in another way.

Is there a problem in the compression stroke of JavaScript?

FireBug

TypeError: j.style is undefined
...ded-content");for(c=0;c<j.length;c++)j.style.display="none";for(c=0;c<i.length;c...

#24 follow-up: @nendeb55
4 years ago

Error I found that to come out only when you enable the SyntaxHighlighter Evolved Plugin.
SyntaxHighlighter Evolved Plugin had been rewritten in this way the script.

for(c=0;c<j.length;c++)j[c].style.display="none";for(c=0;c<i.length;c++)if(d=i[c],d.style.display="

To

for(c=0;c<j.length;c++)j.style.display="none";for(c=0;c<i.length;c++)if(d=i,d.style.display="",

#25 in reply to: ↑ 24 @peterwilsoncc
4 years ago

Replying to nendeb55:

Error I found that to come out only when you enable the SyntaxHighlighter Evolved Plugin.
SyntaxHighlighter Evolved Plugin had been rewritten in this way the script.

I'm unclear, could you please clarify:

  • does the error occur if the plugin is disabled either with/without SCRIPT_DEBUG?
  • is the code edit your making to the embed JavaScript or the plugin's JavaScript?

#26 @pento
4 years ago

@viper007bond - Looks like this change may've had an effect on SyntaxHighlighter Evolved.

#27 @nendeb55
4 years ago

Apparently SyntaxHighlighter Evolved Plugin seems to react to the short code [c].
It was working properly If you remove the c of syntaxhighlighter_brushes definition of syntaxhighlighter.php.

..for(c=0;c<j.length;c++)j[c].style.display="none";for(c=0;c<i.length;c++)if(d=i[c],d.style.display="..

To

..for(c=0;c<j.length;c++)j.style.display="none";for(c=0;c<i.length;c++)if(d=i,d.style.display=""..

#28 follow-up: @Viper007Bond
4 years ago

My plugin stupidly registers a bunch of shortcodes, the idea being you could just do [php]code here[/php] instead of [code lang="php"]code here[/code]. I've since come to regret this.

Unfortunately if I reverse this decision, it could easily break past usages of the shortcode. Perhaps I can play with filter orders instead? Currently my plugin parses its shortcodes at priority 7 (not the default shortcode priority).

Last edited 4 years ago by Viper007Bond (previous) (diff)

#29 @nendeb55
4 years ago

Hi Viper007Bond

Thank you for your comments.
Cause is understood as an individual basis.

#30 in reply to: ↑ 28 ; follow-up: @dd32
4 years ago

Replying to Viper007Bond:

My plugin stupidly registers a bunch of shortcodes, the idea being you could just do [php]code here[/php] instead of [code lang="php"]code here[/code]. I've since come to regret this.

Unfortunately if I reverse this decision, it could easily break past usages of the shortcode. Perhaps I can play with filter orders instead? Currently my plugin parses its shortcodes at priority 7 (not the default shortcode priority).

It sounds like you're parsing shortcodes inside of <script> tags, which you probably shouldn't do :)

This ticket was mentioned in Slack in #core by helen. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 years ago

#33 @pento
4 years ago

  • Keywords dev-feedback removed
  • Resolution set to fixed
  • Status changed from assigned to closed

Closing this ticket now, no-one has anything to add here or in the Slack discussions above.

If any new bugs crop up related to embeds or texturize behaviour, please open a new ticket.

#34 @miqrogroove
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I still need to voice my strong objection to allowing this change in a Release Candidate without more testing and proper unit test coverage. This should be a 4.5-early ticket. Please either revert or remove my name as owner.

#35 @dd32
3 years ago

  • Owner changed from miqrogroove to pento
  • Status changed from reopened to assigned

#36 @pento
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#37 in reply to: ↑ 30 @Viper007Bond
3 years ago

Replying to dd32:

It sounds like you're parsing shortcodes inside of <script> tags, which you probably shouldn't do :)

Well then WordPress is to blame as I'm using core functions, just with only my shortcodes temporarily registered (I restore the normal list when I'm done).

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


3 years ago

#39 @peterwilsoncc
3 years ago

  • Keywords needs-patch added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as the JS file requires a comment about avoiding the & character in future edits.

#40 @peterwilsoncc
3 years ago

  • Keywords has-patch added; needs-patch removed

In 34698.4.diff some docs to warn against using the ampersand character in the embed JavaScript.

#41 @nacin
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 35762:

Embeds: Enforce, via unit tests, the no-ampersand rule for wp-embed.js.

fixes #34698.

#42 @nacin
3 years ago

My commit, though insane, was reviewed by @helen, @markjaquith, and @jorbin.

Thanks for the patch here, @peterwilsoncc. I noticed this same thing a few days ago and had been planning to write a test to validate the lack of ampersands. It's important to not only document decisions, but also verify them where we can. :-)

#43 @nendeb55
3 years ago

WordPress4.4 the release version of wp-embed-template.js
Place I was asked to fix the &, it seems to go back to the beta-version.

#44 @nendeb55
3 years ago

For & and wp-embed-template.js was my misunderstanding.

#45 @johnbillion
2 years ago

In 40526:

Build/Test Tools: Don't trigger a skipped test when the built version of wp-embed.min.js isn't present.

The grunt command assertion prior to the check for the existence of the file is enough to safeguard this test.

See #34698, #40533

Note: See TracTickets for help on using tickets.