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+jcgh4164838gh792c12...@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() {
