Closed Bug 604332 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: DOM: Editor, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
status1.9.2 --- .13-fixed
status1.9.1 --- .16-fixed

People

(Reporter: spocke, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Component: General → Editor
Product: Firefox → Core
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... :(
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.
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
Attached patch Patch (v1) (deleted) — Splinter Review
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]
Status: ASSIGNED → RESOLVED
Closed: 14 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?
my editor does not use _underscore for all of its attributes, so its still busted on copy/paste.
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: CVE-2010-2769
No longer depends on: 596300
No longer depends on: CVE-2010-2769
(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.
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.
(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.

Attachment

General

Created:
Updated:
Size: