Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#27858 closed defect (bug) (wontfix)

TinyMCE strips on* JS actions even when user has unfiltered_html capability

Reported by: ttbos's profile TTBoS Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.9
Component: TinyMCE Keywords:
Focuses: javascript Cc:

Description

Wordpress 3.9 removes the html onmouseover and onmouseout with articles and news.

When I enter the code in the text box after the change in the visual some parts of code are Change by WP.

For example,

<img onmouseover="this.src='poradnik-do-diablo-iii-on.jpg';" onmouseout="this.src='poradnik-do-diablo-iii-off.jpg';" src="poradnik-do-diablo-iii-off.jpg" alt="/diablo 3/" height="79" width="241"></a>

WP turns on:

<img src="poradnik-do-diablo-iii-off.jpg" alt="/diablo 3/" width="241" height="79" />

Attachments (2)

27858.diff (565 bytes) - added by elliott-stocks 10 years ago.
First patch! This fixes the issue with images, though we may need to apply it to all elements.
27858-2.diff (710 bytes) - added by elliott-stocks 10 years ago.

Download all attachments as: .zip

Change History (29)

#1 @TTBoS
10 years ago

  • Keywords needs-patch added

#2 @nacin
10 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

These attributes are insecure and thus will always be stripped out (in any version of WordPress) unless you have the ability to post unfiltered HTML. This capability is usually reserved for users of the Editor and Administrator role.

#3 @Denis-de-Bernardy
10 years ago

  • Component changed from General to TinyMCE
  • Focuses javascript added
  • Resolution invalid deleted
  • Status changed from closed to reopened

When switching between the HTML and Text views in TinyMCE, they disappear regardless of whether you can use unfiltered HTML or not.

#4 @SergeyBiryukov
10 years ago

  • Milestone set to Awaiting Review

Confirmed. This happens in TinyMCE demo as well: http://www.tinymce.com/tryit/full.php.

#5 @nacin
10 years ago

  • Milestone changed from Awaiting Review to 3.9.1

This is a regression in 3.9, then? Moving to 3.9.1 for investigation and review.

This ticket was mentioned in IRC in #wordpress-dev by nacin1. View the logs.


10 years ago

#7 @azaozz
10 years ago

Seems to be a change in MCE 4.0 that has some security implications. All of the on* attributes are disabled by default and can be re-enabled with the extended_valid_elements setting, either for specific tags or globally.

#8 follow-up: @elliott-stocks
10 years ago

Should we allow all of the on* attributes for all elements if the current user has unfiltered_html?

@elliott-stocks
10 years ago

First patch! This fixes the issue with images, though we may need to apply it to all elements.

#9 in reply to: ↑ 8 ; follow-ups: @azaozz
10 years ago

Replying to elliott-stocks:

Should we allow all of the on* attributes for all elements if the current user has unfiltered_html?

Not sure that is a good idea. Unfortunately the browsers in contneteditable mode still run JS added with these attributes.

The patch works however as all attributes for images are replaced, it should include all (HTML 4 and 5) attributes. I'm still 50/50 whether this should be patched in core or should be left for plugins to do. A typical plugin would be something like:

add_filter( 'tiny_mce_before_init', 'my_mce_init', 20 );
function my_mce_init( $init ) {
	if ( current_user_can('unfiltered_html') ) {
		if ( ! empty( $init['extended_valid_elements'] ) ) {
			$init['extended_valid_elements'] .= ',';
		} else {
			$init['extended_valid_elements'] = '';
		}

		$init['extended_valid_elements'] .= 'img[id|accesskey|class|dir|lang|style|tabindex|title|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|src|alt=|usemap|ismap|width|height|name|longdesc|align|border|hspace|vspace|crossorigin|onmouseover|onmouseout]';
	}

	return $init;
}

#10 in reply to: ↑ 9 @elliott-stocks
10 years ago

I'm thinking because it worked pre 3.9 that it should be in the core. I've added a new patch that uses the attributes you suggested :)

Replying to azaozz:

Replying to elliott-stocks:

Should we allow all of the on* attributes for all elements if the current user has unfiltered_html?

Not sure that is a good idea. Unfortunately the browsers in contneteditable mode still run JS added with these attributes.

