You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@struts.apache.org by GitBox <gi...@apache.org> on 2022/06/03 10:01:42 UTC
[GitHub] [struts] yasserzamani commented on a diff in pull request #557: WW-5184 - Add optional parameter value check to ParametersInterceptor
yasserzamani commented on code in PR #557:
URL: https://github.com/apache/struts/pull/557#discussion_r888793387
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -58,6 +64,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;
Review Comment:
I couldn't remember exactly but can't we do exactly like two lines above it? i.e. ExcludedPatternsChecker and AcceptedPatternsChecker
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -335,7 +392,85 @@ 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, acceptedPatterns null)
+ LOG.debug("Sets accepted value patterns to [{}], note this impacts the safety of your application!", patterns);
+ } else {
+ LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and 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, acceptedPatterns null)
+ LOG.debug("Setting excluded value patterns to [{}]", patterns);
+ } else {
+ LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and 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(excludedValuePatterns != null) {
+ for (Pattern excludedPattern : excludedValuePatterns) {
+ if(value != null) {
+ if (excludedPattern.matcher(value).matches()) {
+ LOG.trace("[{}] matches excluded pattern [{}]", value, excludedPattern);
+ return true;
+ }
+ }
+ }
+ }
+ return false;
+ }
+ protected boolean isParamValueAccepted(String value) {
+ if(acceptedValuePatterns != null) {
+ for (Pattern excludedPattern : acceptedValuePatterns) {
+ if(value != null) {
+ if (excludedPattern.matcher(value).matches()) {
+ LOG.trace("[{}] matches excluded pattern [{}]", value, excludedPattern);
+ return true;
+ }
+ }
+ }
+ } else {
+ // acceptedValuePatterns not defined so anything is allowed
+ return true;
+ }
+ return false;
Review Comment:
same here. log the important thing that it's not accepted please.
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -335,7 +392,85 @@ 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, acceptedPatterns null)
+ LOG.debug("Sets accepted value patterns to [{}], note this impacts the safety of your application!", patterns);
+ } else {
+ LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and 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, acceptedPatterns null)
+ LOG.debug("Setting excluded value patterns to [{}]", patterns);
+ } else {
+ LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and 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(excludedValuePatterns != null) {
+ for (Pattern excludedPattern : excludedValuePatterns) {
+ if(value != null) {
+ if (excludedPattern.matcher(value).matches()) {
+ LOG.trace("[{}] matches excluded pattern [{}]", value, excludedPattern);
Review Comment:
It's an important thing that it's excluded. I think log with INFO level.
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -292,6 +327,28 @@ protected boolean acceptableName(String name) {
return accepted;
}
+ /**
+ * Validates:
+ * * Name is within the max length of a parameter name
+ * * Name is not excluded
+ * * Name is accepted
+ * * Value is null/blank
+ * * Value is not excluded
+ * * Value is accepted
+ *
+ * @param name - Name to check
+ * @param value - value to check
+ * @return true if accepted
+ */
+ protected boolean acceptableNameValue(String name, String value) {
+ boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) && isAccepted(name)
+ && (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value)));
+ if (devMode && accepted) { // notify only when in devMode
+ LOG.debug("Parameter [{}] was accepted with value [{}] and will be appended to action!", name, value);
Review Comment:
In devMode we log with INFO level. And I think just log when it's not accepted. No one interested or surprised with accepted params :)
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -177,8 +186,13 @@ protected void setParameters(final Object action, ValueStack stack, HttpParamete
for (Map.Entry<String, Parameter> entry : params.entrySet()) {
String parameterName = entry.getKey();
-
- if (isAcceptableParameter(parameterName, action)) {
+ boolean isAcceptableParameter;
+ if(hasParamValuesToExclude() || hasParamValuesToAccept()) {
Review Comment:
I think you should keep previous check (parameter name check) and add the new check i.e. isAcceptableParameter &= isAcceptableParameterValue(.... With this if statement, you've made it mutually exclusive and looks like in first statement you've checked both nameValue right? If so then not required. Parameter name is already checked. Just add Value check and finally &= its result as mentioned above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org