305873 - Unclosed script data should not be parsed as HTML

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P4)

Description

[Jing Lim]

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]

Attachment: Demonstrate the SCRIPT parsing problem

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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 [:bzbarsky]]

Which IE did you test and in standards or quirks mode?  ;)

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This was IE6 on Windows. The behavior is the same in both standards and quirks mode.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Huh.  Do our cvs logs have any indication why we're doing things this way?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

Confirming based on bug 311395.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 208419 [details] [diff] [review]
Fix

this won't cancel enough serialization

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: "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]

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) (inactive)]

Attachment: Updated to comments

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) (inactive)]

Attachment: 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)]

Comment on attachment 210195 [details] [diff] [review]
With different roundtripping

sr=jst

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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)]

Filed one on Opera.

Comment 19

[Jesse Ruderman]

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]]

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]

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]

*** Bug 362397 has been marked as a duplicate of this bug. ***

Comment 23

[Eric Shepherd [:sheppy]]

http://developer.mozilla.org/en/docs/Updating_web_applications_for_Firefox_3#Change_to_the_SCRIPT_element

Now documented.

Comment 24

[Chris Travers]

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]

See bug 327796 and bug 408702.