You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by Lukasz Lenart <lu...@apache.org> on 2016/07/29 08:11:24 UTC
Fwd: struts git commit: WW-4669 Returns default action/method instead
of throwing exception
Hi,
I have changed logic for cleaning up action/method names - instead of
throwing exception a default action/method name will be returned,
wdyt?
I must updated docs but will wait for your opinion.
Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
---------- Forwarded message ----------
From: <lu...@apache.org>
Date: 2016-07-29 10:09 GMT+02:00
Subject: struts git commit: WW-4669 Returns default action/method
instead of throwing exception
To: commits@struts.apache.org
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 <lu...@apache.org>
Authored: Fri Jul 29 10:09:24 2016 +0200
Committer: Lukasz Lenart <lu...@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);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Fwd: struts git commit: WW-4669 Returns default action/method instead
of throwing exception
Posted by Christoph Nenning <Ch...@lex-com.net>.
> Hi,
>
> I have changed logic for cleaning up action/method names - instead of
> throwing exception a default action/method name will be returned,
> wdyt?
> I must updated docs but will wait for your opinion.
>
>
I'm fine with either way.
Regards,
Christoph
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
>
> ---------- Forwarded message ----------
> From: <lu...@apache.org>
> Date: 2016-07-29 10:09 GMT+02:00
> Subject: struts git commit: WW-4669 Returns default action/method
> instead of throwing exception
> To: commits@struts.apache.org
>
>
> 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 <lu...@apache.org>
> Authored: Fri Jul 29 10:09:24 2016 +0200
> Committer: Lukasz Lenart <lu...@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);
> }
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
This Email was scanned by Sophos Anti Virus
Re: struts git commit: WW-4669 Returns default action/method instead
of throwing exception
Posted by Lukasz Lenart <lu...@apache.org>.
2016-07-29 10:53 GMT+02:00 Greg Huber <gr...@gmail.com>:
> Its handy to know these errors, and get a different a response depending on
> dev mode
>
> With dev mode off get a 404 (same message), rather than a
> Struts Problem Report
>
> Struts has detected an unhandled exception:
> There is no Action mapped for namespace [/admin] and action name
> [messagesDeletez] associated with context path [/myContext].
>
> maybe : if dev mode is ON leave as is. If OFF call the default? (which I
> guess would be execute?)
It will still work, this new functionality is for action that doesn't
match [a-zA-Z0-9._!/\\-]* (default RegEx) - e.g. /my%{#action} or
/${action} - so it most cases it's all about security.
Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: struts git commit: WW-4669 Returns default action/method instead
of throwing exception
Posted by Greg Huber <gr...@gmail.com>.
Its handy to know these errors, and get a different a response depending on
dev mode
With dev mode off get a 404 (same message), rather than a
Struts Problem Report
Struts has detected an unhandled exception:
There is no Action mapped for namespace [/admin] and action name
[messagesDeletez] associated with context path [/myContext].
maybe : if dev mode is ON leave as is. If OFF call the default? (which I
guess would be execute?)
On 29 July 2016 at 09:34, Lukasz Lenart <lu...@apache.org> wrote:
> 2016-07-29 10:28 GMT+02:00 Greg Huber <gr...@gmail.com>:
> > Have you an example of what the default action/name would actually be?
> >
> >
> > <action name="messageDelete!*" method="{1}" class="admin.MessageDelete">
> > <result name="input" type="tiles">.MessageDelete</result>
> > <result name="confirm" type="tiles">.MessageDelete</result>
> > <result name="success" type="chain">messages</result>
> > <result name="cancel" type="redirectAction">
> > <param name="actionName">messages</param>
> > </result>
> > <result name="error" type="chain">messages</result>
> > </action>
>
> It's hardcoded as "index" by default but can be redefined in
> struts.xml with "struts.default.action.name" constant, for method it's
> "execute" and "struts.defult.method.name" - it's all about mapping so
> away before configuration plays its role.
>
> It should be rather treated as <default-action-ref/> when someone
> requested an action that doesn't match allowed.action.names
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
Re: struts git commit: WW-4669 Returns default action/method instead
of throwing exception
Posted by Lukasz Lenart <lu...@apache.org>.
2016-07-29 10:28 GMT+02:00 Greg Huber <gr...@gmail.com>:
> Have you an example of what the default action/name would actually be?
>
>
> <action name="messageDelete!*" method="{1}" class="admin.MessageDelete">
> <result name="input" type="tiles">.MessageDelete</result>
> <result name="confirm" type="tiles">.MessageDelete</result>
> <result name="success" type="chain">messages</result>
> <result name="cancel" type="redirectAction">
> <param name="actionName">messages</param>
> </result>
> <result name="error" type="chain">messages</result>
> </action>
It's hardcoded as "index" by default but can be redefined in
struts.xml with "struts.default.action.name" constant, for method it's
"execute" and "struts.defult.method.name" - it's all about mapping so
away before configuration plays its role.
It should be rather treated as <default-action-ref/> when someone
requested an action that doesn't match allowed.action.names
Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: struts git commit: WW-4669 Returns default action/method instead
of throwing exception
Posted by Greg Huber <gr...@gmail.com>.
Have you an example of what the default action/name would actually be?
<action name="messageDelete!*" method="{1}" class="admin.MessageDelete">
<result name="input" type="tiles">.MessageDelete</result>
<result name="confirm" type="tiles">.MessageDelete</result>
<result name="success" type="chain">messages</result>
<result name="cancel" type="redirectAction">
<param name="actionName">messages</param>
</result>
<result name="error" type="chain">messages</result>
</action>
On 29 July 2016 at 09:11, Lukasz Lenart <lu...@apache.org> wrote:
> Hi,
>
> I have changed logic for cleaning up action/method names - instead of
> throwing exception a default action/method name will be returned,
> wdyt?
> I must updated docs but will wait for your opinion.
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
>
> ---------- Forwarded message ----------
> From: <lu...@apache.org>
> Date: 2016-07-29 10:09 GMT+02:00
> Subject: struts git commit: WW-4669 Returns default action/method
> instead of throwing exception
> To: commits@struts.apache.org
>
>
> 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 <lu...@apache.org>
> Authored: Fri Jul 29 10:09:24 2016 +0200
> Committer: Lukasz Lenart <lu...@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);
> }
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>