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.
A similar case: document.body.innerHTML+="<script>alert('SUCCESS')</script>" May or may not need or may already have a separate bug for this.
This might get fixed by accident as part of bug 39259 when we get to fragment parsing.
Was this bug report motivated by breakage on a real-world site? Compare with https://bugzilla.mozilla.org/show_bug.cgi?id=599588
Yes, it was. I worked around it for release though (and then the product was killed shortly afterwards).
(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.
FWIW, we explicitly decided to make these executable in Firefox 4.
Is this a regression from the HTML5 parser re-write?
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)
(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
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.
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.
(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.
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.
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.
Created attachment 142890 [details] Fixes the bug
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 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.)
(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?
Yeah, perfect.
Committed r117731: <http://trac.webkit.org/changeset/117731>
*** Bug 64860 has been marked as a duplicate of this bug. ***