Repository: struts Updated Branches: refs/heads/master d19b9eaa8 -> 5acc71f7c
WW-4669 Returns default action/method instead of throwing exception Project: http://git-wip-us.apache.org/repos/asf/struts/repo Commit: http://git-wip-us.apache.org/repos/asf/struts/commit/5acc71f7 Tree: http://git-wip-us.apache.org/repos/asf/struts/tree/5acc71f7 Diff: http://git-wip-us.apache.org/repos/asf/struts/diff/5acc71f7 Branch: refs/heads/master Commit: 5acc71f7c30e16807912bf99b4c32cc4b25f586e Parents: d19b9ea Author: Lukasz Lenart <lukaszlen...@apache.org> Authored: Fri Jul 29 10:09:24 2016 +0200 Committer: Lukasz Lenart <lukaszlen...@apache.org> Committed: Fri Jul 29 10:09:24 2016 +0200 ---------------------------------------------------------------------- .../org/apache/struts2/StrutsConstants.java | 7 ++++ .../dispatcher/mapper/DefaultActionMapper.java | 44 ++++++++++++++++++-- .../mapper/DefaultActionMapperTest.java | 44 ++++++++------------ .../apache/struts2/rest/RestActionMapper.java | 5 ++- 4 files changed, 68 insertions(+), 32 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/struts/blob/5acc71f7/core/src/main/java/org/apache/struts2/StrutsConstants.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 186e880..c41d542 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -268,6 +268,13 @@ public final class StrutsConstants { /** actions names' whitelist **/ public static final String STRUTS_ALLOWED_ACTION_NAMES = "struts.allowed.action.names"; + /** default action name to use when action didn't match the whitelist **/ + public static final String STRUTS_DEFAULT_ACTION_NAME = "struts.default.action.name"; + + /** methods names' whitelist **/ + public static final String STRUTS_ALLOWED_METHOD_NAMES = "struts.allowed.method.names"; + /** default method name to use when method didn't match the whitelist **/ + public static final String STRUTS_DEFAULT_METHOD_NAME = "struts.default.method.name"; /** enables action: prefix **/ public static final String STRUTS_MAPPER_ACTION_PREFIX_ENABLED = "struts.mapper.action.prefix.enabled"; http://git-wip-us.apache.org/repos/asf/struts/blob/5acc71f7/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java ---------------------------------------------------------------------- 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 d0e89be..f1fcfee 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 @@ -121,6 +121,11 @@ public class DefaultActionMapper implements ActionMapper { protected boolean alwaysSelectFullNamespace = false; protected PrefixTrie prefixTrie = null; protected Pattern allowedActionNames = Pattern.compile("[a-zA-Z0-9._!/\\-]*"); + protected String defaultActionName = "index"; + + protected Pattern allowedMethodNames = Pattern.compile("[a-zA-Z_]*[0-9]*"); + protected String defaultMethodName = "execute"; + private boolean allowActionPrefix = false; private boolean allowActionCrossNamespaceAccess = false; @@ -137,7 +142,7 @@ public class DefaultActionMapper implements ActionMapper { put(METHOD_PREFIX, new ParameterAction() { public void execute(String key, ActionMapping mapping) { if (allowDynamicMethodCalls) { - mapping.setMethod(cleanupActionName(key.substring(METHOD_PREFIX.length()))); + mapping.setMethod(cleanupMethodName(key.substring(METHOD_PREFIX.length()))); } } }); @@ -149,7 +154,7 @@ public class DefaultActionMapper implements ActionMapper { if (allowDynamicMethodCalls) { int bang = name.indexOf('!'); if (bang != -1) { - String method = cleanupActionName(name.substring(bang + 1)); + String method = cleanupMethodName(name.substring(bang + 1)); mapping.setMethod(method); name = name.substring(0, bang); } @@ -205,6 +210,21 @@ public class DefaultActionMapper implements ActionMapper { this.allowedActionNames = Pattern.compile(allowedActionNames); } + @Inject(value = StrutsConstants.STRUTS_DEFAULT_ACTION_NAME, required = false) + public void setDefaultActionName(String defaultActionName) { + this.defaultActionName = defaultActionName; + } + + @Inject(value = StrutsConstants.STRUTS_ALLOWED_METHOD_NAMES, required = false) + public void setAllowedMethodNames(String allowedMethodNames) { + this.allowedMethodNames = Pattern.compile(allowedMethodNames); + } + + @Inject(value = StrutsConstants.STRUTS_DEFAULT_METHOD_NAME, required = false) + public void setDefaultMethodName(String defaultMethodName) { + this.defaultMethodName = defaultMethodName; + } + @Inject(value = StrutsConstants.STRUTS_MAPPER_ACTION_PREFIX_ENABLED) public void setAllowActionPrefix(String allowActionPrefix) { this.allowActionPrefix = BooleanUtils.toBoolean(allowActionPrefix); @@ -377,7 +397,7 @@ public class DefaultActionMapper implements ActionMapper { } /** - * Cleans up action name from suspicious characters + * Checks action name against allowed pattern if not matched returns default action name * * @param rawActionName action name extracted from URI * @return safe action name @@ -386,7 +406,23 @@ public class DefaultActionMapper implements ActionMapper { if (allowedActionNames.matcher(rawActionName).matches()) { return rawActionName; } else { - throw new StrutsException("Action [" + rawActionName + "] does not match allowed action names pattern [" + allowedActionNames + "]!"); + LOG.warn("{} did not match allowed action names {} - default action {} will be used!", rawActionName, allowedActionNames, defaultActionName); + return defaultActionName; + } + } + + /** + * Checks method name (when DMI is enabled) against allowed pattern if not matched returns default action name + * + * @param rawMethodName method name extracted from URI + * @return safe method name + */ + protected String cleanupMethodName(final String rawMethodName) { + if (allowedMethodNames.matcher(rawMethodName).matches()) { + return rawMethodName; + } else { + LOG.warn("{} did not match allowed method names {} - default method {} will be used!", rawMethodName, allowedMethodNames, defaultMethodName); + return defaultMethodName; } } http://git-wip-us.apache.org/repos/asf/struts/blob/5acc71f7/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java ---------------------------------------------------------------------- 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 b51f569..2008e38 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 @@ -845,37 +845,14 @@ public class DefaultActionMapperTest extends StrutsInternalTestCase { String actionName = "action"; assertEquals(actionName, mapper.cleanupActionName(actionName)); - Throwable expected = null; - actionName = "${action}"; - try { - mapper.cleanupActionName(actionName); - fail(); - } catch (Throwable t) { - expected = t; - } - assertTrue(expected instanceof StrutsException); - assertEquals("Action [${action}] does not match allowed action names pattern [" + mapper.allowedActionNames.pattern() + "]!", expected.getMessage()); + assertEquals(mapper.defaultActionName, mapper.cleanupActionName(actionName)); actionName = "${${%{action}}}"; - try { - mapper.cleanupActionName(actionName); - fail(); - } catch (Throwable t) { - expected = t; - } - assertTrue(expected instanceof StrutsException); - assertEquals("Action [${${%{action}}}] does not match allowed action names pattern [" + mapper.allowedActionNames.pattern() + "]!", expected.getMessage()); + assertEquals(mapper.defaultActionName, mapper.cleanupActionName(actionName)); actionName = "${#foo='action',#foo}"; - try { - mapper.cleanupActionName(actionName); - fail(); - } catch (Throwable t) { - expected = t; - } - assertTrue(expected instanceof StrutsException); - assertEquals("Action [${#foo='action',#foo}] does not match allowed action names pattern [" + mapper.allowedActionNames.pattern() + "]!", expected.getMessage()); + assertEquals(mapper.defaultActionName, mapper.cleanupActionName(actionName)); actionName = "test-action"; assertEquals("test-action", mapper.cleanupActionName(actionName)); @@ -887,4 +864,19 @@ public class DefaultActionMapperTest extends StrutsInternalTestCase { assertEquals("test!bar.action", mapper.cleanupActionName(actionName)); } + public void testAllowedMethodNames() throws Exception { + DefaultActionMapper mapper = new DefaultActionMapper(); + + assertEquals("", mapper.cleanupMethodName("")); + assertEquals("test", mapper.cleanupMethodName("test")); + assertEquals("test_method", mapper.cleanupMethodName("test_method")); + assertEquals("_test", mapper.cleanupMethodName("_test")); + assertEquals("test1", mapper.cleanupMethodName("test1")); + + assertEquals(mapper.defaultMethodName, mapper.cleanupMethodName("2test")); + assertEquals(mapper.defaultMethodName, mapper.cleanupMethodName("%{exp}")); + assertEquals(mapper.defaultMethodName, mapper.cleanupMethodName("${%{foo}}")); + assertEquals(mapper.defaultMethodName, mapper.cleanupMethodName("${#foo='method',#foo}")); + } + } http://git-wip-us.apache.org/repos/asf/struts/blob/5acc71f7/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java ---------------------------------------------------------------------- diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java b/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java index 678a6f3..a56c8e1 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java @@ -121,6 +121,7 @@ public class RestActionMapper extends DefaultActionMapper { private boolean allowDynamicMethodCalls = false; public RestActionMapper() { + this.defaultMethodName = indexMethodName; } public String getIdParameterName() { @@ -299,7 +300,7 @@ public class RestActionMapper extends DefaultActionMapper { fullName = fullName.substring(0, lastSlashPos); } - mapping.setName(fullName); + mapping.setName(cleanupActionName(fullName)); } return mapping; } @@ -320,7 +321,7 @@ public class RestActionMapper extends DefaultActionMapper { mapping.setName(actionName); if (allowDynamicMethodCalls) { - mapping.setMethod(cleanupActionName(actionMethod)); + mapping.setMethod(cleanupMethodName(actionMethod)); } else { mapping.setMethod(null); }
