[google-caja] r4887 committed - Fix handling of 'target' attributes...

[googl...@googlecode.com]

Revision: 4887
Author: ihab...@gmail.com
Date: Wed May 23 19:08:31 2012
Log: Fix handling of 'target' attributes
http://codereview.appspot.com/6240043

Fixes http://code.google.com/p/google-caja/issues/detail?id=1471

This change required passing an out-of-band "this attribute was
unspecified by the guest programmer" signal from TemplateCompiler to
HtmlAttributeRewriter. See changes for details of how this is done.

R=fel...@gmail.com

http://code.google.com/p/google-caja/source/detail?r=4887

Modified:
/trunk/src/com/google/caja/plugin/domado.js
/trunk/src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java
/trunk/src/com/google/caja/plugin/templates/TemplateCompiler.java
/trunk/tests/com/google/caja/plugin/HtmlCompiledPluginTest.java
/trunk/tests/com/google/caja/plugin/es53-test-target-attribute-presets.js
/trunk/tests/com/google/caja/plugin/templates/TemplateCompilerTest.java

/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-dynamic.js

/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-nulluripol.js

/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-static.js
/trunk/tests/com/google/caja/service/HtmlHandlerTest.java

=======================================
--- /trunk/src/com/google/caja/plugin/domado.js Thu May 17 13:48:02 2012
+++ /trunk/src/com/google/caja/plugin/domado.js Wed May 23 19:08:31 2012
@@ -1741,11 +1741,13 @@
return css.join(' ; ');
// Frames are ambient, so disallow reference.
case html4.atype.FRAME_TARGET:
- value = String(value);
- for (var i = 0; i < optTargetAttributePresets.whitelist.length;
- ++i) {
- if (optTargetAttributePresets.whitelist[i] === value) {
- return value;
+ if (value !== null) {
+ value = String(value);
+ for (var i = 0; i <
optTargetAttributePresets.whitelist.length;
+ ++i) {
+ if (optTargetAttributePresets.whitelist[i] === value) {
+ return value;
+ }
}
}
return optTargetAttributePresets.default;
=======================================
--- /trunk/src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java
Fri May 11 13:36:49 2012
+++ /trunk/src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java
Wed May 23 19:08:31 2012
@@ -36,6 +36,7 @@
import com.google.caja.parser.js.Expression;
import com.google.caja.parser.js.FunctionConstructor;
import com.google.caja.parser.js.Identifier;
+import com.google.caja.parser.js.NullLiteral;
import com.google.caja.parser.js.Operation;
import com.google.caja.parser.js.Reference;
import com.google.caja.parser.js.Statement;
@@ -58,6 +59,7 @@
import com.google.caja.util.Lists;
import com.google.caja.util.Maps;
import com.google.caja.util.SyntheticAttributeKey;
+import org.w3c.dom.Attr;

import java.net.URI;
import java.net.URISyntaxException;
@@ -67,8 +69,6 @@
import java.util.Map;
import java.util.regex.Pattern;

-import org.w3c.dom.Attr;
-
/**
* Converts attribute values to expressions that produce safe values.
*
@@ -124,6 +124,8 @@

public static AttrValue fromAttr(
final Attr a, HTML.Attribute attr, JobEnvelope source) {
+ FilePosition pos = a.getValue() != null ?
+ Nodes.getFilePositionForValue(a) : FilePosition.UNKNOWN;
return new AttrValue(source, a, Nodes.getFilePositionForValue(a),
attr) {
@Override
Expression getValueExpr() {
@@ -351,28 +353,21 @@
}

Expression sanitizeFrameTargetValue(AttrValue attr) {
- // Add a known-safe static value to the parent element
- HTML.Attribute info = attr.attrInfo;
- String provisionalSafeValue =
- (info.getDefaultValue() != null
- && info.getValueCriterion().accept(info.getDefaultValue()))
- ? info.getDefaultValue()
- : info.getSafeValue();
-
- // The candidate value to pass to the dynamic client-side policy is
- // either the user-supplied value (if it was provided) or the "safe"
value
- // we have provisionally added.
- String candidateValue = (attr.src == null)
- ? provisionalSafeValue : attr.getPlainValue();
-
- // Return a script that computes a whitelisted dynamic value
+ // If the guest code supplied an attribute value for 'target', we get
it
+ // in 'attr.src'. Otherwise, TemplateCompiler gives us an 'attr.src'
with
+ // a value equal to the empty string, which Domado's
rewriteTargetAttribute
+ // interprets to mean that the guest code did not supply a value.
FilePosition pos = attr.valuePos;
+ boolean unspecified = null !=
+
attr.src.getUserData(TemplateCompiler.ATTRIBUTE_VALUE_WAS_UNSPECIFIED);
+ Expression value = unspecified
+ ? new NullLiteral(pos)
+ : new StringLiteral(pos, attr.src.getValue());
return (Expression) QuasiBuilder.substV(""
+ "IMPORTS___./*@synthetic*/rewriteTargetAttribute___("
+ " @value, @tagName, @attribName)",
- "value", new StringLiteral(pos, candidateValue),
- "tagName", new StringLiteral(
- pos, attr.src.getOwnerElement().getTagName()),
+ "value", value,
+ "tagName", new StringLiteral(pos,
attr.src.getOwnerElement().getTagName()),
"attribName", new StringLiteral(pos,
attr.attrInfo.getKey().localName));
}

=======================================
--- /trunk/src/com/google/caja/plugin/templates/TemplateCompiler.java Wed
May 9 16:01:22 2012
+++ /trunk/src/com/google/caja/plugin/templates/TemplateCompiler.java Wed
May 23 19:08:31 2012
@@ -67,6 +67,9 @@
private final MessageQueue mq;
private final HtmlAttributeRewriter aRewriter;

+ public static final String ATTRIBUTE_VALUE_WAS_UNSPECIFIED =
+ "ATTRIBUTE_VALUE_WAS_UNSPECIFIED";
+
/**
* Maps {@link Node}s to JS parse trees.
*
@@ -209,13 +212,10 @@
Attr unsafe = el.getAttributeNodeNS(aUri, aName);
if (a.getType() == HTML.Attribute.Type.FRAME_TARGET) {
if (unsafe == null) {
- String safeValue =
- (a.getDefaultValue() != null
- && a.getValueCriterion().accept(a.getDefaultValue()))
- ? a.getDefaultValue() : a.getSafeValue();
attr = el.getOwnerDocument().createAttributeNS(
attrKey.ns.uri, attrKey.localName);
- attr.setNodeValue(safeValue);
+ attr.setNodeValue("");
+ attr.setUserData(ATTRIBUTE_VALUE_WAS_UNSPECIFIED, true, null);
el.setAttributeNode(attr);
} else {
// Leave it for later stages to deal with
@@ -298,7 +298,8 @@
*/
private void inspectHtmlAttribute(
JobEnvelope source, Attr attr, HTML.Attribute info) {
- if (Placeholder.ID_ATTR.is(attr)
+ if (attr != null
+ && Placeholder.ID_ATTR.is(attr)
&& scriptsPerPlaceholder.containsKey(attr.getValue())) {
scriptsPerNode.put(attr, null);
} else {
=======================================
--- /trunk/tests/com/google/caja/plugin/HtmlCompiledPluginTest.java Wed
May 9 16:01:22 2012
+++ /trunk/tests/com/google/caja/plugin/HtmlCompiledPluginTest.java Wed May
23 19:08:31 2012
@@ -112,7 +112,7 @@

// Handler is attached separately.
""
- + "assertEquals('<a target=\"rewritten-_blank\">hi</a>',"
+ + "assertEquals('<a target=\"rewritten-null\">hi</a>',"
+ " document.getElementById('test-test').innerHTML);");
}

=======================================
---
/trunk/tests/com/google/caja/plugin/es53-test-target-attribute-presets.js
Wed May 9 16:01:22 2012
+++
/trunk/tests/com/google/caja/plugin/es53-test-target-attribute-presets.js
Wed May 23 19:08:31 2012
@@ -35,7 +35,7 @@
forceES5Mode: inES5Mode,
targetAttributePresets: {
default: 'foo',
- whitelist: [ 'foo', 'bar' ]
+ whitelist: [ 'foo', 'bar', '_blank' ]
}
});

