Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Element Attributes dropped in DesignMode/ContentEditable sections

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Editor
--
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: gamecockguy2003, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {dataloss, regression, testcase})

1.9.2 Branch
mozilla2.0b7
x86
All
dataloss, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 .11+, status1.9.2 .11-fixed, blocking1.9.1 .14+, status1.9.1 .14-fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.55 Safari/534.3
Build Identifier: Firefox/3.6.9

This seems to have been caused by the fix for http://www.mozilla.org/security/announce/2010/mfsa2010-62.html. 

When a document is in DesignMode or if a section of a document is marked as ContentEditable, drag-and-drop of an element within the editable area results in some attributes being dropped from the element.

I've noticed two categories of attributes that are dropped:

1) Script-like attributes such as onmouseover. This clearly seems related to the bug mentioned above. While the change fixes an XSS issue for items dropped in from another browser, it breaks the ability to move an item within its own document without having its attributes removed.

2) Expando attributes. If you have an element with a non-standard (invalid) attribute on an element, then it will be dropped after you drag and then drop the element. This did not occur before upgrading from 3.6.8 to 3.6.9. Even if you use a custom Document Type Definition to define the new attributes, they will be dropped after a drag-and-drop action inside the contenteditable section or in a designmode document.

This creates significant problems for browser-based HTML editors like TinyMCE, etc

Reproducible: Always

Steps to Reproduce:
1.Create an HTML document with a DIV marked as contenteditable="true" and some content inside the DIV.
2.Include an IMG tag with an "onmouseover" attribute and possible a fake attribute such as "test"
3.View the page and notice that the attributes are on the element
4.Drag and drop the image to a different location in the DIV.
5.Notice that the element no longer has either the "test" or "onmouseover" attributes.
Actual Results:  
The IMG element has lost the two attributes.

Expected Results:  
The image element exists at its new location in the DIV with all of its original attributes.

This seems related to the fix for http://www.mozilla.org/security/announce/2010/mfsa2010-62.html.
(Reporter)

Updated

7 years ago
OS: Windows 7 → All

Comment 1

7 years ago
This issue seems to affect documents with custom namespaces declared when calling execCommand():

1. custom tags are completely blocked:
document.execCommand("insertHTML", false, '<custom:tag ... />')
-- inserts nothing

2. custom attributes are blocked as well:
document.execCommand("insertHTML", false, '<a href="..." custom:attr="...">...</a>')
-- inserts just <a href="...">...</a>

Comment 2

7 years ago
Confirmed. I'm also having big problems with it. 
My clients are complaining a lot.

My WYSIWYG editor's features stopped working.

Like someone said before. There is no simple workaround about this problem. I've got hope Firefox developers will take a look with it.

Comment 3

7 years ago
Confirmed
Problems with CKEditor WYSIWYG Editor...

No easy workaround... Webapp does not work on firefox because of this...

Comment 4

7 years ago
Quite common practice to use custom namespace attributes in WYSIWYG Editors. The one I have see used (and broken by this) is the YUI2 simpleditor.

As stated above, there is no easy workaround and the current state breaks a lot of web pages.

Updated

7 years ago
Blocks: 424615

Comment 5

7 years ago
It looks like the issue persists in FF 3.6.10 and affects many users of all major rich text editors, including TinyMCE, CKEditor, YUI2, Movable Type, etc. Can we increase the priority of this ticket?
(Reporter)

Comment 6

7 years ago
Updating importance based on comments above
Severity: normal → major

Updated

7 years ago
blocking1.9.2: --- → ?

Comment 7

7 years ago
It looks like the issue persists in FF 3.6.10 and affects many users of all major rich text editors, including TinyMCE, CKEditor, YUI2, Movable Type, etc. Can we increase the priority of this ticket?

Updated

7 years ago
Keywords: regression

Comment 8

7 years ago
>Can we increase the priority of this ticket?

I think we need more votes on this bug and maybe somebody will look at it. I'd like to know as soon as possible what can I do with my clients, should I wait or if the won't fix it - I need to change scripts or change browser...

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Could you provide a testcase?
Keywords: testcase-wanted

Comment 10

7 years ago
Created attachment 476264 [details]
Simple testcase

