606117 - Element Attributes should not be sanitized (dropped) in DesignMode/ContentEditable sections when copy/paste happens within a domain

Status: NEW

(Core :: DOM: Editor, defect)

Description

[mrandromeda]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; InfoPath.2; .NET CLR 3.5.21022; .NET CLR 3.5.30729; .NET CLR 3.0.30729; OfficeLiveConnector.1.5; OfficeLivePatch.1.3; .NET4.0C)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11 ( .NET CLR 3.5.30729; .NET4.0C)

Note: This is not a duplicate of bug 596300

When you copy and paste in a contenteditable section, some attributes (e.g. like a href with "javascript:") are dropped (sanitized). This should not be done if you copy/paste in the same domain or even in the same contenteditable section. I had hoped this was fixed in 3.6.11 but it is not.

This is a groundbreaking change which effects all are users off our online web applications (> 10.000 users) an we can’t explain to them why the copy/paste does not work anymore. You know this also effects all online webmail systems (like gmail/hotmail etc) and probably all online CMS systems. I think in this case this “security” fix causes more problems than that is fixes. We sanitize our code our self en we want to decide in our web applications what is safe or not, not the browser. There should be at least an option to turn this off.

At the moment we disabled Firefox access/support an advice all our users to use a different browser which do not have this problem (Chrome to the rescue!). If you do not fix this issue it is less Firefox users for you.


Reproducible: Always

Steps to Reproduce:
1.Make a contenteditable div
2.Copy some html code with a link which has a javascript: link
Actual Results:  
The href is completely removed.

Expected Results:  
Leave it as it is.

Comment 1

[Nickolay_Ponomarev]

I'm assuming you're talking about bug 520189, which landed in 3.6.9?

Comment 2

[(no longer active)]

Boris, do you know if the principal is supposed to survive a copy/paste operation?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Which principal?  I think it's reasonable, if we can manage it, to track the principal a clipboard entry came from, if that's what you're asking.... can we do that?

Comment 4

[(no longer active)]

(In reply to comment #3)
> Which principal?

The principal of the source document, from which the content is copied to the clipboard.

> I think it's reasonable, if we can manage it, to track the
> principal a clipboard entry came from, if that's what you're asking.... can we
> do that?

Well, that was sort of my question!  :-)  I can't see any code which serializes the principal in the transferable object, and I'm not sure what stuff needs to be serialized and how in order to make it work.  I thought you might have more information here.

Also, we should keep in mind that a solution which is backport-able to 1.9.2 is what we should opt for, I think.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Can we only store strings in the transferable?  That is, can we not stick an nsIPrincipal* in there?

Comment 6

[(no longer active)]

