[Caja] [google-caja] r3842 committed - Fix rendering of Conditionals...

google-caja Thu, 12 Nov 2009 13:05:58 -0800

Revision: 3842
Author: [email protected]
Date: Thu Nov 12 13:05:03 2009
Log: Fix rendering of Conditionals
http://codereview.appspot.com/154078


It is possible to programatically construct a parse tree containing
conditionals where the rendered form attaches an else to the wrong
conditional.

For e.g., it is possible to programatically move
  if (foo) bar();
into the body of
  if (baz) @body else boo();
which should result in the structure
  if (baz) { if (foo) bar(); } else boo();
but which naively renders to
  if (baz) if (foo) bar(); else boo();
which is equivalent to
  if (baz) { if (foo) bar(); else boo(); }

Our code does not do this right now -- we are liberal with blocks --
so this does not constitute an attack vector.

This adds a method to statement, hasHangingConditional, and changes
the Conditional.render to add curly braces when the structure would
require them to resolve ambiguity.

Please look especially at the definition and use of
hasHangingConditional in Conditional.java and the tests in ParserTest.java

[email protected]

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

Modified:
 /trunk/src/com/google/caja/parser/js/Block.java
 /trunk/src/com/google/caja/parser/js/BreakStmt.java
 /trunk/src/com/google/caja/parser/js/CatchStmt.java
 /trunk/src/com/google/caja/parser/js/Conditional.java
 /trunk/src/com/google/caja/parser/js/ContinueStmt.java
 /trunk/src/com/google/caja/parser/js/DebuggerStmt.java
 /trunk/src/com/google/caja/parser/js/Declaration.java
 /trunk/src/com/google/caja/parser/js/DoWhileLoop.java
 /trunk/src/com/google/caja/parser/js/ExpressionStmt.java
 /trunk/src/com/google/caja/parser/js/FinallyStmt.java
 /trunk/src/com/google/caja/parser/js/ForEachLoop.java
 /trunk/src/com/google/caja/parser/js/ForLoop.java
 /trunk/src/com/google/caja/parser/js/LabeledStmtWrapper.java
 /trunk/src/com/google/caja/parser/js/MultiDeclaration.java
 /trunk/src/com/google/caja/parser/js/Noop.java
 /trunk/src/com/google/caja/parser/js/ReturnStmt.java
 /trunk/src/com/google/caja/parser/js/Statement.java
 /trunk/src/com/google/caja/parser/js/SwitchCase.java
 /trunk/src/com/google/caja/parser/js/SwitchStmt.java
 /trunk/src/com/google/caja/parser/js/ThrowStmt.java
 /trunk/src/com/google/caja/parser/js/TranslatedCode.java
 /trunk/src/com/google/caja/parser/js/TryStmt.java
 /trunk/src/com/google/caja/parser/js/UseSubsetDirective.java
 /trunk/src/com/google/caja/parser/js/WhileLoop.java
 /trunk/src/com/google/caja/parser/js/WithStmt.java
 /trunk/tests/com/google/caja/parser/js/ParserTest.java

=======================================
--- /trunk/src/com/google/caja/parser/js/Block.java     Mon Nov  2 13:56:19 2009
+++ /trunk/src/com/google/caja/parser/js/Block.java     Thu Nov 12 13:05:03 2009
@@ -95,4 +95,6 @@
   public boolean isTerminal() {
     return true;
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/BreakStmt.java Wed Jul 15 11:24:342009+++ /trunk/src/com/google/caja/parser/js/BreakStmt.java Thu Nov 12 13:05:032009

@@ -64,4 +64,6 @@
       out.consume(label);
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/CatchStmt.java Mon Nov 2 13:56:192009+++ /trunk/src/com/google/caja/parser/js/CatchStmt.java Thu Nov 12 13:05:032009

@@ -69,4 +69,6 @@
     out.consume(")");
     body.renderBlock(rc, false);
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/Conditional.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/Conditional.java Thu Nov 1213:05:03 2009

@@ -77,7 +77,10 @@
       out.consume("(");
       condition.render(rc);
       out.consume(")");
+      boolean hanging = body.hasHangingConditional();
+      if (hanging) { out.consume("{"); }
       body.renderBlock(rc, i + 2 < n);
+      if (hanging) { out.consume("}"); }
     }
     if (i < n) {
       Statement body = (Statement) children.get(i);
@@ -85,4 +88,15 @@
       body.renderBlock(rc, false);
     }
   }
