struts-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From yasserzam...@apache.org
Subject [struts] branch struts-2-5-x updated: Disable expressionMaxLength by default for Struts 2.5.x. (#380)
Date Sat, 16 Nov 2019 16:39:31 GMT
This is an automated email from the ASF dual-hosted git repository.

yasserzamani 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 3dfc5a4  Disable expressionMaxLength by default for Struts 2.5.x. (#380)
3dfc5a4 is described below

commit 3dfc5a4074710e66e516d3d1adcacc1c2dc025f1
Author: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com>
AuthorDate: Sat Nov 16 11:39:19 2019 -0500

    Disable expressionMaxLength by default for Struts 2.5.x. (#380)
    
    * Disable struts.ognl.expressionMaxLength by default for Struts 2.5.x.
    - Commented out struts.ognl.expressionMaxLength line in default.properties
    and provided in-place comments about its usage.
    - Changed OgnlValueStack.handleOgnlException() methods to output error
    instead of warn for failures to evaluate expressions due to security
    constraints.
    - Updated existing unit tests to compensate for change in default
    behaviour.
    - Added a unit test to confirm default behaviour for
    struts.ognl.expressionMaxLength is disabled.
    
    * Updated commit for disable struts.ognl.expressionMaxLength by default for
    Struts 2.5.x
    - Additional unit test requested by Y. Zamani for code coverage.
    - Corrected accidental use of wrong (static) toString method in one test.
    - Addition of a minimum struts.ognl.expressionMaxLength value permitted
    by Struts 2 (128).  Any value smaller than that is likely to be a
    configuration error and if a user really wishes to force it they may go to
    OGNL directly to do so.
    
    * Updated commit for disable struts.ognl.expressionMaxLength by default for
    Struts 2.5.x
    - Removed minimum struts.ognl.expressionMaxLength (restored to previous
    behaviour) as requested by Y. Zamani and L. Lenart.
    - Updated unit tests to compensate for the above change.
    - Changed log output from warn to error in applyExpressionMaxLength() on
    exception since it will likely be considered a fatal condition.
---
 .../com/opensymphony/xwork2/ognl/OgnlUtil.java     |  2 +-
 .../opensymphony/xwork2/ognl/OgnlValueStack.java   |  4 +-
 .../org/apache/struts2/default.properties          | 11 ++-
 .../com/opensymphony/xwork2/ognl/OgnlUtilTest.java | 72 ++++++++++++------
 .../xwork2/ognl/OgnlValueStackTest.java            | 87 ++++++++++++++++++----
 5 files changed, 134 insertions(+), 42 deletions(-)

diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
index bc53c75..83e5833 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
@@ -197,7 +197,7 @@ public class OgnlUtil {
                 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;
         }
     }
diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
index 93f8242..6e3bd02 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
@@ -205,7 +205,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
 
     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 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
     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);
         }
diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties
index 23f159e..c84452d 100644
--- a/core/src/main/resources/org/apache/struts2/default.properties
+++ b/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
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
index 9979553..701a1f8 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
@@ -1233,36 +1233,62 @@ public class OgnlUtilTest extends XWorkTestCase {
     }
 
     /**
-     * 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);
         }
     }
 
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
index c601a97..566429f 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
@@ -40,6 +40,7 @@ import java.util.HashMap;
 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 class OgnlValueStackTest extends XWorkTestCase {
         }
     }
 
-    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() {


Mime
View raw message