If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

ASSIGNED
Assigned to

Status

()

Core
Editor
--
major
ASSIGNED
7 years ago
3 years ago

People

(Reporter: mrandromeda, Assigned: Ehsan)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [post-2.0])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

7 years ago
I'm assuming you're talking about bug 520189, which landed in 3.6.9?
Blocks: 520189
Component: General → Editor
Keywords: regression
Product: Firefox → Core
QA Contact: general → editor
Summary: Element Attributes dropped in DesignMode/ContentEditable sections with copy/paste → Element Attributes should not be sanitized (dropped) in DesignMode/ContentEditable sections when copy/paste happens within a domain
Boris, do you know if the principal is supposed to survive a copy/paste operation?
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?
(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.
Can we only store strings in the transferable?  That is, can we not stick an nsIPrincipal* in there?
(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... :(
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.
Depends on: 596300
Created attachment 489364 [details] [diff] [review]
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?
Assignee: nobody → ehsan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #489364 - Flags: feedback?(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.
Created attachment 489495 [details] [diff] [review]
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.
Attachment #489364 - Attachment is obsolete: true
Attachment #489495 - Flags: feedback?(bzbarsky)
Attachment #489364 - Flags: feedback?(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?
(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!
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...
Maybe Neil?
Nothining obviously jumps out at me. Trying another Neil...

Comment 16

7 years ago
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.
(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?
Ehsan, are you still waiting for feedback from me here?
(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...
Whiteboard: [post-2.0]
Attachment #489495 - Flags: feedback?(bzbarsky)

Comment 20

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

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.