This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch struts-2-5-x in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/struts-2-5-x by this push: new 0262574 Proposed fix for WW-5024 in the 2.5.x branch: (#343) 0262574 is described below commit 02625740952b1e673402e21d52320fe41dbda500 Author: JCgH4164838Gh792C124B5 <43964333+jcgh4164838gh792c12...@users.noreply.github.com> AuthorDate: Fri Mar 29 02:20:56 2019 -0400 Proposed fix for WW-5024 in the 2.5.x branch: (#343) - NOTE: If the PR is accepted please credit Robert Hollencamp (Github @rhollencamp) for this change as he found the issue, proposed a solution for 2.6 (master), and opened the JIRA. - Updated HttpParameters, ActionMappingParametersInterceptor to prevent multi-level Parameter wrapping from occuring. - Added a new ActionMappingParametersInterceptorTest to verify the fix. --- .../apache/struts2/dispatcher/HttpParameters.java | 23 +++ .../ActionMappingParametersInterceptor.java | 16 +- .../ActionMappingParametersInterceptorTest.java | 199 +++++++++++++++++++++ 3 files changed, 230 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java b/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java index f46c40d..c01962f 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java @@ -197,5 +197,28 @@ public class HttpParameters implements Map<String, Parameter>, Cloneable { return new HttpParameters(parameters); } + + /** + * Alternate Builder method which avoids wrapping any parameters that are already + * a {@link Parameter} element within another {@link Parameter} wrapper. + * + * @return + */ + public HttpParameters buildNoNestedWrapping() { + Map<String, Parameter> parameters = (parent == null) + ? new HashMap<String, Parameter>() + : new HashMap<>(parent.parameters); + + for (Map.Entry<String, Object> entry : requestParameterMap.entrySet()) { + String name = entry.getKey(); + Object value = entry.getValue(); + Parameter parameterValue = (value instanceof Parameter) + ? (Parameter) value + : new Parameter.Request(name, value); + parameters.put(name, parameterValue); + } + + return new HttpParameters(parameters); + } } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java index 967f748..ba0f265 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java @@ -74,6 +74,8 @@ import java.util.Map; public class ActionMappingParametersInterceptor extends ParametersInterceptor { /** + * Get the parameter map from ActionMapping associated with the provided ActionContext. + * * @param ac The action context * @return the parameters from the action mapping in the context. If none found, returns an empty map. */ @@ -81,27 +83,25 @@ public class ActionMappingParametersInterceptor extends ParametersInterceptor { protected HttpParameters retrieveParameters(ActionContext ac) { ActionMapping mapping = (ActionMapping) ac.get(ServletActionContext.ACTION_MAPPING); if (mapping != null) { - return HttpParameters.create(mapping.getParams()).build(); + return HttpParameters.create(mapping.getParams()).buildNoNestedWrapping(); } else { - return HttpParameters.create().build(); + return HttpParameters.create().buildNoNestedWrapping(); } } /** - * Adds the parameters into context's ParameterMap + * Adds the parameters into the current ActionContext's parameter map. + * + * Note: The method should avoid re-wrapping values which are already of type Parameter. * * @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> */ @Override protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) { HttpParameters previousParams = ac.getParameters(); HttpParameters.Builder combinedParams = HttpParameters.create().withParent(previousParams).withExtraParams(newParams); - ac.setParameters(combinedParams.build()); + ac.setParameters(combinedParams.buildNoNestedWrapping()); } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptorTest.java new file mode 100644 index 0000000..5a102b0 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptorTest.java @@ -0,0 +1,199 @@ +/* + * 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 org.apache.struts2.interceptor; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.XWorkTestCase; + +import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.ServletActionContext; +import org.apache.struts2.dispatcher.Parameter; +import org.apache.struts2.dispatcher.mapper.ActionMapping; + +import java.util.HashMap; +import java.util.Map; + + +/** + * Unit test for {@link ActionMappingParametersInterceptor}. + * + * Adapted from ParametersInterceptor (@author Jason Carreira) + */ +public class ActionMappingParametersInterceptorTest extends XWorkTestCase { + + /** + * Confirm that ActionMappingParametersInterceptor processing methods avoid + * nested wrapping of parameters. + */ + public void testParametersNoNestedWrapping() { + final ActionMappingParametersInterceptor ampi = createActionMappingParametersInterceptor(); + final ActionContext ac = ActionContext.getContext(); + final Map<String, Object> parameters = new HashMap<String, Object>() { + { + put("fooKey", "fooValue"); + put("barKey", "barValue"); + } + }; + final Map<String, Object> moreParameters = new HashMap<String, Object>() { + { + put("fooKey2", "fooValue2"); + put("barKey2", "barValue3"); + } + }; + final Map<String, Object> evenMoreParameters = new HashMap<String, Object>() { + { + put("fooKey3", "fooValue3"); + put("barKey3", "barValue3"); + } + }; + // Set up the initial ActionMapping with parameters in the context + final HttpParameters wrappedParameters = HttpParameters.create(parameters).build(); + final ActionMapping actionMapping = new ActionMapping("testAction", "testNameSpace", "testMethod", parameters); + ac.put(ServletActionContext.ACTION_MAPPING, actionMapping); + + // Retrieve parameters and confirm both builder result equality and that the contained Object + // values of each Parameter element are not of type Parameter (i.e. no double-wrapping). + // Note: ampi.retrieveParameters() only ever returns the parameters associated with the ActionContext's + // ActionMapping (not changed directly by ampi.addParametersToContext()). + final HttpParameters retrievedParameters = ampi.retrieveParameters(ac); + assertNotNull("retrievedParameters null ?", retrievedParameters); + // Note: Cannot perform equality test on HttpParameters directly as hashCode values differ even + // when the contents are equivalent. Instead we compare size and content equality. + assertEquals("retrievedParameters size not equal to wrappedParameters size ?", + retrievedParameters.size(), wrappedParameters.size()); + + for (String parameterName : retrievedParameters.keySet() ) { + Parameter retrievedParameter = retrievedParameters.get(parameterName); + Parameter wrappedParameter = wrappedParameters.get(parameterName); + assertNotNull("retrievedParameter is null ?", retrievedParameter); + assertNotNull("wrappedParameter is null ?", wrappedParameter); + Object retrievedParameterValue = retrievedParameter.getObject(); + Object wrappedParameterValue = wrappedParameter.getObject(); + assertNotNull("retrievedParameterValue is null ?", retrievedParameterValue); + assertNotNull("wrappedParameterValue is null ?", wrappedParameterValue); + assertFalse("retrievedParameterValue is type Parameter ?", retrievedParameterValue instanceof Parameter); + assertEquals("retrievedParameterValue not equal to wrappedParameterValue ?", + retrievedParameterValue, wrappedParameterValue); + } + + // Set up the initial ActionMapping with (artificially) already-wrapped parameters in the context + final Map<String, Object> wrappedParametersMap = new HashMap<>(wrappedParameters.size()); + for (String parameterName : wrappedParameters.keySet() ) { + wrappedParametersMap.put(parameterName, wrappedParameters.get(parameterName)); + } + final ActionMapping actionMappingWrapped = new ActionMapping("testAction", "testNameSpace", "testMethod", wrappedParametersMap); + ac.put(ServletActionContext.ACTION_MAPPING, actionMappingWrapped); + + // Retrieve parameters and confirm that the contained Object values of each Parameter + // element are not of type Parameter (i.e. no double-wrapping). + // Note: ampi.retrieveParameters() only ever returns the parameters associated with the ActionContext's + // ActionMapping (not changed directly by ampi.addParametersToContext()). + final HttpParameters retrievedWrappedParameters = ampi.retrieveParameters(ac); + assertNotNull("retrievedWrappedParameters null ?", retrievedWrappedParameters); + // Note: Cannot perform equality test on HttpParameters directly as hashCode values differ even + // when the contents are equivalent. Instead we compare size and content equality. + assertEquals("retrievedWrappedParameters size not equal to wrappedParametersMap size ?", + retrievedWrappedParameters.size(), wrappedParametersMap.size()); + + for (String parameterName : retrievedWrappedParameters.keySet() ) { + Parameter retrievedParameter = retrievedWrappedParameters.get(parameterName); + Object wrappedParameter = wrappedParametersMap.get(parameterName); + assertNotNull("retrievedParameter is null ?", retrievedParameter); + assertNotNull("wrappedParameter is null ?", wrappedParameter); + assertTrue("wrappedParameter is not a Parameter ?", wrappedParameter instanceof Parameter); + Object retrievedParameterValue = retrievedParameter.getObject(); + Object wrappedParameterValue = ((Parameter) wrappedParameter).getObject(); + assertNotNull("retrievedParameterValue is null ?", retrievedParameterValue); + assertNotNull("wrappedParameterValue is null ?", wrappedParameterValue); + assertFalse("retrievedParameterValue is type Parameter ?", retrievedParameterValue instanceof Parameter); + assertEquals("retrievedParameterValue not equal to wrappedParameterValue ?", + retrievedParameterValue, wrappedParameterValue); + } + + // Add parameters to the context + ampi.addParametersToContext(ac, parameters); + + // Retrieve parameters and confirm the expected size and that the contained Object + // values of each Parameter element are not of type Parameter (i.e. no double-wrapping). + final HttpParameters retrievedACParameters = ac.getParameters(); + assertNotNull("retrievedACParameters null ?", retrievedACParameters); + assertEquals("retrievedACParameters size not equal to parameters size ?", + retrievedACParameters.size(), parameters.size()); + + for (String parameterName : retrievedACParameters.keySet() ) { + Parameter retrievedACParameter = retrievedACParameters.get(parameterName); + assertNotNull("retrievedACParameter is null ?", retrievedACParameter); + Object parameterValue = retrievedACParameter.getObject(); + assertNotNull("parameterValue is null ?", parameterValue); + assertFalse("parameterValue is of type Parameter ?", parameterValue instanceof Parameter); + } + + // Add additional parameters to the context + ampi.addParametersToContext(ac, moreParameters); + + // Retrieve parameters and confirm the expected size and that the contained Object + // values of each Parameter element are not of type Parameter (i.e. no double-wrapping). + final HttpParameters retrievedACMoreParameters = ac.getParameters(); + assertNotNull("retrievedACMoreParameters null ?", retrievedACMoreParameters); + assertEquals("retrievedACMoreParameters size not equal to combined size (parameters + moreParameters) ?", + retrievedACMoreParameters.size(), parameters.size() + moreParameters.size()); + + for (String parameterName : retrievedACMoreParameters.keySet() ) { + Parameter retrievedACMoreParameter = retrievedACMoreParameters.get(parameterName); + assertNotNull("retrievedACMoreParameter is null ?", retrievedACMoreParameter); + Object parameterValue = retrievedACMoreParameter.getObject(); + assertNotNull("parameterValue (more) is null ?", parameterValue); + assertFalse("parameterValue (more) is of type Parameter ?", parameterValue instanceof Parameter); + } + + // Build some "already wrapped" parameters and attempt to add them to the context + final HttpParameters evenMoreWrappedParameters = HttpParameters.create(evenMoreParameters).build(); + assertNotNull("evenMoreWrappedParameters null ?", evenMoreWrappedParameters); + assertEquals("evenMoreWrappedParameters size not equal to evenMoreParameters size ?", + evenMoreWrappedParameters.size(), evenMoreParameters.size()); + ampi.addParametersToContext(ac, evenMoreWrappedParameters); + + // Retrieve parameters and confirm the expected size and that the contained Object + // values of each Parameter element are not of type Parameter (i.e. no double-wrapping). + final HttpParameters retrievedACEvenMoreParameters = ac.getParameters(); + assertNotNull("retrievedACEvenMoreParameters null ?", retrievedACEvenMoreParameters); + assertEquals("retrievedACEvenMoreParameters size not equal to combined size (parameters + moreParameters + evenMoreParameters) ?", + retrievedACEvenMoreParameters.size(), parameters.size() + moreParameters.size() + evenMoreParameters.size()); + + for (String parameterName : retrievedACEvenMoreParameters.keySet() ) { + Parameter retrievedACEvenMoreParameter = retrievedACEvenMoreParameters.get(parameterName); + assertNotNull("retrievedACEvenMoreParameter is null ?", retrievedACEvenMoreParameter); + Object parameterValue = retrievedACEvenMoreParameter.getObject(); + assertNotNull("parameterValue (even more) is null ?", parameterValue); + assertFalse("parameterValue (even more) is of type Parameter ?", parameterValue instanceof Parameter); + } + } + + /** + * Create and configure an ActionMappingParametersInterceptor instance + * + * @return + */ + private ActionMappingParametersInterceptor createActionMappingParametersInterceptor() { + ActionMappingParametersInterceptor ampi = new ActionMappingParametersInterceptor(); + container.inject(ampi); + return ampi; + } + +}