Bug 12234 - Using createContextualFragment to insert a <script> does not cause the script to execute
Summary: Using createContextualFragment to insert a <script> does not cause the script...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Ryosuke Niwa
URL: http://dscoder.com/MessageStyle/testc...
Keywords:
: 64860 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-01-12 14:04 PST by David Smith
Modified: 2012-05-23 18:46 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 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 Alexey Proskuryakov 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 Eric Seidel (no email) 2010-05-26 17:58:30 PDT
This might get fixed by accident as part of bug 39259 when we get to fragment parsing.
Comment 3 Henri Sivonen 2010-10-19 06:44:50 PDT
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 David Smith 2010-10-19 09:42:06 PDT
Yes, it was. I worked around it for release though (and then the product was killed shortly afterwards).
Comment 5 Henri Sivonen 2010-10-20 00:52:22 PDT
(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 Henri Sivonen 2010-11-11 03:36:18 PST
FWIW, we explicitly decided to make these executable in Firefox 4.
Comment 7 Eric Seidel (no email) 2010-11-11 15:49:54 PST
Is this a regression from the HTML5 parser re-write?
Comment 8 Ryosuke Niwa 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 Henri Sivonen 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 Ryosuke Niwa 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 Adam Barth 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 Ryosuke Niwa 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 Ryosuke Niwa 2012-05-18 11:13:00 PDT
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 Ryosuke Niwa 2012-05-19 20:06:03 PDT
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 Ryosuke Niwa 2012-05-19 20:06:10 PDT
Created attachment 142890 [details]
Fixes the bug
Comment 16 Ryosuke Niwa 2012-05-19 20:08:34 PDT
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 Adam Barth 2012-05-20 21:05:02 PDT
Comment on attachment 142890 [details]
Fixes the bug

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 Ryosuke Niwa 2012-05-20 22:02:59 PDT
(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 Adam Barth 2012-05-20 22:06:09 PDT
Yeah, perfect.
Comment 20 Ryosuke Niwa 2012-05-20 23:10:08 PDT
Committed r117731: <http://trac.webkit.org/changeset/117731>
Comment 21 Ryosuke Niwa 2012-05-23 18:46:52 PDT
*** Bug 64860 has been marked as a duplicate of this bug. ***