Revision: 4504
Author: [email protected]
Date: Tue Jun 14 17:26:41 2011
Log: Adds a release valve for CDATA content checks.
http://codereview.appspot.com/4591043
This addresses the issue that some Caja HTML parser clients want to
render content without fixup including content with mismatched
escaping text spans.
The actual implementation of the fixup scheme is in the testcase but
if that looks good overall, it can probably move out of tests into
src.
[email protected]
http://code.google.com/p/google-caja/source/detail?r=4504
Added:
/trunk/src/com/google/caja/parser/html/CdataScriptContentFixer.java
/trunk/src/com/google/caja/parser/html/MarkupFixupRenderContext.java
/trunk/tests/com/google/caja/parser/html/CdataScriptContentFixerTest.java
Modified:
/trunk/src/com/google/caja/parser/html/Nodes.java
/trunk/tests/com/google/caja/parser/html/NodesTest.java
=======================================
--- /dev/null
+++ /trunk/src/com/google/caja/parser/html/CdataScriptContentFixer.java Tue
Jun 14 17:26:41 2011
@@ -0,0 +1,123 @@
+// Copyright (C) 2011 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.caja.parser.html;
+
+import com.google.caja.lexer.CharProducer;
+import com.google.caja.lexer.InputSource;
+import com.google.caja.lexer.JsLexer;
+import com.google.caja.lexer.JsTokenType;
+import com.google.caja.lexer.ParseException;
+import com.google.caja.lexer.Token;
+import com.google.caja.reporting.MarkupRenderMode;
+import com.google.caja.util.ContentType;
+import com.google.caja.util.Strings;
+
+import javax.annotation.Nullable;
+import javax.annotation.ParametersAreNonnullByDefault;
+
+import org.w3c.dom.Element;
+
+
+/**
+ * A strategy for semantics preserving changes to a block of JavaScript to
allow
+ * it to be rendered in {@link MarkupRenderMode#HTML HTML mode}.
+ *
+ * @author Mike Samuel <[email protected]>
+ */
+@ParametersAreNonnullByDefault
+public final class CdataScriptContentFixer {
+
+ /**
+ * HTML 5 requires that {@code <script>} elements' bodies have balanced
+ * escaping text spans. There is valid JavaScript content that does not.
+ * This strategy can fix some of these.
+ *
+ * @param el an element whose content contains invalid escaping text
spans.
+ * @param cdataContent the concatenation of el's text nodes' node values.
+ * @return {@code null} to indicate cannot fix or
+ * {@code textNode.getNodeValue()} transformed to fix the escaping
text
+ * spans.
+ * @see MarkupFixupRenderContext
+ */
+ public static @Nullable String fixUnclosableCdataElement(
+ Element el, String cdataContent) {
+ // Only muck with script elements.
+ // Assume HTML.
+ if (!Strings.equalsIgnoreCase("script", el.getLocalName())) {
+ return null;
+ }
+ // Only muck with Javascript script elements.
+ String type = el.getAttributeNS(Namespaces.HTML_NAMESPACE_URI, "type");
+ if ("".equals(type)) { type = el.getAttribute("type"); }
+ if (!"".equals(type)
+ && ContentType.JS != ContentType.fromMimeType(type)) {
+ return null;
+ }
+ // Lex the body, making three changes.
+ // (1) We insert spaces between tokens so that <!-- becomes
+ // < ! -- and --> becomes -- >.
+ // (2) We drop comments such as those in
+ // <script>...//--></script>
+ // We take care not to muck with comments that could include
+ // conditional compilation directives.
+ // (3) We rework string bodies so "</script>" becomes "<\/script>",
+ // "<!--" becomes "<\!--", and similarly for "-->".
+ // TODO: regular expression literal bodies.
+ StringBuilder sb = new StringBuilder(cdataContent.length() + 16);
+ JsLexer lexer = new JsLexer(CharProducer.Factory.fromString(
+ cdataContent, InputSource.UNKNOWN), false);
+ try {
+ int lastLine = 0;
+ while (lexer.hasNext()) {
+ Token<JsTokenType> tok = lexer.next();
+ String text = tok.text;
+ if (sb.length() != 0) {
+ sb.append(lastLine != tok.pos.startLineNo() ? '\n' : ' ');
+ }
+ lastLine = tok.pos.endLineNo();
+ if (tok.type == JsTokenType.COMMENT) {
+ if (tok.text.indexOf('@') < 0) {
+ // Skip comments that don't contain conditional compilation.
+ // The lastLine check above will turn multiline block comments
+ // into newlines.
+ continue;
+ }
+ } else if (tok.type == JsTokenType.STRING) {
+ int pos = 0;
+ int n = text.length();
+ char last = text.charAt(0);
+ for (int i = 1; i < n; ++i) {
+ char ch = text.charAt(i);
+ if ((last == '<' && (ch == '!' || ch == '/'))
+ || (last == '-' && ch == '>')) {
+ sb.append(text, 0, i).append('\\');
+ pos = i;
+ }
+ last = ch;
+ }
+ sb.append(text, pos, n);
+ continue;
+ }
+ sb.append(tok.text);
+ }
+ } catch (ParseException ex) {
+ return null; // Invalid JS
+ }
+ return sb.toString();
+ }
+
+ private CdataScriptContentFixer() { /* uninstantiable */ }
+
+}
=======================================
--- /dev/null
+++ /trunk/src/com/google/caja/parser/html/MarkupFixupRenderContext.java
Tue Jun 14 17:26:41 2011
@@ -0,0 +1,52 @@
+// Copyright (C) 2011 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.caja.parser.html;
+
+import javax.annotation.Nullable;
+import javax.annotation.ParametersAreNonnullByDefault;
+
+import org.w3c.dom.Element;
+
+/**
+ * An interface that can be implemented by a
+ * {@link com.google.caja.reporting.RenderContext render context} when the
+ * rendered text contains DOM nodes whose content cannot be easily
rendered in
+ * a particular render mode to coerce it to semantically equivalent and
+ * renderable content.
+ * <p>
+ * For example, the XML {@code <script>var x = 1</script>/;</script>}
+ * cannot be rendered as HTML without modifications because HTML CDATA
cannot
+ * contain entities like {@code <}.
+ * </p>
+ *
+ * @author Mike Samuel <[email protected]>
+ */
+@ParametersAreNonnullByDefault
+public interface MarkupFixupRenderContext {
+
+ /**
+ * May be overridden to fix invalid escaping text spans or an embedded
+ * end tag in a given element's content.
+ *
+ * @param el an element whose content contains invalid escaping text
spans.
+ * @param cdataContent the concatenation of el's text nodes' node values.
+ * @param problemIndex the index of the problem in cdataContent
+ * @return {@code null} to indicate cannot fix or
+ * {@code textNode.getNodeValue()} transformed to fix the issue.
+ */
+ @Nullable String fixUnclosableCdataElement(
+ Element el, String cdataContent, int problemIndex);
+
+}
=======================================
--- /dev/null
+++
/trunk/tests/com/google/caja/parser/html/CdataScriptContentFixerTest.java
Tue Jun 14 17:26:41 2011
@@ -0,0 +1,127 @@
+// Copyright (C) 2011 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.caja.parser.html;
+
+import javax.annotation.Nullable;
+
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+
+import com.google.caja.lexer.TokenConsumer;
+import com.google.caja.render.Concatenator;
+import com.google.caja.reporting.MarkupRenderMode;
+import com.google.caja.reporting.RenderContext;
+import com.google.caja.util.CajaTestCase;
+
+
+public class CdataScriptContentFixerTest extends CajaTestCase {
+
+ public final void testFixup() throws Exception {
+ assertFixedUp("<script>foo()</script>", "<script>foo()</script>");
+ assertFixedUp(
+ "<script type=\"text/javascript\">foo()</script>",
+ "<script type=text/javascript>foo()</script>");
+ // Don't touch non-JS script tags.
+ assertFixedUp(
+ null, "<script type=text/opensocial-template>i-->0</script>");
+ // Fix up tokens by spacing them out.
+ assertFixedUp(
+ "<script>foo -- > bar</script>", "<script>foo-->bar</script>");
+ // Leave newlines alone.
+ assertFixedUp(
+ "<script>return\ni -- > j</script>",
+ "<script>return\ni-->j</script>");
+ // Remove comments.
+ assertFixedUp(
+ "<script>foo ( ) </script>", "<script>foo()//--></script>");
+ assertFixedUp(
+ "<script>foo ( ) </script>", "<script>foo()/*-->*/</script>");
+ // Don't remove comments that might be conditional compilation
directives,
+ assertFixedUp(
+ "<script>/* @cc_on */\nfoo ( ) </script>",
+ "<script>/* @cc_on */\nfoo()//--></script>");
+ // even if that leads to breakage.
+ assertFixedUp(
+ null, "<script>foo()// @--></script>");
+ // Fix string literals that contain problematic content.
+ assertFixedUp(
+ "<script>[ '<\\/style' , '<\\!--' , '--\\>' , '--\\>' ]</script>",
+ "<script>['</style', '<!--', '-->', '-->']</script>");
+ assertFixedUp(
+ "<script>'<\\/script>\\n' </script>",
+ "<script>'<\\/script>\\n'//--></script>");
+
+ assertFixedUp(
+ ("<script type=\"text/javascript\">"
+ + "var arr = [ '&foo=' ] ;\n"
+ + "try { if ( typeof $x == \"object\" ) {"
+ + " arr . push ( $x . getBar ( ) . join ( \",\" ) )"
+ + " } } catch ( e ) { }\n"
+ + "</script>"),
+ ("<script type=\"text/javascript\">\n"
+ + " var arr = ['&foo='];\n"
+ + " try{if(typeof $x == \"object\"){"
+ + "arr.push($x.getBar().join(\",\"))"
+ + "}}catch(e){}\n"
+ + "// ]]> --></script>"));
+ }
+
+ private final void assertFixedUp(
+ @Nullable String goldenOrNullForFailure, String html) throws
Exception {
+ Node root = htmlFragment(fromString(html));
+ try {
+ // Check that it works with and without a render context flag set.
+ for (boolean asciiOnly : new boolean[] { true, false }) {
+ TokenConsumer tc = new Concatenator(new StringBuilder());
+ RenderContext rc = new CdataContentFixupRenderContext(tc)
+ .withAsciiOnly(asciiOnly);
+ Nodes.render(root, rc);
+ }
+ } catch (IllegalArgumentException ex) {
+ if (goldenOrNullForFailure != null) {
+ throw ex;
+ }
+ }
+ }
+
+ static final class CdataContentFixupRenderContext
+ extends RenderContext implements MarkupFixupRenderContext {
+ CdataContentFixupRenderContext(TokenConsumer out) {
+ super(out);
+ }
+
+ CdataContentFixupRenderContext(
+ boolean asciiOnly, boolean embeddable, boolean json,
+ MarkupRenderMode markupMode, boolean rawObjKeys, TokenConsumer
out) {
+ super(asciiOnly, embeddable, json, markupMode, rawObjKeys, out);
+ }
+
+
+ @Override
+ public String fixUnclosableCdataElement(
+ Element el, String cdataContent, int problemIndex) {
+ return CdataScriptContentFixer.fixUnclosableCdataElement(
+ el, cdataContent);
+ }
+
+ @Override
+ protected RenderContext derive(
+ boolean asciiOnly, boolean embeddable, boolean json,
+ MarkupRenderMode markupMode, boolean rawObjKeys) {
+ return new CdataContentFixupRenderContext(
+ asciiOnly, embeddable, json, markupMode, rawObjKeys, getOut());
+ }
+ }
+}
=======================================
--- /trunk/src/com/google/caja/parser/html/Nodes.java Thu May 26 14:16:16
2011
+++ /trunk/src/com/google/caja/parser/html/Nodes.java Tue Jun 14 17:26:41
2011
@@ -47,7 +47,7 @@
/**
* Utilities for dealing with HTML/XML DOM trees.
- *
+ *
* WARNING: The renderUnsafe methods in this class are unsafe for cajoled
* code because the Caja pipeline does not sanitize comments. In
particular
* IE comments rendered by renderUnsafe will be executable.
@@ -264,7 +264,7 @@
public static void render(Node node, Namespaces ns, RenderContext rc) {
render(node, ns, rc, false);
}
-
+
/**
* @deprecated For use only by non-caja clients of the parser/render
*/
@@ -291,9 +291,10 @@
RenderContext rc, boolean renderUnsafe) {
render(null, node, ns, rc, renderUnsafe);
}
-
- private static void render(DocumentType docType, Node node, Namespaces
ns,
- RenderContext rc, boolean renderUnsafe) {
+
+ private static void render(
+ DocumentType docType, Node node, Namespaces ns, RenderContext rc,
+ boolean renderUnsafe) {
StringBuilder sb = new StringBuilder(1 << 18);
if (null != docType) {
String rendering = renderDocumentType(docType);
@@ -301,7 +302,7 @@
sb.append(rendering);
}
}
- new Renderer(sb, rc.markupRenderMode(), rc.isAsciiOnly(), ns)
+ new Renderer(rc, sb, rc.markupRenderMode(), rc.isAsciiOnly(), ns)
.render(node, ns, renderUnsafe);
TokenConsumer out = rc.getOut();
FilePosition pos = getFilePositionFor(node);
@@ -439,7 +440,7 @@
MarkupRenderMode renderMode) {
return render(docType, node, renderMode, true);
}
-
+
private static String render(DocumentType docType, Node node,
MarkupRenderMode renderMode, boolean renderUnsafe) {
StringBuilder sb = new StringBuilder();
@@ -457,6 +458,7 @@
}
final class Renderer {
+ final RenderContext rc;
final StringBuilder out;
final MarkupRenderMode mode;
final boolean asXml;
@@ -464,8 +466,9 @@
final int namespaceDepthAtStart;
Renderer(
- StringBuilder out, MarkupRenderMode mode, boolean isAsciiOnly,
- Namespaces ns) {
+ RenderContext rc, StringBuilder out, MarkupRenderMode mode,
+ boolean isAsciiOnly, Namespaces ns) {
+ this.rc = rc;
this.out = out;
this.mode = mode;
this.asXml = mode == MarkupRenderMode.XML;
@@ -482,11 +485,11 @@
void renderUnsafe(Node node, Namespaces ns) {
render(node, ns, true);
}
-
+
void render(Node node, Namespaces ns) {
render(node, ns, false);
}
-
+
void render(Node node, Namespaces ns, boolean renderUnsafe) {
switch (node.getNodeType()) {
case Node.DOCUMENT_NODE: case Node.DOCUMENT_FRAGMENT_NODE:
@@ -581,7 +584,7 @@
}
attrLocalName = emitLocalName(attrLocalName, isHtml);
// http://www.w3.org/TR/html401/intro/sgmltut.html
- // #didx-boolean_attribute
+ // #didx-boolean_attribute
// Authors should be aware that many user agents only recognize
the
// minimized form of boolean attributes and not the full form.
if (!(isHtml && mode == MarkupRenderMode.HTML4_BACKWARDS_COMPAT
@@ -617,15 +620,40 @@
}
// Make sure that the CDATA section does not contain a close
// tag or unbalanced <!-- ... -->.
+ // If it does, try a fallback strategy.
int problemIndex = checkHtmlCdataCloseable(
localName, cdataContent);
if (problemIndex != -1) {
- throw new IllegalArgumentException(
- "XML document not renderable as HTML due to '"
- + cdataContent.subSequence(
- problemIndex,
- Math.min(cdataContent.length(), problemIndex + 10))
- + "' in CDATA element");
+ if (rc instanceof MarkupFixupRenderContext) {
+ MarkupFixupRenderContext markupFixup
+ = (MarkupFixupRenderContext) rc;
+ String cdataContentString = cdataContent.toString();
+ do {
+ @Nullable String fixedCdataContentString
+ = markupFixup.fixUnclosableCdataElement(
+ el, cdataContentString, problemIndex);
+ if (fixedCdataContentString == null
+ // Avoid most common source of inf. looping.
+ ||
fixedCdataContentString.equals(cdataContentString)) {
+ break;
+ }
+ cdataContentString = fixedCdataContentString;
+ // The fixed content needs to be put back in
cdataContent.
+ cdataContent.setLength(0);
+ cdataContent.append(cdataContentString);
+ problemIndex = checkHtmlCdataCloseable(
+ localName, cdataContent);
+ } while (problemIndex != -1);
+ // If problemIndex is -1 then we have fixed the problem.
+ }
+ if (problemIndex != -1) {
+ throw new IllegalArgumentException(
+ "XML document not renderable as HTML due to '"
+ + cdataContent.subSequence(
+ problemIndex,
+ Math.min(cdataContent.length(), problemIndex +
10))
+ + "' in CDATA element");
+ }
}
out.append(cdataContent);
} else {
@@ -712,7 +740,7 @@
// single U+003E GREATER-THAN SIGN character (>), nor start with
a
// U+002D HYPHEN-MINUS character (-) followed by a U+003E
// GREATER-THAN SIGN (>) character, nor contain two consecutive
- // U+002D HYPHEN-MINUS characters (--), nor end with a U+002D
+ // U+002D HYPHEN-MINUS characters (--), nor end with a U+002D
// HYPHEN-MINUS character (-). Finally, the comment must be ended
// by the three character sequence (-->).
=======================================
--- /trunk/tests/com/google/caja/parser/html/NodesTest.java Thu May 26
14:16:16 2011
+++ /trunk/tests/com/google/caja/parser/html/NodesTest.java Tue Jun 14
17:26:41 2011
@@ -369,14 +369,14 @@
+ "\"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd\";>",
"<!DOCTYPE html>"
};
-
+
boolean[] asAsciiModes = { true, false };
boolean[] asEmbeddableModes = { true, false };
-
+
List<Pair<MarkupRenderMode, String>> expectedPairs = List.of(
- Pair.pair(MarkupRenderMode.HTML,
+ Pair.pair(MarkupRenderMode.HTML,
"<html><head></head><body><b>my text</b></body></html>"),
- Pair.pair(MarkupRenderMode.XML,
+ Pair.pair(MarkupRenderMode.XML,
"<html><head /><body><b>my text</b></body></html>")
);
for (String docType : docTypes) {
@@ -387,7 +387,7 @@
DoctypeMaker.parse(docType), null);
Element el = html(fromString("<html><b>my text</b></html>"));
doc.appendChild(doc.adoptNode(el));
-
+
StringBuilder sb = new StringBuilder();
RenderContext rc = new RenderContext(new Concatenator(sb))
.withAsciiOnly(asAscii)
@@ -397,13 +397,13 @@
rc.getOut().noMoreTokens();
String actual = sb.toString();
MoreAsserts.assertStartsWith(docType, actual);
- assertTrue(actual, actual.contains(expectedPair.b));
+ assertTrue(actual, actual.contains(expectedPair.b));
}
}
}
}
}
-
+
public final void testBadDocumentType() throws ParseException {
// bad system id
assertDocType(
@@ -497,6 +497,7 @@
checkEscapingTextSpans("script", "a <!--b && c--> d", true);
checkEscapingTextSpans(
"script", "<!--document.write('<script>f()</script>')-->", true);
+ checkEscapingTextSpans("script", "a <!--b && c--> d", true);
}
public final void testEscapingTextSpansTitle() {
