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 bc53c758e1..83e5833d0d 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 @@ protected void applyExpressionMaxLength(String maxLength) { 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 93f82425ee..6e3bd02092 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 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; @@ -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); } diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 23f159ef40..c84452d2e1 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 9979553062..701a1f8146 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 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() { + 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 c601a9733f..566429f9e2 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 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() {