Last Comment Bug 597784 - The inserthtml command and nsIHTMLEditor::InsertHTML should not use a sanitizing fragment content sink
: The inserthtml command and nsIHTMLEditor::InsertHTML should not use a sanitiz...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: PowerPC All
: -- normal with 4 votes (vote)
: mozilla2.0b7
Assigned To: :Ehsan Akhgari
:
: Makoto Kato [:m_kato]
Mentors:
: 580442 595176 596513 597190 597393 598287 599316 603219 (view as bug list)
Depends on:
Blocks: CVE-2010-2769 596300
  Show dependency treegraph
 
Reported: 2010-09-18 20:48 PDT by Phillip M. Jones, C.E.T.
Modified: 2010-11-29 18:46 PST (History)
27 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
.11+
.11-fixed
.14+
.14-fixed


Attachments
Patch (v1) (19.89 KB, patch)
2010-09-20 21:46 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1) (22.18 KB, patch)
2010-09-24 12:55 PDT, :Ehsan Akhgari
bzbarsky: review+
christian: approval1.9.2.11+
christian: approval1.9.1.14+
Details | Diff | Splinter Review

Description Phillip M. Jones, C.E.T. 2010-09-18 20:48:18 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.1.11) Gecko/20100701 Lightning/1.0b1 Mnenhy/0.8.2 SeaMonkey/2.0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.1.12) Gecko/20100701 Lightning/1.0b1 Mnenhy/0.8.2 SeaMonkey/2.0.6

Starting with SM2.0.7 (gecko rv:1.9.1.12) and up when you attempt in html containing items (sound/Video) either using Embed or object tag using the insert HTML in Newsgroups that allow such content, does not appear. You can type the content  and then choose insert. The  insert c ommand appears to be ignored. 

Reproducible: Always

Steps to Reproduce:
1.Begin Compose Message 
2.Go to insert Menu choose HTML
3.type in desired code for inserted content
4. click on insert
Actual Results:  
inserted content does not get inserted
save as Draft to prove Nothing there.

Expected Results:  
In 2.0.6 (rv:1.9.1.11) and Below
inserted content is inserted 
Save as Draft and view Draft and Embed Content works.

You can Try in safe Mode has No affect. Theme doesn't make a difference.  Although I am using Macintosh OSX, I've heard reports from people with other platforms have the same problem.

I while I don't normal send HTML content in newsgroup messages There are some newsgroup That allow it and I use the abilty to test out code before inserting my website or other web pages.
Comment 1 :Ehsan Akhgari 2010-09-20 11:58:53 PDT
What is the exact HTML code you enter in step 3?
Comment 2 Phillip M. Jones, C.E.T. 2010-09-20 12:20:53 PDT
either:

<EMBED SRC="title" AUTOSTART=TRUE LOOP=FALSE WIDTH=145 HEIGHT=55 ALIGN="CENTER">
</EMBED>


or 

<object
type="application/x-type" data="title"
width="150" height="40" AUTOSTART="FALSE">
<param name="music" value="title" />
</object>


are two samples: 

I use music in these examples  that I have loaded on my website so title would be:

http://www.phillipmjones.net/songtittle/extension

here is  a real example:

<object
type="application/mp3" data="http://www.phillipmjones.net/Brown_Eyed_Handsom_Man.mp3"
width="150" height="40" AUTOSTART="FALSE">
<param name="music" value="http://www.phillipmjones.net/Brown_Eyed_Handsom_Man.mp3" />
</object>

The above code I finally used in my website on a particular page The title was different

or

<EMBED SRC="http://www.phillipmjones.net/Brown_Eyed_Handsom_Man.mp3" AUTOSTART=FALSE LOOP=FALSE WIDTH=145 HEIGHT=55 ALIGN="CENTER">
</EMBED>

 to actually insert title directly hasn't worked since switch from Mozilla to SeaMonkey Note I own this music Title and is not meant to be copied. 

Note I may have typed wrong in live examples but the original code came from the internet and is accurate.
Comment 3 Phillip M. Jones, C.E.T. 2010-09-20 12:25:51 PDT
<object>
<param name="autostart" value="true">
<param name="src" value="eureka.wav">
<param name="autoplay" value="true">
<param name="controller" value="true">
<embed src="eureka.wav" controller="true" autoplay="true" autostart="True" type="audio/wav" />
</object>
 this a another version

in live example would be :

