From commits-return-19658-archive-asf-public=cust-asf.ponee.io@struts.apache.org Wed Mar 31 18:06:31 2021 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mxout1-he-de.apache.org (mxout1-he-de.apache.org [95.216.194.37]) by mx-eu-01.ponee.io (Postfix) with ESMTPS id B8999180607 for ; Wed, 31 Mar 2021 20:06:31 +0200 (CEST) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-he-de.apache.org (ASF Mail Server at mxout1-he-de.apache.org) with SMTP id 1680F60C24 for ; Wed, 31 Mar 2021 18:06:12 +0000 (UTC) Received: (qmail 62740 invoked by uid 500); 31 Mar 2021 18:06:11 -0000 Mailing-List: contact commits-help@struts.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@struts.apache.org Delivered-To: mailing list commits@struts.apache.org Received: (qmail 62716 invoked by uid 99); 31 Mar 2021 18:06:11 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 31 Mar 2021 18:06:11 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 237B18E950; Wed, 31 Mar 2021 18:06:11 +0000 (UTC) Date: Wed, 31 Mar 2021 18:06:12 +0000 To: "commits@struts.apache.org" Subject: [struts] 01/01: fix double evaluations MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit From: yasserzamani@apache.org In-Reply-To: <161721397099.8808.630641224581501452@gitbox.apache.org> References: <161721397099.8808.630641224581501452@gitbox.apache.org> X-Git-Host: gitbox.apache.org X-Git-Repo: struts X-Git-Refname: refs/heads/fix/double_evaluation_master X-Git-Reftype: branch X-Git-Rev: 1b2c8ef63be98e48f530844307095cf6355591f0 X-Git-NotificationType: diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated Message-Id: <20210331180611.237B18E950@gitbox.apache.org> 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 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() {{ 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() {{ 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() {{ 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() {{ 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() {{ 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() {{ 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 @@ - + 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 @@ - +