Disable expressionMaxLength by default for Struts 2.5.x.

core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java

@@ -197,7 +197,7 @@ protected void applyExpressionMaxLength(String maxLength) {
                 Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength));
             }
         } catch (Exception ex) {
-            LOG.warn("Unable to set OGNL Expression Max Length {}.", maxLength);  // Help configuration debugging.
+            LOG.error("Unable to set OGNL Expression Max Length {}.", maxLength);  // Help configuration debugging.
             throw ex;
         }
     }

core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java

@@ -205,7 +205,7 @@ protected void handleRuntimeException(String expr, Object value, boolean throwEx
 
     protected void handleOgnlException(String expr, Object value, boolean throwExceptionOnFailure, OgnlException e) {
         if (e != null && e.getReason() instanceof SecurityException) {
-            LOG.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e);
+            LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e);
         }
     	boolean shouldLog = shouldLogMissingPropertyWarning(e);
     	String msg = null;
@@ -331,7 +331,7 @@ private Object tryFindValueWhenExpressionIsNotNull(String expr, Class asType) th
     protected Object handleOgnlException(String expr, boolean throwExceptionOnFailure, OgnlException e) {
         Object ret = null;
         if (e != null && e.getReason() instanceof SecurityException) {
-            LOG.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e);
+            LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e);
         } else {
             ret = findInContext(expr);
         }

core/src/main/resources/org/apache/struts2/default.properties

@@ -220,7 +220,14 @@ struts.ognl.enableExpressionCache=true
 ### or simply rethrow it as a ServletException to allow future processing by other frameworks like Spring Security
 struts.handle.exception=true
 
-### applies maximum length allowed on OGNL expressions for security enhancement
-struts.ognl.expressionMaxLength=200
+### Applies maximum length allowed on OGNL expressions for security enhancement (optional)
+###
+### **WARNING**: If developers enable this option (by configuration) they should make sure that they understand the implications of setting 
+###   struts.ognl.expressionMaxLength.  They must choose a value large enough to permit ALL valid OGNL expressions used within the application.
+###   Values larger than the 200-400 range have diminishing security value (at which point it is really only a "style guard" for long OGNL
+###   expressions in an application.  Setting a value of null or "" will also disable the feature.
+###
+### NOTE: The sample line below is *INTENTIONALLY* commented out, as this feature is disabled by default.
+# struts.ognl.expressionMaxLength=256
 
 ### END SNIPPET: complete_file

core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java

@@ -1233,36 +1233,62 @@ public void testStaticFieldGetValue() {
     }
 
     /**
-     * Test OGNL Expression Max Length feature setting via OgnlUtil.
+     * Test OGNL Expression Max Length feature setting via OgnlUtil is disabled by default (in default.properties).
      * 
      * @since 2.5.21
      */
