Skip to content

WW-5184 - Add optional parameter value check to ParametersInterceptor #559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukaszlenart merged 3 commits into from
Sep 14, 2022
Merged

WW-5184 - Add optional parameter value check to ParametersInterceptor #559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
105b22f
WW-5184 - Add optional parameter value check to ParametersInterceptor
brianandle Jun 5, 2022
5763476
WW-5184 - Change info to warn from peer review
brianandle Jun 6, 2022
584634a
WW-5184 - Added ParameterValueAware interface and unit test
brianandle Jun 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions core/src/main/java/com/opensymphony/xwork2/interceptor/ParameterValueAware.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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