-}
+
+  public boolean hasHangingConditional() {
+    List<? extends ParseTreeNode> children = children();
+    int n = children.size();
+    // If there is no else clause, then an else following would change the
+    // meaning.
+    if ((n & 1) == 0) { return true; }
+    // Otherwise if the else clause has a hanging conditional, then an else
+    // would change the meaning.
+    return ((Statement) children.get(n - 1)).hasHangingConditional();
+  }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/ContinueStmt.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/ContinueStmt.java Thu Nov 1213:05:03 2009

@@ -63,4 +63,6 @@
       out.consume(label);
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/DebuggerStmt.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/DebuggerStmt.java Thu Nov 1213:05:03 2009

@@ -49,4 +49,6 @@
     out.mark(getFilePosition());
     out.consume("debugger");
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/Declaration.java Thu Oct 2214:07:39 2009+++ /trunk/src/com/google/caja/parser/js/Declaration.java Thu Nov 1213:05:03 2009

@@ -102,4 +102,6 @@
       if (isComma) { out.consume(")"); }
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/DoWhileLoop.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/DoWhileLoop.java Thu Nov 1213:05:03 2009

@@ -68,4 +68,6 @@
     condition.render(rc);
     out.consume(")");
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/ExpressionStmt.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/ExpressionStmt.java Thu Nov 1213:05:03 2009

@@ -84,4 +84,6 @@
       expr.render(rc);
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/FinallyStmt.java Mon Nov 213:56:19 2009+++ /trunk/src/com/google/caja/parser/js/FinallyStmt.java Thu Nov 1213:05:03 2009

@@ -55,4 +55,6 @@
     rc.getOut().consume("finally");
     body.renderBlock(rc, false);
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/ForEachLoop.java Thu Oct 1515:42:13 2009+++ /trunk/src/com/google/caja/parser/js/ForEachLoop.java Thu Nov 1213:05:03 2009

@@ -102,4 +102,8 @@
     out.consume(")");
     body.renderBlock(rc, false);
   }
-}
+
+  public boolean hasHangingConditional() {
+    return body.hasHangingConditional();
+  }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/ForLoop.java Wed Jul 15 11:24:342009+++ /trunk/src/com/google/caja/parser/js/ForLoop.java Thu Nov 12 13:05:032009

@@ -94,4 +94,8 @@
     out.consume(")");
     getBody().renderBlock(rc, false);
   }
-}
+
+  public boolean hasHangingConditional() {
+    return body.hasHangingConditional();
+  }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/LabeledStmtWrapper.java Thu Oct 1515:42:13 2009+++ /trunk/src/com/google/caja/parser/js/LabeledStmtWrapper.java Thu Nov 1213:05:03 2009

@@ -68,4 +68,8 @@
     }
     body.render(rc);
   }
-}
+
+  public boolean hasHangingConditional() {
+    return body.hasHangingConditional();
+  }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/MultiDeclaration.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/MultiDeclaration.java Thu Nov 1213:05:03 2009

@@ -78,4 +78,6 @@
       decl.renderShort(rc);
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================
--- /trunk/src/com/google/caja/parser/js/Noop.java      Wed Jul 15 11:24:34 2009
+++ /trunk/src/com/google/caja/parser/js/Noop.java      Thu Nov 12 13:05:03 2009
@@ -39,4 +39,6 @@
   public void render(RenderContext rc) {

// render for Noop is a noop as renderBlock will take care of thesemicolon

   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/ReturnStmt.java Wed Jul 1511:24:34 2009+++ /trunk/src/com/google/caja/parser/js/ReturnStmt.java Thu Nov 1213:05:03 2009

@@ -68,4 +68,6 @@
       returnValue.render(rc);
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/Statement.java Mon Mar 31 10:43:022008+++ /trunk/src/com/google/caja/parser/js/Statement.java Thu Nov 12 13:05:032009

@@ -60,4 +60,10 @@
    *   semicolon if not a block.
    */
   void renderBlock(RenderContext rc, boolean terminate);
-}
+
+  /**

+ * True if the rendered form of the statement would consume more tokensif

+   * parsed followed by the tokens "else" and ";".
+   */
+  boolean hasHangingConditional();
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/SwitchCase.java Wed Feb 1120:17:12 2009+++ /trunk/src/com/google/caja/parser/js/SwitchCase.java Thu Nov 1213:05:03 2009

