Skip to content

Commit

Permalink
fix double evaluations
Browse files Browse the repository at this point in the history 
* fixes `ComponentUtils.containsExpression` method, consequently `Component.recursion` method removed
* fixes and improves `OgnlTextParser.evaluate` method
** makes it safe from order of `openChars`
** adds support of nested expressions
** escapes middle in last loop
** removes not needed `if` conditions
* fixes current double evaluation test of `UIBean.evaluateNameValue`
* adds test that verifies double evaluation still is possible via nested variables if user really wants
* adjusts tests to the new policy - a translated variable isn't allowed to introduce new variables
  • Loading branch information
@yasserzamani
yasserzamani committed Mar 31, 2021
1 parent 2ba1a3e commit 1b2c8ef
Show file tree
Hide file tree
Showing 11 changed files with 204 additions and 66 deletions.
115 changes: 86 additions & 29 deletions core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
View file
Expand Up @@ -19,39 +19,30 @@
package com.opensymphony.xwork2.util;

import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

/**
* OGNL implementation of {@link TextParser}
*/
public class OgnlTextParser implements TextParser {
private static final Logger LOG = LogManager.getLogger(OgnlTextParser.class);

public Object evaluate(char[] openChars, String expression, TextParseUtil.ParsedValueEvaluator evaluator, int maxLoopCount) {
// deal with the "pure" expressions first!
//expression = expression.trim();
Object result = expression = (expression == null) ? "" : expression;
int pos = 0;

for (char open : openChars) {
int loopCount = 1;
//this creates an implicit StringBuffer and shouldn't be used in the inner loop
final String lookupChars = open + "{";

while (true) {
int start = expression.indexOf(lookupChars, pos);
if (start == -1) {
loopCount++;
start = expression.indexOf(lookupChars);
}
if (loopCount > maxLoopCount) {
// translateVariables prevent infinite loop / expression recursive evaluation
break;
}
for (int loopCount = 1; loopCount <= maxLoopCount; loopCount++) {
int pos = 0;
int start = findMostInnerVariableStart(expression, pos, openChars);
while (start >= pos) {
int length = expression.length();
int x = start + 2;
int end;
char c;
int count = 1;
while (start != -1 && x < length && count != 0) {
while (x < length && count != 0) {
c = expression.charAt(x++);
if (c == '{') {
count++;
Expand All @@ -61,7 +52,7 @@ public Object evaluate(char[] openChars, String expression, TextParseUtil.Parsed
}
end = x - 1;

if ((start != -1) && (end != -1) && (count == 0)) {
if ((end != -1) && (count == 0)) {
String var = expression.substring(start + 2, end);

Object o = evaluator.evaluate(var);
Expand All @@ -71,14 +62,23 @@ public Object evaluate(char[] openChars, String expression, TextParseUtil.Parsed
String middle = null;
if (o != null) {
middle = o.toString();
if (StringUtils.isEmpty(left)) {
result = o;
} else {
result = left.concat(middle);
if (loopCount >= maxLoopCount) {
middle = escape(openChars, middle);
}

if (StringUtils.isNotEmpty(right)) {
result = result.toString().concat(right);
if (StringUtils.isEmpty(left) && StringUtils.isEmpty(right)) {
if (o instanceof String) {
result = middle;
} else {
result = o;
}
} else {
result = middle;
if (StringUtils.isNotEmpty(left)) {
result = left.concat(result.toString());
}
if (StringUtils.isNotEmpty(right)) {
result = result.toString().concat(right);
}
}

expression = left.concat(middle).concat(right);
Expand All @@ -87,15 +87,72 @@ public Object evaluate(char[] openChars, String expression, TextParseUtil.Parsed
expression = left.concat(right);
result = expression;
}
pos = (left != null && left.length() > 0 ? left.length() - 1: 0) +
(middle != null && middle.length() > 0 ? middle.length() - 1: 0) +
1;
pos = Math.max(pos, 1);
pos = left.length() + (middle != null ? middle.length() : 0);
start = findMostInnerVariableStart(expression, pos, openChars);
} else {
break;
}
}
}
return result;
}

/**
* Finds the first start index (after pos) of most inner variable in a given expression
* @param expression the whole expression
* @param pos the index that the search starts from inclusively
* @param openChars the chars that indicate variables
* @return the first start index (after pos) of most inner variable
* @since 2.5.27
*/
private int findMostInnerVariableStart(String expression, int pos, char[] openChars) {
final String openCharsStr = new String(openChars);
int start = expression.indexOf('}', pos);
int nextPos = start + 1;
do {
while (start > pos) {
start--;
if ('{' == expression.charAt(start) && (start < 2 || '\\' != expression.charAt(start - 2))
&& start - 1 >= pos && openCharsStr.indexOf(expression.charAt(start - 1)) >= 0) {
return start - 1;
}
}
start = expression.indexOf('}', nextPos);
nextPos = start + 1;
} while (start != -1);

return -1;
}

/**
* Currently Struts doesn't want to support nested variables owing to risk of user confusing potential double
* evaluations. That being said, when variables translated, then the translated expression shouldn't introduce
* another variable.
* @param openChars variable place holders
* @param evaluated the translated expression which shouldn't introduce another variable
* @return an escaped expression which don't introduce another variable
* @since 2.5.27
*/
private String escape(char[] openChars, String evaluated) {
int len = evaluated.length();
for (char open : openChars) {
final String lookupChars = "" + open + '{';
int pos = 0;
int start = evaluated.indexOf(lookupChars, pos);
while (start >= 0) {
if (start == 0 || '\\' != evaluated.charAt(start - 1)) {
evaluated = new StringBuilder(evaluated).insert(start, '\\').toString();
}
pos = start + 2;
start = evaluated.indexOf(lookupChars, pos);
}
}
if (len != evaluated.length()) {
LOG.warn("Since 2.5.27 variables aren't allowed to introduce new variables once translated, owing to risk " +
"of confusing potential double evaluations. Please consider a new design if this escaped expression " +
"doesn't work for you: {}", evaluated);
}

return evaluated;
}
}
10 changes: 0 additions & 10 deletions core/src/main/java/org/apache/struts2/components/Component.java
View file
Expand Up @@ -376,16 +376,6 @@ protected Object findValue(String expression, Class<?> toType) {
}
}

/**
* Detects if expression already contains %{...}
*
* @param expression a string to examined
* @return true if expression contains %{...}
*/
protected boolean recursion(String expression) {
return ComponentUtils.containsExpression(expression);
}

/**
* Renders an action URL by consulting the {@link org.apache.struts2.dispatcher.mapper.ActionMapper}.
*
Expand Down
6 changes: 1 addition & 5 deletions core/src/main/java/org/apache/struts2/components/UIBean.java
View file
Expand Up @@ -805,11 +805,7 @@ public void evaluateParams() {
addParameter("nameValue", findValue(value, valueClazz));
} else if (name != null) {
String expr = completeExpression(name);
if (recursion(name)) {
addParameter("nameValue", expr);
} else {
addParameter("nameValue", findValue(expr, valueClazz));
}
addParameter("nameValue", findValue(expr, valueClazz));
}
} else {
if (value != null) {
Expand Down
12 changes: 11 additions & 1 deletion core/src/main/java/org/apache/struts2/util/ComponentUtils.java
View file
Expand Up @@ -48,7 +48,17 @@ public static boolean isExpression(String expr) {
}

public static boolean containsExpression(String expr) {
return expr != null && expr.contains("%{") && expr.contains("}");
if (expr == null) {
return false;
}
int start = -2;
int end;
do {
start = expr.indexOf("%{", start + 2);
end = start >= 0 ? expr.indexOf('}', start + 2) : -1;
} while (start > 0 && '\\' == expr.charAt(start - 1));

return end >= start + 2;
}

}
77 changes: 70 additions & 7 deletions core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
View file
Expand Up @@ -101,11 +101,11 @@ public void testTranslateVariables() {
assertEquals("count must be between 123 and 456, current value is 98765.", s);
}

public void testNestedExpression() throws Exception {
public void testNestedExpressionEscape() throws Exception {
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new HashMap<String, Object>() {{ put("foo", "${%{1+1}}"); }});
String s = TextParseUtil.translateVariables("${foo}", stack);
assertEquals("${%{1+1}}", s);
assertEquals("\\${\\%{1+1}}", s);
stack.pop();
}

Expand All @@ -115,7 +115,70 @@ public void testMixedOpenChars() throws Exception {
String s = TextParseUtil.translateVariables("${foo}-%{foo}", stack);
assertEquals("bar-bar", s);
s = TextParseUtil.translateVariables("%{foo}-${foo}", stack);
assertEquals("%{foo}-bar", s); // this is bad, but it is the only way not to double evaluate passed expression
assertEquals("bar-bar", s);
stack.pop();
}

public void testBoundaries() throws Exception {
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new HashMap<String, Object>() {{ put("foo", "bar"); }});
String s = TextParseUtil.translateVariables(null, stack);
assertEquals("", s);
s = TextParseUtil.translateVariables("", stack);
assertEquals("", s);
s = TextParseUtil.translateVariables("}${foo}", stack);
assertEquals("}bar", s);
s = TextParseUtil.translateVariables("{}%{foo}", stack);
assertEquals("{}bar", s);
s = TextParseUtil.translateVariables("}${foo}{", stack);
assertEquals("}bar{", s);
s = TextParseUtil.translateVariables("{%{foo}}${foo", stack);
assertEquals("{bar}${foo", s);
s = TextParseUtil.translateVariables("%}foo{", stack);
assertEquals("%}foo{", s);
s = TextParseUtil.translateVariables("$}{foo}${", stack);
assertEquals("$}{foo}${", s);
s = TextParseUtil.translateVariables("${}", stack);
assertEquals("", s);
s = TextParseUtil.translateVariables("%${foo}%{", stack);
assertEquals("%bar%{", s);
s = TextParseUtil.translateVariables("$foo", stack);
assertEquals("$foo", s);
s = TextParseUtil.translateVariables("\\${foo}", stack);
assertEquals("\\${foo}", s);
s = TextParseUtil.translateVariables("$ {foo}", stack);
assertEquals("$ {foo}", s);
s = TextParseUtil.translateVariables("${foo}\\%{foo}", stack);
assertEquals("bar\\%{foo}", s);
s = TextParseUtil.translateVariables("%{${foo}}", stack);
assertEquals("%{bar}", s);
stack.pop();
}

public void testMixedOpenCharsOrderAndEscapeSafetyAtMaxLoopCountPlusTwo() throws Exception {
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new HashMap<String, Object>() {{ put("foo", "${bar}"); put("bar", "%{baz}"); put("baz", "\\${foo}"); }});
char[] openChars = new char[] {'$', '%'};
Object s = TextParseUtil.translateVariables(openChars, "${foo}-%{foo}-${foo}", stack, String.class, null, 5);
assertEquals("\\${foo}-\\${foo}-\\${foo}", s);
s = TextParseUtil.translateVariables(openChars, "%{foo}-${foo}-%{foo}", stack, String.class, null, 5);
assertEquals("\\${foo}-\\${foo}-\\${foo}", s);
stack.pop();
}

public void testNestedVariablesAndEscapesByLoopCount() throws Exception {
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new HashMap<String, Object>() {{ put("foo", "${1+1}"); put("bar", "${1+${1+${foo}}}"); }});
String s = TextParseUtil.translateVariables("${bar}", stack);
assertEquals("\\${1+\\${1+\\${foo}}}", s);
Object o = TextParseUtil.translateVariables('$', "${bar}", stack, String.class, null, 2);
assertEquals("${1+${1+\\${1+1}}}", o);
o = TextParseUtil.translateVariables('$', "${bar}", stack, String.class, null, 3);
assertEquals("${1+${1+2}}", o);
o = TextParseUtil.translateVariables('$', "${bar}", stack, String.class, null, 4);
assertEquals("${1+3}", o);
o = TextParseUtil.translateVariables('$', "${bar}", stack, String.class, null, 5);
assertEquals("4", o);
stack.pop();
}

Expand Down Expand Up @@ -144,23 +207,23 @@ public void testTranslateNoVariables() {
assertEquals("foo: ", s);
}

public void testTranslateVariablesNoRecursive() {
public void testTranslateVariablesEscape() {
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new HashMap<String, Object>() {{ put("foo", "${1+1}"); }});

Object s = TextParseUtil.translateVariables('$', "foo: ${foo}", stack, String.class, null, 1);
assertEquals("foo: ${1+1}", s);
assertEquals("foo: \\${1+1}", s);
}

public void testTranslateVariablesRecursive() {
public void testTranslateVariablesRecursiveAndEscape() {
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new HashMap<String, Object>() {{ put("foo", "${1+1}"); put("bar", "${${1+2}}"); }});

Object s = TextParseUtil.translateVariables('$', "foo: ${foo}", stack, String.class, null, 2);
assertEquals("foo: 2", s);

s = TextParseUtil.translateVariables('$', "foo: ${bar}", stack, String.class, null, 1);
assertEquals("foo: ${${1+2}}", s);
assertEquals("foo: \\${\\${1+2}}", s);
}

public void testTranslateVariablesWithNull() {
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/com/opensymphony/xwork2/validator/URLValidatorTest.java
View file
Expand Up @@ -287,8 +287,8 @@ public void testCollectionOfUrlsSafness() throws Exception {
assertFalse(validator.getValidatorContext().hasActionMessages());
assertTrue(validator.getValidatorContext().hasFieldErrors());
assertEquals(2, validator.getValidatorContext().getFieldErrors().get("urlSafeness").size());
assertEquals("Wrong URL provided: ${1+2}", validator.getValidatorContext().getFieldErrors().get("urlSafeness").get(0));
assertEquals("Wrong URL provided: %{2+3}", validator.getValidatorContext().getFieldErrors().get("urlSafeness").get(1));
assertEquals("Wrong URL provided: \\${1+2}", validator.getValidatorContext().getFieldErrors().get("urlSafeness").get(0));
assertEquals("Wrong URL provided: \\%{2+3}", validator.getValidatorContext().getFieldErrors().get("urlSafeness").get(1));
}

@Override
Expand Down
34 changes: 28 additions & 6 deletions core/src/test/java/org/apache/struts2/components/UIBeanTest.java
View file
Expand Up @@ -35,6 +35,8 @@
import java.util.HashMap;
import java.util.Map;

import static org.junit.Assert.assertNotEquals;

public class UIBeanTest extends StrutsInternalTestCase {

public void testPopulateComponentHtmlId1() throws Exception {
Expand Down Expand Up @@ -283,25 +285,45 @@ public void testValueParameterEvaluation() {
assertEquals(value, txtFld.getParameters().get("nameValue"));
}

public void testValueParameterRecursion() {
/**
* reported by and credits for Man Yue Mo from the Semmle Security Research team
*/
public void testValueParameterEscape() {
ValueStack stack = ActionContext.getContext().getValueStack();
MockHttpServletRequest req = new MockHttpServletRequest();
MockHttpServletResponse res = new MockHttpServletResponse();

stack.push(new Object() {
public String getMyValue() {
return "%{myBad}";
}
public String getMyBad() {
throw new IllegalStateException("Recursion detected!");
return "%{1+1}";
}
});

TextField txtFld = new TextField(stack, req, res);
txtFld.setName("%{myValue}");
txtFld.evaluateParams();

assertEquals("%{myBad}", txtFld.getParameters().get("nameValue"));
assertNotEquals("double evaluation?!", "2", txtFld.getParameters().get("nameValue").toString());
assertEquals("ognl evaluated an escaped \\%?!", "", txtFld.getParameters().get("nameValue"));
assertEquals("not escaped?!", "\\%{1+1}", txtFld.getParameters().get("name"));
}
public void testNestedValueParameter() {
ValueStack stack = ActionContext.getContext().getValueStack();
MockHttpServletRequest req = new MockHttpServletRequest();
MockHttpServletResponse res = new MockHttpServletResponse();

stack.push(new Object() {
public String getMyValue() {
return "1+1";
}
});

TextField txtFld = new TextField(stack, req, res);
txtFld.setName("%{%{myValue}}");
txtFld.evaluateParams();

assertEquals("nested vars not working?!", "2", txtFld.getParameters().get("nameValue"));
assertEquals("nested vars not working?!", "%{1+1}", txtFld.getParameters().get("name"));
}

public void testSetClass() {
Expand Down

0 comments on commit 1b2c8ef

Please sign in to comment.