You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "kusalk (via GitHub)" <gi...@apache.org> on 2024/01/03 10:05:02 UTC
[PR] WW-5352 Refactor ParametersInterceptor [struts]
kusalk opened a new pull request, #831:
URL: https://github.com/apache/struts/pull/831
WW-5352
--
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #831:
URL: https://github.com/apache/struts/pull/831#issuecomment-1875147886
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[17 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=831&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=831&resolved=false&inNewCodePeriod=true)
[87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=831&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=831&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk merged PR #831:
URL: https://github.com/apache/struts/pull/831
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #831:
URL: https://github.com/apache/struts/pull/831#discussion_r1442640530
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Object action = invocation.getAction();
- if (!(action instanceof NoParameters)) {
- ActionContext ac = invocation.getInvocationContext();
- HttpParameters parameters = retrieveParameters(ac);
+ if (action instanceof NoParameters) {
+ return invocation.invoke();
+ }
- if (LOG.isDebugEnabled()) {
- LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
- }
+ ActionContext actionContext = invocation.getInvocationContext();
+ HttpParameters parameters = retrieveParameters(actionContext);
- if (parameters != null) {
- Map<String, Object> contextMap = ac.getContextMap();
- try {
- ReflectionContextState.setCreatingNullObjects(contextMap, true);
- ReflectionContextState.setDenyMethodExecution(contextMap, true);
- ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
- ValueStack stack = ac.getValueStack();
- setParameters(action, stack, parameters);
- } finally {
- ReflectionContextState.setCreatingNullObjects(contextMap, false);
- ReflectionContextState.setDenyMethodExecution(contextMap, false);
- ReflectionContextState.setReportingConversionErrors(contextMap, false);
- }
- }
+ if (parameters == null) {
+ return invocation.invoke();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
+ }
+
+ Map<String, Object> contextMap = actionContext.getContextMap();
+ batchSetReflectionContextState(contextMap, true);
+ try {
+ setParameters(action, actionContext.getValueStack(), parameters);
+ } finally {
+ batchSetReflectionContextState(contextMap, false);
}
+
return invocation.invoke();
}
/**
* Gets the parameter map to apply from wherever appropriate
*
- * @param ac The action context
+ * @param actionContext The action context
* @return The parameter map to apply
*/
- protected HttpParameters retrieveParameters(ActionContext ac) {
- return ac.getParameters();
+ protected HttpParameters retrieveParameters(ActionContext actionContext) {
+ return actionContext.getParameters();
}
/**
* Adds the parameters into context's ParameterMap
+ * <p>
+ * In this class this is a no-op, since the parameters were fetched from the same location. In subclasses both this
+ * and {@link #retrieveParameters} should be overridden.
*
* @param ac The action context
* @param newParams The parameter map to apply
- * <p>
- * In this class this is a no-op, since the parameters were fetched from the same location.
- * In subclasses both retrieveParameters() and addParametersToContext() should be overridden.
- * </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) {
}
protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) {
- HttpParameters params;
- Map<String, Parameter> acceptableParameters;
- if (ordered) {
- params = HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
- acceptableParameters = new TreeMap<>(getOrderedComparator());
- } else {
- params = HttpParameters.create().withParent(parameters).build();
- acceptableParameters = new TreeMap<>();
- }
+ Map<String, Parameter> acceptableParameters = toAcceptableParameters(parameters, action);
- for (Map.Entry<String, Parameter> entry : params.entrySet()) {
- String parameterName = entry.getKey();
- boolean isAcceptableParameter = isAcceptableParameter(parameterName, action);
- isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action);
+ ValueStack newStack = toNewStack(stack);
+ batchSetReflectionContextState(newStack.getContext(), true);
+ setMemberAccessProperties(newStack);
- if (isAcceptableParameter) {
- acceptableParameters.put(parameterName, entry.getValue());
- }
+ setParametersOnStack(newStack, acceptableParameters, action);
+
+ if (newStack instanceof ClearableValueStack) {
+ stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
}
+ addParametersToContext(ActionContext.getContext(), acceptableParameters);
+ }
+
+ protected void batchSetReflectionContextState(Map<String, Object> context, boolean value) {
+ ReflectionContextState.setCreatingNullObjects(context, value);
+ ReflectionContextState.setDenyMethodExecution(context, value);
+ ReflectionContextState.setReportingConversionErrors(context, value);
+ }
+
+ protected ValueStack toNewStack(ValueStack stack) {
ValueStack newStack = valueStackFactory.createValueStack(stack);
- boolean clearableStack = newStack instanceof ClearableValueStack;
- if (clearableStack) {
- //if the stack's context can be cleared, do that to prevent OGNL
- //from having access to objects in the stack, see XW-641
+ if (newStack instanceof ClearableValueStack) {
((ClearableValueStack) newStack).clearContextValues();
- Map<String, Object> context = newStack.getContext();
- ReflectionContextState.setCreatingNullObjects(context, true);
- ReflectionContextState.setDenyMethodExecution(context, true);
- ReflectionContextState.setReportingConversionErrors(context, true);
-
- //keep locale from original context
newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack);
}
+ return newStack;
+ }
+
+ protected void setMemberAccessProperties(ValueStack stack) {
Review Comment:
I'll rename all of these :) They are `protected` so shouldn't be accessible from expressions but I agree with the premise
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #831:
URL: https://github.com/apache/struts/pull/831#issuecomment-1875163105
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[17 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=831&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=831&resolved=false&inNewCodePeriod=true)
[87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=831&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=831&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #831:
URL: https://github.com/apache/struts/pull/831#issuecomment-1878876455
@lukaszlenart Any idea how to configure a second base branch for SonarCloud dedicated to 7.x so we don't get wonky results?
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #831:
URL: https://github.com/apache/struts/pull/831#discussion_r1441161370
##########
core/src/main/java/org/apache/struts2/dispatcher/Parameter.java:
##########
@@ -58,7 +58,7 @@ public String getName() {
@Override
public String getValue() {
String[] values = toStringArray();
- return (values != null && values.length > 0) ? values[0] : null;
Review Comment:
`#toStringArray` never returns a null value
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Review Comment:
Used early returns to reduce nesting and improve readability
##########
core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java:
##########
@@ -32,37 +32,37 @@ public interface AcceptedPatternsChecker {
* @param value to check
* @return object containing result of matched pattern and pattern itself
*/
- public IsAccepted isAccepted(String value);
Review Comment:
Redundant in interfaces
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Object action = invocation.getAction();
- if (!(action instanceof NoParameters)) {
- ActionContext ac = invocation.getInvocationContext();
- HttpParameters parameters = retrieveParameters(ac);
+ if (action instanceof NoParameters) {
+ return invocation.invoke();
+ }
- if (LOG.isDebugEnabled()) {
- LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
- }
+ ActionContext actionContext = invocation.getInvocationContext();
+ HttpParameters parameters = retrieveParameters(actionContext);
- if (parameters != null) {
- Map<String, Object> contextMap = ac.getContextMap();
- try {
- ReflectionContextState.setCreatingNullObjects(contextMap, true);
- ReflectionContextState.setDenyMethodExecution(contextMap, true);
- ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
- ValueStack stack = ac.getValueStack();
- setParameters(action, stack, parameters);
- } finally {
- ReflectionContextState.setCreatingNullObjects(contextMap, false);
- ReflectionContextState.setDenyMethodExecution(contextMap, false);
- ReflectionContextState.setReportingConversionErrors(contextMap, false);
- }
- }
+ if (parameters == null) {
+ return invocation.invoke();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
+ }
+
+ Map<String, Object> contextMap = actionContext.getContextMap();
+ batchSetReflectionContextState(contextMap, true);
+ try {
+ setParameters(action, actionContext.getValueStack(), parameters);
+ } finally {
+ batchSetReflectionContextState(contextMap, false);
}
+
return invocation.invoke();
}
/**
* Gets the parameter map to apply from wherever appropriate
*
- * @param ac The action context
+ * @param actionContext The action context
* @return The parameter map to apply
*/
- protected HttpParameters retrieveParameters(ActionContext ac) {
- return ac.getParameters();
+ protected HttpParameters retrieveParameters(ActionContext actionContext) {
+ return actionContext.getParameters();
}
/**
* Adds the parameters into context's ParameterMap
+ * <p>
+ * In this class this is a no-op, since the parameters were fetched from the same location. In subclasses both this
+ * and {@link #retrieveParameters} should be overridden.
*
* @param ac The action context
* @param newParams The parameter map to apply
- * <p>
- * In this class this is a no-op, since the parameters were fetched from the same location.
- * In subclasses both retrieveParameters() and addParametersToContext() should be overridden.
- * </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) {
}
protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) {
Review Comment:
Extracted a bunch of helper methods to make this method much more readable
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -338,18 +344,17 @@ protected boolean acceptableName(String name) {
return false;
}
boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) && isAccepted(name);
- if (devMode && accepted) { // notify only when in devMode
Review Comment:
Cleaned out comments for behaviours which are fairly obvious from code
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -287,13 +302,11 @@ 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 acceptableParamValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue()));
- if (hasParamValuesToExclude() || hasParamValuesToAccept()) {
- // Additional validations to process
- acceptableParamValue &= acceptableValue(param.getName(), param.getValue());
Review Comment:
`&=` doesn't short-circuit
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #831:
URL: https://github.com/apache/struts/pull/831#discussion_r1442537671
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Object action = invocation.getAction();
- if (!(action instanceof NoParameters)) {
- ActionContext ac = invocation.getInvocationContext();
- HttpParameters parameters = retrieveParameters(ac);
+ if (action instanceof NoParameters) {
+ return invocation.invoke();
+ }
- if (LOG.isDebugEnabled()) {
- LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
- }
+ ActionContext actionContext = invocation.getInvocationContext();
+ HttpParameters parameters = retrieveParameters(actionContext);
- if (parameters != null) {
- Map<String, Object> contextMap = ac.getContextMap();
- try {
- ReflectionContextState.setCreatingNullObjects(contextMap, true);
- ReflectionContextState.setDenyMethodExecution(contextMap, true);
- ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
- ValueStack stack = ac.getValueStack();
- setParameters(action, stack, parameters);
- } finally {
- ReflectionContextState.setCreatingNullObjects(contextMap, false);
- ReflectionContextState.setDenyMethodExecution(contextMap, false);
- ReflectionContextState.setReportingConversionErrors(contextMap, false);
- }
- }
+ if (parameters == null) {
+ return invocation.invoke();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
+ }
+
+ Map<String, Object> contextMap = actionContext.getContextMap();
+ batchSetReflectionContextState(contextMap, true);
+ try {
+ setParameters(action, actionContext.getValueStack(), parameters);
+ } finally {
+ batchSetReflectionContextState(contextMap, false);
}
+
return invocation.invoke();
}
/**
* Gets the parameter map to apply from wherever appropriate
*
- * @param ac The action context
+ * @param actionContext The action context
* @return The parameter map to apply
*/
- protected HttpParameters retrieveParameters(ActionContext ac) {
- return ac.getParameters();
+ protected HttpParameters retrieveParameters(ActionContext actionContext) {
+ return actionContext.getParameters();
}
/**
* Adds the parameters into context's ParameterMap
+ * <p>
+ * In this class this is a no-op, since the parameters were fetched from the same location. In subclasses both this
+ * and {@link #retrieveParameters} should be overridden.
*
* @param ac The action context
* @param newParams The parameter map to apply
- * <p>
- * In this class this is a no-op, since the parameters were fetched from the same location.
- * In subclasses both retrieveParameters() and addParametersToContext() should be overridden.
- * </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) {
}
protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) {
- HttpParameters params;
- Map<String, Parameter> acceptableParameters;
- if (ordered) {
- params = HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
- acceptableParameters = new TreeMap<>(getOrderedComparator());
- } else {
- params = HttpParameters.create().withParent(parameters).build();
- acceptableParameters = new TreeMap<>();
- }
+ Map<String, Parameter> acceptableParameters = toAcceptableParameters(parameters, action);
- for (Map.Entry<String, Parameter> entry : params.entrySet()) {
- String parameterName = entry.getKey();
- boolean isAcceptableParameter = isAcceptableParameter(parameterName, action);
- isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action);
+ ValueStack newStack = toNewStack(stack);
+ batchSetReflectionContextState(newStack.getContext(), true);
+ setMemberAccessProperties(newStack);
- if (isAcceptableParameter) {
- acceptableParameters.put(parameterName, entry.getValue());
- }
+ setParametersOnStack(newStack, acceptableParameters, action);
+
+ if (newStack instanceof ClearableValueStack) {
+ stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
}
+ addParametersToContext(ActionContext.getContext(), acceptableParameters);
+ }
+
+ protected void batchSetReflectionContextState(Map<String, Object> context, boolean value) {
+ ReflectionContextState.setCreatingNullObjects(context, value);
+ ReflectionContextState.setDenyMethodExecution(context, value);
+ ReflectionContextState.setReportingConversionErrors(context, value);
+ }
+
+ protected ValueStack toNewStack(ValueStack stack) {
ValueStack newStack = valueStackFactory.createValueStack(stack);
- boolean clearableStack = newStack instanceof ClearableValueStack;
- if (clearableStack) {
- //if the stack's context can be cleared, do that to prevent OGNL
- //from having access to objects in the stack, see XW-641
+ if (newStack instanceof ClearableValueStack) {
((ClearableValueStack) newStack).clearContextValues();
- Map<String, Object> context = newStack.getContext();
- ReflectionContextState.setCreatingNullObjects(context, true);
- ReflectionContextState.setDenyMethodExecution(context, true);
- ReflectionContextState.setReportingConversionErrors(context, true);
-
- //keep locale from original context
newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack);
}
+ return newStack;
+ }
+
+ protected void setMemberAccessProperties(ValueStack stack) {
Review Comment:
I would avoid using _setter/getter_ pattern if this isn't a setter _per se_ - this also reduces a risk of using such method in expression evaluation. What about using `applyMemberAccessProperties()` instead?
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Object action = invocation.getAction();
- if (!(action instanceof NoParameters)) {
- ActionContext ac = invocation.getInvocationContext();
- HttpParameters parameters = retrieveParameters(ac);
+ if (action instanceof NoParameters) {
+ return invocation.invoke();
+ }
- if (LOG.isDebugEnabled()) {
- LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
- }
+ ActionContext actionContext = invocation.getInvocationContext();
+ HttpParameters parameters = retrieveParameters(actionContext);
- if (parameters != null) {
- Map<String, Object> contextMap = ac.getContextMap();
- try {
- ReflectionContextState.setCreatingNullObjects(contextMap, true);
- ReflectionContextState.setDenyMethodExecution(contextMap, true);
- ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
- ValueStack stack = ac.getValueStack();
- setParameters(action, stack, parameters);
- } finally {
- ReflectionContextState.setCreatingNullObjects(contextMap, false);
- ReflectionContextState.setDenyMethodExecution(contextMap, false);
- ReflectionContextState.setReportingConversionErrors(contextMap, false);
- }
- }
+ if (parameters == null) {
+ return invocation.invoke();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
+ }
+
+ Map<String, Object> contextMap = actionContext.getContextMap();
+ batchSetReflectionContextState(contextMap, true);
+ try {
+ setParameters(action, actionContext.getValueStack(), parameters);
+ } finally {
+ batchSetReflectionContextState(contextMap, false);
}
+
return invocation.invoke();
}
/**
* Gets the parameter map to apply from wherever appropriate
*
- * @param ac The action context
+ * @param actionContext The action context
* @return The parameter map to apply
*/
- protected HttpParameters retrieveParameters(ActionContext ac) {
- return ac.getParameters();
+ protected HttpParameters retrieveParameters(ActionContext actionContext) {
+ return actionContext.getParameters();
}
/**
* Adds the parameters into context's ParameterMap
+ * <p>
+ * In this class this is a no-op, since the parameters were fetched from the same location. In subclasses both this
+ * and {@link #retrieveParameters} should be overridden.
*
* @param ac The action context
* @param newParams The parameter map to apply
- * <p>
- * In this class this is a no-op, since the parameters were fetched from the same location.
- * In subclasses both retrieveParameters() and addParametersToContext() should be overridden.
- * </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) {
}
protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) {
- HttpParameters params;
- Map<String, Parameter> acceptableParameters;
- if (ordered) {
- params = HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
- acceptableParameters = new TreeMap<>(getOrderedComparator());
- } else {
- params = HttpParameters.create().withParent(parameters).build();
- acceptableParameters = new TreeMap<>();
- }
+ Map<String, Parameter> acceptableParameters = toAcceptableParameters(parameters, action);
- for (Map.Entry<String, Parameter> entry : params.entrySet()) {
- String parameterName = entry.getKey();
- boolean isAcceptableParameter = isAcceptableParameter(parameterName, action);
- isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action);
+ ValueStack newStack = toNewStack(stack);
+ batchSetReflectionContextState(newStack.getContext(), true);
+ setMemberAccessProperties(newStack);
- if (isAcceptableParameter) {
- acceptableParameters.put(parameterName, entry.getValue());
- }
+ setParametersOnStack(newStack, acceptableParameters, action);
+
+ if (newStack instanceof ClearableValueStack) {
+ stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
}
+ addParametersToContext(ActionContext.getContext(), acceptableParameters);
+ }
+
+ protected void batchSetReflectionContextState(Map<String, Object> context, boolean value) {
+ ReflectionContextState.setCreatingNullObjects(context, value);
+ ReflectionContextState.setDenyMethodExecution(context, value);
+ ReflectionContextState.setReportingConversionErrors(context, value);
+ }
+
+ protected ValueStack toNewStack(ValueStack stack) {
ValueStack newStack = valueStackFactory.createValueStack(stack);
- boolean clearableStack = newStack instanceof ClearableValueStack;
- if (clearableStack) {
- //if the stack's context can be cleared, do that to prevent OGNL
- //from having access to objects in the stack, see XW-641
+ if (newStack instanceof ClearableValueStack) {
((ClearableValueStack) newStack).clearContextValues();
- Map<String, Object> context = newStack.getContext();
- ReflectionContextState.setCreatingNullObjects(context, true);
- ReflectionContextState.setDenyMethodExecution(context, true);
- ReflectionContextState.setReportingConversionErrors(context, true);
-
- //keep locale from original context
newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack);
}
+ return newStack;
+ }
+
+ protected void setMemberAccessProperties(ValueStack stack) {
+ if (!(stack instanceof MemberAccessValueStack)) {
+ return;
+ }
+ ((MemberAccessValueStack) stack).useAcceptProperties(acceptedPatterns.getAcceptedPatterns());
+ ((MemberAccessValueStack) stack).useExcludeProperties(excludedPatterns.getExcludedPatterns());
+ }
+
+ protected Map<String, Parameter> toAcceptableParameters(HttpParameters parameters, Object action) {
+ HttpParameters newParams = initNewHttpParameters(parameters);
+ Map<String, Parameter> acceptableParameters = initParameterMap();
+
+ for (Map.Entry<String, Parameter> entry : newParams.entrySet()) {
+ String parameterName = entry.getKey();
+ Parameter parameterValue = entry.getValue();
+ if (isAcceptableParameter(parameterName, action) && isAcceptableParameterValue(parameterValue, action)) {
+ acceptableParameters.put(parameterName, parameterValue);
+ }
+ }
+ return acceptableParameters;
+ }
- boolean memberAccessStack = newStack instanceof MemberAccessValueStack;
- if (memberAccessStack) {
- //block or allow access to properties
- //see WW-2761 for more details
- MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack;
- accessValueStack.useAcceptProperties(acceptedPatterns.getAcceptedPatterns());
- accessValueStack.useExcludeProperties(excludedPatterns.getExcludedPatterns());
+ protected Map<String, Parameter> initParameterMap() {
+ if (ordered) {
+ return new TreeMap<>(getOrderedComparator());
+ } else {
+ return new TreeMap<>();
}
+ }
+
+ protected HttpParameters initNewHttpParameters(HttpParameters parameters) {
+ if (ordered) {
+ return HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
+ } else {
+ return HttpParameters.create().withParent(parameters).build();
+ }
+ }
- for (Map.Entry<String, Parameter> entry : acceptableParameters.entrySet()) {
- String name = entry.getKey();
- Parameter value = entry.getValue();
+ protected void setParametersOnStack(ValueStack stack, Map<String, Parameter> parameters, Object action) {
Review Comment:
`applyParametersOnStack`?
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5352 Refactor ParametersInterceptor [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #831:
URL: https://github.com/apache/struts/pull/831#issuecomment-1878836292
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831) **Quality Gate failed**
Failed conditions
[10 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=831&resolved=false&inNewCodePeriod=true)
[28.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=831&metric=new_coverage&view=list) (required ≥ 80%)
[4.1% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=831&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)
[E Security Rating on New Code](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831) (required ≥ A)
[E Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=831)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org