svn commit: r1164516 - in /struts/struts2/branches/STRUTS_2_2_3_1: core/src/main/java/org/apache/struts2/interceptor/ core/src/main/resources/template/simple/ core/src/test/java/org/apache/struts2/interceptor/ xwork-core/src/main/java/com/opensymphony/...

Fri, 02 Sep 2011 05:55:29 -0700 

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 -->
- * 
+ *
  * &lt;!-- myJspPage.jsp --&gt;
  * &lt;ww:form action="someAction" method="POST"&gt;
  *   ....
- *   &lt;ww:textfield 
+ *   &lt;ww:textfield
  *       label="My Integer Field"
  *       name="myIntegerField" /&gt;
  *   ....
- *   &lt;ww:submit /&gt;       
+ *   &lt;ww:submit /&gt;
  * &lt;/ww:form&gt;
- * 
+ *
  * <!-- END SNIPPET: exampleJspPage -->
  * </pre>
- * 
+ *
  * <pre>
  * <!-- START SNIPPET: exampleXwork -->
- * 
+ *
  * &lt;!-- xwork.xml --&gt;
  * &lt;xwork&gt;
  * &lt;include file="xwork-default.xml" /&gt;
@@ -88,31 +89,31 @@ import java.util.Map;
  * &lt;/package&gt;
  * ....
  * &lt;/xwork&gt;
- * 
+ *
  * <!-- END SNIPPET:exampleXwork -->
  * </pre>
- * 
- * 
+ *
+ *
  * <pre>
  * <!-- START SNIPPET: exampleJava -->
- * 
+ *
  * &lt;!-- MyActionSupport.java --&gt;
  * 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 -->
- * 
+ *
  * &lt;!-- MyActionSupport-someAction-validation.xml --&gt;
  * &lt;validators&gt;
  *   ...
@@ -124,24 +125,24 @@ import java.util.Map;
  *   &lt;/field&gt;
  *   ...
  * &lt;/validators&gt;
- * 
+ *
  * <!-- 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));

Reply via email to