@@ -72,6 +72,16 @@
'<form action="http://example.com/"></form>',
'target="foo"');

+ registerTargetTest(
+ 'testFormTargetNoValue',
+ '<form target action="http://example.com/"></form>',
+ 'target="foo"');
+
+ registerTargetTest(
+ 'testFormTargetEmptyValue',
+ '<form action="http://example.com/" target=></form>',
+ 'target="foo"');
+
registerTargetTest(
'testAnchorTargetNone',
'<a>a</a>',
=======================================
--- /trunk/tests/com/google/caja/plugin/templates/TemplateCompilerTest.java
Fri May 11 13:36:49 2012
+++ /trunk/tests/com/google/caja/plugin/templates/TemplateCompilerTest.java
Wed May 23 19:08:31 2012
@@ -192,7 +192,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'form', 'target'));"
+ + " null, 'form', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -216,7 +216,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'a', 'target'));"
+ + " null, 'a', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -239,7 +239,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'a', 'target'));"
+ + " null, 'a', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -275,7 +275,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'form', 'target'));"
+ + " null, 'form', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -312,7 +312,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'form', 'target'));"
+ + " null, 'form', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -347,7 +347,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'a', 'target'));"
+ + " null, 'a', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -383,7 +383,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'a', 'target'));"
+ + " null, 'a', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -409,7 +409,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'form', 'target'));"
+ + " null, 'form', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -612,7 +612,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'a', 'target'));"
+ + " null, 'a', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -884,7 +884,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'area', 'target'));"
+ + " null, 'area', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -918,7 +918,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'area', 'target'));"
+ + " null, 'area', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
@@ -1052,7 +1052,7 @@
+ " el___,"
+ " 'target',"
+ " IMPORTS___.rewriteTargetAttribute___("
- + " '_blank', 'a', 'target'));"
+ + " null, 'a', 'target'));"
+ " emitter___.rmAttr(el___, 'id');"
+ " el___ = emitter___.finish();"
+ " emitter___.signalLoaded();"
=======================================
---
/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-dynamic.js
Wed May 9 16:01:22 2012
+++
/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-dynamic.js
Wed May 23 19:08:31 2012
@@ -66,7 +66,7 @@
emitter___.setAttr(
el___,
'target',
- IMPORTS___.rewriteTargetAttribute___('_blank', 'a', 'target'));
+ IMPORTS___.rewriteTargetAttribute___(null, 'a', 'target'));
emitter___.rmAttr(el___, 'id');
el___ = emitter___.byId('id_6___');
emitter___.setAttr(el___, 'id', 'zag-' + IMPORTS___.getIdClass___());
=======================================
---
/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-nulluripol.js
Wed May 9 16:01:22 2012
+++
/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-nulluripol.js
Wed May 23 19:08:31 2012
@@ -68,7 +68,7 @@
emitter___.setAttr(
el___,
'target',
- IMPORTS___.rewriteTargetAttribute___('_blank', 'a', 'target'));
+ IMPORTS___.rewriteTargetAttribute___(null, 'a', 'target'));
emitter___.rmAttr(el___, 'id');
el___ = emitter___.byId('id_6___');
emitter___.setAttr(el___, 'style', 'color: red; background-image:
url(' +
=======================================
---
/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-static.js
Wed May 9 16:01:22 2012
+++
/trunk/tests/com/google/caja/plugin/templates/template-compiler-golden1-static.js
Wed May 23 19:08:31 2012
@@ -63,7 +63,7 @@
emitter___.setAttr(
el___,
'target',
- IMPORTS___.rewriteTargetAttribute___('_blank', 'a', 'target'));
+ IMPORTS___.rewriteTargetAttribute___(null, 'a', 'target'));
emitter___.rmAttr(el___, 'id');
el___ = emitter___.finish();
emitter___.signalLoaded();
=======================================
--- /trunk/tests/com/google/caja/service/HtmlHandlerTest.java Wed May 9
16:01:22 2012
+++ /trunk/tests/com/google/caja/service/HtmlHandlerTest.java Wed May 23
19:08:31 2012
@@ -111,7 +111,7 @@
+ ")");
assertContainsIgnoreSpace(
(String) json.get("js"),
- "IMPORTS___.rewriteTargetAttribute___('_blank','a','target')");
+ "IMPORTS___.rewriteTargetAttribute___(null,'a','target')");
}

public final void testTargetAttribs() throws Exception {
@@ -136,7 +136,7 @@
"<a id=\"id_1___\" target=\"_blank\">Default</a>");
assertContainsIgnoreSpace(
(String) json.get("js"),
- "IMPORTS___.rewriteTargetAttribute___('_blank','a','target')");
+ "IMPORTS___.rewriteTargetAttribute___(null,'a','target')");
assertContainsIgnoreSpace(
(String) json.get("html"),
"<a id=\"id_2___\" target=\"_blank\">Self</a>");