Last Comment Bug 305873 - Unclosed script data should not be parsed as HTML
: Unclosed script data should not be parsed as HTML
Status: RESOLVED FIXED
[patch]
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: x86 Windows XP
: P4 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
: 362397 (view as bug list)
Depends on: 325500 327796 386794 393281 408702 412114 448895 651936
Blocks: xss 311395 321142
  Show dependency treegraph
 
Reported: 2005-08-25 00:03 PDT by Jing Lim
Modified: 2011-04-21 11:58 PDT (History)
13 users (show)
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Demonstrate the SCRIPT parsing problem (88 bytes, text/html)
2005-08-25 00:05 PDT, Jing Lim
no flags Details
Fix (49.03 KB, patch)
2006-01-13 15:37 PST, Blake Kaplan (:mrbkap)
jonas: review-
Details | Diff | Splinter Review
"Fix" serialization (57.69 KB, patch)
2006-01-27 14:46 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Updated to comments (59.43 KB, patch)
2006-01-27 16:13 PST, Blake Kaplan (:mrbkap)
jonas: review+
Details | Diff | Splinter Review
With different roundtripping (45.85 KB, patch)
2006-01-30 17:09 PST, Blake Kaplan (:mrbkap)
jonas: review+
jst: superreview+
jonas: approval‑branch‑1.8.1+
Details | Diff | Splinter Review

Description Jing Lim 2005-08-25 00:03:58 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

If a web server that is serving a page shuts down, or if the http connection
gets reset, a </script> may not be sent to the browser.

Firefox should then ignore the <script> block. It SHOULD NOT parse the data as
HTML! e.g.

<script><!--
var x = '><iframe src="http://www.google.com"></iframe>';

This bug is VERY bad, because random web pages may get requested. And there
doesn't seem to be a way from preventing the browser from processing the data as
HTML if the end-script isn't sent.




Reproducible: Always

