604332 - Security changes for copy/paste of html contents into contentEditable breaks TinyMCE

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Spocke]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729; .NET4.0E)

Copy/paste of images breaks in TinyMCE since the custom attributes with the _mce_ prefix gets removed. This is a regression and isn't an issue on other browsers it will break existing systems out there that uses TinyMCE as it's rich text editor.

Reproducible: Always

Steps to Reproduce:
See this bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=596300
Actual Results:  
Custom attributes gets removed.

Expected Results:  
Custom attributes should be retained.

Comment 1

[(no longer active)]

One option here is to white-list custom attributes in _mce "namespace", but I'm not sure if that's the cleanest solution.  roc, bz, what do you guys think?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Are there other common editor widgets we need to worry about?

Or put another way, won't that be entrenching the market position of TinyMCE and stifling competition?

Comment 3

[(no longer active)]

(In reply to comment #2)
> Are there other common editor widgets we need to worry about?
> 
> Or put another way, won't that be entrenching the market position of TinyMCE
> and stifling competition?

Well, yes, there are other editor widgets.  To address this problem in general, I still don't see any other way besides just allowing custom attributes... :(

Comment 4

[Spocke]

I think the best way would be to keep browser vendor attributes starting with "_" it would solve CKEditor as well since they use _ck_<something> if I remember correctly. Also it would be really nice if it was possible to extract the unsanitized html code using the dataTransfer object that would be a feature in Firefox that would be useful for everyone. Something like event.dataTransfer.getData('text/html'); but the problem with that method is that we then would need people to upgrade their TinyMCE installations to use that new logic. Having access to the raw HTML would then make it up to the developers to secure it?

Comment 5

[(no longer active)]

(In reply to comment #4)
> I think the best way would be to keep browser vendor attributes starting with
> "_" it would solve CKEditor as well since they use _ck_<something> if I
> remember correctly.

White-listing attribute names beginning with "_" might be an option to consider.

> Also it would be really nice if it was possible to extract
> the unsanitized html code using the dataTransfer object that would be a feature
> in Firefox that would be useful for everyone. Something like
> event.dataTransfer.getData('text/html'); but the problem with that method is
> that we then would need people to upgrade their TinyMCE installations to use
> that new logic. Having access to the raw HTML would then make it up to the
> developers to secure it?

Why would you need this functionality?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's white-list _* for now. But we should work on getting editor widgets to use data- attributes so we can stop whitelisting _*; it's not entirely safe.

Comment 7

[Sacha Varma]

I don't pretend to understand the security issues around preserving custom attributes, but:

Is allowing one class of custom attributes (those beginning with an underscore) consistent with the security considerations that led you to strip off all custom attributes? (In other words, could it lead to an exploit.)

Comment 8

[(no longer active)]

(In reply to comment #7)
> Is allowing one class of custom attributes (those beginning with an underscore)
> consistent with the security considerations that led you to strip off all
> custom attributes? (In other words, could it lead to an exploit.)

In theory, yes, it could.  In practice, we're only worried about attribute names which have the potential of running JS code in other browsers.  There aren't any such attributes which begin with "_" to the best of my knowledge.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 483492 [details] [diff] [review]
Patch (v1)

It's actually possible that other browsers have (or will have) vendor attributes starting with _ that might be unsafe. But this will have to do for now.

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/378f617276a8

Comment 12

[Mike]

my editor does not use _underscore for all of its attributes, so its still busted on copy/paste.

Comment 13

[(no longer active)]

(In reply to comment #12)
> my editor does not use _underscore for all of its attributes, so its still
> busted on copy/paste.

You should consider switching to HTML5 data-* attributes.

Comment 14

[Mike]

I did, but I'm reiterating what someone said earlier that your giving preference to the "_attribute" and the editors that use those rather than "attribute". Its a slippery slope your riding when you exclude "attribute" over "_attribute". Eventually some browser, somewhere, will implement that.

Comment 15

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a995dbfcbe2
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0fe7a05219fd

Comment 16

[(no longer active)]

(In reply to comment #14)
> I did, but I'm reiterating what someone said earlier that your giving
> preference to the "_attribute" and the editors that use those rather than
> "attribute". Its a slippery slope your riding when you exclude "attribute" over
> "_attribute". Eventually some browser, somewhere, will implement that.

See comment 6, comment 8 and comment 10 please.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes. To be clear: all editor authors should start transitioning to data-* attributes immediately. At some point we will start stripping all unknown, non data-* attributes.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And yes, I agree this issue needs a lot more visibility. Ehsan, can you put together a post for Blizzard's hacks blog?

Comment 19

[(no longer active)]

(In reply to comment #18)
> And yes, I agree this issue needs a lot more visibility. Ehsan, can you put
> together a post for Blizzard's hacks blog?

Sure.