#27858 closed defect (bug) (wontfix)
TinyMCE strips on* JS actions even when user has unfiltered_html capability
Reported by: | 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)
Change History (29)
#2
@
11 years ago
- Milestone Awaiting Review deleted
- Resolution set to invalid
- Status changed from new to closed
#3
@
11 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
@
11 years ago
- Milestone set to Awaiting Review
Confirmed. This happens in TinyMCE demo as well: http://www.tinymce.com/tryit/full.php.
#5
@
11 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.
11 years ago
#7
@
11 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:
↓ 9
@
11 years ago
Should we allow all of the on*
attributes for all elements if the current user has unfiltered_html?
@
11 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:
↓ 10
↓ 15
@
11 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
@
11 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
@
11 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.
#13
follow-up:
↓ 14
@
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:
↓ 16
@
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 todata-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
@
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:
↓ 19
@
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:
↓ 20
@
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
@
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
@
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
@
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 onlyonmouseover
andonmouseout
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:
↓ 22
@
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 oneditor.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 the schema, would take some work to make it into a public method. Seems it would be worth it, will submit a patch.
#22
in reply to:
↑ 21
@
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 oneditor.on('BeforeSetContent', ...)
andeditor.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
@
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
@
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
@
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.
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.