This is an automated email from the ASF dual-hosted git repository.
yasserzamani pushed a commit to branch fix/double_evaluation_master
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 1b2c8ef63be98e48f530844307095cf6355591f0
Author: Yasser Zamani <yasserzam...@apache.org>
AuthorDate: Wed Mar 31 22:32:29 2021 +0430
fix double evaluations
* 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
---
.../opensymphony/xwork2/util/OgnlTextParser.java | 115 +++++++++++++++------
.../org/apache/struts2/components/Component.java | 10 --
.../java/org/apache/struts2/components/UIBean.java | 6 +-
.../org/apache/struts2/util/ComponentUtils.java | 12 ++-
.../xwork2/util/TextParseUtilTest.java | 77 ++++++++++++--
.../xwork2/validator/URLValidatorTest.java | 4 +-
.../org/apache/struts2/components/UIBeanTest.java | 34 ++++--
.../org/apache/struts2/util/StrutsUtilTest.java | 4 +-
.../apache/struts2/views/jsp/ui/TextfieldTest.java | 4 +-
.../apache/struts2/views/jsp/ui/Textfield-5.txt | 2 +-
.../apache/struts2/views/jsp/ui/Textfield-6.txt | 2 +-
11 files changed, 204 insertions(+), 66 deletions(-)
diff --git
a/core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
b/core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
index f3ad98b..8e6dd48 100644
--- a/core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
+++ b/core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
@@ -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++;
@@ -61,7 +52,7 @@ public class OgnlTextParser implements TextParser {
}
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);
@@ -71,14 +62,23 @@ public class OgnlTextParser implements TextParser {
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);
@@ -87,10 +87,8 @@ public class OgnlTextParser implements TextParser {
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;
}
@@ -98,4 +96,63 @@ public class OgnlTextParser implements TextParser {
}
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;
+ }
}
diff --git a/core/src/main/java/org/apache/struts2/components/Component.java
b/core/src/main/java/org/apache/struts2/components/Component.java
index e20b4f5..4a7533c 100644
--- a/core/src/main/java/org/apache/struts2/components/Component.java
+++ b/core/src/main/java/org/apache/struts2/components/Component.java
@@ -377,16 +377,6 @@ public class Component {
}
/**
- * 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}.
*
* @param action the action
diff --git a/core/src/main/java/org/apache/struts2/components/UIBean.java
b/core/src/main/java/org/apache/struts2/components/UIBean.java
index 2bafabc..ccf9a6e 100644
--- a/core/src/main/java/org/apache/struts2/components/UIBean.java
+++ b/core/src/main/java/org/apache/struts2/components/UIBean.java
@@ -805,11 +805,7 @@ public abstract class UIBean extends Component {
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) {
diff --git a/core/src/main/java/org/apache/struts2/util/ComponentUtils.java
b/core/src/main/java/org/apache/struts2/util/ComponentUtils.java
index ab15e3d..fb0efcb 100644
--- a/core/src/main/java/org/apache/struts2/util/ComponentUtils.java
+++ b/core/src/main/java/org/apache/struts2/util/ComponentUtils.java
@@ -48,7 +48,17 @@ public class ComponentUtils {
}
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;
}
}
diff --git
a/core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
b/core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
index 82d1fa2..6194668 100644
--- a/core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
@@ -101,11 +101,11 @@ public class TextParseUtilTest extends XWorkTestCase {
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();
}
@@ -115,7 +115,70 @@ public class TextParseUtilTest extends XWorkTestCase {
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();
}
@@ -144,15 +207,15 @@ public class TextParseUtilTest extends XWorkTestCase {
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}}"); }});
@@ -160,7 +223,7 @@ public class TextParseUtilTest extends XWorkTestCase {
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() {
diff --git
a/core/src/test/java/com/opensymphony/xwork2/validator/URLValidatorTest.java
b/core/src/test/java/com/opensymphony/xwork2/validator/URLValidatorTest.java
index ac9119a..98dfca3 100644
--- a/core/src/test/java/com/opensymphony/xwork2/validator/URLValidatorTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/validator/URLValidatorTest.java
@@ -287,8 +287,8 @@ public class URLValidatorTest extends XWorkTestCase {
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
diff --git a/core/src/test/java/org/apache/struts2/components/UIBeanTest.java
b/core/src/test/java/org/apache/struts2/components/UIBeanTest.java
index e4b0c63..ad66de5 100644
--- a/core/src/test/java/org/apache/struts2/components/UIBeanTest.java
+++ b/core/src/test/java/org/apache/struts2/components/UIBeanTest.java
@@ -35,6 +35,8 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
+import static org.junit.Assert.assertNotEquals;
+
public class UIBeanTest extends StrutsInternalTestCase {
public void testPopulateComponentHtmlId1() throws Exception {
@@ -283,17 +285,17 @@ public class UIBeanTest extends StrutsInternalTestCase {
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}";
}
});
@@ -301,7 +303,27 @@ public class UIBeanTest extends StrutsInternalTestCase {
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() {
diff --git a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java
b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java
index 02be0f2..28d5d9f 100644
--- a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java
+++ b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java
@@ -189,7 +189,7 @@ public class StrutsUtilTest extends StrutsInternalTestCase {
assertEquals(obj1, "try: bar");
}
- public void testTranslateVariablesRecursion() throws Exception {
+ public void testTranslateVariablesEscape() throws Exception {
stack.push(new Object() {
public String getFoo() {
return "%{bar}";
@@ -201,7 +201,7 @@ public class StrutsUtilTest extends StrutsInternalTestCase {
String obj1 = strutsUtil.translateVariables("try: %{foo}");
assertNotNull(obj1);
- assertEquals("try: %{bar}", obj1);
+ assertEquals("try: \\%{bar}", obj1);
}
// === Junit Hook
diff --git
a/core/src/test/java/org/apache/struts2/views/jsp/ui/TextfieldTest.java
b/core/src/test/java/org/apache/struts2/views/jsp/ui/TextfieldTest.java
index 8bbefcb..11dd6e4 100644
--- a/core/src/test/java/org/apache/struts2/views/jsp/ui/TextfieldTest.java
+++ b/core/src/test/java/org/apache/struts2/views/jsp/ui/TextfieldTest.java
@@ -146,7 +146,7 @@ public class TextfieldTest extends AbstractUITagTest {
verify(TextFieldTag.class.getResource("Textfield-1.txt"));
}
- public void testSimple_recursionTest() throws Exception {
+ public void testSimple_escapeTest() throws Exception {
TestAction testAction = (TestAction) action;
testAction.setFoo("%{1+1}");
@@ -163,7 +163,7 @@ public class TextfieldTest extends AbstractUITagTest {
verify(TextFieldTag.class.getResource("Textfield-5.txt"));
}
- public void testSimple_recursionTestNoValue() throws Exception {
+ public void testSimple_escapeTestNoValue() throws Exception {
TestAction testAction = (TestAction) action;
testAction.setFoo("%{1+1}");
diff --git
a/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-5.txt
b/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-5.txt
index 08326c7..77326ec 100644
--- a/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-5.txt
+++ b/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-5.txt
@@ -1,4 +1,4 @@
<tr>
<td class="tdLabel"><label for="myname" class="label">mylabel:</label></td>
- <td class="tdInput"><input type="text" name="myname" size="10"
value="%{1+1}" id="myname"/></td>
+ <td class="tdInput"><input type="text" name="myname" size="10"
value="\%{1+1}" id="myname"/></td>
</tr>
diff --git
a/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-6.txt
b/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-6.txt
index 01809d0..0f44200 100644
--- a/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-6.txt
+++ b/core/src/test/resources/org/apache/struts2/views/jsp/ui/Textfield-6.txt
@@ -1,4 +1,4 @@
<tr>
<td class="tdLabel"><label for="foo" class="label">mylabel:</label></td>
- <td class="tdInput"><input type="text" name="foo" size="10" value="%{1+1}"
id="foo"/></td>
+ <td class="tdInput"><input type="text" name="foo" size="10"
value="\%{1+1}" id="foo"/></td>
</tr>
