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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
