Skip to content

Commit

Permalink
Merge pull request #559 from brianandle/WW-5184_v2
Browse files Browse the repository at this point in the history 
WW-5184 - Add optional parameter value check to ParametersInterceptor
  • Loading branch information
@lukaszlenart
lukaszlenart authored Sep 14, 2022
2 parents 8545729 + 584634a commit a589972
Show file tree
Hide file tree
Showing 3 changed files with 327 additions and 3 deletions.
37 changes: 37 additions & 0 deletions core/src/main/java/com/opensymphony/xwork2/interceptor/ParameterValueAware.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package com.opensymphony.xwork2.interceptor;

/**
* This interface is implemented by actions that want to declare acceptable parameter value. Works in conjunction with {@link
* ParametersInterceptor}. For example, actions may want to create a white list of parameter values they will accept or a
* blacklist of parameter values they will reject to prevent clients from setting other unexpected (and possibly dangerous)
* parameter values.
*/
public interface ParameterValueAware {

/**
* Tests if the the action will accept the parameter with the given value.
*
* @param parameterValue the parameter value
* @return <tt>true</tt> if accepted, <tt>false</tt> otherwise
*/
boolean acceptableParameterValue(String parameterValue);

}
137 changes: 134 additions & 3 deletions core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,14 @@
import org.apache.struts2.dispatcher.HttpParameters;

import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
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.
Expand All @@ -62,6 +66,9 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
private ValueStackFactory valueStackFactory;
private ExcludedPatternsChecker excludedPatterns;
private AcceptedPatternsChecker acceptedPatterns;
private Set<Pattern> excludedValuePatterns = null;
private Set<Pattern> acceptedValuePatterns = null;


@Inject
public void setValueStackFactory(ValueStackFactory valueStackFactory) {
Expand Down Expand Up @@ -183,8 +190,10 @@ protected void setParameters(final Object action, ValueStack stack, HttpParamete

for (Map.Entry<String, Parameter> entry : params.entrySet()) {
String parameterName = entry.getKey();
boolean isAcceptableParameter = isAcceptableParameter(parameterName, action);
isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action);

if (isAcceptableParameter(parameterName, action)) {
if (isAcceptableParameter) {
acceptableParameters.put(parameterName, entry.getValue());
}
}
Expand Down Expand Up @@ -263,6 +272,23 @@ protected boolean isAcceptableParameter(String name, Object action) {
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
*
* @param param the parameter
* @param action current 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;
}

/**
* Gets an instance of the comparator to use for the ordered sorting. Override this
Expand Down Expand Up @@ -290,7 +316,16 @@ protected String getParameterLogMap(HttpParameters parameters) {

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
*/
protected boolean acceptableName(String name) {
if (isIgnoredDMI(name)) {
LOG.trace("DMI is enabled, ignoring DMI method: {}", name);
Expand All @@ -311,6 +346,24 @@ private boolean isIgnoredDMI(String name) {
}
}

/**
* Validates:
* * Value is null/blank
* * Value is not excluded
* * Value is accepted
*
* @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)));
if (!accepted) {
LOG.warn("Parameter [{}] was not accepted with value [{}] and will be dropped!", name, value);
}
return accepted;
}

protected boolean isWithinLengthLimit(String name) {
boolean matchLength = name.length() <= paramNameMaxLength;
if (!matchLength) {
Expand Down Expand Up @@ -354,7 +407,86 @@ protected boolean isExcluded(String paramName) {
}
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));
}
} finally {
acceptedValuePatterns = Collections.unmodifiableSet(acceptedValuePatterns);
}
}

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;
}

private boolean hasParamValuesToAccept() {
return acceptedValuePatterns != null && acceptedValuePatterns.size() > 0;
}

/**
* Whether to order the parameters or not
*
Expand Down Expand Up @@ -396,5 +528,4 @@ public void setAcceptParamNames(String commaDelim) {
public void setExcludeParams(String commaDelim) {
excludedPatterns.setExcludedPatterns(commaDelim);
}

}
156 changes: 156 additions & 0 deletions core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,162 @@ public void testBeanListSingleValue() throws Exception {
assertFalse(action.getBeanList().isEmpty());
}

public void testExcludedParametersValuesAreIgnored() throws Exception {
ParametersInterceptor pi = createParametersInterceptor();
// Contains (based on pattern)
pi.setExcludeValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*");

assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}"));

final Map<String, Object> actual = injectValueStackFactory(pi);
ValueStack stack = injectValueStack(actual);

final Map<String, Object> expected = new HashMap<String, Object>() {
{
put("fooKey", "fooValue");
put("fooKey2", "");
}
};

Map<String, Object> parameters = new HashMap<String, Object>() {
{
put("barKey$", "${2+2}");
put("barKey2$", "foo${2+2}");
put("barKey3$", "foo${2+2}foo");
put("barKey%", "%{2+2}");
put("barKey2%", "foo%{2+2}");
put("barKey3%", "foo%{2+2}foo");
put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
put("fooKey", "fooValue");
put("fooKey2", "");
}
};
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);

final Map<String, Object> expected = new HashMap<String, Object>() {
{
put("fooKey", "fooValue");
put("fooKey2", "");
put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
}
};

Map<String, Object> parameters = new HashMap<String, Object>() {
{
put("barKey$", "${2+2}");
put("barKey2$", "foo${2+2}");
put("barKey3$", "foo${2+2}foo");
put("barKey%", "%{2+2}");
put("barKey2%", "foo%{2+2}");
put("barKey3%", "foo%{2+2}foo");
put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
put("fooKey", "fooValue");
put("fooKey2", "");
}
};
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\\}.*");

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);

final Map<String, Object> expected = new HashMap<String, Object>() {
{
put("fooKey", "fooValue");
put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
put("fooKey2", "");
}
};

Map<String, Object> parameters = new HashMap<String, Object>() {
{
put("barKey$", "${2+2}");
put("barKey2$", "foo${2+2}");
put("barKey%", "%{2+2}");
put("barKey2%", "foo%{2+2}");
put("barKey3", "nothing");

put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
put("fooKey", "fooValue");
put("fooKey2", "");
}
};
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(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*");

assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}"));

final Map<String, Object> actual = injectValueStackFactory(pi);
ValueStack stack = injectValueStack(actual);

final Map<String, Object> expected = new HashMap<String, Object>() {
{
// acceptableParameterValue only allows fooValue even though fooKey2 and fooKey3 pass the excludeValuePatterns check
put("fooKey", "fooValue");
}
};

Object a = new ParameterValueAware() {
@Override
public boolean acceptableParameterValue(String parameterValue) {
// Only fooValue will be allowed because the excludeValuePatterns will block ${2+2}
return parameterValue.equals("fooValue") || parameterValue.equals("${2+2}");
}
};

Map<String, Object> parameters = new HashMap<String, Object>() {
{
put("barKey$", "${2+2}");
put("barKey2$", "foo${2+2}");
put("barKey3$", "foo${2+2}foo");
put("barKey%", "%{2+2}");
put("barKey2%", "foo%{2+2}");
put("barKey3%", "foo%{2+2}foo");
put("allowedKey", "${foo}");
put("allowedKey2", "%{bar}");
put("fooKey", "fooValue");
put("fooKey2", "fooValue2");
put("fooKey3", "");
}
};
pi.setParameters(a, stack, HttpParameters.create(parameters).build());
assertEquals(expected, actual);
}


private ValueStack injectValueStack(Map<String, Object> actual) {
ValueStack stack = createStubValueStack(actual);
container.inject(stack);
Expand Down

0 comments on commit a589972

Please sign in to comment.