Author: lukaszlenart
Date: Thu Jun 6 05:29:48 2013
New Revision: 1490149
URL: http://svn.apache.org/r1490149
Log:
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]
Modified:
struts/struts2/trunk/ (props changed)
struts/struts2/trunk/core/src/main/java/org/apache/struts2/StrutsConstants.java
struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
struts/struts2/trunk/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
Propchange: struts/struts2/trunk/
------------------------------------------------------------------------------
Merged
/struts/struts2/branches/STRUTS_2_3_14_2_X:r1488895,1488897,1488899-1488900
Modified:
struts/struts2/trunk/core/src/main/java/org/apache/struts2/StrutsConstants.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/core/src/main/java/org/apache/struts2/StrutsConstants.java?rev=1490149&r1=1490148&r2=1490149&view=diff
==============================================================================
---
struts/struts2/trunk/core/src/main/java/org/apache/struts2/StrutsConstants.java
(original)
+++
struts/struts2/trunk/core/src/main/java/org/apache/struts2/StrutsConstants.java
Thu Jun 6 05:29:48 2013
@@ -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";
+
}
Modified:
struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java?rev=1490149&r1=1490148&r2=1490149&view=diff
==============================================================================
---
struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
(original)
+++
struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
Thu Jun 6 05:29:48 2013
@@ -27,6 +27,8 @@ import com.opensymphony.xwork2.config.Co
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.dispatcher.Ser
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.*;
/**
* <!-- START SNIPPET: javadoc -->
@@ -162,6 +159,8 @@ import java.util.Set;
*/
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 impleme
protected boolean allowSlashesInActionNames = false;
protected boolean alwaysSelectFullNamespace = false;
protected PrefixTrie prefixTrie = null;
+ protected String allowedActionNames = "[a-z]*[A-Z]*[0-9]*[.\\-_!/]*";
protected List<String> extensions = new ArrayList<String>() {{
add("action");
@@ -260,6 +260,11 @@ public class DefaultActionMapper impleme
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 @@ public class DefaultActionMapper impleme
}
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;
+ }
}
/**
Modified:
struts/struts2/trunk/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java?rev=1490149&r1=1490148&r2=1490149&view=diff
==============================================================================
---
struts/struts2/trunk/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
(original)
+++
struts/struts2/trunk/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
Thu Jun 6 05:29:48 2013
@@ -747,4 +747,23 @@ public class DefaultActionMapperTest ext
}
+ 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));
+ }
+
}
Modified:
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java?rev=1490149&r1=1490148&r2=1490149&view=diff
==============================================================================
---
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
(original)
+++
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/OgnlTextParser.java
Thu Jun 6 05:29:48 2013
@@ -11,17 +11,16 @@ public class OgnlTextParser implements T
// 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);
}
Modified:
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java?rev=1490149&r1=1490148&r2=1490149&view=diff
==============================================================================
---
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
(original)
+++
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java
Thu Jun 6 05:29:48 2013
@@ -97,6 +97,24 @@ public class TextParseUtilTest extends X
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<String, Object>() {{ 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<String, Object>() {{ 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<String>(Arrays.asList("foo", "bar", "tee")),
@@ -132,10 +150,13 @@ public class TextParseUtilTest extends X
public void testTranslateVariablesRecursive() {
ValueStack stack = ActionContext.getContext().getValueStack();
- stack.push(new HashMap<String, Object>() {{ put("foo", "${1+1}"); }});
+ 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);
}
public void testTranslateVariablesWithNull() {
