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
uyid · Jun 6, 2013 · 711cf02 · 711cf02
1 parent 66d71be
commit 711cf02
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 11 deletions.
diff --git 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
@@ -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.*;
 
 /**
  * <!-- START SNIPPET: javadoc -->
@@ -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<String> extensions = new ArrayList<String>() {{
         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
@@ -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
@@ -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
@@ -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<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 void testTranslateVariablesNoRecursive() {
 
     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() {