(In reply to comment #5)
> Can we only store strings in the transferable?  That is, can we not stick an
> nsIPrincipal* in there?

The nsITransferable documentation <http://mxr.mozilla.org/mozilla-central/source/widget/public/nsITransferable.idl#109> claims that we could pass any nsISupportsPrimitive, but looking at <http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsPrimitiveHelpers.cpp#82> and <http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsPrimitiveHelpers.cpp#131> I can say with almost certainty that those are just big lies, and all we can put in a transferable is either strings or files... :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, nsIPrincipal is not an nsISupportsPrimitive anyway.

We do have code for serializing and deserializing principals (and can even do that as base64, if we can't have embedded nulls).  This will lose information in some very rare cases, but should work fine for web sites, for the most part.

If the question is how to serialize a principal, the simplest thing (will do base64 stuff, but the extra cost of that even if we don't need it probably doesn't matter much) is NS_SerializeToString and NS_DeserializeObject in nsSerializationHelper.h.  So QI the principal to nsISerializable, and if that succeeds serialize to string.  On the other end, deserialize into an nsCOMPtr<nsISupports> and QI to nsIPrincipal.

Comment 8

[(no longer active)]

Attachment: WIP

This patch should work for both copy/paste and drag/drop.  Except that it doesn't for some reason!  I've verified that the DOM transfer object does have a text/_moz_htmldocinfo flavor when a drag operation is started, but by the time that the drop happens, the value for the text/_moz_htmldocinfo flavor is null.  Boris, do you happen to know if I'm missing something obvious here?

Comment 9

[Boris Zbarsky [:bzbarsky]]

So in Produce |data| ends up null?

Or do we get null in InsertFromTransferable?  And is that null for genericDataObj, or null for srcPrincipal?  Or is the length check failing?

Your call to NS_DeserializeObject is wrong.  You need to deserialize into an nsCOMPtr<nsISupports> and then QI to nsIPrincipal; anything else makes assumptions about the principal's vtable.

Comment 10

[(no longer active)]

Attachment: WIP 2

(In reply to comment #9)
> So in Produce |data| ends up null?
> 
> Or do we get null in InsertFromTransferable?  And is that null for
> genericDataObj, or null for srcPrincipal?  Or is the length check failing?

|transferable->GetTransferData(kHTMLDocInfo, getter_AddRefs(docInfoData), &docInfoLen)| fails.  The transferable doesn't have any data associated with the doc info mimetype.

> Your call to NS_DeserializeObject is wrong.  You need to deserialize into an
> nsCOMPtr<nsISupports> and then QI to nsIPrincipal; anything else makes
> assumptions about the principal's vtable.

Oh, you're right.  This version of the patch fixes this plus a few other minor issues.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> The transferable doesn't have any data associated with the doc info mimetype.

Odd.  I have no idea why not.  We probably need to sort that out before I look at the patch more, right?

Comment 12

[(no longer active)]

(In reply to comment #11)
> > The transferable doesn't have any data associated with the doc info mimetype.
> 
> Odd.  I have no idea why not.  We probably need to sort that out before I look
> at the patch more, right?

Probably.

Also, I should mention that the same problem exists for copy/paste, which is *really* odd!

Comment 13

[(no longer active)]

Is there someone which I can CC on this bug and ask about this oddness?  I've banged my head against the wall with this patch for quite some time, and I'm at a point where I don't think I'm going to make much progress on this without help from someone who knows this transferable/domdatatransfer stuff better than I do...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Maybe Neil?

Comment 15

[[e-mail]]

Nothining obviously jumps out at me. Trying another Neil...

Comment 16

[Neil Deakin]

Did you add the flavour to the transferable (using AddDataFlavor) beforehand? As nsHTMLEditor::PrepareHTMLTransferable does?

Note that nsDOMDataTransfer already stores the principal of the data. You shouldn't be storing a separate principal. You can also just check nsINSDOMDataTransfer::mozSourceNode.

Comment 17

[(no longer active)]

(In reply to comment #16)
> Did you add the flavour to the transferable (using AddDataFlavor) beforehand?
> As nsHTMLEditor::PrepareHTMLTransferable does?

On the source, AddString takes care of that.  On the destination, I tried adding the flavor in PrepareHTMLTransferable, and it doesn't make any difference.  Actually when I poke inside the DOM transferable object in gdb after a drop, the value is _not_ stored in the object at all...

> Note that nsDOMDataTransfer already stores the principal of the data. You
> shouldn't be storing a separate principal. You can also just check
> nsINSDOMDataTransfer::mozSourceNode.

What about copy/paste?  We'll need the principal in that case, right?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ehsan, are you still waiting for feedback from me here?

Comment 19

[(no longer active)]

(In reply to comment #18)
> Ehsan, are you still waiting for feedback from me here?

Well, I guess not.  I think I should look into this again post 2.0, because I have a weird feeling that I've been missing something embarrassing before.  We'll see whether that is the case...

Comment 20

[Ojan Vafai]

FWIW, this caused a bug in Google Sites where metadata about images stored in attributes was being lost. They worked around it by using data- attributes, which are not stripped apparently. I'm not sure if there are other Google properties that use the same module as Google Sites that was causing them this problem.

Comment 21

[(no longer active)]

(In reply to Ojan Vafai from comment #20)
> FWIW, this caused a bug in Google Sites where metadata about images stored
> in attributes was being lost. They worked around it by using data-
> attributes, which are not stripped apparently. I'm not sure if there are
> other Google properties that use the same module as Google Sites that was
> causing them this problem.

Using data attributes is the currently supported way of making sure that attributes are not sanitized.

Comment 22

[ejsanders]

No progress in 2 years? On VisualEditor we use RDFa attributes to store meaningful data and because of this bug have to serialize everything in to a data attribute. It would be nice if it was fixed.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.