diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ServletRedirectResult.java b/core/src/main/java/org/apache/struts2/dispatcher/ServletRedirectResult.java index e4347b0036..ae254635bf 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ServletRedirectResult.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/ServletRedirectResult.java @@ -204,9 +204,9 @@ protected void doExecute(String finalLocation, ActionInvocation invocation) thro List prohibitedResultParams = getProhibitedResultParams(); for (Map.Entry e : resultConfigParams.entrySet()) { if (!prohibitedResultParams.contains(e.getKey())) { - String potentialValue = e.getValue() == null ? "" : conditionalParse(e.getValue(), invocation); - if (!suppressEmptyParameters || ((potentialValue != null) && (potentialValue.length() > 0))) { - requestParameters.put(e.getKey(), potentialValue); + Collection values = conditionalParseCollection(e.getValue(), invocation, suppressEmptyParameters); + if (!suppressEmptyParameters || !values.isEmpty()) { + requestParameters.put(e.getKey(), values); } } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/StrutsResultSupport.java b/core/src/main/java/org/apache/struts2/dispatcher/StrutsResultSupport.java index 676b0b916d..269ed872e7 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/StrutsResultSupport.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/StrutsResultSupport.java @@ -23,6 +23,8 @@ import java.io.UnsupportedEncodingException; import java.net.URLEncoder; +import java.util.ArrayList; +import java.util.Collection; import org.apache.struts2.StrutsStatics; @@ -195,31 +197,64 @@ public void execute(ActionInvocation invocation) throws Exception { */ protected String conditionalParse(String param, ActionInvocation invocation) { if (parse && param != null && invocation != null) { - return TextParseUtil.translateVariables(param, invocation.getStack(), - new TextParseUtil.ParsedValueEvaluator() { - public Object evaluate(String parsedValue) { - if (encode) { - if (parsedValue != null) { - try { - // use UTF-8 as this is the recommended encoding by W3C to - // avoid incompatibilities. - return URLEncoder.encode(parsedValue, "UTF-8"); - } - catch(UnsupportedEncodingException e) { - if (LOG.isWarnEnabled()) { - LOG.warn("error while trying to encode ["+parsedValue+"]", e); - } - } - } - } - return parsedValue; - } - }); + return TextParseUtil.translateVariables( + param, + invocation.getStack(), + new EncodingParsedValueEvaluator()); } else { return param; } } + /** + * As {@link #conditionalParse(String, ActionInvocation)} but does not + * convert found object into String. If found object is a collection it is + * returned if found object is not a collection it is wrapped in one. + * + * @param param + * @param invocation + * @param excludeEmptyElements + * @return + */ + protected Collection conditionalParseCollection(String param, ActionInvocation invocation, boolean excludeEmptyElements) { + if (parse && param != null && invocation != null) { + return TextParseUtil.translateVariablesCollection( + param, + invocation.getStack(), + excludeEmptyElements, + new EncodingParsedValueEvaluator()); + } else { + Collection collection = new ArrayList(1); + collection.add(param); + return collection; + } + } + + /** + * {@link com.opensymphony.xwork2.util.TextParseUtil.ParsedValueEvaluator} to do URL encoding for found values. To be + * used for single strings or collections. + * + */ + private final class EncodingParsedValueEvaluator implements TextParseUtil.ParsedValueEvaluator { + public Object evaluate(String parsedValue) { + if (encode) { + if (parsedValue != null) { + try { + // use UTF-8 as this is the recommended encoding by W3C to + // avoid incompatibilities. + return URLEncoder.encode(parsedValue, "UTF-8"); + } + catch(UnsupportedEncodingException e) { + if (LOG.isWarnEnabled()) { + LOG.warn("error while trying to encode ["+parsedValue+"]", e); + } + } + } + } + return parsedValue; + } + } + /** * Executes the result given a final location (jsp page, action, etc) and the action invocation * (the state in which the action was executed). Subclasses must implement this class to handle diff --git a/core/src/test/java/org/apache/struts2/dispatcher/ServletRedirectResultTest.java b/core/src/test/java/org/apache/struts2/dispatcher/ServletRedirectResultTest.java index 5d9bf93467..6a9e871a3e 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/ServletRedirectResultTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/ServletRedirectResultTest.java @@ -21,16 +21,24 @@ package org.apache.struts2.dispatcher; -import com.mockobjects.dynamic.C; -import com.mockobjects.dynamic.Mock; -import com.opensymphony.xwork2.ActionContext; -import com.opensymphony.xwork2.ActionInvocation; -import com.opensymphony.xwork2.ActionProxy; -import com.opensymphony.xwork2.config.entities.ActionConfig; -import com.opensymphony.xwork2.config.entities.PackageConfig; -import com.opensymphony.xwork2.config.entities.ResultConfig; -import com.opensymphony.xwork2.mock.MockActionInvocation; +import static javax.servlet.http.HttpServletResponse.SC_SEE_OTHER; +import static org.easymock.EasyMock.createControl; +import static org.easymock.EasyMock.createNiceMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.replay; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + import ognl.Ognl; + import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.StrutsStatics; @@ -40,18 +48,16 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import java.io.PrintWriter; -import java.io.StringWriter; -import java.util.HashMap; -import java.util.Map; - -import static javax.servlet.http.HttpServletResponse.SC_SEE_OTHER; -import static org.easymock.EasyMock.createControl; -import static org.easymock.EasyMock.createNiceMock; -import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.replay; +import com.mockobjects.dynamic.C; +import com.mockobjects.dynamic.Mock; +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.ActionProxy; +import com.opensymphony.xwork2.config.entities.ActionConfig; +import com.opensymphony.xwork2.config.entities.PackageConfig; +import com.opensymphony.xwork2.config.entities.ResultConfig; +import com.opensymphony.xwork2.mock.MockActionInvocation; +import com.opensymphony.xwork2.util.ValueStack; /** @@ -225,6 +231,67 @@ public void testIncludeParameterInResult() throws Exception { control.verify(); } + public void testIncludeCollectionParameterInResult() throws Exception { + List paramValues = new ArrayList(); + paramValues.add("value 1"); + paramValues.add(""); + paramValues.add("value 2"); + paramValues.add(null); + + ResultConfig resultConfig = new ResultConfig.Builder("", "") + .addParam("namespace", "someNamespace") + .addParam("param", "${list}") + .build(); + + ActionContext context = ActionContext.getContext(); + MockHttpServletRequest req = new MockHttpServletRequest(); + MockHttpServletResponse res = new MockHttpServletResponse(); + context.put(ServletActionContext.HTTP_REQUEST, req); + context.put(ServletActionContext.HTTP_RESPONSE, res); + + Map results= new HashMap(); + results.put("myResult", resultConfig); + + ActionConfig actionConfig = new ActionConfig.Builder("", "", "") + .addResultConfigs(results).build(); + + ServletRedirectResult result = new ServletRedirectResult(); + result.setLocation("/myNamespace/myAction.action"); + result.setParse(true); + result.setEncode(false); + result.setPrependServletContext(false); + result.setUrlHelper(new DefaultUrlHelper()); + result.setSuppressEmptyParameters(true); + + IMocksControl control = createControl(); + ActionProxy mockActionProxy = control.createMock(ActionProxy.class); + ActionInvocation mockInvocation = control.createMock(ActionInvocation.class); + + ValueStack mockValueStack = control.createMock(ValueStack.class); + Map mockContext = new HashMap(); + mockContext.put(ActionContext.CONTAINER, container); + + expect(mockInvocation.getStack()).andReturn(mockValueStack); + expect(mockValueStack.getContext()).andReturn(mockContext); + + expect(mockInvocation.getStack()).andReturn(mockValueStack); + + expect(mockValueStack.findValue("list")).andReturn(paramValues); // no asType !!! + + expect(mockInvocation.getProxy()).andReturn(mockActionProxy); + expect(mockInvocation.getResultCode()).andReturn("myResult"); + expect(mockActionProxy.getConfig()).andReturn(actionConfig); + expect(mockInvocation.getInvocationContext()).andReturn(context); + + expect(mockValueStack.getContext()).andReturn(mockContext); + + control.replay(); + result.setActionMapper(container.getInstance(ActionMapper.class)); + result.execute(mockInvocation); + assertEquals("/myNamespace/myAction.action?param=value+1¶m=value+2", res.getRedirectedUrl()); + control.verify(); + } + protected void setUp() throws Exception { super.setUp(); configurationManager.getConfiguration(). diff --git a/core/src/test/java/org/apache/struts2/dispatcher/StrutsResultSupportTest.java b/core/src/test/java/org/apache/struts2/dispatcher/StrutsResultSupportTest.java index 1d3556efc9..cc4bf6c55c 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/StrutsResultSupportTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/StrutsResultSupportTest.java @@ -21,6 +21,9 @@ package org.apache.struts2.dispatcher; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; import org.apache.struts2.StrutsInternalTestCase; import org.easymock.EasyMock; @@ -109,6 +112,34 @@ public String getMyLocation() { EasyMock.verify(mockActionInvocation); } + public void testConditionalParseCollection() throws Exception { + ValueStack stack = ActionContext.getContext().getValueStack(); + stack.push(new ActionSupport() { + public List getList() { + return new ArrayList(){{ + add("val 1"); + add("val 2"); + }}; + } + }); + + ActionInvocation mockActionInvocation = EasyMock.createNiceMock(ActionInvocation.class); + mockActionInvocation.getStack(); + EasyMock.expectLastCall().andReturn(stack); + EasyMock.replay(mockActionInvocation); + + InternalStrutsResultSupport result = new InternalStrutsResultSupport(); + result.setParse(true); + result.setEncode(true); + + Collection collection = result.conditionalParseCollection("${list}", mockActionInvocation, true); + + assertNotNull(collection); + assertEquals(2, collection.size()); + assertTrue(collection.contains("val+1")); + assertTrue(collection.contains("val+2")); + EasyMock.verify(mockActionInvocation); + } public static class InternalStrutsResultSupport extends StrutsResultSupport { private String _internalLocation = null; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/util/TextParseUtil.java b/xwork-core/src/main/java/com/opensymphony/xwork2/util/TextParseUtil.java index 4b54e81e4d..a3938ef331 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/util/TextParseUtil.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/util/TextParseUtil.java @@ -16,9 +16,13 @@ package com.opensymphony.xwork2.util; import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Container; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; +import java.util.Map; import java.util.Set; @@ -167,6 +171,89 @@ public Object evaluate(String parsedValue) { return parser.evaluate(openChars, expression, ognlEval, maxLoopCount); } + /** + * @see #translateVariablesCollection(char[], String, ValueStack, boolean, ParsedValueEvaluator, int) + * + * @param expression + * @param stack + * @param excludeEmptyElements + * @param evaluator + * @return + */ + public static Collection translateVariablesCollection(String expression, ValueStack stack, boolean excludeEmptyElements, ParsedValueEvaluator evaluator) { + return translateVariablesCollection(new char[]{'$', '%'}, expression, stack, excludeEmptyElements, evaluator, MAX_RECURSION); + } + + /** + * Resolves given expression on given ValueStack. If found element is a + * collection each element will be converted to String. If just a single + * object is found it is converted to String and wrapped in a collection. + * + * @param openChars + * @param expression + * @param stack + * @param excludeEmptyElements + * @param evaluator + * @param maxLoopCount + * @return + */ + public static Collection translateVariablesCollection( + char[] openChars, String expression, final ValueStack stack, boolean excludeEmptyElements, + final ParsedValueEvaluator evaluator, int maxLoopCount) { + + ParsedValueEvaluator ognlEval = new ParsedValueEvaluator() { + public Object evaluate(String parsedValue) { + return stack.findValue(parsedValue); // no asType !!! + } + }; + + Map context = stack.getContext(); + TextParser parser = ((Container)context.get(ActionContext.CONTAINER)).getInstance(TextParser.class); + + Object result = parser.evaluate(openChars, expression, ognlEval, maxLoopCount); + + XWorkConverter conv = ((Container)context.get(ActionContext.CONTAINER)).getInstance(XWorkConverter.class); + + Collection resultCol; + if (result instanceof Collection) { + @SuppressWarnings("unchecked") + Collection casted = (Collection)result; + resultCol = new ArrayList(casted.size()); + for (Object element : casted) { + String stringElement = (String)conv.convertValue(context, element, String.class); + if (shallBeIncluded(stringElement, excludeEmptyElements)) { + if (evaluator != null) { + stringElement = evaluator.evaluate(stringElement).toString(); + } + resultCol.add(stringElement); + } + } + } else { + resultCol = new ArrayList(1); + String stringResult = (String)conv.convertValue(context, result, String.class); + if (shallBeIncluded(stringResult, excludeEmptyElements)) { + if (evaluator != null) { + stringResult = evaluator.evaluate(stringResult).toString(); + } + resultCol.add(stringResult); + } + } + + return resultCol; + } + + /** + * Tests if given string is not null and not empty when excluding of empty + * elements is requested. + * + * @param str String to check. + * @param excludeEmptyElements Whether empty elements shall be excluded. + * @return True if given string can be included in collection. + */ + private static boolean shallBeIncluded(String str, boolean excludeEmptyElements) { + return !excludeEmptyElements || ((str != null) && (str.length() > 0)); + } + /** * Returns a set from comma delimted Strings. * @param s The String to parse. diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java index 45e79709fc..a624a10d3d 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/util/TextParseUtilTest.java @@ -15,11 +15,19 @@ */ package com.opensymphony.xwork2.util; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.junit.Assert; + import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.XWorkTestCase; -import java.util.*; - /** * Unit test of {@link TextParseUtil}. * @@ -177,4 +185,37 @@ public Object evaluate(String parsedValue) { assertEquals("foo: ", s); } + public void testTranslateVariablesCollection() { + ValueStack stack = ActionContext.getContext().getValueStack(); + final List list = new ArrayList() {{ + add("val 1"); + add("val 2"); + }}; + stack.push(new HashMap() {{ put("list", list); }}); + + Collection collection = TextParseUtil.translateVariablesCollection("${list}", stack, true, null); + + Assert.assertNotNull(collection); + Assert.assertEquals(2, collection.size()); + } + + public void testTranslateVariablesCollectionWithExpressions() { + ValueStack stack = ActionContext.getContext().getValueStack(); + final List list = new ArrayList() {{ + add("${val1}"); + add("%{val2}"); + }}; + stack.push(new HashMap() {{ put("list", list); put("val1", 1); put("val2", "Value 2"); }}); + + Collection collection = TextParseUtil.translateVariablesCollection("${list}", stack, true, null); + + Assert.assertNotNull(collection); + Assert.assertEquals(2, collection.size()); + + // if this starts passing, probably an double evaluation expression vulnerability was introduced + // carefully review changes as this can affect users and allows break in intruders + Assert.assertEquals("${val1}", collection.toArray()[0]); + Assert.assertEquals("%{val2}", collection.toArray()[1]); + } + }