Author: mcucchiara
Date: Fri Sep 2 12:54:49 2011
New Revision: 1164516
URL: http://svn.apache.org/viewvc?rev=1164516&view=rev
Log:
Security patch applied
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/resources/template/simple/text.ftl
struts/struts2/branches/STRUTS_2_2_3_1/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
URL:
http://svn.apache.org/viewvc/struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java?rev=1164516&r1=1164515&r2=1164516&view=diff
==============================================================================
---
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
(original)
+++
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
Fri Sep 2 12:54:49 2011
@@ -80,7 +80,7 @@ public class StrutsConversionErrorInterc
try {
stack.push(value);
- return "'" + stack.findValue("top", String.class) + "'";
+ return escape(stack.findString("top"));
} finally {
stack.pop();
}
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/resources/template/simple/text.ftl
URL:
http://svn.apache.org/viewvc/struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/resources/template/simple/text.ftl?rev=1164516&r1=1164515&r2=1164516&view=diff
==============================================================================
---
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/resources/template/simple/text.ftl
(original)
+++
struts/struts2/branches/STRUTS_2_2_3_1/core/src/main/resources/template/simple/text.ftl
Fri Sep 2 12:54:49 2011
@@ -29,7 +29,7 @@
maxlength="${parameters.maxlength?html}"<#rt/>
</#if>
<#if parameters.nameValue??>
- value="<@s.property value="parameters.nameValue"/>"<#rt/>
+ value="${parameters.nameValue?html}"<#rt/>
</#if>
<#if parameters.disabled?default(false)>
disabled="disabled"<#rt/>
@@ -50,4 +50,4 @@
<#include "/${parameters.templateDir}/simple/scripting-events.ftl" />
<#include "/${parameters.templateDir}/simple/common-attributes.ftl" />
<#include "/${parameters.templateDir}/simple/dynamic-attributes.ftl" />
-/>
\ No newline at end of file
+/>
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
URL:
http://svn.apache.org/viewvc/struts/struts2/branches/STRUTS_2_2_3_1/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java?rev=1164516&r1=1164515&r2=1164516&view=diff
==============================================================================
---
struts/struts2/branches/STRUTS_2_2_3_1/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
(original)
+++
struts/struts2/branches/STRUTS_2_2_3_1/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
Fri Sep 2 12:54:49 2011
@@ -21,11 +21,6 @@
package org.apache.struts2.interceptor;
-import java.util.HashMap;
-import java.util.Map;
-
-import org.apache.struts2.StrutsTestCase;
-
import com.mockobjects.dynamic.C;
import com.mockobjects.dynamic.Mock;
import com.opensymphony.xwork2.Action;
@@ -33,7 +28,10 @@ import com.opensymphony.xwork2.ActionCon
import com.opensymphony.xwork2.ActionInvocation;
import com.opensymphony.xwork2.ActionSupport;
import com.opensymphony.xwork2.util.ValueStack;
-import com.opensymphony.xwork2.util.ValueStackFactory;
+import org.apache.struts2.StrutsTestCase;
+
+import java.util.HashMap;
+import java.util.Map;
/**
@@ -44,14 +42,14 @@ public class StrutsConversionErrorInterc
protected ActionContext context;
protected ActionInvocation invocation;
- protected Map conversionErrors;
+ protected Map<String, Object> conversionErrors;
protected Mock mockInvocation;
protected ValueStack stack;
protected StrutsConversionErrorInterceptor interceptor;
public void testEmptyValuesDoNotSetFieldErrors() throws Exception {
- conversionErrors.put("foo", new Long(123));
+ conversionErrors.put("foo", 123L);
conversionErrors.put("bar", "");
conversionErrors.put("baz", new String[]{""});
@@ -70,7 +68,7 @@ public class StrutsConversionErrorInterc
}
public void testFieldErrorAdded() throws Exception {
- conversionErrors.put("foo", new Long(123));
+ conversionErrors.put("foo", 123L);
ActionSupport action = new ActionSupport();
mockInvocation.expectAndReturn("getAction", action);
@@ -89,7 +87,7 @@ public class StrutsConversionErrorInterc
invocation = (ActionInvocation) mockInvocation.proxy();
stack = ActionContext.getContext().getValueStack();
context = new ActionContext(stack.getContext());
- conversionErrors = new HashMap();
+ conversionErrors = new HashMap<String, Object>();
context.setConversionErrors(conversionErrors);
mockInvocation.matchAndReturn("getInvocationContext", context);
mockInvocation.expectAndReturn("invoke", Action.SUCCESS);
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
URL:
http://svn.apache.org/viewvc/struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java?rev=1164516&r1=1164515&r2=1164516&view=diff
==============================================================================
---
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
(original)
+++
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
Fri Sep 2 12:54:49 2011
@@ -20,6 +20,7 @@ import com.opensymphony.xwork2.ActionInv
import com.opensymphony.xwork2.ValidationAware;
import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
import com.opensymphony.xwork2.util.ValueStack;
+import org.apache.commons.lang.StringEscapeUtils;
import java.util.HashMap;
import java.util.Map;
@@ -84,7 +85,11 @@ public class ConversionErrorInterceptor
public static final String ORIGINAL_PROPERTY_OVERRIDE =
"original.property.override";
protected Object getOverrideExpr(ActionInvocation invocation, Object
value) {
- return "'" + value + "'";
+ return escape(value);
+ }
+
+ protected String escape(Object value) {
+ return "\"" + StringEscapeUtils.escapeJava(String.valueOf(value)) +
"\"";
}
@Override
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
URL:
http://svn.apache.org/viewvc/struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java?rev=1164516&r1=1164515&r2=1164516&view=diff
==============================================================================
---
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
(original)
+++
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
Fri Sep 2 12:54:49 2011
@@ -22,58 +22,59 @@ import com.opensymphony.xwork2.util.Valu
import com.opensymphony.xwork2.util.logging.Logger;
import com.opensymphony.xwork2.util.logging.LoggerFactory;
import com.opensymphony.xwork2.validator.ValidationException;
+import org.apache.commons.lang.StringEscapeUtils;
import java.util.LinkedHashMap;
import java.util.Map;
/**
- *
- *
+ *
+ *
* An abstract base class that adds in the capability to populate the stack
with
* a fake parameter map when a conversion error has occurred and the
'repopulateField'
* property is set to "true".
- *
+ *
* <p/>
- *
+ *
*
* <!-- START SNIPPET: javadoc -->
*
- * The capability of auto-repopulating the stack with a fake parameter map
when
- * a conversion error has occurred can be done with 'repopulateField' property
- * set to "true".
+ * The capability of auto-repopulating the stack with a fake parameter map when
+ * a conversion error has occurred can be done with 'repopulateField' property
+ * set to "true".
*
* <p/>
*
- * This is typically usefull when one wants to repopulate the field with the
original value
- * when a conversion error occurred. Eg. with a textfield that only allows an
Integer
+ * This is typically usefull when one wants to repopulate the field with the
original value
+ * when a conversion error occurred. Eg. with a textfield that only allows an
Integer
* (the action class have an Integer field declared), upon conversion error,
the incorrectly
* entered integer (maybe a text 'one') will not appear when dispatched back.
With 'repopulateField'
- * porperty set to true, it will, meaning the textfield will have 'one' as its
value
+ * porperty set to true, it will, meaning the textfield will have 'one' as its
value
* upon conversion error.
- *
+ *
* <!-- END SNIPPET: javadoc -->
- *
+ *
* <p/>
- *
+ *
* <pre>
* <!-- START SNIPPET: exampleJspPage -->
- *
+ *
* <!-- myJspPage.jsp -->
* <ww:form action="someAction" method="POST">
* ....
- * <ww:textfield
+ * <ww:textfield
* label="My Integer Field"
* name="myIntegerField" />
* ....
- * <ww:submit />
+ * <ww:submit />
* </ww:form>
- *
+ *
* <!-- END SNIPPET: exampleJspPage -->
* </pre>
- *
+ *
* <pre>
* <!-- START SNIPPET: exampleXwork -->
- *
+ *
* <!-- xwork.xml -->
* <xwork>
* <include file="xwork-default.xml" />
@@ -88,31 +89,31 @@ import java.util.Map;
* </package>
* ....
* </xwork>
- *
+ *
* <!-- END SNIPPET:exampleXwork -->
* </pre>
- *
- *
+ *
+ *
* <pre>
* <!-- START SNIPPET: exampleJava -->
- *
+ *
* <!-- MyActionSupport.java -->
* public class MyActionSupport extends ActionSupport {
* private Integer myIntegerField;
- *
+ *
* public Integer getMyIntegerField() { return this.myIntegerField; }
- * public void setMyIntegerField(Integer myIntegerField) {
- * this.myIntegerField = myIntegerField;
+ * public void setMyIntegerField(Integer myIntegerField) {
+ * this.myIntegerField = myIntegerField;
* }
* }
- *
+ *
* <!-- END SNIPPET: exampleJava -->
* </pre>
- *
- *
+ *
+ *
* <pre>
* <!-- START SNIPPET: exampleValidation -->
- *
+ *
* <!-- MyActionSupport-someAction-validation.xml -->
* <validators>
* ...
@@ -124,24 +125,24 @@ import java.util.Map;
* </field>
* ...
* </validators>
- *
+ *
* <!-- END SNIPPET: exampleValidation -->
* </pre>
- *
+ *
* @author tm_jee
* @version $Date$ $Id$
*/
public abstract class RepopulateConversionErrorFieldValidatorSupport extends
FieldValidatorSupport {
-
+
private static final Logger LOG =
LoggerFactory.getLogger(RepopulateConversionErrorFieldValidatorSupport.class);
-
+
private String repopulateFieldAsString = "false";
private boolean repopulateFieldAsBoolean = false;
-
- public String getRepopulateField() {
+
+ public String getRepopulateField() {
return repopulateFieldAsString;
}
-
+
public void setRepopulateField(String repopulateField) {
this.repopulateFieldAsString = repopulateField == null ?
repopulateField : repopulateField.trim();
this.repopulateFieldAsBoolean =
"true".equalsIgnoreCase(this.repopulateFieldAsString) ? (true) : (false);
@@ -153,12 +154,12 @@ public abstract class RepopulateConversi
repopulateField(object);
}
}
-
+
public void repopulateField(Object object) throws ValidationException {
-
+
ActionInvocation invocation =
ActionContext.getContext().getActionInvocation();
Map<String, Object> conversionErrors =
ActionContext.getContext().getConversionErrors();
-
+
String fieldName = getFieldName();
String fullFieldName =
getValidatorContext().getFullFieldName(fieldName);
if (conversionErrors.containsKey(fullFieldName)) {
@@ -170,19 +171,23 @@ public abstract class RepopulateConversi
if (value instanceof String[]) {
// take the first element, if possible
String[] tmpValue = (String[]) value;
- if (tmpValue != null && (tmpValue.length > 0)) {
+ if ((tmpValue.length > 0)) {
doExprOverride = true;
- fakeParams.put(fullFieldName, "'" + tmpValue[0] + "'");
+ fakeParams.put(fullFieldName, escape(tmpValue[0]));
} else {
- LOG.warn("value is an empty array of String or with first
element in it as null [" + value + "], will not repopulate conversion error ");
+ if (LOG.isWarnEnabled()) {
+ LOG.warn("value is an empty array of String or with
first element in it as null [" + value + "], will not repopulate conversion
error ");
+ }
}
} else if (value instanceof String) {
String tmpValue = (String) value;
doExprOverride = true;
- fakeParams.put(fullFieldName, "'" + tmpValue + "'");
+ fakeParams.put(fullFieldName, escape(tmpValue));
} else {
// opps... it should be
- LOG.warn("conversion error value is not a String or array of
String but instead is [" + value + "], will not repopulate conversion error");
+ if (LOG.isWarnEnabled()) {
+ LOG.warn("conversion error value is not a String or array
of String but instead is [" + value + "], will not repopulate conversion
error");
+ }
}
if (doExprOverride) {
@@ -194,7 +199,11 @@ public abstract class RepopulateConversi
});
}
}
- }
-
- protected abstract void doValidate(Object object) throws
ValidationException;
-}
+ }
+
+ protected String escape(String value) {
+ return "\"" + StringEscapeUtils.escapeJava(value) + "\"";
+ }
+
+ protected abstract void doValidate(Object object) throws
ValidationException;
+}
\ No newline at end of file
Modified:
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
URL:
http://svn.apache.org/viewvc/struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java?rev=1164516&r1=1164515&r2=1164516&view=diff
==============================================================================
---
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
(original)
+++
struts/struts2/branches/STRUTS_2_2_3_1/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
Fri Sep 2 12:54:49 2011
@@ -35,13 +35,13 @@ public class ConversionErrorInterceptorT
protected ActionContext context;
protected ActionInvocation invocation;
protected ConversionErrorInterceptor interceptor;
- protected Map conversionErrors;
+ protected Map<String, Object> conversionErrors;
protected Mock mockInvocation;
protected ValueStack stack;
public void testFieldErrorAdded() throws Exception {
- conversionErrors.put("foo", new Long(123));
+ conversionErrors.put("foo", 123L);
SimpleAction action = new SimpleAction();
mockInvocation.expectAndReturn("getAction", action);
@@ -69,19 +69,12 @@ public class ConversionErrorInterceptorT
public void testWithPreResultListener() throws Exception {
conversionErrors.put("foo", "Hello");
- ActionContext ac = new ActionContext(stack.getContext());
- ac.setConversionErrors(conversionErrors);
- ac.setValueStack(stack);
+ ActionContext ac = createActionContext();
+ MockActionInvocation mai = createActionInvocation(ac);
+ SimpleAction action = createAction(mai);
- MockActionInvocation mai = new MockActionInvocation();
- mai.setInvocationContext(ac);
- mai.setStack(stack);
- SimpleAction action = new SimpleAction();
- action.setFoo(55);
- mai.setAction(action);
- stack.push(action);
assertNull(action.getFieldErrors().get("foo"));
- assertEquals(new Integer(55), stack.findValue("foo"));
+ assertEquals(55, stack.findValue("foo"));
interceptor.intercept(mai);
@@ -91,6 +84,52 @@ public class ConversionErrorInterceptorT
assertEquals("Hello", stack.findValue("foo")); // assume that the
original value is reset
}
+ /**
+ * See WW-3668
+ *
+ * @throws Exception
+ */
+ public void testWithPreResultListenerAgainstMaliciousCode() throws
Exception {
+ conversionErrors.put("foo", "\" + #root + \"");
+
+ ActionContext ac = createActionContext();
+
+ MockActionInvocation mai = createActionInvocation(ac);
+
+ SimpleAction action = createAction(mai);
+ assertNull(action.getFieldErrors().get("foo"));
+ assertEquals(55, stack.findValue("foo"));
+
+ interceptor.intercept(mai);
+
+ assertTrue(action.hasFieldErrors());
+ assertNotNull(action.getFieldErrors().get("foo"));
+
+ assertEquals("\" + #root + \"", stack.findValue("foo"));
+ }
+
+ private MockActionInvocation createActionInvocation(ActionContext ac) {
+ MockActionInvocation mai = new MockActionInvocation();
+ mai.setInvocationContext(ac);
+ mai.setStack(stack);
+ return mai;
+ }
+
+ private SimpleAction createAction(MockActionInvocation mai) {
+ SimpleAction action = new SimpleAction();
+ action.setFoo(55);
+ mai.setAction(action);
+ stack.push(action);
+ return action;
+ }
+
+ private ActionContext createActionContext() {
+ ActionContext ac = new ActionContext(stack.getContext());
+ ac.setConversionErrors(conversionErrors);
+ ac.setValueStack(stack);
+ return ac;
+ }
+
@Override
protected void setUp() throws Exception {
super.setUp();
@@ -99,7 +138,7 @@ public class ConversionErrorInterceptorT
invocation = (ActionInvocation) mockInvocation.proxy();
stack = ActionContext.getContext().getValueStack();
context = new ActionContext(stack.getContext());
- conversionErrors = new HashMap();
+ conversionErrors = new HashMap<String, Object>();
context.setConversionErrors(conversionErrors);
mockInvocation.matchAndReturn("getInvocationContext", context);
mockInvocation.expect("addPreResultListener",
C.isA(PreResultListener.class));
