You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2022/09/30 06:12:43 UTC
[struts] 01/01: WW-5184 Improves logging around excluding/accepting values of incoming parameters
This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch WW-5184-log
in repository https://gitbox.apache.org/repos/asf/struts.git
commit cbfd3a7ab94b1fda96536a88eee56a1e9e63fc64
Author: Lukasz Lenart <lu...@apache.org>
AuthorDate: Wed Sep 28 11:35:12 2022 +0200
WW-5184 Improves logging around excluding/accepting values of incoming parameters
---
.../xwork2/interceptor/ParametersInterceptor.java | 189 ++++++++++++---------
.../interceptor/ParametersInterceptorTest.java | 33 ++--
2 files changed, 120 insertions(+), 102 deletions(-)
diff --git a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
index ff962c895..73f650442 100644
--- a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
+++ b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
@@ -26,6 +26,7 @@ import com.opensymphony.xwork2.security.AcceptedPatternsChecker;
import com.opensymphony.xwork2.security.ExcludedPatternsChecker;
import com.opensymphony.xwork2.util.ClearableValueStack;
import com.opensymphony.xwork2.util.MemberAccessValueStack;
+import com.opensymphony.xwork2.util.TextParseUtil;
import com.opensymphony.xwork2.util.ValueStack;
import com.opensymphony.xwork2.util.ValueStackFactory;
import com.opensymphony.xwork2.util.reflection.ReflectionContextState;
@@ -33,8 +34,8 @@ import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
-import org.apache.struts2.dispatcher.Parameter;
import org.apache.struts2.dispatcher.HttpParameters;
+import org.apache.struts2.dispatcher.Parameter;
import java.util.Collection;
import java.util.Collections;
@@ -44,7 +45,6 @@ import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.regex.Pattern;
-import com.opensymphony.xwork2.util.TextParseUtil;
/**
* This interceptor sets all parameters on the value stack.
@@ -69,7 +69,6 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
private Set<Pattern> excludedValuePatterns = null;
private Set<Pattern> acceptedValuePatterns = null;
-
@Inject
public void setValueStackFactory(ValueStackFactory valueStackFactory) {
this.valueStackFactory = valueStackFactory;
@@ -246,8 +245,8 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
if (action instanceof TextProvider) {
TextProvider tp = (TextProvider) action;
developerNotification = tp.getText("devmode.notification",
- "Developer Notification:\n{0}",
- new String[]{developerNotification}
+ "Developer Notification:\n{0}",
+ new String[]{developerNotification}
);
}
@@ -281,13 +280,13 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
* @return true if parameter is accepted
*/
protected boolean isAcceptableParameterValue(Parameter param, Object action) {
- ParameterValueAware parameterValueAware = (action instanceof ParameterValueAware) ? (ParameterValueAware) action : null;
- boolean acceptableParmValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue()));
- if(hasParamValuesToExclude() || hasParamValuesToAccept()) {
- // Additional validations to process
- acceptableParmValue &= acceptableValue(param.getName(), param.getValue());
- }
- return acceptableParmValue;
+ ParameterValueAware parameterValueAware = (action instanceof ParameterValueAware) ? (ParameterValueAware) action : null;
+ boolean acceptableParamValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue()));
+ if (hasParamValuesToExclude() || hasParamValuesToAccept()) {
+ // Additional validations to process
+ acceptableParamValue &= acceptableValue(param.getName(), param.getValue());
+ }
+ return acceptableParamValue;
}
/**
@@ -352,7 +351,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
* * Value is not excluded
* * Value is accepted
*
- * @param name - Param name (for logging)
+ * @param name - Param name (for logging)
* @param value - value to check
* @return true if accepted
*/
@@ -374,9 +373,9 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
if (!matchLength) {
if (devMode) { // warn only when in devMode
LOG.warn("Parameter [{}] is too long, allowed length is [{}]. Use Interceptor Parameter Overriding " +
- "to override the limit, see more at\n" +
- "https://struts.apache.org/core-developers/interceptors.html#interceptor-parameter-overriding",
- name, paramNameMaxLength);
+ "to override the limit, see more at\n" +
+ "https://struts.apache.org/core-developers/interceptors.html#interceptor-parameter-overriding",
+ name, paramNameMaxLength);
} else {
LOG.warn("Parameter [{}] is too long, allowed length is [{}]", name, paramNameMaxLength);
}
@@ -390,8 +389,8 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
return true;
} else if (devMode) { // warn only when in devMode
LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
- "https://struts.apache.org/security/#accepted--excluded-patterns",
- paramName, result.getAcceptedPattern());
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getAcceptedPattern());
} else {
LOG.debug("Parameter [{}] didn't match accepted pattern [{}]!", paramName, result.getAcceptedPattern());
}
@@ -403,8 +402,8 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
if (result.isExcluded()) {
if (devMode) { // warn only when in devMode
LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
- "https://struts.apache.org/security/#accepted--excluded-patterns",
- paramName, result.getExcludedPattern());
+ "https://struts.apache.org/security/#accepted--excluded-patterns",
+ paramName, result.getExcludedPattern());
} else {
LOG.debug("Parameter [{}] matches excluded pattern [{}]!", paramName, result.getExcludedPattern());
}
@@ -413,83 +412,55 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
return false;
}
-
- public void setAcceptedValuePatterns(String commaDelimitedPatterns) {
- Set<String> patterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns);
- if (acceptedValuePatterns == null) {
- // Limit unwanted log entries (for 1st call, acceptedValuePatterns null)
- LOG.debug("Sets accepted value patterns to [{}], note this may impact the safety of your application!", patterns);
- } else {
- LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and may impact the safety of your application!",
- acceptedValuePatterns, patterns);
- }
- acceptedValuePatterns = new HashSet<>(patterns.size());
- try {
- for (String pattern : patterns) {
- acceptedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
+ protected boolean isParamValueExcluded(String value) {
+ if (hasParamValuesToExclude()) {
+ for (Pattern excludedPattern : excludedValuePatterns) {
+ if (value != null) {
+ if (excludedPattern.matcher(value).matches()) {
+ if (devMode) {
+ LOG.warn("Parameter value [{}] matches excluded pattern [{}]! See Accepting/Excluding parameter values at\n" +
+ "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values",
+ value, excludedValuePatterns);
+ } else {
+ LOG.debug("Parameter value [{}] matches excluded pattern [{}]", value, excludedPattern);
+ }
+ return true;
+ }
+ }
}
- } finally {
- acceptedValuePatterns = Collections.unmodifiableSet(acceptedValuePatterns);
}
+ return false;
}
- public void setExcludeValuePatterns(String commaDelimitedPatterns) {
- Set<String> patterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns);
- if (excludedValuePatterns == null) {
- // Limit unwanted log entries (for 1st call, excludedValuePatterns null)
- LOG.debug("Setting excluded value patterns to [{}]", patterns);
+ protected boolean isParamValueAccepted(String value) {
+ if (hasParamValuesToAccept()) {
+ for (Pattern excludedPattern : acceptedValuePatterns) {
+ if (value != null) {
+ if (excludedPattern.matcher(value).matches()) {
+ return true;
+ }
+ }
+ }
} else {
- LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and may impact safety of your application!",
- excludedValuePatterns, patterns);
+ // acceptedValuePatterns not defined so anything is allowed
+ return true;
}
- excludedValuePatterns = new HashSet<>(patterns.size());
- try {
- for (String pattern : patterns) {
- excludedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
- }
- } finally {
- excludedValuePatterns = Collections.unmodifiableSet(excludedValuePatterns);
+ if (devMode) {
+ LOG.warn("Parameter value [{}] didn't match accepted pattern [{}]! See Accepting/Excluding parameter values at\n" +
+ "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values",
+ value, acceptedValuePatterns);
+ } else {
+ LOG.debug("Parameter value [{}] was not accepted!", value);
}
+ return false;
}
- protected boolean isParamValueExcluded(String value) {
- if (hasParamValuesToExclude()) {
- for (Pattern excludedPattern : excludedValuePatterns) {
- if (value != null) {
- if (excludedPattern.matcher(value).matches()) {
- LOG.warn("Parameter value [{}] matches excluded pattern [{}] and will be dropped.", value,
- excludedPattern);
- return true;
- }
- }
- }
- }
- return false;
- }
-
- protected boolean isParamValueAccepted(String value) {
- if (hasParamValuesToAccept()) {
- for (Pattern excludedPattern : acceptedValuePatterns) {
- if (value != null) {
- if (excludedPattern.matcher(value).matches()) {
- return true;
- }
- }
- }
- } else {
- // acceptedValuePatterns not defined so anything is allowed
- return true;
- }
- LOG.warn("Parameter value [{}] did not match any acceptedValuePattern pattern and will be dropped.", value);
- return false;
- }
-
private boolean hasParamValuesToExclude() {
- return excludedValuePatterns != null && excludedValuePatterns.size() > 0;
+ return excludedValuePatterns != null && excludedValuePatterns.size() > 0;
}
private boolean hasParamValuesToAccept() {
- return acceptedValuePatterns != null && acceptedValuePatterns.size() > 0;
+ return acceptedValuePatterns != null && acceptedValuePatterns.size() > 0;
}
/**
@@ -533,4 +504,54 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
public void setExcludeParams(String commaDelim) {
excludedPatterns.setExcludedPatterns(commaDelim);
}
+
+ /**
+ * Sets a comma-delimited list of regular expressions to match
+ * values of parameters that should be accepted and included in the parameter map.
+ *
+ * @param commaDelimitedPatterns A comma-delimited set of regular expressions
+ */
+ public void setAcceptedValuePatterns(String commaDelimitedPatterns) {
+ Set<String> patterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns);
+ if (acceptedValuePatterns == null) {
+ // Limit unwanted log entries (for 1st call, acceptedValuePatterns null)
+ LOG.debug("Sets accepted value patterns to [{}], note this may impact the safety of your application!", patterns);
+ } else {
+ LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this may impact safety of your application!",
+ acceptedValuePatterns, patterns);
+ }
+ acceptedValuePatterns = new HashSet<>(patterns.size());
+ try {
+ for (String pattern : patterns) {
+ acceptedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
+ }
+ } finally {
+ acceptedValuePatterns = Collections.unmodifiableSet(acceptedValuePatterns);
+ }
+ }
+
+ /**
+ * Sets a comma-delimited list of regular expressions to match
+ * values of parameters that should be removed from the parameter map.
+ *
+ * @param commaDelimitedPatterns A comma-delimited set of regular expressions
+ */
+ public void setExcludedValuePatterns(String commaDelimitedPatterns) {
+ Set<String> patterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns);
+ if (excludedValuePatterns == null) {
+ // Limit unwanted log entries (for 1st call, excludedValuePatterns null)
+ LOG.debug("Setting excluded value patterns to [{}]", patterns);
+ } else {
+ LOG.warn("Replacing excluded value patterns [{}] with [{}], be aware that this may impact safety of your application!",
+ excludedValuePatterns, patterns);
+ }
+ excludedValuePatterns = new HashSet<>(patterns.size());
+ try {
+ for (String pattern : patterns) {
+ excludedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
+ }
+ } finally {
+ excludedValuePatterns = Collections.unmodifiableSet(excludedValuePatterns);
+ }
+ }
}
diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
index ce8fe8498..245e868ad 100644
--- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
@@ -34,7 +34,6 @@ import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
import com.opensymphony.xwork2.mock.MockActionInvocation;
import com.opensymphony.xwork2.ognl.OgnlValueStack;
import com.opensymphony.xwork2.ognl.OgnlValueStackFactory;
-import com.opensymphony.xwork2.ognl.SecurityMemberAccess;
import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.ValueStack;
@@ -45,11 +44,9 @@ import ognl.PropertyAccessor;
import org.apache.struts2.config.StrutsXmlConfigurationProvider;
import org.apache.struts2.dispatcher.HttpParameters;
import org.junit.Assert;
-import org.springframework.ejb.access.SimpleRemoteStatelessSessionProxyFactoryBean;
import java.io.File;
import java.lang.reflect.Field;
-import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -781,10 +778,10 @@ public class ParametersInterceptorTest extends XWorkTestCase {
public void testExcludedParametersValuesAreIgnored() throws Exception {
ParametersInterceptor pi = createParametersInterceptor();
// Contains (based on pattern)
- pi.setExcludeValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*");
-
+ pi.setExcludedValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*");
+
assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}"));
-
+
final Map<String, Object> actual = injectValueStackFactory(pi);
ValueStack stack = injectValueStack(actual);
@@ -812,14 +809,14 @@ public class ParametersInterceptorTest extends XWorkTestCase {
pi.setParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build());
assertEquals(expected, actual);
}
-
+
public void testAcceptedParametersValuesAreIgnored() throws Exception {
ParametersInterceptor pi = createParametersInterceptor();
// Starts with (based on pattern)
pi.setAcceptedValuePatterns("^\\$\\{foo\\}.*,^%\\{bar\\}.*,^fooValue");
assertTrue("${foo} was allowed by isParamValueAccepted", pi.isParamValueAccepted("${foo}"));
-
+
final Map<String, Object> actual = injectValueStackFactory(pi);
ValueStack stack = injectValueStack(actual);
@@ -849,16 +846,16 @@ public class ParametersInterceptorTest extends XWorkTestCase {
pi.setParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build());
assertEquals(expected, actual);
}
-
+
public void testAcceptedAndExcludedParametersValuesAreIgnored() throws Exception {
ParametersInterceptor pi = createParametersInterceptor();
// Starts with (based on pattern)
pi.setAcceptedValuePatterns("^\\$\\{foo\\}.*,^%\\{bar\\}.*,^fooValue");
- pi.setExcludeValuePatterns(".*\\$\\{2.*2\\}.*,.*\\%\\{2.*2\\}.*");
-
+ pi.setExcludedValuePatterns(".*\\$\\{2.*2\\}.*,.*\\%\\{2.*2\\}.*");
+
assertTrue("${foo} was allowed by isParamValueAccepted", pi.isParamValueAccepted("${foo}"));
assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}"));
-
+
final Map<String, Object> actual = injectValueStackFactory(pi);
ValueStack stack = injectValueStack(actual);
@@ -878,7 +875,7 @@ public class ParametersInterceptorTest extends XWorkTestCase {
put("barKey%", "%{2+2}");
put("barKey2%", "foo%{2+2}");
put("barKey3", "nothing");
-
+
put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
put("fooKey", "fooValue");
@@ -888,14 +885,14 @@ public class ParametersInterceptorTest extends XWorkTestCase {
pi.setParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build());
assertEquals(expected, actual);
}
-
+
public void testExcludedParametersValuesAreIgnoredWithParameterValueAware() throws Exception {
ParametersInterceptor pi = createParametersInterceptor();
// Contains (based on pattern)
- pi.setExcludeValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*");
-
+ pi.setExcludedValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*");
+
assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}"));
-
+
final Map<String, Object> actual = injectValueStackFactory(pi);
ValueStack stack = injectValueStack(actual);
@@ -905,7 +902,7 @@ public class ParametersInterceptorTest extends XWorkTestCase {
put("fooKey", "fooValue");
}
};
-
+
Object a = new ParameterValueAware() {
@Override
public boolean acceptableParameterValue(String parameterValue) {