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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Spocke, Assigned: Ehsan)

Tracking

({regression})

unspecified
mozilla2.0b7
x86
Windows Vista
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 .13-fixed, status1.9.1 .16-fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Component: General → Editor
Product: Firefox → Core
(Reporter)

Updated

7 years ago
Depends on: 596300
QA Contact: general → editor
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?
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?
(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... :(
(Reporter)

Comment 4

7 years ago
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?
(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?
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

7 years ago
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.)
(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.
Assignee: nobody → ehsan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Created attachment 483492 [details] [diff] [review]
Patch (v1)
Attachment #483492 - Flags: review?(roc)
Attachment #483492 - Flags: approval2.0?
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.
Attachment #483492 - Flags: review?(roc)
Attachment #483492 - Flags: review+
Attachment #483492 - Flags: approval2.0?
Attachment #483492 - Flags: approval2.0+
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/378f617276a8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Attachment #483492 - Flags: approval1.9.2.12?
Attachment #483492 - Flags: approval1.9.1.15?

Comment 12

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

Updated

7 years ago
Attachment #483492 - Flags: approval1.9.2.12?
Attachment #483492 - Flags: approval1.9.2.12+
Attachment #483492 - Flags: approval1.9.1.15?
Attachment #483492 - Flags: approval1.9.1.15+
Keywords: regression
Depends on: 520189
No longer depends on: 596300
Blocks: 520189
No longer depends on: 520189
(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

7 years ago
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.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a995dbfcbe2
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0fe7a05219fd
status1.9.1: --- → .15-fixed
status1.9.2: --- → .12-fixed
(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.
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.
And yes, I agree this issue needs a lot more visibility. Ehsan, can you put together a post for Blizzard's hacks blog?
(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.
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Depends on: 619287
You need to log in before you can comment on or make changes to this bug.