@@ -17,8 +17,12 @@
 import com.google.caja.lexer.FilePosition;
 import com.google.caja.parser.ParseTreeNode;

-/** @see SwitchStmt */
+/**
+ * Base class for case and default blocks.
+ * @see SwitchStmt
+ */
 public abstract class SwitchCase extends AbstractStatement {

protected SwitchCase(FilePosition pos) { super(pos,ParseTreeNode.class); }

-  // base class for case and default blocks
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/SwitchStmt.java Thu Oct 1515:42:13 2009+++ /trunk/src/com/google/caja/parser/js/SwitchStmt.java Thu Nov 1213:05:03 2009

@@ -104,4 +104,6 @@
   public boolean isTerminal() {
     return true;
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/ThrowStmt.java Thu Oct 15 15:42:132009+++ /trunk/src/com/google/caja/parser/js/ThrowStmt.java Thu Nov 12 13:05:032009

@@ -59,4 +59,6 @@
     rc.getOut().consume("throw");
     exception.render(rc);
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/TranslatedCode.java Fri Jul 2417:10:36 2009+++ /trunk/src/com/google/caja/parser/js/TranslatedCode.java Thu Nov 1213:05:03 2009

@@ -74,6 +74,8 @@
     children().get(0).render(rc);
     out.consume("/* End translated code */");
   }
+
+  public boolean hasHangingConditional() { return false; }

   @Override
   public boolean isTerminal() {
=======================================

--- /trunk/src/com/google/caja/parser/js/TryStmt.java Mon Nov 2 13:56:192009+++ /trunk/src/com/google/caja/parser/js/TryStmt.java Thu Nov 12 13:05:032009

@@ -92,4 +92,6 @@
       fin.renderBlock(rc, false);
     }
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/UseSubsetDirective.java Fri Aug 2819:16:06 2009+++ /trunk/src/com/google/caja/parser/js/UseSubsetDirective.java Thu Nov 1213:05:03 2009

@@ -31,12 +31,12 @@
  * <p>
  * "Use subsets" are they were defined in ES5.
  * Since then the grammar was changed to use a separate string literal per
- * use-directive. The use-directives do not necessarily define subsets. But
- * they typically should define at least almost subsets in practice so
- * the code within their scope will still work on browsers that do not

- * recognize the "subset" to be used. For example, ES5/strict is not asubset

+ * use-directive. The use-directives do not necessarily define subsets. But
+ * they typically should define at least almost subsets in practice so
+ * the code within their scope will still work on browsers that do not

+ * recognize the "subset" to be used. For example, ES5/strict is not asubset

  * of ES5/non-strict. See issue 1111.
- *
+ *

* <p>An <ahref="http://wiki.ecmascript.org/lib/exe/fetch.php?id=es3.1%3Aes3.1_proposal_working_draft&cache=cache&media=es3.1:tc39-es31-draft27oct08.pdf";

  * >ES5 draft</a> defined the use grammar as:<blockquote><pre>
  * UseSubsetDirective opt :
@@ -54,7 +54,7 @@
  *   <li>Evaluate SubsetList
  *   <li>Return Result(1)
  * </ol>
- *
+ *
  * <p>
  * The production SubsetList : Subset is evaluated as follows:<ol>

* <li>If Subset is not the name of a usage subset that is supported bythis

@@ -142,4 +142,6 @@
   public boolean isTerminal() {
     return true;
   }
-}
+
+  public boolean hasHangingConditional() { return false; }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/WhileLoop.java Wed Jul 15 11:24:342009+++ /trunk/src/com/google/caja/parser/js/WhileLoop.java Thu Nov 12 13:05:032009

