Skip to content

Commit

Permalink
Merge 264f660 into fdfeb32
Browse files Browse the repository at this point in the history
  • Loading branch information
@JCgH4164838Gh792C124B5
JCgH4164838Gh792C124B5 committed Nov 11, 2019
2 parents fdfeb32 + 264f660 commit bf2b7be
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 42 deletions.
7 changes: 6 additions & 1 deletion core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
View file
Expand Up @@ -52,6 +52,7 @@
*/
public class OgnlUtil {

protected static final int MININUM_OGNL_EXPRESSION_MAXLENGTH = 128; // Minimum permitted value for STRUTS_OGNL_EXPRESSION_MAX_LENGTH
private static final Logger LOG = LogManager.getLogger(OgnlUtil.class);

private final ConcurrentMap<String, Object> expressions = new ConcurrentHashMap<>();
Expand Down Expand Up @@ -194,7 +195,11 @@ protected void applyExpressionMaxLength(String maxLength) {
// user is going to disable this functionality
Ognl.applyExpressionMaxLength(null);
} else {
Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength));
final Integer maxLengthInteger = Integer.parseInt(maxLength);
if (maxLengthInteger < MININUM_OGNL_EXPRESSION_MAXLENGTH) {
throw new IllegalArgumentException("OGNL Expression Max Length less than " + MININUM_OGNL_EXPRESSION_MAXLENGTH + " not permitted.");
}
Ognl.applyExpressionMaxLength(maxLengthInteger);
}
} catch (Exception ex) {
LOG.warn("Unable to set OGNL Expression Max Length {}.", maxLength); // Help configuration debugging.
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
11 changes: 9 additions & 2 deletions core/src/main/resources/org/apache/struts2/default.properties
View file
Expand Up @@ -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
88 changes: 65 additions & 23 deletions core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
View file
Expand Up @@ -1233,36 +1233,78 @@ 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() {
try {
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.
}
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("0");
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 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 {
assertTrue(OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH + " is not > 0 ?", OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH > 0);
ognlUtil.applyExpressionMaxLength("0");
fail ("applyExpressionMaxLength accepted maxlength string 0 when minimum maxlength is " + OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH + " ?");
} catch (Exception ex) {
// Expected rejection of length 0 < MININUM_OGNL_EXPRESSION_MAXLENGTH
}
try {
assertTrue(OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH + " is not > " + (OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH - 1) + " ?",
OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH > (OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH - 1));
ognlUtil.applyExpressionMaxLength(Integer.valueOf(OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH - 1).toString());
fail ("applyExpressionMaxLength accepted maxlength string " + (OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH - 1) + " when minimum maxlength is " +
OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH + " ?");
} catch (Exception ex) {
// Expected rejection of length (MININUM_OGNL_EXPRESSION_MAXLENGTH - 1) < MININUM_OGNL_EXPRESSION_MAXLENGTH
}
try {
ognlUtil.applyExpressionMaxLength(Integer.valueOf(OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH).toString());
} catch (Exception ex) {
fail ("applyExpressionMaxLength did not accept maxlength string " + OgnlUtil.MININUM_OGNL_EXPRESSION_MAXLENGTH + " ?");
}
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);
}
}

Expand Down
87 changes: 73 additions & 14 deletions core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit bf2b7be

Please sign in to comment.