<object>
<param name="autostart" value="false">
<param name="src" value="http://www.phillipmjones.net/Brown_Eyed_Handsom_Man.mp3">
<param name="autoplay" value="false">
<param name="controller" value="true">
<embed src="http://www.phillipmjones.net/Brown_Eyed_Handsom_Man.mp3" controller="true" autoplay="false" autostart="False" type="audio/mp3" />
</object>
Comment 4 Phillip M. Jones, C.E.T. 2010-09-20 12:27:23 PDT
the second example just above is what I now use on my website.
Comment 5 :Ehsan Akhgari 2010-09-20 15:23:10 PDT
Morphing this bug to switch the inserthtml command not to use a sanitizing fragment content sink.
Comment 6 :Ehsan Akhgari 2010-09-20 15:23:39 PDT
*** Bug 580442 has been marked as a duplicate of this bug. ***
Comment 7 Phillip M. Jones, C.E.T. 2010-09-20 17:16:31 PDT
I use the ability to to test such code in newsgroups that allow such (Embed and Object)  so that I can get a second opinion on something I can use in my website. 

In fact since this bug has reared its ugly head. I have discontinued upgrading SeaMonkey at 2.0.6 Gecko  rv:1.9.1.11. And I will continue not upgrading until it is fixed or some method is created to bypass it through about config, a menu choice, or someone comes up a hack to get around it. 

No one has the right to tell me how I can use an application (especially an open Source application that depends upon the good graces of users to promote it). Most of the people  that use such are honest, and intend no harm.

Instead of going about the way why not have a preference setting to allow person on other end to block the items. Then the users get to choose whether they want to do or see or heard such. 