This simple testcase shows how the three attributes "onclick", "data-custom" and "testing" are removed from a link.
It works correctly in 3.6.8, it's broken in 3.6.9 and 3.6.10 and 4.0b5.
Works fine also in Chrome 6, Safari 5
(Reporter)

Comment 11

7 years ago
Created attachment 476267 [details]
Shows that moving an element results in custom and script attributes being dropped.

Adding a test case that illustrates custom attributes and script attributes are dropped when moving an element inside of an editable div or designmode=on document.

Updated

7 years ago
Component: General → Editor
Keywords: testcase-wanted
Product: Firefox → Core
QA Contact: general → editor
Version: unspecified → 1.9.2 Branch

Comment 12

7 years ago
Created attachment 476272 [details]
execCommand() issue demo

This example demonstrates the issue with execCommand().
In FF 3.6.9 custom tags and attributes are ignored.

Updated

7 years ago
blocking2.0: --- → ?

Updated

7 years ago
Assignee: nobody → ehsan
The regressing fix for bug 520189 landed on the 1.9.1 branch  -- can this be reproduced in 3.5.12 too?
Blocks: 520189
blocking1.9.1: --- → ?
blocking1.9.2: ? → .11+
status1.9.1: --- → ?
status1.9.2: --- → wanted
(Reporter)

Comment 14

