Strip downlevel IE conditional comments (#19)

The sanitizer walked only element nodes, so a script hidden in a
downlevel IE conditional comment slipped through. golang.org/x/net/html
parses <!--[if lt IE 9]><script src="..."></script><![endif]--> as a
single comment node whose data holds the raw markup, so the element walk
never sees the <script> and it rendered straight back out, a live-CDN
script reference left sitting in a page kage promises is inert.

Plenty of older docs sites (clojure.org, cordova.apache.org, and the
async library docs among them) still ship html5shiv, respond.js, or
placeholders.js this way.

Drop conditional comments in the walk. The downlevel-hidden form is one
comment and goes whole; the downlevel-revealed form keeps its content,
which lives in sibling nodes, and loses only the two markers.

sanitize/sanitize.go

@@ -2,9 +2,11 @@
 // the saved page is inert: a photograph, not a program.
 //
 // It parses with golang.org/x/net/html, walks the tree, and deletes scripts,
-// event handlers, javascript: URLs, and the dead preconnect/preload hints that
-// mean nothing offline — while leaving styles, images, fonts, forms, and all
-// semantic markup untouched so the layout survives intact.
+// event handlers, javascript: URLs, downlevel IE conditional comments (which
+// can smuggle a <script> past an element-only walk), and the dead
+// preconnect/preload hints that mean nothing offline — while leaving styles,
+// images, fonts, forms, and all semantic markup untouched so the layout
+// survives intact.
 package sanitize
 
 import (
@@ -31,13 +33,14 @@ type Options struct {
 
 // Report counts what was removed, for the run summary and for tests.
 type Report struct {
-	ScriptsRemoved     int
-	HandlersRemoved    int
-	NoscriptRemoved    int
-	NoscriptUnwrapped  int
-	JSURLsNeutralized  int
-	MetaRefreshRemoved int
-	DeadLinksRemoved   int
+	ScriptsRemoved      int
+	HandlersRemoved     int
+	NoscriptRemoved     int
+	NoscriptUnwrapped   int
+	JSURLsNeutralized   int
+	MetaRefreshRemoved  int
+	DeadLinksRemoved    int
+	CondCommentsRemoved int
 }
 
 // jsURLAttrs are attributes whose value may be a javascript: URL.
@@ -78,6 +81,18 @@ func clean(n *html.Node, opts Options, rep *Report) {
 	var next *html.Node
 	for c := n.FirstChild; c != nil; c = next {
 		next = c.NextSibling
+		if c.Type == html.CommentNode {
+			// A downlevel IE conditional comment (<!--[if lt IE 9]>...<![endif]-->)
+			// parses as one comment whose data holds raw markup — a <script src>
+			// among it. The element walk never sees that script, so drop the whole
+			// comment. Downlevel-revealed content lives in sibling nodes, not the
+			// comment data, so it is untouched.
+			if isConditionalComment(c.Data) {
+				n.RemoveChild(c)
+				rep.CondCommentsRemoved++
+			}
+			continue
+		}
 		if c.Type == html.ElementNode {
 			switch c.DataAtom {
 			case atom.Script:
@@ -175,6 +190,19 @@ func isDeadLink(n *html.Node) bool {
 	return false
 }
 
+// isConditionalComment reports whether a comment's data is a downlevel IE
+// conditional-comment marker. Both the downlevel-hidden form (the whole
+// "[if lt IE 9]>...<![endif]" in one comment) and the two markers of the
+// downlevel-revealed form ("[if gte IE 9]><!" and "<![endif]") match, so the
+// markers are stripped while any revealed content, which sits in sibling
+// nodes, stays.
+func isConditionalComment(data string) bool {
+	d := strings.TrimSpace(data)
+	return strings.HasPrefix(d, "[if") ||
+		strings.HasPrefix(d, "<![endif]") ||
+		strings.HasPrefix(d, "[endif]")
+}
+
 // unwrapNoscript replaces a <noscript> with its content. Because x/net/html
 // parses noscript content as raw text (scripting enabled), the text is
 // re-parsed as a fragment in the parent's context and spliced in before the

sanitize/sanitize_test.go

@@ -107,6 +107,48 @@ func TestKeepNoscriptUnwraps(t *testing.T) {
 	}
 }
 
+func TestConditionalCommentScriptRemoved(t *testing.T) {
+	// A downlevel-hidden IE conditional comment hides a <script src> inside a
+	// single comment node, where an element-only walk never reaches it.
+	in := `<html><head>
+<!--[if lt IE 9]><script src="//oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script><![endif]-->
+</head><body><p>real</p></body></html>`
+	out, rep, err := Strip([]byte(in), Options{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	s := string(out)
+	if strings.Contains(s, "<script") || strings.Contains(s, "html5shiv") {
+		t.Errorf("conditional-comment script survived:\n%s", s)
+	}
+	if strings.Contains(s, "[if lt IE 9]") {
+		t.Errorf("conditional comment survived:\n%s", s)
+	}
+	if rep.CondCommentsRemoved != 1 {
+		t.Errorf("CondCommentsRemoved = %d, want 1", rep.CondCommentsRemoved)
+	}
+	if !strings.Contains(s, "<p>real</p>") {
+		t.Errorf("real content must survive:\n%s", s)
+	}
+}
+
+func TestConditionalCommentRevealedContentKept(t *testing.T) {
+	// The downlevel-revealed form shows its content to non-IE browsers; the
+	// content lives in sibling nodes, so only the two markers are stripped.
+	in := `<html><body><!--[if gte IE 9]><!--><span class="modern">keep me</span><!--<![endif]--></body></html>`
+	out, _, err := Strip([]byte(in), Options{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	s := string(out)
+	if !strings.Contains(s, `<span class="modern">keep me</span>`) {
+		t.Errorf("revealed content was dropped:\n%s", s)
+	}
+	if strings.Contains(s, "[if") || strings.Contains(s, "<![endif]") {
+		t.Errorf("conditional markers survived:\n%s", s)
+	}
+}
+
 func TestKeepMetaRefreshPlain(t *testing.T) {
 	in := `<html><head><meta http-equiv="refresh" content="5;url=/next"></head><body></body></html>`
 	out, _, err := Strip([]byte(in), Options{KeepMetaRefresh: true})