From 711cf0201cdd319a38cf29238913312355db29ba Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 6 Jun 2013 05:29:48 +0000 Subject: [PATCH] Merged from STRUTS_2_3_14_2_X WW-4090 Itroduces actions names' whitelisting [from revision 1488895] WW-4090 Removes double evaluation of parsed expression [from revision 1488897] WW-4090 Add some logging [from revision 1488899] WW-4090 Uses warn level instead of debug [from revision 1488900] git-svn-id: https://svn.apache.org/repos/asf/struts/struts2/trunk@1490149 13f79535-47bb-0310-9956-ffa450edef68 --- .../org/apache/struts2/StrutsConstants.java | 3 ++ .../mapper/DefaultActionMapper.java | 44 ++++++++++++++++--- .../mapper/DefaultActionMapperTest.java | 19 ++++++++ .../xwork2/util/OgnlTextParser.java | 5 +-- .../xwork2/util/TextParseUtilTest.java | 23 +++++++++- 5 files changed, 83 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 8a68109213..eb8fe6fcda 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -252,4 +252,7 @@ public final class StrutsConstants { public static final String STRUTS_EXPRESSION_PARSER = "struts.expression.parser"; + /** actions names' whitelist **/ + public static final String STRUTS_ALLOWED_ACTION_NAMES = "struts.allowed.action.names"; + } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java b/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java index 6e7d8a6be7..57b251da65 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java @@ -27,6 +27,8 @@ import com.opensymphony.xwork2.config.entities.PackageConfig; import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; import org.apache.commons.lang3.StringUtils; import org.apache.struts2.RequestUtils; import org.apache.struts2.ServletActionContext; @@ -35,12 +37,7 @@ import org.apache.struts2.util.PrefixTrie; import javax.servlet.http.HttpServletRequest; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; /** * @@ -162,6 +159,8 @@ */ public class DefaultActionMapper implements ActionMapper { + private static final Logger LOG = LoggerFactory.getLogger(DefaultActionMapper.class); + protected static final String METHOD_PREFIX = "method:"; protected static final String ACTION_PREFIX = "action:"; protected static final String REDIRECT_PREFIX = "redirect:"; @@ -171,6 +170,7 @@ public class DefaultActionMapper implements ActionMapper { protected boolean allowSlashesInActionNames = false; protected boolean alwaysSelectFullNamespace = false; protected PrefixTrie prefixTrie = null; + protected String allowedActionNames = "[a-z]*[A-Z]*[0-9]*[.\\-_!/]*"; protected List extensions = new ArrayList() {{ add("action"); @@ -260,6 +260,11 @@ public void setAlwaysSelectFullNamespace(String val) { this.alwaysSelectFullNamespace = "true".equals(val); } + @Inject(value = StrutsConstants.STRUTS_ALLOWED_ACTION_NAMES, required = false) + public void setAllowedActionNames(String allowedActionNames) { + this.allowedActionNames = allowedActionNames; + } + @Inject public void setContainer(Container container) { this.container = container; @@ -417,7 +422,32 @@ protected void parseNameAndNamespace(String uri, ActionMapping mapping, Configur } mapping.setNamespace(namespace); - mapping.setName(name); + mapping.setName(cleanupActionName(name)); + } + + /** + * Cleans up action name from suspicious characters + * + * @param rawActionName action name extracted from URI + * @return safe action name + */ + protected String cleanupActionName(final String rawActionName) { + if (rawActionName.matches(allowedActionNames)) { + return rawActionName; + } else { + if (LOG.isWarnEnabled()) { + LOG.warn("Action [#0] do not match allowed action names pattern [#1], cleaning it up!", + rawActionName, allowedActionNames); + } + String cleanActionName = rawActionName; + for(String chunk : rawActionName.split(allowedActionNames)) { + cleanActionName = cleanActionName.replace(chunk, ""); + } + if (LOG.isDebugEnabled()) { + LOG.debug("Cleaned action name [#0]", cleanActionName); + } + return cleanActionName; + } } /** diff --git a/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java b/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java index e2513b2cae..c6ecd4bfac 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java @@ -747,4 +747,23 @@ public void testSetExtension() throws Exception { } + public void testAllowedActionNames() throws Exception { + DefaultActionMapper mapper = new DefaultActionMapper(); + + String actionName = "action"; + assertEquals(actionName, mapper.cleanupActionName(actionName)); + + actionName = "${action}"; + assertEquals("action", mapper.cleanupActionName(actionName)); + + actionName = "${${%{action}}}"; + assertEquals("action", mapper.cleanupActionName(actionName)); + + actionName = "${#foo='action',#foo}"; + assertEquals("fooactionfoo", mapper.cleanupActionName(actionName)); + + actionName = "test-action"; + assertEquals("test-action", mapper.cleanupActionName(actionName)); + } + } diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java b/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java index c25298adb8..fa390bf80f 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java @@ -11,17 +11,16 @@ public Object evaluate(char[] openChars, String expression, TextParseUtil.Parsed // deal with the "pure" expressions first! //expression = expression.trim(); Object result = expression; + int pos = 0; + for (char open : openChars) { int loopCount = 1; - int pos = 0; - //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) { - pos = 0; loopCount++; start = expression.indexOf(lookupChars); } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java index e18fb0f77a..45e79709fc 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java @@ -97,6 +97,24 @@ public void testTranslateVariables() { assertEquals("count must be between 123 and 456, current value is 98765.", s); } + public void testNestedExpression() 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); + stack.pop(); + } + + public void testMixedOpenChars() throws Exception { + ValueStack stack = ActionContext.getContext().getValueStack(); + stack.push(new HashMap() {{ put("foo", "bar"); }}); + 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 + stack.pop(); + } + public void testCommaDelimitedStringToSet() { assertEquals(0, TextParseUtil.commaDelimitedStringToSet("").size()); assertEquals(new HashSet(Arrays.asList("foo", "bar", "tee")), @@ -132,10 +150,13 @@ public void testTranslateVariablesNoRecursive() { public void testTranslateVariablesRecursive() { ValueStack stack = ActionContext.getContext().getValueStack(); - stack.push(new HashMap() {{ put("foo", "${1+1}"); }}); + stack.push(new HashMap() {{ 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); } public void testTranslateVariablesWithNull() {