7 years ago
(In reply to comment #13)
> The regressing fix for bug 520189 landed on the 1.9.1 branch  -- can this be
> reproduced in 3.5.12 too?

Couldn't find 3.5.12, but I did just verify this issue occurs on 3.5.13.  This is consistent with the theory that it is related to the fix for 520189.

Updated

7 years ago
Keywords: testcase

Updated

7 years ago
Keywords: dataloss

Comment 15

7 years ago
Is there a list of "valid" elements and attributes? The canvas element seems to get stripped, too, but i suppose it to be a valid element, since firefox supports it for years.

Comment 16

7 years ago
Looks like 3.5.13 is also affected.  It looks like is also affects aria attributes.

Comment 17

7 years ago
> Is there a list of "valid" elements and attributes?

As of today, the allowed tags are http://hg.mozilla.org/mozilla-central/file/b439e73f33b7/content/base/src/nsContentSink.cpp#l1732 and the allowed attributes are http://hg.mozilla.org/mozilla-central/file/b439e73f33b7/content/base/src/nsContentSink.cpp#l1819

Probably worth filing separate bugs on adding aria attributes (which may be safe to leave) and canvas (which I think should also be safe) to the whitelists.
Sorry for not commenting here sooner.  Here's a rundown summary on how I'm planning to address this issue:

1. For HTML5 elements and attributes, I filed bug 598090.
2. For HTML5 data-* attributes, I filed bug 598105.
3. For custom attributes, my initial reaction was that we should just drop them for security purposes.  Boris, what do you think?
4. I'm also beginning to think that we should maybe not use the sanitizing fragment content sink for execCommand("inserthtml"), since page script can already inject whatever it wants in that case, using things like innerHTML.  The original idea in bug 520189 was protecting paste and drop operations, and I might have got carried away there.  :-)  Boris, if you agree, let me know and I'll file a bug for that.

As far as I can tell, these four issues should cover everything that this bug is about, right?
Depends on: 598090, 598105
(In reply to comment #18)
> 4. I'm also beginning to think that we should maybe not use the sanitizing
> fragment content sink for execCommand("inserthtml"), since page script can
> already inject whatever it wants in that case, using things like innerHTML. 

One case in point is bug 597784, FWIW.

Comment 21

7 years ago
I agree on item 3.  For item 4... it's probably more web-compatible to allow pages to shoot themselves in the foot there. :(

Updated

7 years ago
blocking1.9.1: ? → .14+
status1.9.1: ? → wanted
(In reply to comment #21)
> I agree on item 3.  For item 4... it's probably more web-compatible to allow
> pages to shoot themselves in the foot there. :(

By "agree on item 3", do you mean that you agree that we should continue dropping those attributes, or that we should allow them?

Comment 23

7 years ago
I think we should probably keep dropping them for now and see how things go.
(Reporter)

Comment 24

7 years ago
(In reply to comment #18)
> Sorry for not commenting here sooner.  Here's a rundown summary on how I'm
> planning to address this issue:
> 
> 1. For HTML5 elements and attributes, I filed bug 598090.
> 2. For HTML5 data-* attributes, I filed bug 598105.
> 3. For custom attributes, my initial reaction was that we should just drop them
> for security purposes.  Boris, what do you think?
> 4. I'm also beginning to think that we should maybe not use the sanitizing
> fragment content sink for execCommand("inserthtml"), since page script can
> already inject whatever it wants in that case, using things like innerHTML. 
> The original idea in bug 520189 was protecting paste and drop operations, and I
> might have got carried away there.  :-)  Boris, if you agree, let me know and
> I'll file a bug for that.
> 
> As far as I can tell, these four issues should cover everything that this bug
> is about, right?

For item 3, it would seem that if an attribute is "custom" (not recognized by Firefox) that there wouldn't be a security risk of Firefox interpreting and executing anything based on that unrecognized attribute.

Other commonly used browsers, similar to previous versions of Firefox, leave these attributes as they are, and many WYSIYG implementations rely on this.  Dropping this functionality without warning is going to be a serious pain for a lot of developers, and will remove the value of resolving this bug for many that have been following it.

Comment 25

7 years ago
> that there wouldn't be a security risk of Firefox interpreting 

Right.  The security risk is that of Firefox being used as a vector for delivering exploits targeted at other browsers.

Comment 26

7 years ago
> Other commonly used browsers, similar to previous versions of Firefox, leave
> these attributes as they are, and many WYSIYG implementations rely on this. 
> Dropping this functionality without warning is going to be a serious pain for a
> lot of developers, and will remove the value of resolving this bug for many
> that have been following it.

Please, don't change something which was working almost from the beginning.

I agree with @gamecockguy2003 in 100%!

Comment 27

7 years ago
I tend to agree.  It is up to other browsers to protect against the exploits you mention.  I don't think this is the concern of FF.  A malicious user can just as easily craft some HTML using a text editor and upload it with these "custom" attributes.  

Is there no solution that can meet your security concerns that does not involve such a major breaking change?

Comment 28

7 years ago
> Please, don't change something which was working almost from the beginning.

We've done that before, e.g. when we implemented a popup blocker.  The question is the tradeoff.

> It is up to other browsers to protect against the exploits you mention.

How?  The issue is if HTML5 adds a new scripting feature, some other browser implements it, and Firefox doesn't.  Then from our point of view the attribute is unknown and we'd leave it, and it would run script in the other browser.  Whereas if we also implemented the feature we'd not allow the attribute since it runs script... and then it wouldn't run script in either browser.

That's the fundamental issue with blacklist vs whitelist: blacklisting means that as soon as some new stuff gets added you end up insecure, whereas whitelisting means that when new stuff gets added it doesn't work until you add it to your whitelist and that completely unknown stuff just doesn't work.

> A malicious user can just as easily craft some HTML using a text editor and
> upload it with these "custom" attributes.  

Can they?  A lot of places where contenteditable is used do not allow arbitrary file uploads.

> Is there no solution that can meet your security concerns that does not
> involve such a major breaking change?

We clearly haven't thought of one yet; otherwise we wouldn't be asking for trouble like this, obviously.
(In reply to comment #24)
> Other commonly used browsers, similar to previous versions of Firefox, leave
> these attributes as they are, and many WYSIYG implementations rely on this. 
> Dropping this functionality without warning is going to be a serious pain for a
> lot of developers, and will remove the value of resolving this bug for many
> that have been following it.

Editor widgets should move to using data-* attributes if they want to attach custom data to elements which survive copying and pasting.
(In reply to comment #27)
> Is there no solution that can meet your security concerns that does not involve
> such a major breaking change?

Unfortunately there doesn't seem to be.  Please note that while we could choose to ignore the security of users using other browsers and only care about Firefox users, we decided to go against that convenient choice and try to protect as many users as possible, even if they're not using Firefox.

As I said in comment 29, a work-around is to use data-* attributes, which are specified by the HTML5 spec, and are guaranteed to work in any HTML5 compliant browser.
(Reporter)

Comment 31

7 years ago
(In reply to comment #29)
> (In reply to comment #24)
> > Other commonly used browsers, similar to previous versions of Firefox, leave
> > these attributes as they are, and many WYSIYG implementations rely on this. 
> > Dropping this functionality without warning is going to be a serious pain for a
> > lot of developers, and will remove the value of resolving this bug for many
> > that have been following it.
> 
> Editor widgets should move to using data-* attributes if they want to attach
> custom data to elements which survive copying and pasting.

Agreed that this is the proper solution going forward.  Unfortunately, it means many broken clients for lots of companies in the meantime.  Ideally, this would be phased out rather than being disabled without notice.

Comment 32

7 years ago
Please, correct me if I'm wrong. 

You will allow custom attributes: data-*

But what about attributes like "onclick"? 
Will you still drop them?
(Reporter)

Comment 33

7 years ago
(In reply to comment #28)
> > A malicious user can just as easily craft some HTML using a text editor and
> > upload it with these "custom" attributes.  
> 
> Can they?  A lot of places where contenteditable is used do not allow arbitrary
> file uploads.
> 

You are correct that they probably can't upload files.  However, usually it isn't the HTML of the page being itself that is submitted to the server in one of these editors.  Typically it will be serialized and submitted through a HIDDEN input or some other mechanism.  This means that a user could create a request that would submit whatever HTML they want.

My point is that this change does not reduce the need to sanitize the HTML that comes from a content-editable section.  As a result, this change does little in that regard, but does a lot in terms of breaking existing solutions until developers can scramble to get hotfixes out.
(In reply to comment #21)
> I agree on item 3.  For item 4... it's probably more web-compatible to allow
> pages to shoot themselves in the foot there. :(

Morphed byug 597784 to deal with that.
Depends on: 597784

Comment 35

7 years ago
I respect that you are trying to protect as many users as possible, even if they are not FF users, but there will always be cracks to plug that are well outside of your control.

The methods for loading content into a system are determined by the system.  Ultimately, filtering should be done at display-time; this has not changed, and nor will it with these changes.
(In reply to comment #32)
> Please, correct me if I'm wrong. 
> 
> You will allow custom attributes: data-*

Starting from tomorrow's mozilla-central nightly, yes.  That patch will also be backported to the next dot-release of 3.5 and 3.6.

> But what about attributes like "onclick"? 
> Will you still drop them?

Yes, because they can result in code execution.
(In reply to comment #33)
> My point is that this change does not reduce the need to sanitize the HTML that
> comes from a content-editable section.  As a result, this change does little in
> that regard, but does a lot in terms of breaking existing solutions until
> developers can scramble to get hotfixes out.

This is not the rationale behind this change.  I'm afraid you need to wait until bug 520189 becomes publicly visible to know more about the security risk involved, and why we chose to fix things this way.
(In reply to comment #35)
> I respect that you are trying to protect as many users as possible, even if
> they are not FF users, but there will always be cracks to plug that are well
> outside of your control.

If we were to accept this as a principle, we wouldn't fix any security holes, on the grounds that there will always be cracks to plug that are out of our control!  This is not how security works.

> The methods for loading content into a system are determined by the system. 
> Ultimately, filtering should be done at display-time; this has not changed, and
> nor will it with these changes.

As I said, the fix to bug 520189 protects users against a real security threat which cannot be prevented by any kind of filtering at display-time.  I'm afraid that I cannot provide more details about this at this time.  :-(
(Reporter)

Comment 39

7 years ago
(In reply to comment #37)
> (In reply to comment #33)
> > My point is that this change does not reduce the need to sanitize the HTML that
> > comes from a content-editable section.  As a result, this change does little in
> > that regard, but does a lot in terms of breaking existing solutions until
> > developers can scramble to get hotfixes out.
> 
> This is not the rationale behind this change.  I'm afraid you need to wait
> until bug 520189 becomes publicly visible to know more about the security risk
> involved, and why we chose to fix things this way.

I understand.  I suppose it will be more clear when that becomes public knowledge.  Though I will say that if that isn't the rationale, I can't foresee any reason to be concerned about attributes unknown to Firefox that are scripting attributes in other browsers.

Comment 40

7 years ago
> If we were to accept this as a principle, we wouldn't fix any security holes,
> on the grounds that there will always be cracks to plug that are out of our
> control!  This is not how security works.

No.  I meant that it should not be the concern of FF to protect against exploits in other browsers.
 
> As I said, the fix to bug 520189 protects users against a real security threat
> which cannot be prevented by any kind of filtering at display-time.  I'm afraid
> that I cannot provide more details about this at this time.  :-(

I look forward to hearing more about it.

Comment 41

7 years ago
Maybe I'm mistaken but:

"Security researcher Paul Stone reported that when an HTML selection containing JavaScript is copy-and-pasted or dropped onto a document with designMode enabled the JavaScript will be executed within the context of the site where the code was dropped. A malicious site could leverage this issue in an XSS attack by persuading a user into taking such an action and in the process running malicious JavaScript within the context of another site."

That suggests the security vulnerability is about scripts executing *in Firefox* -- not another browser.  So if that's the issue here, I don't think unknown tags would be an issue.   But I guess we'll see.

Comment 42

7 years ago
We are only guessing until 520189 is made public, but here are my thoughts:

Doing this kind of changes means that deployed system from years ago are failing with new Firefoxes and the only solution (without spending time and money changing the html editor to a new one unreleased that is able to work correctly) is to switch browsers. 

My personal idea about what that bug is about has two options:
1. People is tricked to copy/dnd a malicious content into their editor. That content includes scripts/event handlers. Then it's saved and when a visitor comes it's executed in the context of the main site.
This can be fixed if the site is using something like http://htmlpurifier.org/ (and it should be using it already)

2. People is tricked to copy/dnd a malicious content into their editor. That content includes scripts/event handlers. Those scripts or handlers are executed right away. This happens only if the site is using contentEditable so scripts aren't stoped in the page.

To avoid those problems, the editor widget could use the clipboardData (sorry, I don't have time right now to check the proper names and urls, so I might be mixing things but I think that you'll understand what I mean) in the onPaste or onDrop events (maybe dataTransfer?). If the editor can read properly that data then it can apply whatever sanitation it needs on the data and the call "insertHTML" with something that has been checked. Of course, the html available from that clipboard should be the real HTML, not something already sanitized or changed in any way.

In order to help with that sanitation, IE8 introduced something like "toSafeHtml" and Firefox should create such a function using the whitelisting that it seems that it's currently being applied automatically. That way every updated script can use the latest security features available in the browser.

The popup blocker has always (IIRC) shown a button or any warning to the user explaining that something has been blocked and allowing with to whitelist that use. In this case it might be harder because most of the people won't be able to recognize if something is safe or not, but it could be at least a preference so it can be changed in about:config without changing browsers.

Also, a new execCommand can be introduced "safeClipboard" that toggles this automatic feature from strict to "previous behavior".

When the user drags and drop some element inside the editor, the last thing that he expects is that some sanitation is applied to that operation and the element loses some of its attributes or contents, so any dnd should verify if the source and destination is the same document and skip any processing of the data in that case. Also, copy and paste should behave in a similar way, maybe storing an extra clipboard flavor with the url and raw html of the source and checking that flavor before pasting it (that means don't apply this extra sanitation and also preserving the url of img, links... without turning them into absolute urls)
(Reporter)

Comment 43

7 years ago
Has there been any consideration on this issue in regard to Document Type Definitions?  The whole point of DTD's is to define what attributes are allowed on what tags.  The idea that a browser would come in after that with its own list of safe attributes seems like the wrong solution.  It eliminates a lot of the extensibility goodness of a DTD.

Ideally, the DTD itself would be able to define if a given attribute in the context of a given tag is "safe" or "notsafe".

Comment 44

7 years ago
I've tried the latest build for 1.6.x version of firefox and attributes like data-* are still dropped by the execCommand 'inserthtml'...

Is this normal because some here said earlier it has been done...
(Reporter)

Comment 45

7 years ago
(In reply to comment #44)
> I've tried the latest build for 1.6.x version of firefox and attributes like
> data-* are still dropped by the execCommand 'inserthtml'...
> 
> Is this normal because some here said earlier it has been done...

Can you confirm if they are working with drag/drop?

Comment 46

7 years ago
The simpleTestCase provided still doesn't work neither with friefox 3.6.11beta nor with firefox 4.0beta (nightly build from 20th september)

Comment 47

7 years ago
Can we _please_ have some reading comprehension?  Comment 36 is very clear about when data-* can be expected to work and neither of the builds mentioned in comment 46 is it.
Guys, please calm down!  Let's keep this bug focused on the issue it was filed for.

In case you have not looked through the dependency list, here's a summary of the matters so far:

* Bug 598090 updated our whitelist to the safe elements from the latest HTML5 draft spec.  It has landed on mozilla-central, which means that it's available starting from yesterday's nightlies, and we will take it on branches as well, which means that it will be available in the next release of Firefox 3.5 and 3.6.

* Bug 598105 added support for whitelisting HTML5 data-* attributes, which means that the custom attributes will be preserved if the author of the web page chooses to use the recommended way of storing them, which is data-* attributes.  This bug too has landed on mozilla-central, which means that it's available starting from yesterday's nightlies, and we will take it on branches as well, which means that it will be available in the next release of Firefox 3.5 and 3.6.

* With bug 598105, I think the need of web developers in preserving custom attributes will be satisfied without risking the security of the users of other browsers.  Right now I think that this could be the best middle-ground.

* The on* event attributes can allow code execution, and therefore will continue to be stripped off during paste/drop operations.  Therefore, that part of comment 0 is WONTFIX.

* I've also attached a patch to bug 597784 to not use the paranoid fragment sink, which means that the HTML code inserted using execCommand("inserthtml") _will not_ be subject to any white-list, as was the case before bug 520189.  The patch is currently waiting for review, and will be included in the next release of Firefox 3.5, 3.6 and 4.0.

* The only controversial issue at this point is whether we should preserve custom attributes or not.  I've explained our position towards this issue, but I'm still trying to think of ways that we can switch to accepting those attributes without risking the safety of users of other browsers.  If someone can think of such a solution, I'd be interested to hear their thoughts.

Comment 49

7 years ago
"* The only controversial issue at this point is whether we should preserve
custom attributes or not.  I've explained our position towards this issue, but
I'm still trying to think of ways that we can switch to accepting those
attributes without risking the safety of users of other browsers.  If someone
can think of such a solution, I'd be interested to hear their thoughts."


I suppose we can't be much help with that solution because of the undisclosed bug 520189.  I'll take your word for it, but at this point I don't see how drag-and-drop interaction within Firefox poses a security threat to another browser.  As gamecockguy said, the only way I see that posing a threat is if the resulting HTML is later posted to the webserver, not sanitized, and later sent to another browser from the webserver.  In that situation, the threat is still there because you could post dangerous HTML regardless of the drag-and-drop feature, so I don't see how this adds security -- maybe a tiny bit of obfuscation?     However, if there's some other potential threat for *other  browsers* described in 520189, then good luck in finding a resolution internally, but we're working blind here and I don't suppose could be of much help.
Duplicate of this bug: 594725

Comment 51

7 years ago
Attributes data-* work on this night's mozilla central's build of 4.0b7.

Maybe i didn't check on the right repository yesterday (checked on the 4.0b7 but not mozilla central's, apologies...)

Comment 52

7 years ago
I think that any drag&drop or clipboard operation done within the same document shouldn't be affected by any filtering. There's no risk of injection as that code is already in the document and the user just wants to move it.

And my mention yesterday about toSafeHTML was meant to say toStaticHTML as mentioned in bug 443564. It would be good if the current sanitation method is exposed that way so any page can opt-in to use it.
I think bug 597784 should be marked to block branches instead of this bug...
(In reply to comment #53)
> I think bug 597784 should be marked to block branches instead of this bug...

Well, this proves that I'm blind!

Comment 55

7 years ago
"* The only controversial issue at this point is whether we should preserve
custom attributes or not.  I've explained our position towards this issue, but
I'm still trying to think of ways that we can switch to accepting those
attributes without risking the safety of users of other browsers.  If someone
can think of such a solution, I'd be interested to hear their thoughts."

I would just like to say that there IS a work-around to this and its quite easy to do. I'm not going to post how to do it though, because I disagree with you on the issue... and frankly I need my editor to work. To me, the solution is to just not do a blanket strip of all custom tags. This breaks almost every editor on the market right now.
(In reply to comment #55)
> "* The only controversial issue at this point is whether we should preserve
> custom attributes or not.  I've explained our position towards this issue, but
> I'm still trying to think of ways that we can switch to accepting those
> attributes without risking the safety of users of other browsers.  If someone
> can think of such a solution, I'd be interested to hear their thoughts."
> 
> I would just like to say that there IS a work-around to this and its quite easy
> to do. I'm not going to post how to do it though, because I disagree with you
> on the issue... and frankly I need my editor to work. To me, the solution is to
> just not do a blanket strip of all custom tags. This breaks almost every editor
> on the market right now.

Well, if you have a workaround and you don't post it here because you disagree with me, the chances of the situation getting better here will be much lower.  If it's a good workaround, I'm really eager to hear about it.
It does seem to me that dragging HTML within the same editable region would be safe. I think we should do that.
blocking2.0: ? → betaN+
(In reply to comment #57)
> It does seem to me that dragging HTML within the same editable region would be
> safe. I think we should do that.

Should I piggy back this bug for this in order to use the blocking flag or file another bug and nominate it for blocking?
Rereading comment #0, I think that's exactly what this bug is about.

Comment 60

7 years ago
I found this bug after hours of debugging - I can not believe that you did such a fundamental change whitout considering the effect for all WYSIWYG editors.
Our CMS depends on CKEDITOR or FCKEditor and on custom attributes.

This code could be inserted with InsertHtml before the security "fix":
<img width="220" height="120" src="http://myserver/jpg2/animal02.jpg" newobject="1" fileid="6918" objectid="5043" />

After this "fix" my code is reduced to this:
<img width="220" height="120" src="../jpg2/arena.jpg" />

So not only all custom attributes are gone, also my img src was converted from absolute to relative format.

Please rethink this "fix". I would love to hear about the workaround!

Comment 61

7 years ago
>* I've also attached a patch to bug 597784 to not use the paranoid fragment
>sink, which means that the HTML code inserted using execCommand("inserthtml")
>_will not_ be subject to any white-list, as was the case before bug 520189. 
>The patch is currently waiting for review, and will be included in the next
>release of Firefox 3.5, 3.6 and 4.0.

If I understood this comment correctly, this means that execCommand("inserthtml") will be fixed (reverted to the old functionality) with this patch?

Comment 62

7 years ago
Hey fellow. Im a programmer who's using FCKEditor (2.6.2, we can't upgrade that
fast) too. I created an workaround for the InsertHtml() "issue". I'll mail it
to you. I just won't to post it here as I don't want to risk, that our Mozilla
colleagues get noticed about the "backdoor". :x

The workaround will not fix the drag&drop issue. That one I bypassed by
prohibiting drag/drop of threatened (placeholder) images, for now.

As far as I can see, the drag&drop issue will be rolled back within the next
versions soon. I hope so.

Comment 63

7 years ago
I suggest that the inserthtml command shouldn't be sanitized at all. And drop and paste should only be sanitized when it comes from an external source not within the same document. The sanitation should only include: event attributes like onclick, IE CSS expressions and javascript: urls in links and images everything else like custom attributes should remain intact.

But the contents should be sanitized on the server anyway to avoid XSS attacks by bots so I doesn't really matter much that the browser is secure here. Other browsers doesn't address this a.f.a.i.k so it seems a bit overkill to add this feature to Firefox.

Comment 64

7 years ago
I suggest that everyone READ THE BUG before commenting.  It was decided that insertHTML shouldn't sanitize three days ago, and it says so in comment 48.  Now can people please stop spamming the bug they haven't read?

Comment 65

7 years ago
I fully agree that sanitation is a thing that should be done by the RTE-Framework or the user of such an framework, not by the browser.

I can live with it, as I have a workaround that replaces the InsertHtml command.

When it comes to ignoring/sanitizing non-standard attributes and element, I think this is something that should phased out in a major release after introducing data- attributes. So RTE-Framework programmers and their users have enough time to migrate. 

Making such an heavy change within a minor release is crazy.

Sanitizing on drag/drop is unnecessary when dragging elements within the same document because, ideally, they have been cleaned up before. Sanitizing content that has been dragged into the document from elsewhere might be OK and consistend with the InsertHtml change.

My personal attitude on the whole topic is completely different: the editor should just block the Javascript engine and plugins from executing interactive content when existing within an editable document. Than the browser *may* provide an (discussed above) passive method that cleans up content and it's up to the programmer to filter malicious code. But as I understood the above discussion it appears to that the Firefox developers want to rescue the world (wide web). :)

I think, blocking/removing interactive content within an editable document is a big regression in Web 2.0.

Comment 66

7 years ago
I also solved the drag and drop problem. So if need be, theres a solution for that as well. I have only shown this to 1 other person. I believe they have decided however (if im reading correctly) to not strip out custom attributes if drag and drop occurs within the same document (Comment 57, shortly after my first post). This seems reasonable enough to me.
Attachment #476264 - Attachment is obsolete: true
Attachment #476272 - Attachment is obsolete: true
Created attachment 478395 [details] [diff] [review]
Fix

Here's the fix, still working on the test...
Attachment #478395 - Flags: review?(bzbarsky)

Comment 68

7 years ago
Comment on attachment 478395 [details] [diff] [review]
Fix

r=me
Attachment #478395 - Flags: review?(bzbarsky) → review+
So, according to Neil, it's currently not possible to test drag and drop in the editor, because we can't raise systemgroup events from privileged Javascript code.

So, I'm going to land this without a test, and set in-litmus? and in-testsuite? flags on the bug.
Status: NEW → ASSIGNED
Flags: in-testsuite?
Flags: in-litmus?(hskupin)
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/e460eac1db7a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b7
Duplicate of this bug: 595152
Attachment #478395 - Flags: approval1.9.2.11?
Attachment #478395 - Flags: approval1.9.1.14?

Updated

7 years ago
Attachment #478395 - Flags: approval1.9.2.11?
Attachment #478395 - Flags: approval1.9.2.11+
Attachment #478395 - Flags: approval1.9.1.14?
Attachment #478395 - Flags: approval1.9.1.14+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/89526ee020eb
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/93d186178c72
status1.9.1: wanted → .14-fixed
status1.9.2: wanted → .11-fixed
Bustage fix:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/147a84a5ceea

(got the bug # wrong in the commit message...)
This got backed out from 1.9.2 along with bug 597784 as there were persisting test failures.
status1.9.2: .11-fixed → ---
Relanded:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23cfc084d4a9
status1.9.2: --- → .11-fixed

Comment 76

7 years ago
> The patch is currently waiting for review, and will be included in the next
release of Firefox 3.5, 3.6 and 4.0.

Ehsan, any idea when the next 3.6 release should be expected? Thanks.

Comment 77

7 years ago
Found the answer ;)
https://wiki.mozilla.org/Releases
(In reply to comment #76)
> > The patch is currently waiting for review, and will be included in the next
> release of Firefox 3.5, 3.6 and 4.0.
> 
> Ehsan, any idea when the next 3.6 release should be expected? Thanks.

https://wiki.mozilla.org/Releases/Firefox_3.6.11

Comment 79

7 years ago
I just tested this with 3.6.11 build2 and while the issue with Drag and Drop appeares to be solved, the error is still reproducible by copy pasting an element (img in my case) inside a contendEditable div.
Please see comment 48 and 57.

Comment 81

7 years ago
If it removes custom attributes from paste as it currently does in the nightly it will break existing products like TinyMCE. We can release a patch for TinyMCE to solve this using data- attributes. However a huge amount of existing systems like Wordpress, Drupal and Joomla just to name a few will break since they will then be using an older version of TinyMCE.

Since we use custom attributes like _mce_<something> could you at least add the possibility to retain prefixed attributes like this so it handles browser vendor specific attributes like _moz_<something> or _webkit_<something> as well and in this case our custom ones?
(In reply to comment #81)
> If it removes custom attributes from paste as it currently does in the nightly
> it will break existing products like TinyMCE. We can release a patch for
> TinyMCE to solve this using data- attributes. However a huge amount of existing
> systems like Wordpress, Drupal and Joomla just to name a few will break since
> they will then be using an older version of TinyMCE.
> 
> Since we use custom attributes like _mce_<something> could you at least add the
> possibility to retain prefixed attributes like this so it handles browser
> vendor specific attributes like _moz_<something> or _webkit_<something> as well
> and in this case our custom ones?

Would you mind filing another bug for this, so that we can discuss what a good solution would look like there?  Thanks!

Updated

7 years ago
Blocks: 604332

Comment 83

7 years ago
I added a new bug report and added it as a dependency to this one since it's related: https://bugzilla.mozilla.org/show_bug.cgi?id=604332
No longer blocks: 604332
Duplicate of this bug: 605722
Blocks: 606117

Updated

7 years ago
Blocks: 613130
Flags: in-litmus?(hskupin) → in-litmus?(mozillamarcia.knous)
Flags: in-litmus?(mozillamarcia.knous)
You need to log in before you can comment on or make changes to this bug.