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.
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 @@
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 @@
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 @@
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 @@
### 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 @@
}
/**
+ * Test OGNL Expression Max Length feature setting via OgnlUtil is disabled by default (in default.properties).
+ *
+ * @since 2.5.21
+ */
+ public void testDefaultExpressionMaxLengthDisabled() {
+ final String LONG_OGNL_EXPRESSION = "true == ThisIsAReallyLongOGNLExpressionOfRepeatedGarbageText." + new String(new char[65535]).replace('\0', 'A'); // Expression larger than 64KB.
+ try {
+ 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 ("Unable to compile expression (unexpected). Exception: " + ex);
+ }
+ }
+
+ /**
* Test OGNL Expression Max Length feature setting via OgnlUtil.
*
* @since 2.5.21
*/
public void testApplyExpressionMaxLength() {
try {
+ 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);
- } 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 ?");
- }
- try {
- ognlUtil.applyExpressionMaxLength(Integer.toString(Integer.MAX_VALUE, 10));
- } catch (Exception ex) {
- fail ("applyExpressionMaxLength did not accept MAX_VALUE maxlength string ?");
}
}
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.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 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
- Dog dog = new Dog();
- vs.push(dog);
+ // 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() + " ?");
+ }
- String value = StringUtils.repeat('.', repeat + 1);
+ OgnlValueStack vs = createValueStack();
- vs.setValue("name", value);
+ Dog dog = new Dog();
+ vs.push(dog);
- assertEquals(value, dog.getName());
+ String value = StringUtils.repeat('.', repeat + 1);
+
+ vs.setValue("name", value);
+
+ assertEquals(value, dog.getName());
+ } finally {
+ // Reset expressionMaxLength value to default (disabled)
+ ognlUtil.applyExpressionMaxLength(null);
+ }
}
public void testFailsOnMethodThatThrowsException() {