@@ -67,4 +67,8 @@
     out.consume(")");
     body.renderBlock(rc, false);
   }
-}
+
+  public boolean hasHangingConditional() {
+    return body.hasHangingConditional();
+  }
+}
=======================================

--- /trunk/src/com/google/caja/parser/js/WithStmt.java Fri Aug 28 19:16:062009+++ /trunk/src/com/google/caja/parser/js/WithStmt.java Thu Nov 12 13:05:032009

@@ -25,7 +25,7 @@
  * ES3-12.10: The with statement adds a computed object to the front
  * of the scope chain of the current execution context, then executes a
  * statement with this augmented scope chain, then restores the scope
- * chain. (ES5 expresses this in terms of environment records but with
+ * chain. (ES5 expresses this in terms of environment records but with
  * similar effect.)
  *
  * @author [email protected]
@@ -71,4 +71,8 @@
     out.consume(")");
     getBody().renderBlock(rc, false);
   }
-}
+
+  public boolean hasHangingConditional() {
+    return getBody().hasHangingConditional();
+  }
+}
=======================================

--- /trunk/tests/com/google/caja/parser/js/ParserTest.java Fri Aug 2819:16:06 2009+++ /trunk/tests/com/google/caja/parser/js/ParserTest.java Thu Nov 1213:05:03 2009

@@ -19,7 +19,9 @@
 import com.google.caja.lexer.TokenConsumer;
 import com.google.caja.lexer.ParseException;
 import com.google.caja.parser.AncestorChain;
+import com.google.caja.parser.MutableParseTreeNode;
 import com.google.caja.parser.ParseTreeNode;
+import com.google.caja.parser.Visitor;
 import com.google.caja.render.Concatenator;
 import com.google.caja.render.JsPrettyPrinter;
 import com.google.caja.reporting.Message;
@@ -29,7 +31,9 @@
 import com.google.caja.reporting.MessageType;
 import com.google.caja.reporting.RenderContext;
 import com.google.caja.util.CajaTestCase;
+import com.google.caja.util.Lists;
 import com.google.caja.util.MoreAsserts;
+import com.google.caja.util.Pair;
 import com.google.caja.util.Strings;
 import com.google.caja.util.TestUtil;

@@ -345,6 +349,23 @@
         MessageLevel.ERROR,
         MessagePart.Factory.valueOf("C\0327"));
   }
+
+  /**
+   * If conditionals are moved around, they can get in a configuration that
+   * would be impossible to parse.
+   */

+ public final void testRenderingOfRebuiltConditionals() throwsParseException {

+    assertEquals(
+        render(js(fromString("if (foo) { if (bar);} else baz"))),

+ render(stripBlocks(js(fromString("if (foo) { if (bar) {} } elsebaz"))))

+        );
+    assertEquals(
+        render(js(fromString(
+            "if (foo) { while (foo) if (bar) break; } else baz"))),
+        render(stripBlocks(js(fromString(
+            "if (foo) { while (foo) if (bar) break; } else baz"))))
+        );
+  }

   public void assertExpectedSemi() {
     assertParseFails("foo(function () {return;");
@@ -603,4 +624,27 @@
       throw new RuntimeException(msg, ex);
     }
   }
-}
+
+  private static Statement stripBlocks(Block b) {
+    b.acceptPostOrder(new Visitor() {
+      public boolean visit(AncestorChain<?> chain) {
+        if (chain.node instanceof Block && chain.parent != null) {
+          Block b = chain.cast(Block.class).node;
+          List<? extends Statement> children = b.children();
+          switch (children.size()) {
+            case 0:

+chain.parent.cast(MutableParseTreeNode.class).node.replaceChild(

+                  new Noop(b.getFilePosition()), b);
+              break;
+            case 1:

+chain.parent.cast(MutableParseTreeNode.class).node.replaceChild(

+                  children.get(0), b);
+              break;
+          }
+        }
+        return true;
+      }
+    }, null);
+    return b;
+  }
+}

[Caja] [google-caja] r3842 committed - Fix rendering of Conditionals...

Reply via email to