Again we have developer run amuck, deciding what they feel is right for the user rather than letting the users decide.
Comment 8 :Ehsan Akhgari 2010-09-20 21:46:50 PDT
Created attachment 477032 [details] [diff] [review]
Patch (v1)
Comment 9 :Ehsan Akhgari 2010-09-21 09:17:32 PDT
*** Bug 597393 has been marked as a duplicate of this bug. ***
Comment 10 :Ehsan Akhgari 2010-09-21 09:18:41 PDT
*** Bug 598287 has been marked as a duplicate of this bug. ***
Comment 11 :Ehsan Akhgari 2010-09-21 09:18:44 PDT
*** Bug 595176 has been marked as a duplicate of this bug. ***
Comment 12 hans 2010-09-21 14:06:07 PDT
Will the paste command be modified as well?
Comment 13 :Ehsan Akhgari 2010-09-21 14:43:04 PDT
(In reply to comment #12)
> Will the paste command be modified as well?

No.
Comment 14 :Ehsan Akhgari 2010-09-21 14:43:16 PDT
*** Bug 596513 has been marked as a duplicate of this bug. ***
Comment 15 hans 2010-09-21 16:12:51 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Will the paste command be modified as well?
> 
> No.

So when this patch is applied, a user (of Seasmonkey / Thunderbird composer or some CMS in FF), will be able to insert an object, but won't be able to move it using cut/paste. That's not very intuitive.
Comment 16 :Ehsan Akhgari 2010-09-21 17:02:59 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Will the paste command be modified as well?
> > 
> > No.
> 
> So when this patch is applied, a user (of Seasmonkey / Thunderbird composer or
> some CMS in FF), will be able to insert an object, but won't be able to move it
> using cut/paste. That's not very intuitive.

SeaMonkey and Thunderbird can switch to using the InsertHTML API if they want to enable pasting unfiltered HTML.
Comment 17 hans 2010-09-21 23:53:37 PDT
Ok. Thanks for updating the white list of the sanitizing fragment sink. It would be nice to have the data-* attributes white listed too. Otherwise web developers may start abusing other attributes or elements to fit their needs and workaround the behavior in Firefox.
http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#embedding-custom-non-visible-data
Comment 18 :Ehsan Akhgari 2010-09-22 08:13:53 PDT
(In reply to comment #17)
> Ok. Thanks for updating the white list of the sanitizing fragment sink. It
> would be nice to have the data-* attributes white listed too. Otherwise web
> developers may start abusing other attributes or elements to fit their needs
> and workaround the behavior in Firefox.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#embedding-custom-non-visible-data

See bug 598105.
Comment 19 Daniel Veditz [:dveditz] 2010-09-22 10:42:51 PDT
We need to make sure this fix doesn't regress the security problem we were originally addressing.
Comment 20 :Ehsan Akhgari 2010-09-22 12:47:43 PDT
(In reply to comment #19)
> We need to make sure this fix doesn't regress the security problem we were
> originally addressing.

It won't.  These are two separate code paths, and we have tests for the security fix.
Comment 21 :Ehsan Akhgari 2010-09-22 16:41:19 PDT
bz: review ping...
Comment 22 Osbron 2010-09-22 21:47:16 PDT
After fixing this bug, I would recommend this patch to be included into the newer version of fiefox 3.6.11 so that everyone would be able to enjo this patch. As seen from the responses, this problem had affected many, and thus its highly recommended that this bug is fixed, and fast.

Other users experiencing this problem are mainly website developers especially using joomla.
Comment 23 :Ehsan Akhgari 2010-09-23 12:51:12 PDT
Osbron: the fix to this bug will be shipped in the text dot release of Firefox 3.5 and 3.6.
Comment 24 Phillip M. Jones, C.E.T. 2010-09-23 17:07:13 PDT
will this also be fixed in next "." version of SeaMonkey as well?
Comment 25 :Ehsan Akhgari 2010-09-23 21:37:11 PDT
(In reply to comment #24)
> will this also be fixed in next "." version of SeaMonkey as well?

Kairo is probably a better person to answer this question.  I don't know a lot about SM's release schedule.
Comment 26 Robert Kaiser 2010-09-24 05:21:42 PDT
Anything that goes into the platform for the next Firefox 3.5 release goes into the next SeaMonkey 2.0 release, so given comment #23, the answer seems to be yes.
Comment 27 :Ehsan Akhgari 2010-09-24 12:55:28 PDT
Created attachment 478369 [details] [diff] [review]
Patch (v1)

I just realized that I had forgotten to add a test that I wrote to the patch!
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2010-09-24 13:16:23 PDT
Comment on attachment 478369 [details] [diff] [review]
Patch (v1)

You don't need approval; this is a blocker.

I'd prefer DoInsertHTMLWithContext as the function name.  The header needs some documentation.  And the copy/pasted comments in the code you moved have typos.  r=me with all that fixed.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2010-09-24 13:17:01 PDT
I do wonder whether this needs to block b7 or not, though...
Comment 30 Matthias Versen [:Matti] 2010-09-24 18:56:24 PDT
*** Bug 599316 has been marked as a duplicate of this bug. ***
Comment 31 :Ehsan Akhgari 2010-09-27 10:13:26 PDT
(In reply to comment #29)
> I do wonder whether this needs to block b7 or not, though...

Well, I'm going to land it today since it's a branch blocker as well, but yes, I'd think that this should have been a beta7 blocker...
Comment 34 :Ehsan Akhgari 2010-09-27 19:43:09 PDT
Test fix follow-up on branches: nsIEditor::PasteFromTransferable is not available on branches, so I've disabled the part of the test which used it.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/22b0a1db44b6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/69d435c2f4ec
Comment 35 Wolfgang Rosenauer [:wolfiR] 2010-09-27 23:42:33 PDT
Disabling the part has bad side effects? Please see orange.
Comment 36 Mark Banner (:standard8, limited time in Dec) 2010-09-28 01:16:53 PDT
Backed out from 1.9.2 due to persisting test failures:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95a205a82cb8
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ef28900b1c02

Please check 1.9.1 as well.
Comment 37 :Ehsan Akhgari 2010-09-28 09:10:29 PDT
(In reply to comment #36)
> Backed out from 1.9.2 due to persisting test failures:
> 
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95a205a82cb8
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ef28900b1c02
> 
> Please check 1.9.1 as well.

Oh, sorry about the oranges.  :(  Backporting tests to branches is harder than I expected.  I'll reland with a test fix later today.
Comment 39 :Ehsan Akhgari 2010-09-28 11:18:39 PDT
Test fix ported to 1.9.1:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ee5091e706e
Comment 40 Frank Wein [:mcsmurf] 2010-10-03 14:07:04 PDT
*** Bug 601483 has been marked as a duplicate of this bug. ***
Comment 41 Joe Sabash [:JoeS1] 2010-10-04 05:57:03 PDT
*** Bug 597190 has been marked as a duplicate of this bug. ***
Comment 42 Ludovic Hirlimann [:Usul] 2010-10-14 05:36:38 PDT
*** Bug 603219 has been marked as a duplicate of this bug. ***
Comment 43 Graham 2010-11-02 10:47:35 PDT
Frank,
Repeat of Bug 601483: Just tested SM 2.0.9 and 2.0.10 - NOT resolved, NOT fixed: the fault is still the same. 
In SeaMonkey Composer 2.0.9 and 2.0.10, copy-and-paste of onclick='document.location.href="#top"' fails. Version 2.0.6 is OK.
Graham.
Comment 44 :Ehsan Akhgari 2010-11-02 11:52:09 PDT
(In reply to comment #43)
> Frank,
> Repeat of Bug 601483: Just tested SM 2.0.9 and 2.0.10 - NOT resolved, NOT
> fixed: the fault is still the same. 
> In SeaMonkey Composer 2.0.9 and 2.0.10, copy-and-paste of
> onclick='document.location.href="#top"' fails. Version 2.0.6 is OK.
> Graham.

Please file a bug in the appropriate SeaMonkey component for this.
Comment 45 Frank Wein [:mcsmurf] 2010-11-09 15:24:54 PST
Bug 601483 has been filed on this, I marked it as a dupe of this one a while ago as I thought this bug should have fixed the bug. Is inserting
<div style="text-align: right;"> <input value="Back to Top"
onclick='document.location.href="#top"' type="button"> </div>
via the Insert HTML... dialog another problem?
Comment 46 :Ehsan Akhgari 2010-11-09 15:35:24 PST
(In reply to comment #45)
> Bug 601483 has been filed on this, I marked it as a dupe of this one a while
> ago as I thought this bug should have fixed the bug. Is inserting
> <div style="text-align: right;"> <input value="Back to Top"
> onclick='document.location.href="#top"' type="button"> </div>
> via the Insert HTML... dialog another problem?

Depends on how SeaMonkey has implemented its Insert HTML command.  If it's using nsIHTMLEditor::InsertHTML, then this should be fixed in this bug.  Otherwise, it should switch to using that API!  :-)
Comment 47 Jérôme Poirrier 2010-11-28 06:28:15 PST
(In reply to comment #43)
> Frank,
> Repeat of Bug 601483: Just tested SM 2.0.9 and 2.0.10 - NOT resolved, NOT
> fixed: the fault is still the same. 
> In SeaMonkey Composer 2.0.9 and 2.0.10, copy-and-paste of
> onclick='document.location.href="#top"' fails. Version 2.0.6 is OK.
> Graham.

November 28th 2010
I've got the same thing, with SM 2.0.9 and 2.0.10. When I copy-paste a button, the javascript properties are lost in the resulting copies. 

I have felt complied to revert to SM 2.0.2 several times (unfortunately, as a kee-jerk reaction,I sometimes do allow automatic upgrading  SM 2.0.10, and I can no longer work and it drives me crazy). 

How on earth can this bug be deemed "RESOLVED FIXED" ??!!!
Comment 48 Jérôme Poirrier 2010-11-28 06:30:32 PST
Oops, sorry for the typo : read "knee-jerk reaction" instead of "kee-jerk"
Comment 49 Jérôme Poirrier 2010-11-28 06:32:34 PST
Oops, another typo :   upgrading *to* SM 2.0.10
Comment 50 Jérôme Poirrier 2010-11-28 06:39:22 PST
Oops III : "compelled" instead of "complied".
(It's time I got myself some sleep instead of coffee, 
and I'd better post the whole msg again) :

November 28th 2010
I've got the same thing, with SM 2.0.9 and 2.0.10. When I copy-paste a button,
the javascript properties are lost in the resulting copies. 

I have felt compelled to revert to SM 2.0.2 several times (unfortunately, as a
knee-jerk reaction,I sometimes do allow automatic upgrading to SM 2.0.10, and I
can no longer work and it drives me crazy). 

How on earth can this bug be deemed "RESOLVED FIXED" ??!!!
Comment 51 :Ehsan Akhgari 2010-11-29 12:18:12 PST
Please file a new bug in the SeaMonkey specific component and ask the SeaMonkey developers to pick up the fix to this bug.  This has been fixed in core, and I'm not sure if SeaMonkey has plans to pick up the fix for this bug, but that doesn't have anything to do with this core bug.
Comment 52 Graham 2010-11-29 12:43:30 PST
Response to comment 51 (comments 43 from me and 50 from Jerome refer):
Ehsan, this issue is being bounced around with no-one seeming to take ownership. I originated bug report 601483 in SeaMonkey Composer - is that the 'specific component' you mention?
Somebody thought it was a dupe of this 597784. Whoever it belongs to, it is NOT fixed. I will post this same message in 601483 and hope someone picks it up.
Thanks.
Graham.
Comment 53 :Ehsan Akhgari 2010-11-29 18:46:11 PST
Please stop posting SeaMonkey specific stuff here.  This is a core bug.  For issues with SeaMonkey, you need to either talk to SM developers or file bugs in SM bugzilla components.

Note You need to log in before you can comment on or make changes to this bug.