The patch works however as all attributes for images are replaced, it should include all (HTML 4 and 5) attributes. I'm still 50/50 whether this should be patched in core or should be left for plugins to do. A typical plugin would be something like:

add_filter( 'tiny_mce_before_init', 'my_mce_init', 20 );
function my_mce_init( $init ) {
	if ( current_user_can('unfiltered_html') ) {
		if ( ! empty( $init['extended_valid_elements'] ) ) {
			$init['extended_valid_elements'] .= ',';
		} else {
			$init['extended_valid_elements'] = '';
		}

		$init['extended_valid_elements'] .= 'img[id|accesskey|class|dir|lang|style|tabindex|title|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|src|alt=|usemap|ismap|width|height|name|longdesc|align|border|hspace|vspace|crossorigin|onmouseover|onmouseout]';
	}

	return $init;
}

#11 @kirasong
10 years ago

  • Keywords has-patch added; needs-patch removed

So, we should probably chat about this today in IRC.

Two points in either direction, though:

  • It's a bad idea to strip existing post data and break sites if we can help it.
  • If we're going to change our stance on these attributes, this is the time to do it, since we've just upgraded to TinyMCE 4.0, and changes in behaviour are more expected.

#12 @nacin
10 years ago

  • Milestone changed from 3.9.1 to 3.9.2

#13 follow-up: @azaozz
10 years ago

  • Keywords has-patch removed
  • Milestone changed from 3.9.2 to 4.0

There is also another way to keep these attributes inside the editor, quite more complex though. We can convert all of the on* attributes to data-wp-on* when loading the post_content and convert them back then "saving" the post_content to the textarea.

Leaving the ticket on the 4.0 milestone for now in case somebody wants to try to make a patch for this.

#14 in reply to: ↑ 13 ; follow-up: @adamsilverstein
10 years ago

This seems a bit kudgy, still this is something that needs to be addressed. The attributes are stripped when switching from visual to text mode, seems like we would need to catch and substitute there, not just on saving/loading.

Replying to azaozz:

There is also another way to keep these attributes inside the editor, quite more complex though. We can convert all of the on* attributes to data-wp-on* when loading the post_content and convert them back then "saving" the post_content to the textarea.

Leaving the ticket on the 4.0 milestone for now in case somebody wants to try to make a patch for this.

#15 in reply to: ↑ 9 @adamsilverstein
10 years ago

azaozz - testing the patch it works as expected. When you say "browsers in contenteditable mode still run JS added with these attributes." - can you explain a bit more? testing in chrome and firefox I added an onclick handler - I don't see it firing anywhere when i'm in the editor.

Replying to azaozz:

Replying to elliott-stocks:

Should we allow all of the on* attributes for all elements if the current user has unfiltered_html?

Not sure that is a good idea. Unfortunately the browsers in contneteditable mode still run JS added with these attributes.

The patch works however as all attributes for images are replaced, it should include all (HTML 4 and 5) attributes. I'm still 50/50 whether this should be patched in core or should be left for plugins to do. A typical plugin would be something like:

add_filter( 'tiny_mce_before_init', 'my_mce_init', 20 );
function my_mce_init( $init ) {
	if ( current_user_can('unfiltered_html') ) {
		if ( ! empty( $init['extended_valid_elements'] ) ) {
			$init['extended_valid_elements'] .= ',';
		} else {
			$init['extended_valid_elements'] = '';
		}

		$init['extended_valid_elements'] .= 'img[id|accesskey|class|dir|lang|style|tabindex|title|contenteditable|contextmenu|draggable|dropzone|hidden|spellcheck|translate|src|alt=|usemap|ismap|width|height|name|longdesc|align|border|hspace|vspace|crossorigin|onmouseover|onmouseout]';
	}

	return $init;
}

#16 in reply to: ↑ 14 ; follow-up: @azaozz
10 years ago

Replying to adamsilverstein:

Uh, sorry, didn't explain well :)

TinyMCE "loads" the content from the textarea on page load and when switching Text => Visual. It "saves" the content to the textarea on form submit, on autosave, and on switching Visual => Text.

#17 follow-up: @azaozz
10 years ago

...I added an onclick handler - I don't see it firing anywhere when i'm in the editor.

Generally all browsers disable links, forms and scripts in contentEditable. However the on* attributes are not disabled. The above patch prevents MCE from filtering only onmouseover and onmouseout for images, perhaps test with: <img src="x" onmouseover="alert('xss')">

