From cbfd3a7ab94b1fda96536a88eee56a1e9e63fc64 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 28 Sep 2022 11:35:12 +0200 Subject: [PATCH] WW-5184 Improves logging around excluding/accepting values of incoming parameters --- .../interceptor/ParametersInterceptor.java | 189 ++++++++++-------- .../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 ff962c8959..73f6504429 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.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.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.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 excludedValuePatterns = null; private Set acceptedValuePatterns = null; - @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { this.valueStackFactory = valueStackFactory; @@ -246,8 +245,8 @@ protected void notifyDeveloperParameterException(Object action, String property, 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 @@ protected boolean isAcceptableParameter(String name, Object action) { * @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 @@ private boolean isIgnoredDMI(String name) { * * 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 @@ protected boolean isWithinLengthLimit(String name) { 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 @@ protected boolean isAccepted(String paramName) { 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 @@ protected boolean isExcluded(String paramName) { 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 @@ protected boolean isExcluded(String paramName) { return false; } - - public void setAcceptedValuePatterns(String commaDelimitedPatterns) { - Set 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 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 void setAcceptParamNames(String commaDelim) { 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 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 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 ce8fe8498c..245e868ad0 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.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 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 void testBeanListSingleValue() throws Exception { 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 actual = injectValueStackFactory(pi); ValueStack stack = injectValueStack(actual); @@ -812,14 +809,14 @@ public void testExcludedParametersValuesAreIgnored() throws Exception { 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 actual = injectValueStackFactory(pi); ValueStack stack = injectValueStack(actual); @@ -849,16 +846,16 @@ public void testAcceptedParametersValuesAreIgnored() throws Exception { 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 actual = injectValueStackFactory(pi); ValueStack stack = injectValueStack(actual); @@ -878,7 +875,7 @@ public void testAcceptedAndExcludedParametersValuesAreIgnored() throws Exception 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 void testAcceptedAndExcludedParametersValuesAreIgnored() throws Exception 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 actual = injectValueStackFactory(pi); ValueStack stack = injectValueStack(actual); @@ -905,7 +902,7 @@ public void testExcludedParametersValuesAreIgnoredWithParameterValueAware() thro put("fooKey", "fooValue"); } }; - + Object a = new ParameterValueAware() { @Override public boolean acceptableParameterValue(String parameterValue) {