Steps to Reproduce:
1. Open the HTML shown above. (I'll attach the test case as well)
2.
3.

Actual Results:  
The iframe is shown.

Expected Results:  
The page should be blank. The script data should be ignored. That's what IE does.
Comment 1 Jing Lim 2005-08-25 00:05:53 PDT
Created attachment 193791 [details]
Demonstrate the SCRIPT parsing problem
Comment 2 Blake Kaplan (:mrbkap) 2005-08-25 09:51:07 PDT
I always thought we did this for compat with IE, but investigating shows that IE
actually consumes <script> and <style> non-conservatively (i.e., it eats to the
end of the document if we don't find the closing </script> or </style>).
However, IE refuses to execute such scripts.

Should we follow IE here? Opera does what we do. I'm not sure about other browsers.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2005-08-25 12:53:21 PDT
Which IE did you test and in standards or quirks mode?  ;)
Comment 4 Blake Kaplan (:mrbkap) 2005-08-25 13:01:24 PDT
This was IE6 on Windows. The behavior is the same in both standards and quirks mode.
Comment 5 Boris Zbarsky [:bz] (TPAC) 2005-08-25 13:05:07 PDT
Huh.  Do our cvs logs have any indication why we're doing things this way?
Comment 6 Blake Kaplan (:mrbkap) 2005-08-25 16:36:11 PDT
The logs blame bug 35456, where harishd says that he implemented the current
behavior for compatibility with Nav4.x, but when I tested with 4.8, I saw really
weird behavior (a random character appeared on the page that went away when I
tried to select it). So it looks like the only sticking points for emulating IE
are: Opera's behavior (do we care?) and that we shouldn't run the script if
there is a malformed or missing </script> tag.
Comment 7 Blake Kaplan (:mrbkap) 2005-10-28 18:25:34 PDT
Confirming based on bug 311395.
Comment 8 Blake Kaplan (:mrbkap) 2006-01-13 15:37:32 PST
Created attachment 208419 [details] [diff] [review]
Fix
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-01-18 18:21:36 PST
Comment on attachment 208419 [details] [diff] [review]
Fix

this won't cancel enough serialization
Comment 10 Blake Kaplan (:mrbkap) 2006-01-27 14:46:26 PST
Created attachment 209903 [details] [diff] [review]
"Fix" serialization

The viewPartialSource.js fixes are really hacks, but they work and they produce the "right" output. If anybody actually does use DOM2 elements to append elements to a document that has an unclosed <script>, they get what's coming to them!
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-01-27 15:39:03 PST
Comment on attachment 209903 [details] [diff] [review]
"Fix" serialization

>Index: content/base/src/nsHTMLContentSerializer.cpp
>+static PRBool
>+HasNextSibling(nsIContent *aContent)

"sibling" isn't really used right here. Alternative names: HasNextInDocumentOrder, HasFollowingInDocumentOrder, ContentOrParentHasFollowingSibling

>@@ -724,36 +751,59 @@ nsHTMLContentSerializer::AppendElementSt
>   SerializeAttributes(content, name, aStr);
> 
>   AppendToString(kGreaterThan, aStr);
> 
>   if (LineBreakAfterOpen(name, hasDirtyAttr)) {
>     AppendToString(mLineBreak, aStr);
>     mMayIgnoreLineBreakSequence = PR_TRUE;
>     mColPos = 0;
>   }
> 
>+  if (name == nsHTMLAtoms::script) {
>+    // Handle malformed script serialization. In general, we don't want to
>+    // serialize end tags for scripts, so that round-tripping a document that

"serialize end tags of malformed scripts"

>+    // doesn't have a </script> won't suddently start executing scripts.
>+    // However, if someone was evil, and appended elements (using DOM2 methods)

What's wrong with DOM1 or DOM0 methods :) just say "DOM"
Comment 12 Blake Kaplan (:mrbkap) 2006-01-27 16:13:08 PST
Created attachment 209915 [details] [diff] [review]
Updated to comments
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-01-27 16:26:56 PST
Comment on attachment 209915 [details] [diff] [review]
Updated to comments

>Index: toolkit/components/viewsource/content/viewPartialSource.js
...
>+    var insertEndMarker = true;
>+    var tmpNode = ancestorContainer;
>+
>+    if (tmpNode instanceof HTMLScriptElement)
>+      tmpNode = tmpNode.parent;
>+
>+    while (tmpNode) {
>+      if (tmpNode.lastChild instanceof HTMLScriptElement) {
>+        var inner = tmpNode.innerHTML;
>+        if (inner != "<script>" && inner.charAt(inner.length - 1) != '>') {
>+          insertEndMarker = false;
>+        }
>+
>+        break;
>+      }
>+      tmpNode = tmpNode.lastChild;
>+    }


You can change this to 

    while (tmpNode) {
      if (tmpNode instanceof HTMLScriptElement && tmpNode.parentNode) {
        var inner = tmpNode.parentNode.innerHTML;
        if (inner != "<script>" && inner.charAt(inner.length - 1) != '>') {
          insertEndMarker = false;
        }

        break;
      }
      tmpNode = tmpNode.lastChild;
    }

To avoid the extra tmpNode = tmpNode.parentNode

r=me either way.
Comment 14 Blake Kaplan (:mrbkap) 2006-01-30 17:09:29 PST
Created attachment 210195 [details] [diff] [review]
With different roundtripping

We went back in time to my original serialization behavior. Ugh, here's hoping that this is the final patch.
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2006-01-30 18:10:45 PST
Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

sr=jst
Comment 16 Blake Kaplan (:mrbkap) 2006-01-31 14:29:40 PST
Fix checked into trunk.
Comment 17 Boris Zbarsky [:bz] (TPAC) 2006-02-01 15:27:58 PST
So with this patch we have IE compat and not Opera/Konqueror compat, right?  I think that's fine, but perhaps we should file equivalent bugs on them?  And get Hixie to spec this out in HTML5?
Comment 18 Anne (:annevk) 2006-02-03 15:17:36 PST
Filed one on Opera.
Comment 19 Jesse Ruderman 2006-02-19 00:54:19 PST
I filed bug 327796, "Unclosed scripts with src attribute should be allowed to run".  Anne and Hixie, since you're involved in speccing this and/or trying to ensure cross-browser consistency, you might be interested.
Comment 20 Daniel Veditz [:dveditz] 2006-06-09 13:12:20 PDT
Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

Is this one worth fixing for FF2? This particular patch changes the nsIScriptElement interface, though, so a 1.8 patch might not be worth the effort.
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-06-09 21:14:39 PDT
Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

I'm not sure if it's worth the effort, but if someone wants to do it please go ahead. a=me if so.
Comment 22 Jesse Ruderman 2006-12-02 03:17:13 PST
*** Bug 362397 has been marked as a duplicate of this bug. ***
Comment 24 Chris Travers 2008-05-18 08:34:29 PDT
You may find the following commentary interesting:

http://slashdot.org/~einhverfr/journal/203094

Maybe others can request that this change be made too.
Comment 25 Jesse Ruderman 2008-05-19 00:04:17 PDT
See bug 327796 and bug 408702.

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