#18 @DrewAPicture
10 years ago

Sounds like something we need to fix, though I wonder if there's something that could also be done upstream to make this less "kudgy" on our end.

#19 in reply to: ↑ 16 @adamsilverstein
10 years ago

Aha. Thanks, trying to grok all this if only to better understand tinymce internals and the interactions with core. I'll try to dig in to see where this happens and how we could hook in.

Replying to azaozz:

Replying to adamsilverstein:

Uh, sorry, didn't explain well :)

TinyMCE "loads" the content from the textarea on page load and when switching Text => Visual. It "saves" the content to the textarea on form submit, on autosave, and on switching Visual => Text.

#20 in reply to: ↑ 17 @adamsilverstein
10 years ago

Replying to azaozz:

...I added an onclick handler - I don't see it firing anywhere when i'm in the editor.

Generally all browsers disable links, forms and scripts in contentEditable. However the on* attributes are not disabled. The above patch prevents MCE from filtering only onmouseover and onmouseout for images, perhaps test with: <img src="x" onmouseover="alert('xss')">

Ok, missed that - didn't look at the patch carefully enough. You are correct, the action fires in the editor which isn't great - however testing in 3.8 I see the same behavior so this isn't a regression and seems much better that stripping the existing data. What do you think, still worth the effort at the kludgy fix described above?

#21 follow-up: @azaozz
10 years ago

Considering the security aspect, I'm starting to think this should be a "plugin material". Two reasons:

  • These attributes are currently disabled (in 3.9).
  • Legitimate uses seem very rare.

I've only heard of two user cases:

  • Rollover images that can probably be done from CSS or from a dedicated plugin that handles the onmouseover storing/restoring on editor.on('BeforeSetContent', ...) and editor.on('GetContent', ...).
  • Capture clicks on links for SEO that should probably be handled "globally" from a dedicated script.

I wonder if there's something that could also be done upstream to make this less "kudgy" on our end.

Yeah, talked to the TinyMCE developers about that too. There is a private method in MCE that can add more "valid attributes" to any tag in the schema, would take some work to make it into a public method. Seems it would be worth it, will submit a patch.

Last edited 10 years ago by azaozz (previous) (diff)

#22 in reply to: ↑ 21 @adamsilverstein
10 years ago

If these are already stripped in 3.9 it seems reasonable to leave it in - especially considering how easy it is to filter.

Sounds like won't fix, I leave that in your capable hands :)

Replying to azaozz:

Considering the security aspect, I'm starting to think this should be a "plugin material". Two reasons:

  • These attributes are currently disabled (in 3.9).
  • Legitimate uses seem very rare.

I've only heard of two user cases:

  • Rollover images that can probably be done from CSS or from a dedicated plugin that handles the onmouseover storing/restoring on editor.on('BeforeSetContent', ...) and editor.on('GetContent', ...).
  • Capture clicks on links for SEO that should probably be handled "globally" from a dedicated script.

I wonder if there's something that could also be done upstream to make this less "kudgy" on our end.

Yeah, talked to the TinyMCE developers about that too. There is a private method in MCE that can add more "valid attributes" to any tag in the schema, would take some work to make it into a public method. Seems it would be worth it, will submit a patch.

#23 @adamsilverstein
10 years ago

  • Summary changed from Bug HTML onmouseover and onmouseout to TinyMCE strips on* JS actions even when user has unfiltered_html capability

#24 @adamsilverstein
10 years ago

in case someone needs it, here is a filter allowing (i hope) all on* JS handlers for images - https://gist.github.com/adamsilverstein/7741b38133fe3a593382

#25 @DrewAPicture
10 years ago

  • Milestone 4.0 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Closing as wontfix in favor of plugins having the ability to selectively re-enable the on* attributes via a filter, see https://gist.github.com/adamsilverstein/7741b38133fe3a593382.

As @azaozz said in comment:21, the on* actions have been disabled since we upgraded to TinyMCE 4.0 in 3.9 with few complaints. If somebody has a legitimate use case, they can use the filter to re-allow selected actions.

#26 @dd32
9 years ago

#30546 was marked as a duplicate.

#27 @dd32
7 years ago

#40640 was marked as a duplicate.

Note: See TracTickets for help on using tickets.