-    public void testApplyExpressionMaxLength() {
+    public void testDefaultExpressionMaxLengthDisabled() {
+        final String LONG_OGNL_EXPRESSION = "true == ThisIsAReallyLongOGNLExpressionOfRepeatedGarbageText." + new String(new char[65535]).replace('\0', 'A');  // Expression larger than 64KB.
         try {
-            ognlUtil.applyExpressionMaxLength(null);
+            Object compileResult = ognlUtil.compile(LONG_OGNL_EXPRESSION);
+            assertNotNull("Long OGNL expression compilation produced a null result ?", compileResult);
+        } catch (OgnlException oex) {
+             if (oex.getReason() instanceof SecurityException) {
+                 fail ("Unable to compile expression (unexpected).  'struts.ognl.expressionMaxLength' may have accidentally been enabled by default.  Exception: " + oex);
+             } else {
+                 fail ("Unable to compile expression (unexpected).  Exception: " + oex);
+             }
         } catch (Exception ex) {
-            fail ("applyExpressionMaxLength did not accept null maxlength string ?");
-        }
-        try {
-            ognlUtil.applyExpressionMaxLength("");
-        } catch (Exception ex) {
-            fail ("applyExpressionMaxLength did not accept empty maxlength string ?");
-        }
-        try {
-            ognlUtil.applyExpressionMaxLength("-1");
-            fail ("applyExpressionMaxLength accepted negative maxlength string ?");
-        } catch (IllegalArgumentException iae) {
-            // Expected rejection of -ive length.
-        }
-        try {
-            ognlUtil.applyExpressionMaxLength("0");
-        } catch (Exception ex) {
-            fail ("applyExpressionMaxLength did not accept maxlength string 0 ?");
+            fail ("Unable to compile expression (unexpected).  Exception: " + ex);
         }
+    }
+
+    /**
+     * Test OGNL Expression Max Length feature setting via OgnlUtil.
+     * 
+     * @since 2.5.21
+     */
+    public void testApplyExpressionMaxLength() {
         try {
-            ognlUtil.applyExpressionMaxLength(Integer.toString(Integer.MAX_VALUE, 10));
-        } catch (Exception ex) {
-            fail ("applyExpressionMaxLength did not accept MAX_VALUE maxlength string ?");
+            try {
+                ognlUtil.applyExpressionMaxLength(null);
+            } catch (Exception ex) {
+                fail ("applyExpressionMaxLength did not accept null maxlength string (disable feature) ?");
+            }
+            try {
+                ognlUtil.applyExpressionMaxLength("");
+            } catch (Exception ex) {
+                fail ("applyExpressionMaxLength did not accept empty maxlength string (disable feature) ?");
+            }
+            try {
+                ognlUtil.applyExpressionMaxLength("-1");
+                fail ("applyExpressionMaxLength accepted negative maxlength string ?");
+            } catch (IllegalArgumentException iae) {
+                // Expected rejection of -ive length.
+            }
+            try {
+                ognlUtil.applyExpressionMaxLength("0");
+            } catch (Exception ex) {
+                fail ("applyExpressionMaxLength did not accept maxlength string 0 ?");
+            }
+            try {
+                ognlUtil.applyExpressionMaxLength(Integer.toString(Integer.MAX_VALUE, 10));
+            } catch (Exception ex) {
+                fail ("applyExpressionMaxLength did not accept MAX_VALUE maxlength string ?");
+            }
+        } finally {
+            // Reset expressionMaxLength value to default (disabled)
+            ognlUtil.applyExpressionMaxLength(null);
         }
     }
 

core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java

@@ -40,6 +40,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import ognl.ParseException;
 
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
@@ -350,36 +351,94 @@ public void testFailOnMissingMethod() {
         }
     }
 
