Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 14:47:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #607 from apache/WW-5184-log

Browse Source 
[WW-5184] Uses debug log level when parameter value was not accepted
This commit is contained in:
Lukasz Lenart
2022-09-30 11:21:34 +02:00
committed by GitHub
parent 04cf1dae5f e0da03c262
commit 58f287bf4a
4 changed files with 138 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Jenkinsfile
Vendored
+2 -2
View File
@@ -79,7 +79,7 @@ pipeline {
}
stage('Test') {
steps {
sh './mvnw -B test'
sh './mvnw -B verify -Pcoverage -DskipAssembly'
}
post {
always {
@@ -94,7 +94,7 @@ pipeline {
}
steps {
withCredentials([string(credentialsId: 'asf-struts-sonarcloud', variable: 'SONARCLOUD_TOKEN')]) {
sh './mvnw sonar:sonar -DskipAssembly -Dsonar.login=${SONARCLOUD_TOKEN}'
sh './mvnw -B -Pcoverage -DskipAssembly -Dsonar.login=${SONARCLOUD_TOKEN} sonar:sonar'
}
}
}
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
+120 -97
View File
@@ -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}
);
}
@@ -272,7 +271,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
ParameterNameAware parameterNameAware = (action instanceof ParameterNameAware) ? (ParameterNameAware) action : null;
return acceptableName(name) && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name));
}
/**
* Checks if parameter value can be accepted or thrown away
*
@@ -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;
}
/**
@@ -316,13 +315,13 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
return logEntry.toString();
}
/**
* Validates the name passed is:
* * Within the max length of a parameter name
* * Is not excluded
* * Is accepted
*
*
* @param name - Name to check
* @return true if accepted
*/
@@ -351,27 +350,32 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
* * Value is null/blank
* * 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
*/
protected boolean acceptableValue(String name, String value) {
boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value)));
boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value)));
if (!accepted) {
LOG.warn("Parameter [{}] was not accepted with value [{}] and will be dropped!", name, value);
String message = "Value [{}] of parameter [{}] was not accepted and will be dropped!";
if (devMode) {
LOG.warn(message, value, name);
} else {
LOG.debug(message, value, name);
}
}
return accepted;
}
protected boolean isWithinLengthLimit(String name) {
boolean matchLength = name.length() <= paramNameMaxLength;
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);
}
@@ -385,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());
}
@@ -398,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());
}
@@ -407,86 +411,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);
protected boolean isParamValueExcluded(String value) {
if (!hasParamValuesToExclude()) {
LOG.debug("'excludedValuePatterns' not defined so anything is allowed");
return false;
}
acceptedValuePatterns = new HashSet<>(patterns.size());
try {
for (String pattern : patterns) {
acceptedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
for (Pattern excludedValuePattern : excludedValuePatterns) {
if (excludedValuePattern.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, excludedValuePattern);
}
return true;
}
} finally {
acceptedValuePatterns = Collections.unmodifiableSet(acceptedValuePatterns);
}
return false;
}
protected boolean isParamValueAccepted(String value) {
if (!hasParamValuesToAccept()) {
LOG.debug("'acceptedValuePatterns' not defined so anything is allowed");
return true;
}
for (Pattern acceptedValuePattern : acceptedValuePatterns) {
if (acceptedValuePattern.matcher(value).matches()) {
return true;
}
}
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;
}
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);
} else {
LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and 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);
}
}
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;
}
/**
* Whether to order the parameters or not
*
@@ -528,4 +501,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);
}
}
}
core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
+15 -18
View File
@@ -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) {
pom.xml
+1 -3
View File
@@ -125,9 +125,9 @@
<fluido-skin.version>1.9</fluido-skin.version>
<!-- Sonar -->
<sonar.organization>apache</sonar.organization>
<sonar.projectKey>apache_struts</sonar.projectKey>
<sonar.moduleKey>${project.artifactId}</sonar.moduleKey>
<sonar.organization>apache</sonar.organization>
<sonar.host.url>https://sonarcloud.io</sonar.host.url>
<sonar.exclusions>apps/**</sonar.exclusions>
@@ -213,8 +213,6 @@
<id>coverage</id>
<properties>
<sonar.host.url>https://sonarcloud.io</sonar.host.url>
<sonar.organization>apache</sonar.organization>
<sonar.projectKey>apache_struts</sonar.projectKey>
</properties>
<build>
<plugins>