Bug 12234 - Using createContextualFragment to insert a <script> does not cause the script to execute
: Using createContextualFragment to insert a <script> does not cause the script...
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
: http://dscoder.com/MessageStyle/testc...
:
:
:
  Show dependency treegraph
 
Reported: 2007-01-12 14:04 PST by
Modified: 2012-05-23 18:46 PST (History)


Attachments
Fixes the bug (34.09 KB, patch)
2012-05-19 20:06 PST, Ryosuke Niwa
abarth: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2007-01-12 14:04:36 PST
The testcase url creates an document fragment from an html string containing a script element, but the script is not actually run. Firefox runs the script as expected.
------- Comment #1 From 2007-01-12 14:18:09 PST -------
A similar case:

document.body.innerHTML+="<script>alert('SUCCESS')</script>"

May or may not need or may already have a separate bug for this.
------- Comment #2 From 2010-05-26 17:58:30 PST -------
This might get fixed by accident as part of bug 39259 when we get to fragment parsing.
------- Comment #3 From 2010-10-19 06:44:50 PST -------
Was this bug report motivated by breakage on a real-world site?

Compare with https://bugzilla.mozilla.org/show_bug.cgi?id=599588
------- Comment #4 From 2010-10-19 09:42:06 PST -------
Yes, it was. I worked around it for release though (and then the product was killed shortly afterwards).
------- Comment #5 From 2010-10-20 00:52:22 PST -------
(In reply to comment #4)
> Yes, it was. I worked around it for release though (and then the product was killed shortly afterwards).

What was the use case you had? Note that if WebKit made createContextualFragment create fragments with executable scripts, inserting the fragments to the document wouldn't guarantee the execution order between external scripts or between external and internal scripts.
------- Comment #6 From 2010-11-11 03:36:18 PST -------
FWIW, we explicitly decided to make these executable in Firefox 4.
------- Comment #7 From 2010-11-11 15:49:54 PST -------
Is this a regression from the HTML5 parser re-write?
------- Comment #8 From 2010-11-11 16:22:51 PST -------
I talked with Eric about this and it seems like HTML5 spec intentionally prohibits the script elements coming from the fragments to run.

See http://www.whatwg.org/specs/web-apps/current-work/#parsing-main-inhead where it says:

If the parser was originally created for the HTML fragment parsing algorithm, then mark the script element as "already started". (fragment case)
------- Comment #9 From 2010-11-11 22:29:26 PST -------
(In reply to comment #8)
> I talked with Eric about this and it seems like HTML5 spec intentionally prohibits the script elements coming from the fragments to run.

Yes, but http://www.w3.org/Bugs/Public/show_bug.cgi?id=11191
------- Comment #10 From 2010-11-27 09:37:53 PST -------
I'm concerned about security implication of enabling scripts.  I can't convince myself that there aren't any websites that rely on the fact WebKit does not execute scripts coming from createContextualFragment.  

While not running scripts expected to run will break the websites, running scripts not expected to run will create a XSS security vulnerability.

I do understand that a fragment created by createContextualFragment is no different than the fragment created by other means and Firefox folks want WebKit to be compatible with Firefox, however, I would avoid the risk of creating a XSS vulnerability at all cost.
------- Comment #11 From 2010-11-29 08:45:08 PST -------
rniwa, thanks for being sensitive to creating XSS vulnerabilities.  However, in this case, we're not opening up a new vulnerability.  The attacker can already use other syntactic constructs to execute script, similar to how the attacker can run script via innerHTML even though innerHTML doesn't execute <script> tags.
------- Comment #12 From 2010-11-29 17:05:31 PST -------
(In reply to comment #11)
> rniwa, thanks for being sensitive to creating XSS vulnerabilities.  However, in this case, we're not opening up a new vulnerability.  The attacker can already use other syntactic constructs to execute script, similar to how the attacker can run script via innerHTML even though innerHTML doesn't execute <script> tags.

Ok.  Then we probably should fix this bug to be compatible with Firefox.  Special-casing fragment parsing first seemed strange weird but a number of developers pointed out that the fragment created by createContextualFragment is no different from a fragment created by manually assembling nodes.  And because the script element manually inserted into a fragment runs when the fragment is inserted into a document, we should also run the script parsed by createContextualFragment when the fragment is inserted into a document.
------- Comment #13 From 2012-05-18 11:13:00 PST -------
Per whatwg discussion, we should fix this bug: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-February/030351.html

Also see the bug 86376.
------- Comment #14 From 2012-05-19 20:06:03 PST -------
Note there appears to be a bug in the spec: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-May/036088.html

I'm going to post a patch based on my assumption that we should be clearing both parser inserted and already started flags in createContextualFragment.
------- Comment #15 From 2012-05-19 20:06:10 PST -------
Created an attachment (id=142890) [details]
Fixes the bug
------- Comment #16 From 2012-05-19 20:08:34 PST -------
I can separate the enum rename if reviewers wanted but I didn't feel a need for this simple refactoring other than to boost the number of patches I'm going to commit :)
------- Comment #17 From 2012-05-20 21:05:02 PST -------
(From update of attachment 142890 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=142890&action=review

ok

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:340
> +    // For createContextualFragment, the specifications say to mark it parser-inserted and already-started and later unmark them.
> +    // However, we short circuit that logic to avoid the subtree traversal to find script elements since scripts can never see
> +    // those flags or effects thereof

Thanks for adding this comment.  Can we add a link to the spec?  We like to do that in the parser because we track the spec so closely.  (nit: please add a . at the end of the sentence.)
------- Comment #18 From 2012-05-20 22:02:59 PST -------
(In reply to comment #17)
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:340
> > +    // For createContextualFragment, the specifications say to mark it parser-inserted and already-started and later unmark them.
> > +    // However, we short circuit that logic to avoid the subtree traversal to find script elements since scripts can never see
> > +    // those flags or effects thereof
> 
> Thanks for adding this comment.  Can we add a link to the spec?  We like to do that in the parser because we track the spec so closely.  (nit: please add a . at the end of the sentence.)

Would http://html5.org/specs/dom-parsing.html#dom-range-createcontextualfragment work?
------- Comment #19 From 2012-05-20 22:06:09 PST -------
Yeah, perfect.
------- Comment #20 From 2012-05-20 23:10:08 PST -------
Committed r117731: <http://trac.webkit.org/changeset/117731>
------- Comment #21 From 2012-05-23 18:46:52 PST -------
*** Bug 64860 has been marked as a duplicate of this bug. ***