-    public void testFailOnTooLongExpressionWithDefaultProperties() {
+    public void testFailOnTooLongExpressionLongerThan192_ViaOverriddenProperty() {
+        try {
+            loadConfigurationProviders(new StubConfigurationProvider() {
+                @Override
+                public void register(ContainerBuilder builder,
+                                     LocatableProperties props) throws ConfigurationException {
+                    props.setProperty(StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH, "192");
+                }
+            });
+            Integer repeat = Integer.parseInt(
+                    container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH));
+
+            OgnlValueStack vs = createValueStack();
+            try {
+                vs.findValue(StringUtils.repeat('.', repeat + 1), true);
+                fail("Failed to throw exception on too long expression");
+            } catch (Exception ex) {
+                assertTrue(ex.getCause() instanceof OgnlException);
+                assertTrue(((OgnlException) ex.getCause()).getReason() instanceof SecurityException);
+            }
+        } finally {
+            // Reset expressionMaxLength value to default (disabled)
+            ognlUtil.applyExpressionMaxLength(null);
+        }
+    }
+
+    public void testNotFailOnTooLongExpressionWithDefaultProperties() {
         loadConfigurationProviders(new DefaultPropertiesProvider());
-        Integer repeat = Integer.parseInt(
-                container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH));
+
+        Object defaultMaxLengthFromConfiguration = container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH);
+        if (defaultMaxLengthFromConfiguration != null) {
+            assertTrue("non-null defaultMaxLengthFromConfiguration not a String ?", defaultMaxLengthFromConfiguration instanceof String);
+            assertTrue("non-null defaultMaxLengthFromConfiguration not empty string by default ?", ((String) defaultMaxLengthFromConfiguration).length() == 0);
+        } else {
+            assertNull("defaultMaxLengthFromConfiguration not null ?", defaultMaxLengthFromConfiguration);
+        }
+        // Original test logic was to confirm failure of exceeding the default value.  Now the feature should be disabled by default,
+        // so this test's expectations are now changed.
+        Integer repeat = Integer.valueOf(256);  // Since maxlength is disabled by default, just choose an arbitrary value for test
 
         OgnlValueStack vs = createValueStack();
         try {
             vs.findValue(StringUtils.repeat('.', repeat + 1), true);
-            fail("Failed to throw exception on too long expression");
+            fail("findValue did not throw any exception (should either fail as invalid expression syntax or security exception) ?");
         } catch (Exception ex) {
+            // If STRUTS_OGNL_EXPRESSION_MAX_LENGTH feature is disabled (default), the parse should fail due to a reason of invalid expression syntax
+            // with ParseException.  Previously when it was enabled the reason for the failure would have been SecurityException.
             assertTrue(ex.getCause() instanceof OgnlException);
-            assertTrue(((OgnlException) ex.getCause()).getReason() instanceof SecurityException);
+            assertTrue(((OgnlException) ex.getCause()).getReason() instanceof ParseException);
         }
     }
 
     public void testNotFailOnTooLongValueWithDefaultProperties() {
-        loadConfigurationProviders(new DefaultPropertiesProvider());
-        Integer repeat = Integer.parseInt(
-                container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH));
+        try {
+            loadConfigurationProviders(new DefaultPropertiesProvider());
 
-        OgnlValueStack vs = createValueStack();
+            Object defaultMaxLengthFromConfiguration = container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH);
+            if (defaultMaxLengthFromConfiguration != null) {
+                assertTrue("non-null defaultMaxLengthFromConfiguration not a String ?", defaultMaxLengthFromConfiguration instanceof String);
+                assertTrue("non-null defaultMaxLengthFromConfiguration not empty string by default ?", ((String) defaultMaxLengthFromConfiguration).length() == 0);
+            } else {
+                assertNull("defaultMaxLengthFromConfiguration not null ?", defaultMaxLengthFromConfiguration);
+            }
+            // Original test logic is unchanged (testing that values can be larger than maximum expression length), but since the feature is disabled by
+            // default we will now have to enable it with an arbitrary value, test, and reset it to disabled.
+            Integer repeat = Integer.valueOf(256);  // Since maxlength is disabled by default, just choose an arbitrary value for test
+
+            // Apply a non-default value for expressionMaxLength (as it should be disabled by default)
+            try {
+                ognlUtil.applyExpressionMaxLength(repeat.toString());
+            } catch (Exception ex) {
+                fail ("applyExpressionMaxLength did not accept maxlength string " + repeat.toString() + " ?");
+            }
 
-        Dog dog = new Dog();
-        vs.push(dog);
+            OgnlValueStack vs = createValueStack();
+
+            Dog dog = new Dog();
+            vs.push(dog);
 
-        String value = StringUtils.repeat('.', repeat + 1);
+            String value = StringUtils.repeat('.', repeat + 1);
 
-        vs.setValue("name", value);
+            vs.setValue("name", value);
 
-        assertEquals(value, dog.getName());
+            assertEquals(value, dog.getName());
+        } finally {
+            // Reset expressionMaxLength value to default (disabled)
+            ognlUtil.applyExpressionMaxLength(null);
+        }
     }
 
     public void testFailsOnMethodThatThrowsException() {