Modernize handling of <!--...--> in CDATA

"Escaping text spans" were part of early drafts of the HTML5 spec but were removed in favor of special handling in CSS (see [CDO/CDC][1]) and [EcmaScript][2] grammar. This change * makes parsing consistent with recent versions of HTML 5 * updates rendering to be consistent with documented [script content restrictions][3] The script content restrictions are also applied to other CDATA tags like `<style>`. See also [issue OWASP#153][] [1]: https://www.w3.org/TR/CSS2/grammar.html#scanner [2]: http://www.ecma-international.org/ecma-262/6.0/#sec-html-like-comments [3]: https://www.w3.org/TR/html5/scripting-1.html#restrictions-for-contents-of-script-elements [issue OWASP#153]: OWASP#153

Author: mikesamuel

src/main/java/org/owasp/html/HtmlLexer.java

@@ -348,36 +348,6 @@ private static enum State {
  BOGUS_COMMENT,
  SERVER_CODE,
  SERVER_CODE_PCT,
-
- // From HTML 5 section 8.1.2.6
-
- // The text in CDATA and RCDATA elements must not contain any
- // occurrences of the string "</" followed by characters that
- // case-insensitively match the tag name of the element followed
- // by one of U+0009 CHARACTER TABULATION, U+000A LINE FEED (LF),
- // U+000B LINE TABULATION, U+000C FORM FEED (FF), U+0020 SPACE,
- // U+003E GREATER-THAN SIGN (>), or U+002F SOLIDUS (/), unless
- // that string is part of an escaping text span.
-
- // An escaping text span is a span of text (in CDATA and RCDATA
- // elements) and character entity references (in RCDATA elements)
- // that starts with an escaping text span start that is not itself
- // in an escaping text span, and ends at the next escaping text
- // span end.
-
- // An escaping text span start is a part of text that consists of
- // the four character sequence "<!--".
-
- // An escaping text span end is a part of text that consists of
- // the three character sequence "-->".
-
- // An escaping text span start may share its U+002D HYPHEN-MINUS characters
- // with its corresponding escaping text span end.
- UNESCAPED_LT_BANG, // <!
- UNESCAPED_LT_BANG_DASH, // <!-
- ESCAPING_TEXT_SPAN, // Inside an escaping text span
- ESCAPING_TEXT_SPAN_DASH, // Seen - inside an escaping text span
- ESCAPING_TEXT_SPAN_DASH_DASH, // Seen -- inside an escaping text span
  ;
  }
 
@@ -471,16 +441,6 @@ private HtmlToken parseToken() {
  case '!': // Comment or declaration
  if (!this.inEscapeExemptBlock) {
  state = State.BANG;
- } else if (HtmlTextEscapingMode.allowsEscapingTextSpan(
- escapeExemptTagName)) {
- // Directives, and cdata suppressed in escape
- // exempt mode as they could obscure the close of the
- // escape exempty block, but comments are similar to escaping
- // text spans, and are significant in all CDATA and RCDATA
- // blocks except those inside <xmp> tags.
- // See "Escaping text spans" in section 8.1.2.6 of HTML5.
- // http://www.w3.org/html/wg/html5/#cdata-rcdata-restrictions
- state = State.UNESCAPED_LT_BANG;
  }
  ++end;
  break;
@@ -603,47 +563,6 @@ && canonicalName(start + 2, end)
  state = State.SERVER_CODE;
  }
  break;
- case UNESCAPED_LT_BANG:
- if ('-' == ch) {
- state = State.UNESCAPED_LT_BANG_DASH;
- } else {
- type = HtmlTokenType.TEXT;
- state = State.DONE;
- }
- break;
- case UNESCAPED_LT_BANG_DASH:
- if ('-' == ch) {
- // According to HTML 5 section 8.1.2.6
-
- // An escaping text span start may share its
- // U+002D HYPHEN-MINUS characters with its
- // corresponding escaping text span end.
- state = State.ESCAPING_TEXT_SPAN_DASH_DASH;
- } else {
- type = HtmlTokenType.TEXT;
- state = State.DONE;
- }
- break;
- case ESCAPING_TEXT_SPAN:
- if ('-' == ch) {
- state = State.ESCAPING_TEXT_SPAN_DASH;
- }
- break;
- case ESCAPING_TEXT_SPAN_DASH:
- if ('-' == ch) {
- state = State.ESCAPING_TEXT_SPAN_DASH_DASH;
- } else {
- state = State.ESCAPING_TEXT_SPAN;
- }
- break;
- case ESCAPING_TEXT_SPAN_DASH_DASH:
- if ('>' == ch) {
- type = HtmlTokenType.TEXT;
- state = State.DONE;
- } else if ('-' != ch) {
- state = State.ESCAPING_TEXT_SPAN;
- }
- break;
  case DONE:
  throw new AssertionError(
  "Unexpectedly DONE while lexing HTML token stream");

src/main/java/org/owasp/html/HtmlStreamRenderer.java

@@ -288,67 +288,65 @@ private final void writeText(String text) throws IOException {
 
  private static int checkHtmlCdataCloseable(
  String localName, StringBuilder sb) {
- int escapingTextSpanStart = -1;
+ // www.w3.org/TR/html51/semantics-scripting.html#restrictions-for-contents-of-script-elements
+ // www.w3.org/TR/html5/scripting-1.html#restrictions-for-contents-of-script-elements
+ // 4.12.1.3. Restrictions for contents of script elements
+ // The textContent of a script element must match the script production in the following ABNF, the character set for which is Unicode. [ABNF]
+ //
+ // script = outer *( comment-open inner comment-close outer )
+ //
+ // outer = < any string that doesn’t contain a substring that matches not-in-outer >
+ // not-in-outer = comment-open
+ // inner = < any string that doesn’t contain a substring that matches not-in-inner >
+ // not-in-inner = comment-close / script-open
+ //
+ // comment-open = "<!--"
+ // comment-close = "-->"
+ // script-open = "<" s c r i p t tag-end
+
+ // We apply the above restrictions to all CDATA (modulo local name).
+ int innerStart = -1;
  for (int i = 0, n = sb.length(); i < n; ++i) {
  char ch = sb.charAt(i);
  switch (ch) {
  case '<':
- if (i + 3 < n
- && '!' == sb.charAt(i + 1)
- && '-' == sb.charAt(i + 2)
- && '-' == sb.charAt(i + 3)) {
- if (escapingTextSpanStart == -1) {
- escapingTextSpanStart = i;
- } else {
- return i;
+ if (i + 3 < n && sb.charAt(i + 1) == '!') {
+ if (sb.charAt(i + 2) == '-'
+ && sb.charAt(i + 3) == '-') {
+ if (innerStart >= 0) { return i; } // Nesting
+ innerStart = i;
  }
- } else if (i + 1 + localName.length() < n
- && '/' == sb.charAt(i + 1)
- && Strings.regionMatchesIgnoreCase(
- sb, i + 2, localName, 0, localName.length())) {
- // A close tag contained in the content.
- if (escapingTextSpanStart < 0) {
- // We could try some recovery strategies here.
- // E.g. prepending "/<!--\n" to sb if "script".equals(localName)
- return i;
+ } else { // Look for embedded <script or </script
+ int start = i + 1;
+ if (start + 1 < n && sb.charAt(start) == '/') {
+ ++start;
+ } else if (innerStart < 0) {
+ break;
  }
- if (!"script".equals(localName)) {
- // Script tags are commonly included inside script tags.
- // <script><!--document.write('<script>f()</script>');--></script>
- // but this does not happen in other CDATA element types.
- // Actually allowing an end tag inside others is problematic.
- // Specifically,
- // <style><!--</style>-->/* foo */</style>
- // displays the text "/* foo */" on some browsers.
+ // We don't need to do any suffix checks to preserve concatenation safety
+ // since we buffer pending unescaped above.
+ int end = start + localName.length();
+ if (end <= n
+ && Strings.regionMatchesIgnoreCase(
+ sb, start, localName, 0, end - start)
+ && (end == n || isTagEnd(sb.charAt(end)))) {
  return i;
  }
  }
  break;
  case '>':
- // From the HTML5 spec:
- // The text in style, script, title, and textarea elements must not
- // have an escaping text span start that is not followed by an
- // escaping text span end.
- // We look left since the HTML 5 spec allows the escaping text span
- // end to share dashes with the start.
- if (i >= 2 && '-' == sb.charAt(i - 1) && '-' == sb.charAt(i - 2)) {
- if (escapingTextSpanStart < 0) { return i - 2; }
- escapingTextSpanStart = -1;
+ if (i >= 2 && sb.charAt(i - 2) == '-' && sb.charAt(i - 2) == '-') {
+ if (innerStart < 0) { return i; }
+ // Merged start and end like <!--->
+ if (innerStart + 6 >= i) { return i; }
+ innerStart = -1;
  }
  break;
- default:
- break;
  }
  }
- if (escapingTextSpanStart >= 0) {
- // We could try recovery strategies here.
- // E.g. appending "//-->" to the buffer if "script".equals(localName)
- return escapingTextSpanStart;
- }
- return -1;
+ return innerStart;
  }
 
-
  @VisibleForTesting
  static boolean isValidHtmlName(String name) {
  int n = name.length();
@@ -421,4 +419,17 @@ public void close() throws IOException {
  closeable.close();
  }
  }
+
+ private static final long TAG_ENDS = 0L
+ | (1L << '\t')
+ | (1L << '\n')
+ | (1L << '\f')
+ | (1L << '\r')
+ | (1L << ' ')
+ | (1L << '/')
+ | (1L << '>');
+
+ private static boolean isTagEnd(char ch) {
+ return ch < 63 && 0 != (TAG_ENDS & (1L << ch));
+ }
 }

src/main/java/org/owasp/html/HtmlTextEscapingMode.java

@@ -143,18 +143,6 @@ public static HtmlTextEscapingMode getModeForTag(String canonTagName) {
  return mode != null ? mode : PCDATA;
  }
 
- /**
- * True iff the content following the given tag allows escaping text
- * spans: {@code <!--&hellip;-->} that escape even things that might
- * be an end tag for the corresponding open tag.
- */
- public static boolean allowsEscapingTextSpan(String canonTagName) {
- // <xmp> and <plaintext> do not admit escaping text spans.
- return "style".equals(canonTagName) || "script".equals(canonTagName)
- || "noembed".equals(canonTagName) || "noscript".equals(canonTagName)
- || "noframes".equals(canonTagName);
- }
-
  /**
  * True if content immediately following the start tag must be treated as
  * special CDATA so that &lt;'s are not treated as starting tags, comments

src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java

@@ -34,6 +34,7 @@
 import org.junit.Test;
 
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableSet;
 
 import junit.framework.TestCase;
 
@@ -175,6 +176,18 @@ public static final void testLinksWithNofollow() {
  .requireRelNofollowOnLinks()));
  }
 
+ @Test
+ public static final void testLinksWithNofollowAlreadyPresent() {
+ assertEquals(
+ "html <a href=\"/\" rel=\"nofollow\">link</a>",
+ apply(
+ new HtmlPolicyBuilder()
+ .allowElements("a")
+ .allowAttributes("href").onElements("a")
+ .requireRelNofollowOnLinks(),
+ "html <a href='/' rel='nofollow'>link</a>"));
+ }
+
  @Test
  public static final void testImagesAllowed() {
  assertEquals(
@@ -763,6 +776,47 @@ public static final void testDirLi() {
  "<dir compact=\"compact\"><li>something</li></dir>"));
  }
 
+ @Test
+ public static void testScriptTagWithCommentBlockContainingHtmlCommentEnd() {
+ PolicyFactory scriptSanitizer = new HtmlPolicyBuilder()
+ // allow scripts of type application/json
+ .allowElements(
+ new ElementPolicy() {
+ public String apply(String elementName, List<String> attrs) {
+ int typeIndex = attrs.indexOf("type");
+ if (typeIndex < 0 || attrs.size() < typeIndex + 1
+ || !attrs.get(typeIndex + 1).equals("application/json")) {
+ return null;
+ }
+ return elementName;
+ }
+ },
+ "script")
+ // allow contents in this script tag
+ .allowTextIn("script")
+ // keep type attribute in application/json script tag
+ .allowAttributes("type").matching(true, ImmutableSet.of("application/json")).onElements("script")
+ .toFactory();
+
+ String mismatchedHtmlComments = "<script type=\"application/json\">\n" +
+ "<!--\n" +
+ "{\"field\":\"-->\"}\n" +
+ "// -->\n" +
+ "</script>";
+ assertEquals(
+ "<script type=\"application/json\"></script>",
+ scriptSanitizer.sanitize(mismatchedHtmlComments));
+
+ String htmlMetaCharsEscaped = "<script type=\"application/json\">\n" +
+ "<!--\n" +
+ "{\"field\":\"--\\u003c\"}\n" +
+ "// -->\n" +
+ "</script>";
+ assertEquals(
+ htmlMetaCharsEscaped,
+ scriptSanitizer.sanitize(htmlMetaCharsEscaped));
+ }
+
  private static String apply(HtmlPolicyBuilder b) {
  return apply(b, EXAMPLE);
  }