You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by he...@apache.org on 2018/02/07 10:04:00 UTC
[commons-jexl] 02/03: JEXL-252,
JEXL-250: hardened logic and more tests
This is an automated email from the ASF dual-hosted git repository.
henrib pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-jexl.git
commit 2aa8022c4c330a8640c7d5415d53ab5d3ab71733
Author: Henri Biestro <he...@apache.org>
AuthorDate: Wed Feb 7 11:01:58 2018 +0100
JEXL-252, JEXL-250: hardened logic and more tests
---
.../apache/commons/jexl3/internal/Interpreter.java | 92 +++++++++-------------
.../commons/jexl3/parser/ASTIdentifierAccess.java | 5 ++
.../org/apache/commons/jexl3/parser/JexlNode.java | 72 ++++++++++++++++-
.../org/apache/commons/jexl3/Issues200Test.java | 47 +++++++++++
4 files changed, 157 insertions(+), 59 deletions(-)
diff --git a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java
index cdf48e9..ce301b5 100644
--- a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java
+++ b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+//CSOFF: FileLength
package org.apache.commons.jexl3.internal;
@@ -936,7 +937,7 @@ public class Interpreter extends InterpreterBase {
if (value == null
&& !(node.jjtGetParent() instanceof ASTReference)
&& !context.has(name)
- && !isTernaryProtected(node)) {
+ && !node.isTernaryProtected()) {
return unsolvableVariable(node, name, true);
}
return value;
@@ -966,31 +967,6 @@ public class Interpreter extends InterpreterBase {
}
/**
- * Check if a null evaluated expression is protected by a ternary expression.
- * <p>
- * The rationale is that the ternary / elvis expressions are meant for the user to explictly take control
- * over the error generation; ie, ternaries can return null even if the engine in strict mode
- * would normally throw an exception.
- * </p>
- * @param node the expression node
- * @return true if nullable variable, false otherwise
- */
- protected boolean isTernaryProtected(JexlNode node) {
- for (JexlNode walk = node.jjtGetParent(); walk != null; walk = walk.jjtGetParent()) {
- if (walk instanceof ASTTernaryNode) {
- return true;
- }
- if (walk instanceof ASTNullpNode) {
- return true;
- }
- if (!(walk instanceof ASTReference || walk instanceof ASTArrayAccess)) {
- break;
- }
- }
- return false;
- }
-
- /**
* Checks whether a reference child node holds a local variable reference.
* @param node the reference node
* @param which the child we are checking
@@ -1012,30 +988,25 @@ public class Interpreter extends InterpreterBase {
if (node instanceof ASTIdentifierAccessJxlt) {
final ASTIdentifierAccessJxlt accessJxlt = (ASTIdentifierAccessJxlt) node;
final String src = node.getName();
+ Throwable cause = null;
TemplateEngine.TemplateExpression expr = (TemplateEngine.TemplateExpression) accessJxlt.getExpression();
- if (expr == null) {
- TemplateEngine jxlt = jexl.jxlt();
- try {
+ try {
+ if (expr == null) {
+ TemplateEngine jxlt = jexl.jxlt();
expr = jxlt.parseExpression(node.jexlInfo(), src, frame != null ? frame.getScope() : null);
- } catch(JxltEngine.Exception xjxlt) {
- return node.isSafe()? null : unsolvableProperty(node, src, xjxlt);
+ accessJxlt.setExpression(expr);
}
- accessJxlt.setExpression(expr);
- }
- if (expr != null) {
- Object name;
- try {
- name = expr.evaluate(frame, context);
- } catch(JxltEngine.Exception xjxlt) {
- return node.isSafe()? null : unsolvableProperty(node, src, xjxlt);
- }
- if (name != null) {
- Integer id = ASTIdentifierAccess.parseIdentifier(name.toString());
- return id != null? id : name;
+ if (expr != null) {
+ Object name = expr.evaluate(frame, context);
+ if (name != null) {
+ Integer id = ASTIdentifierAccess.parseIdentifier(name.toString());
+ return id != null ? id : name;
+ }
}
- return null;
+ } catch (JxltEngine.Exception xjxlt) {
+ cause = xjxlt;
}
- return node.isSafe()? null : unsolvableProperty(node, src, null);
+ return node.isSafe() ? null : unsolvableProperty(node, src, cause);
} else {
return node.getIdentifier();
}
@@ -1043,8 +1014,11 @@ public class Interpreter extends InterpreterBase {
@Override
protected Object visit(ASTIdentifierAccess node, Object data) {
+ if (data == null) {
+ return null;
+ }
Object id = evalIdentifier(node);
- return data != null ? getAttribute(data, id, node) : null;
+ return getAttribute(data, id, node);
}
@Override
@@ -1053,10 +1027,10 @@ public class Interpreter extends InterpreterBase {
throw new JexlException.Cancel(node);
}
final int numChildren = node.jjtGetNumChildren();
- JexlNode parent = node.jjtGetParent();
+ final JexlNode parent = node.jjtGetParent();
// pass first piece of data in and loop through children
Object object = null;
- JexlNode objectNode;
+ JexlNode objectNode = null;
JexlNode ptyNode = null;
StringBuilder ant = null;
boolean antish = !(parent instanceof ASTReference);
@@ -1096,23 +1070,28 @@ public class Interpreter extends InterpreterBase {
if (ant == null) {
JexlNode first = node.jjtGetChild(0);
if (first instanceof ASTIdentifier) {
- if (((ASTIdentifier) first).getSymbol() < 0) {
- ant = new StringBuilder(((ASTIdentifier) first).getName());
+ ASTIdentifier afirst = (ASTIdentifier) first;
+ if (afirst.getSymbol() < 0) {
+ ant = new StringBuilder(afirst.getName());
} else {
- break;
+ break main;
}
} else {
ptyNode = objectNode;
- break;
+ break main;
}
}
for (; v <= c; ++v) {
JexlNode child = node.jjtGetChild(v);
if (child instanceof ASTIdentifierAccess) {
+ ASTIdentifierAccess achild = (ASTIdentifierAccess) child;
+ if (achild.isSafe() || achild.isExpression()) {
+ break main;
+ }
ant.append('.');
ant.append(((ASTIdentifierAccess) objectNode).getName());
} else {
- break;
+ break main;
}
}
object = context.get(ant.toString());
@@ -1122,14 +1101,15 @@ public class Interpreter extends InterpreterBase {
break; //
}
}
- if (object == null && !isTernaryProtected(node)) {
+ if (object == null && !node.isTernaryProtected()) {
if (antish && ant != null) {
boolean undefined = !(context.has(ant.toString()) || isLocalVariable(node, 0));
// variable unknown in context and not a local
- return unsolvableVariable(node, ant.toString(), undefined);
+ return node.isSafeLhs()? null : unsolvableVariable(node, ant.toString(), undefined);
}
if (ptyNode != null) {
- return unsolvableProperty(node, ptyNode.toString(), null);
+ // am I the left-hand side of a safe op ?
+ return ptyNode.isSafeLhs()? null : unsolvableProperty(node, ptyNode.toString(), null);
}
}
return object;
diff --git a/src/main/java/org/apache/commons/jexl3/parser/ASTIdentifierAccess.java b/src/main/java/org/apache/commons/jexl3/parser/ASTIdentifierAccess.java
index 941d5f3..9a105f5 100644
--- a/src/main/java/org/apache/commons/jexl3/parser/ASTIdentifierAccess.java
+++ b/src/main/java/org/apache/commons/jexl3/parser/ASTIdentifierAccess.java
@@ -37,6 +37,11 @@ public class ASTIdentifierAccess extends JexlNode {
identifier = parseIdentifier(id);
}
+ @Override
+ public boolean isGlobalVar() {
+ return !isSafe() && !isExpression();
+ }
+
/**
* Whether this is a dot or a question-mark-dot aka safe-navigation access.
* @return true is ?., false if .
diff --git a/src/main/java/org/apache/commons/jexl3/parser/JexlNode.java b/src/main/java/org/apache/commons/jexl3/parser/JexlNode.java
index 4772d5b..705788e 100644
--- a/src/main/java/org/apache/commons/jexl3/parser/JexlNode.java
+++ b/src/main/java/org/apache/commons/jexl3/parser/JexlNode.java
@@ -157,6 +157,9 @@ public abstract class JexlNode extends SimpleNode {
return false;
}
+ /**
+ * @return true if this node looks like a global var
+ */
public boolean isGlobalVar() {
if (this instanceof ASTVar) {
return false;
@@ -164,9 +167,6 @@ public abstract class JexlNode extends SimpleNode {
if (this instanceof ASTIdentifier) {
return ((ASTIdentifier) this).getSymbol() < 0;
}
- if (this instanceof ASTIdentifierAccess) {
- return true;
- }
int nc = this.jjtGetNumChildren() - 1;
if (nc >= 0) {
JexlNode first = this.jjtGetChild(0);
@@ -178,7 +178,73 @@ public abstract class JexlNode extends SimpleNode {
return false;
}
+ /**
+ * Whether this node is a local variable.
+ * @return true if local, false otherwise
+ */
public boolean isLocalVar() {
return this instanceof ASTIdentifier && ((ASTIdentifier) this).getSymbol() >= 0;
}
+
+ /**
+ * Whether this node is the left-hand side of a safe access identifier as in.
+ * For instance, in 'x?.y' , 'x' is safe.
+ * @return true if safe lhs, false otherwise
+ */
+ public boolean isSafeLhs() {
+ if (this instanceof ASTReference) {
+ return jjtGetChild(0).isSafeLhs();
+ }
+ JexlNode parent = this.jjtGetParent();
+ if (parent == null) {
+ return false;
+ }
+ // find this node in its parent
+ int nsiblings = parent.jjtGetNumChildren();
+ int rhs = -1;
+ for(int s = 0; s < nsiblings; ++s) {
+ JexlNode sibling = parent.jjtGetChild(s);
+ if (sibling == this) {
+ // the next chid offset of this nodes parent
+ rhs = s + 1;
+ break;
+ }
+ }
+ // seek next child in parent
+ if (rhs >= 0 && rhs < nsiblings) {
+ JexlNode rsibling = parent.jjtGetChild(rhs);
+ if (rsibling instanceof ASTIdentifierAccess && ((ASTIdentifierAccess) rsibling).isSafe()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Check if a null evaluated expression is protected by a ternary expression.
+ * <p>
+ * The rationale is that the ternary / elvis expressions are meant for the user to explictly take control
+ * over the error generation; ie, ternaries can return null even if the engine in strict mode
+ * would normally throw an exception.
+ * </p>
+ * @param node the expression node
+ * @return true if nullable variable, false otherwise
+ */
+ public boolean isTernaryProtected() {
+ JexlNode node = this;
+ for (JexlNode walk = node.jjtGetParent(); walk != null; walk = walk.jjtGetParent()) {
+ if (walk instanceof ASTTernaryNode) {
+ return true;
+ }
+ if (walk instanceof ASTNullpNode) {
+ return true;
+ }
+ if (!(walk instanceof ASTReference || walk instanceof ASTArrayAccess)) {
+ break;
+ }
+ }
+ return false;
+ }
+
+
}
diff --git a/src/test/java/org/apache/commons/jexl3/Issues200Test.java b/src/test/java/org/apache/commons/jexl3/Issues200Test.java
index c935e2e..3a59515 100644
--- a/src/test/java/org/apache/commons/jexl3/Issues200Test.java
+++ b/src/test/java/org/apache/commons/jexl3/Issues200Test.java
@@ -343,6 +343,53 @@ public class Issues200Test extends JexlTestCase {
}
@Test
+ public void test250() throws Exception {
+ MapContext ctx = new MapContext();
+ HashMap<Object,Object> x = new HashMap<Object, Object> ();
+ x.put(2, "123456789");
+ ctx.set("x", x);
+ JexlEngine engine = new JexlBuilder().strict(true).silent(false).create();
+ String stmt = "x.2.class.name";
+ JexlScript script = engine.createScript(stmt);
+ Object result = script.execute(ctx);
+ Assert.assertEquals("java.lang.String", result);
+
+ try {
+ stmt = "x.3?.class.name";
+ script = engine.createScript(stmt);
+ result = script.execute(ctx);
+ Assert.assertNull(result);
+ } catch (JexlException xany) {
+ Assert.fail("Should have evaluated to null");
+ }
+ try {
+ stmt = "x?.3.class.name";
+ script = engine.createScript(stmt);
+ result = script.execute(ctx);
+ Assert.fail("Should have thrown, fail on 3");
+ Assert.assertNull(result);
+ } catch (JexlException xany) {
+ Assert.assertTrue(xany.detailedMessage().contains("3"));
+ }
+ try {
+ stmt = "x?.3?.class.name";
+ script = engine.createScript(stmt);
+ result = script.execute(ctx);
+ Assert.assertNull(result);
+ } catch (JexlException xany) {
+ Assert.fail("Should have evaluated to null");
+ }
+ try {
+ stmt = "y?.3.class.name";
+ script = engine.createScript(stmt);
+ result = script.execute(ctx);
+ Assert.assertNull(result);
+ } catch(JexlException xany) {
+ Assert.fail("Should have evaluated to null");
+ }
+ }
+
+ @Test
public void test252() throws Exception {
MapContext ctx = new MapContext();
JexlEngine engine = new JexlBuilder().strict(true).silent(false).create();
--
To stop receiving notification emails like this one, please contact
henrib@apache.org.