Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 22:56:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

Disable expressionMaxLength by default for Struts 2.5.x. (#380)

Browse Source 
* 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.
This commit is contained in:
JCgH4164838Gh792C124B5
2019-11-16 11:39:19 -05:00
committed by Yasser Zamani
parent fdfeb3233a
commit 3dfc5a4074
5 changed files with 134 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
+1 -1
View File
@@ -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;
}
}
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
+2 -2
View File
@@ -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);
}
core/src/main/resources/org/apache/struts2/default.properties
+9 -2
View File
@@ -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
+49 -23
View File
@@ -1232,6 +1232,27 @@ public class OgnlUtilTest extends XWorkTestCase {
}
}
/**
* 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.
*
@@ -1239,30 +1260,35 @@ public class OgnlUtilTest extends XWorkTestCase {
*/
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 ?");
}
}
core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
+73 -14
View File
@@ -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
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() {