From 255038405549562593227c221c04a6cb096a0c05 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 25 Apr 2014 14:57:07 +0200 Subject: [PATCH 01/48] Defines new logic to allow exclude some properties (eg. getClass) --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 26 ++++++ .../xwork2/ognl/OgnlUtilTest.java | 91 ++++++++++++++++++- 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index fa907e320..a0231bc97 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -19,6 +19,7 @@ import com.opensymphony.xwork2.XWorkConstants; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.CompoundRoot; +import com.opensymphony.xwork2.util.TextParseUtil; import com.opensymphony.xwork2.util.logging.Logger; import com.opensymphony.xwork2.util.logging.LoggerFactory; import com.opensymphony.xwork2.util.reflection.ReflectionException; @@ -36,7 +37,9 @@ import java.beans.PropertyDescriptor; import java.lang.reflect.Method; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -58,6 +61,8 @@ public class OgnlUtil { private boolean enableExpressionCache = true; private boolean enableEvalExpression; + private Set excludedProperties = new HashSet(); + @Inject public void setXWorkConverter(XWorkConverter conv) { this.defaultConverter = new OgnlTypeConverterWrapper(conv); @@ -82,6 +87,11 @@ public class OgnlUtil { } } + @Inject(value = XWorkConstants.OGNL_EXCLUDED_PROPERTIES, required = false) + public void setExcludedProperties(String commaDelimitedProperties) { + excludedProperties = TextParseUtil.commaDelimitedStringToSet(commaDelimitedProperties); + } + /** * Sets the object's properties using the default type converter, defaulting to not throw * exceptions for problems setting the properties. @@ -279,11 +289,13 @@ public class OgnlUtil { if (tree == null) { tree = Ognl.parseExpression(expression); checkEnableEvalExpression(tree, context); + checkExcludedPropertiesAccess(tree, null); expressions.putIfAbsent(expression, tree); } } else { tree = Ognl.parseExpression(expression); checkEnableEvalExpression(tree, context); + checkExcludedPropertiesAccess(tree, null); } @@ -293,6 +305,20 @@ public class OgnlUtil { return exec; } + private void checkExcludedPropertiesAccess(Object tree, SimpleNode parent) throws OgnlException { + if (tree instanceof SimpleNode) { + SimpleNode node = (SimpleNode) tree; + for (String excludedPattern : excludedProperties) { + if (excludedPattern.equalsIgnoreCase(node.toString())) { + throw new OgnlException("Tree [" + (parent != null ? parent : tree) + "] trying access excluded pattern [" + excludedPattern + "]"); + } + for (int i = 0; i < node.jjtGetNumChildren(); i++) { + checkExcludedPropertiesAccess(node.jjtGetChild(i), node); + } + } + } + } + public Object compile(String expression, Map context) throws OgnlException { return compileAndExecute(expression,context,new OgnlTask() { public Object execute(Object tree) throws OgnlException { diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index 8bd5e23f4..d4711832e 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -630,7 +630,96 @@ public class OgnlUtilTest extends XWorkTestCase { stack.setValue("1114778947765", foo); stack.setValue("1234", foo); } - + + public void testAvoidCallingMethodsOnObjectClass() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("class"); + + Exception expected = null; + try { + util.setValue("class.classLoader.defaultAssertionStatus", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [class.classLoader.defaultAssertionStatus] trying access excluded pattern [class]"); + } + + public void testAvoidCallingMethodsOnObjectClassUpperCased() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("class"); + + Exception expected = null; + try { + util.setValue("Class.ClassLoader.DefaultAssertionStatus", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [Class.ClassLoader.DefaultAssertionStatus] trying access excluded pattern [class]"); + } + + public void testAvoidCallingMethodsOnObjectClassAsMap() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("class"); + + Exception expected = null; + try { + util.setValue("class['classLoader']['defaultAssertionStatus']", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); + } + + public void testAvoidCallingMethodsOnObjectClassAsMapWithQuotes() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("class"); + + Exception expected = null; + try { + util.setValue("class[\"classLoader\"]['defaultAssertionStatus']", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); + } + + public void testAvoidCallingToString() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("toString"); + + Exception expected = null; + try { + util.setValue("toString", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [toString] trying access excluded pattern [toString]"); + } public static class Email { String address; From bbcee42f669f9e11e1ba1892eddbd612506616d2 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 25 Apr 2014 14:57:44 +0200 Subject: [PATCH 02/48] Adds constant under which excluded properties can be defined --- .../src/main/java/com/opensymphony/xwork2/XWorkConstants.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java index 19363680e..1894372fc 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java @@ -17,4 +17,6 @@ public final class XWorkConstants { public static final String RELOAD_XML_CONFIGURATION = "reloadXmlConfiguration"; public static final String ALLOW_STATIC_METHOD_ACCESS = "allowStaticMethodAccess"; public static final String XWORK_LOGGER_FACTORY = "xwork.loggerFactory"; + public static final String OGNL_EXCLUDED_PROPERTIES = "ognlExcludedProperties"; + } From 14ad0ab00662e847b7959022d0106adfaf3219ea Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 25 Apr 2014 14:58:40 +0200 Subject: [PATCH 03/48] Extends tests to check if excluded properties works on higher level --- .../xwork2/interceptor/ParametersInterceptorTest.java | 11 ++++++++--- xwork-core/src/test/resources/xwork-param-test.xml | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 5a4485d64..f0adf029b 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -161,12 +161,14 @@ public class ParametersInterceptorTest extends XWorkTestCase { // given final String pollution1 = "class.classLoader.jarPath"; final String pollution2 = "model.class.classLoader.jarPath"; + final String pollution3 = "class.classLoader.defaultAssertionStatus"; loadConfigurationProviders(new XWorkConfigurationProvider(), new XmlConfigurationProvider("xwork-param-test.xml")); final Map params = new HashMap() { { put(pollution1, "bad"); put(pollution2, "very bad"); + put(pollution3, true); } }; @@ -190,16 +192,19 @@ public class ParametersInterceptorTest extends XWorkTestCase { pi.setParameters(action, vs, params); // then - assertEquals(2, action.getActionMessages().size()); + assertEquals(3, action.getActionMessages().size()); String msg1 = action.getActionMessage(0); String msg2 = action.getActionMessage(1); + String msg3 = action.getActionMessage(2); - assertEquals("Error setting expression 'class.classLoader.jarPath' with value 'bad'", msg1); - assertEquals("Error setting expression 'model.class.classLoader.jarPath' with value 'very bad'", msg2); + assertEquals("Error setting expression 'class.classLoader.defaultAssertionStatus' with value 'true'", msg1); + assertEquals("Error setting expression 'class.classLoader.jarPath' with value 'bad'", msg2); + assertEquals("Error setting expression 'model.class.classLoader.jarPath' with value 'very bad'", msg3); assertFalse(excluded.get(pollution1)); assertFalse(excluded.get(pollution2)); + assertFalse(excluded.get(pollution3)); } public void testDoesNotAllowMethodInvocations() throws Exception { diff --git a/xwork-core/src/test/resources/xwork-param-test.xml b/xwork-core/src/test/resources/xwork-param-test.xml index fa081c49f..3ca616a5b 100644 --- a/xwork-core/src/test/resources/xwork-param-test.xml +++ b/xwork-core/src/test/resources/xwork-param-test.xml @@ -4,4 +4,5 @@ + \ No newline at end of file From aff3a3a625dc89f93f5b6548887245ffd6bba3d3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 25 Apr 2014 14:59:38 +0200 Subject: [PATCH 04/48] Adds conversion of Struts property to XWork property --- core/src/main/java/org/apache/struts2/StrutsConstants.java | 4 ++++ .../apache/struts2/config/DefaultBeanSelectionProvider.java | 1 + core/src/main/resources/struts-default.xml | 3 +++ 3 files changed, 8 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 3423ec8bd..6be58ad36 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -281,4 +281,8 @@ public final class StrutsConstants { /** Allows override default DispatcherErrorHandler **/ public static final String STRUTS_DISPATCHER_ERROR_HANDLER = "struts.dispatcher.errorHandler"; + + /** Comma delimited set of excluded properties which cannot be accessed via expressions **/ + public static final String STRUTS_EXCLUDED_PROPERTIES = "struts.excludedProperties"; + } diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index b6b5b4590..4cc2d61fd 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -391,6 +391,7 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider convertIfExist(props, StrutsConstants.STRUTS_ENABLE_OGNL_EVAL_EXPRESSION, XWorkConstants.ENABLE_OGNL_EVAL_EXPRESSION); convertIfExist(props, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, XWorkConstants.ALLOW_STATIC_METHOD_ACCESS); convertIfExist(props, StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, XWorkConstants.RELOAD_XML_CONFIGURATION); + convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_PROPERTIES, XWorkConstants.OGNL_EXCLUDED_PROPERTIES); LocalizedTextUtil.addDefaultResourceBundle("org/apache/struts2/struts-messages"); loadCustomResourceBundles(props); diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 87f1ff51a..7cb687ef5 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -37,6 +37,9 @@ "http://struts.apache.org/dtds/struts-2.3.dtd"> + + + From 58a58615cf45c669800deafd462f7c427b677caf Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 26 Apr 2014 06:57:42 +0200 Subject: [PATCH 05/48] Includes check for braces in expression --- .../src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index a0231bc97..81f970005 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -309,7 +309,8 @@ public class OgnlUtil { if (tree instanceof SimpleNode) { SimpleNode node = (SimpleNode) tree; for (String excludedPattern : excludedProperties) { - if (excludedPattern.equalsIgnoreCase(node.toString())) { + // TODO lukaszlenart: need a better way to check 'toString' and 'toString()' call + if (excludedPattern.equalsIgnoreCase(node.toString()) || (excludedPattern + "()").equalsIgnoreCase(node.toString())) { throw new OgnlException("Tree [" + (parent != null ? parent : tree) + "] trying access excluded pattern [" + excludedPattern + "]"); } for (int i = 0; i < node.jjtGetNumChildren(); i++) { From bcc0327ee49c60b10d158354c0d1be06eb8a9f52 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 26 Apr 2014 06:58:50 +0200 Subject: [PATCH 06/48] Uses OgnlUtil to execute action/method instead of Reflection --- .../xwork2/DefaultActionInvocation.java | 54 +++++++------------ 1 file changed, 18 insertions(+), 36 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java b/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java index 531a72560..4539e56b8 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java @@ -22,14 +22,14 @@ import com.opensymphony.xwork2.config.entities.ResultConfig; import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.interceptor.PreResultListener; +import com.opensymphony.xwork2.ognl.OgnlUtil; import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.util.ValueStackFactory; import com.opensymphony.xwork2.util.logging.Logger; import com.opensymphony.xwork2.util.logging.LoggerFactory; import com.opensymphony.xwork2.util.profiling.UtilTimerStack; +import ognl.OgnlException; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Iterator; import java.util.List; @@ -46,18 +46,8 @@ import java.util.Map; */ public class DefaultActionInvocation implements ActionInvocation { - private static final long serialVersionUID = -585293628862447329L; - - //static { - // if (ObjectFactory.getContinuationPackage() != null) { - // continuationHandler = new ContinuationHandler(); - // } - //} private static final Logger LOG = LoggerFactory.getLogger(DefaultActionInvocation.class); - private static final Class[] EMPTY_CLASS_ARRAY = new Class[0]; - private static final Object[] EMPTY_OBJECT_ARRAY = new Object[0]; - protected Object action; protected ActionProxy proxy; protected List preResultListeners; @@ -75,6 +65,7 @@ public class DefaultActionInvocation implements ActionInvocation { protected ValueStackFactory valueStackFactory; protected Container container; protected UnknownHandlerManager unknownHandlerManager; + protected OgnlUtil ognlUtil; public DefaultActionInvocation(final Map extraContext, final boolean pushAction) { this.extraContext = extraContext; @@ -106,6 +97,11 @@ public class DefaultActionInvocation implements ActionInvocation { this.actionEventListener = listener; } + @Inject + public void setOgnlUtil(OgnlUtil ognlUtil) { + this.ognlUtil = ognlUtil; + } + public Object getAction() { return action; } @@ -420,22 +416,19 @@ public class DefaultActionInvocation implements ActionInvocation { try { UtilTimerStack.push(timerKey); - boolean methodCalled = false; - Object methodResult = null; - Method method = null; + Object methodResult; try { - method = getAction().getClass().getMethod(methodName, EMPTY_CLASS_ARRAY); - } catch (NoSuchMethodException e) { + methodResult = ognlUtil.getValue(methodName + "()", getStack().getContext(), action); + } catch (OgnlException e) { // hmm -- OK, try doXxx instead try { - String altMethodName = "do" + methodName.substring(0, 1).toUpperCase() + methodName.substring(1); - method = getAction().getClass().getMethod(altMethodName, EMPTY_CLASS_ARRAY); - } catch (NoSuchMethodException e1) { + String altMethodName = "do" + methodName.substring(0, 1).toUpperCase() + methodName.substring(1) + "()"; + methodResult = ognlUtil.getValue(altMethodName, ActionContext.getContext().getContextMap(), action); + } catch (OgnlException e1) { // well, give the unknown handler a shot if (unknownHandlerManager.hasUnknownHandlers()) { try { methodResult = unknownHandlerManager.handleUnknownMethod(action, methodName); - methodCalled = true; } catch (NoSuchMethodException e2) { // throw the original one throw e; @@ -445,29 +438,18 @@ public class DefaultActionInvocation implements ActionInvocation { } } } - - if (!methodCalled) { - methodResult = method.invoke(action, EMPTY_OBJECT_ARRAY); - } - return saveResult(actionConfig, methodResult); - } catch (NoSuchMethodException e) { - throw new IllegalArgumentException("The " + methodName + "() is not defined in action " + getAction().getClass() + ""); - } catch (InvocationTargetException e) { + } catch (OgnlException e) { // We try to return the source exception. - Throwable t = e.getTargetException(); + //Throwable t = e.getTargetException(); if (actionEventListener != null) { - String result = actionEventListener.handleException(t, getStack()); + String result = actionEventListener.handleException(e, getStack()); if (result != null) { return result; } } - if (t instanceof Exception) { - throw (Exception) t; - } else { - throw e; - } + throw e; } finally { UtilTimerStack.pop(timerKey); } From 5d8aa8a80be131dbcf412c28aec0435d3bdc23e3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 26 Apr 2014 06:59:18 +0200 Subject: [PATCH 07/48] Updates tests as using Object's methods is prohibited --- .../ExecuteAndWaitInterceptorTest.java | 2 ++ .../struts2/views/jsp/PropertyTagTest.java | 30 +++++++++++-------- .../struts2/views/jsp/ui/SelectTest.java | 2 +- .../rest/RestActionInvocationTest.java | 2 ++ .../xwork2/DefaultActionInvocationTest.java | 8 +++++ 5 files changed, 31 insertions(+), 13 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index 01d1a6eaa..5a01015bf 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -32,6 +32,7 @@ import com.opensymphony.xwork2.config.entities.ResultConfig; import com.opensymphony.xwork2.inject.ContainerBuilder; import com.opensymphony.xwork2.interceptor.ParametersInterceptor; import com.opensymphony.xwork2.mock.MockResult; +import com.opensymphony.xwork2.ognl.OgnlUtil; import com.opensymphony.xwork2.util.location.LocatableProperties; import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; @@ -222,6 +223,7 @@ public class ExecuteAndWaitInterceptorTest extends StrutsInternalTestCase { public void register(ContainerBuilder builder, LocatableProperties props) throws ConfigurationException { builder.factory(ObjectFactory.class); builder.factory(ActionProxyFactory.class, DefaultActionProxyFactory.class); + builder.factory(OgnlUtil.class, OgnlUtil.class); } } diff --git a/core/src/test/java/org/apache/struts2/views/jsp/PropertyTagTest.java b/core/src/test/java/org/apache/struts2/views/jsp/PropertyTagTest.java index cce9a0ccf..a2b77ba8c 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/PropertyTagTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/PropertyTagTest.java @@ -180,11 +180,13 @@ public class PropertyTagTest extends StrutsInternalTestCase { pageContext.setRequest(request); // test - {PropertyTag tag = new PropertyTag(); - tag.setPageContext(pageContext); - tag.setValue("%{toString()}"); - tag.doStartTag(); - tag.doEndTag();} + { + PropertyTag tag = new PropertyTag(); + tag.setPageContext(pageContext); + tag.setValue("%{formatTitle()}"); + tag.doStartTag(); + tag.doEndTag(); + } // verify test request.verify(); @@ -212,7 +214,7 @@ public class PropertyTagTest extends StrutsInternalTestCase { tag.setEscape(false); tag.setEscapeJavaScript(true); tag.setPageContext(pageContext); - tag.setValue("%{toString()}"); + tag.setValue("%{formatTitle()}"); tag.doStartTag(); tag.doEndTag();} @@ -242,7 +244,7 @@ public class PropertyTagTest extends StrutsInternalTestCase { tag.setEscape(false); tag.setEscapeXml(true); tag.setPageContext(pageContext); - tag.setValue("%{toString()}"); + tag.setValue("%{formatTitle()}"); tag.doStartTag(); tag.doEndTag();} @@ -272,7 +274,7 @@ public class PropertyTagTest extends StrutsInternalTestCase { tag.setEscape(false); tag.setEscapeCsv(true); tag.setPageContext(pageContext); - tag.setValue("%{toString()}"); + tag.setValue("%{formatTitle()}"); tag.doStartTag(); tag.doEndTag();} @@ -300,7 +302,7 @@ public class PropertyTagTest extends StrutsInternalTestCase { // test {PropertyTag tag = new PropertyTag(); tag.setPageContext(pageContext); - tag.setValue("toString()"); + tag.setValue("formatTitle()"); tag.doStartTag(); tag.doEndTag();} @@ -328,7 +330,7 @@ public class PropertyTagTest extends StrutsInternalTestCase { // test {PropertyTag tag = new PropertyTag(); tag.setPageContext(pageContext); - tag.setValue("toString()"); + tag.setValue("formatTitle()"); tag.doStartTag(); tag.doEndTag();} @@ -356,7 +358,7 @@ public class PropertyTagTest extends StrutsInternalTestCase { // test {PropertyTag tag = new PropertyTag(); tag.setPageContext(pageContext); - tag.setValue("%{toString()}"); + tag.setValue("%{formatTitle()}"); tag.doStartTag(); tag.doEndTag();} @@ -385,8 +387,12 @@ public class PropertyTagTest extends StrutsInternalTestCase { return title; } - public String toString() { + public String formatTitle() { return "Foo is: " + title; } + + public String toString() { + return formatTitle(); + } } } diff --git a/core/src/test/java/org/apache/struts2/views/jsp/ui/SelectTest.java b/core/src/test/java/org/apache/struts2/views/jsp/ui/SelectTest.java index 094cfc921..06b7e805f 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/ui/SelectTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/ui/SelectTest.java @@ -494,7 +494,7 @@ public class SelectTest extends AbstractUITagTest { tag.setList("list2"); tag.setListKey("id"); tag.setListValue("name"); - tag.setValue("fooInt.toString()"); + tag.setValue("fooInt"); // header stuff tag.setHeaderKey("headerKey"); diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/RestActionInvocationTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/RestActionInvocationTest.java index af2a7fd01..6db05f159 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/RestActionInvocationTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/RestActionInvocationTest.java @@ -10,6 +10,7 @@ import com.opensymphony.xwork2.config.entities.InterceptorMapping; import com.opensymphony.xwork2.config.entities.ResultConfig; import com.opensymphony.xwork2.mock.MockActionProxy; import com.opensymphony.xwork2.mock.MockInterceptor; +import com.opensymphony.xwork2.ognl.OgnlUtil; import com.opensymphony.xwork2.util.XWorkTestCaseHelper; import junit.framework.TestCase; import org.apache.struts2.ServletActionContext; @@ -228,6 +229,7 @@ public class RestActionInvocationTest extends TestCase { request.setMethod("GET"); + restActionInvocation.setOgnlUtil(new OgnlUtil()); restActionInvocation.invoke(); assertEquals(123, response.getStatus()); diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java index 1b93a5c5c..e0aa8ba7e 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java @@ -1,9 +1,14 @@ package com.opensymphony.xwork2; +import com.mockobjects.dynamic.Mock; import com.opensymphony.xwork2.config.entities.InterceptorMapping; import com.opensymphony.xwork2.mock.MockActionProxy; import com.opensymphony.xwork2.mock.MockContainer; import com.opensymphony.xwork2.mock.MockInterceptor; +import com.opensymphony.xwork2.ognl.OgnlUtil; +import com.opensymphony.xwork2.util.ValueStackFactory; +import org.easymock.EasyMock; +import org.easymock.IMocksControl; import java.util.ArrayList; import java.util.HashMap; @@ -39,6 +44,9 @@ public class DefaultActionInvocationTest extends XWorkTestCase { mockInterceptor3.setExpectedFoo("test3"); DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocationTester(interceptorMappings); + container.inject(defaultActionInvocation); + defaultActionInvocation.stack = container.getInstance(ValueStackFactory.class).createValueStack(); + defaultActionInvocation.invoke(); assertTrue(mockInterceptor1.isExecuted()); assertTrue(mockInterceptor2.isExecuted()); From 53fb5ba5f89c641a92a4f7bee7584e7764741572 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 1 May 2014 09:39:55 +0200 Subject: [PATCH 08/48] Extends patterns with parenthesis during initialisation --- .../main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 81f970005..5e0697704 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -89,7 +89,11 @@ public class OgnlUtil { @Inject(value = XWorkConstants.OGNL_EXCLUDED_PROPERTIES, required = false) public void setExcludedProperties(String commaDelimitedProperties) { - excludedProperties = TextParseUtil.commaDelimitedStringToSet(commaDelimitedProperties); + Set props = TextParseUtil.commaDelimitedStringToSet(commaDelimitedProperties); + for (String prop : props) { + excludedProperties.add(prop); + excludedProperties.add(prop + "()"); + } } /** @@ -309,8 +313,7 @@ public class OgnlUtil { if (tree instanceof SimpleNode) { SimpleNode node = (SimpleNode) tree; for (String excludedPattern : excludedProperties) { - // TODO lukaszlenart: need a better way to check 'toString' and 'toString()' call - if (excludedPattern.equalsIgnoreCase(node.toString()) || (excludedPattern + "()").equalsIgnoreCase(node.toString())) { + if (excludedPattern.equalsIgnoreCase(node.toString())) { throw new OgnlException("Tree [" + (parent != null ? parent : tree) + "] trying access excluded pattern [" + excludedPattern + "]"); } for (int i = 0; i < node.jjtGetNumChildren(); i++) { From ee3c8d5630b077e2f2708bc4cbeeb933150a71fe Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 1 May 2014 09:40:33 +0200 Subject: [PATCH 09/48] Additional use cases to check method access --- .../xwork2/ognl/OgnlUtilTest.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index d4711832e..98ff6719e 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -685,6 +685,24 @@ public class OgnlUtilTest extends XWorkTestCase { assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); } + public void testAvoidCallingMethodsOnObjectClassAsMap2() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("class"); + + Exception expected = null; + try { + util.setValue("model['class']['classLoader']['defaultAssertionStatus']", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); + } + public void testAvoidCallingMethodsOnObjectClassAsMapWithQuotes() throws Exception { Foo foo = new Foo(); OgnlUtil util = new OgnlUtil(); @@ -721,6 +739,42 @@ public class OgnlUtilTest extends XWorkTestCase { assertEquals(expected.getMessage(), "Tree [toString] trying access excluded pattern [toString]"); } + public void testAvoidCallingMethodsWithBraces() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("toString"); + + Exception expected = null; + try { + util.setValue("toString()", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [toString()] trying access excluded pattern [toString()]"); + } + + public void testAvoidCallingSomeClasses() throws Exception { + Foo foo = new Foo(); + OgnlUtil util = new OgnlUtil(); + util.setEnableExpressionCache("false"); + util.setExcludedProperties("Runtime"); + + Exception expected = null; + try { + util.setValue("@java.lang.Runtime@getRuntime().exec('mate')", ActionContext.getContext().getContextMap(), foo, true); + fail(); + } catch (OgnlException e) { + expected = e; + } + assertNotNull(expected); + assertSame(expected.getClass(), OgnlException.class); + assertEquals(expected.getMessage(), "Tree [toString()] trying access excluded pattern [toString()]"); + } + public static class Email { String address; From c778297e80e19c7e16389e5c5bb3487512695c0a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:12:14 +0200 Subject: [PATCH 10/48] Extends SecurityMemberAccess to included excluded classes --- .../xwork2/ognl/SecurityMemberAccess.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 7bbcbda10..9d84702bb 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -35,6 +35,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess { private final boolean allowStaticMethodAccess; private Set excludeProperties = Collections.emptySet(); private Set acceptProperties = Collections.emptySet(); + private Set> excludedClasses = Collections.emptySet(); public SecurityMemberAccess(boolean method) { super(false); @@ -49,6 +50,9 @@ public class SecurityMemberAccess extends DefaultMemberAccess { public boolean isAccessible(Map context, Object target, Member member, String propertyName) { + if (isClassExcluded(target.getClass(), member.getDeclaringClass())) { + return false; + } boolean allow = true; int modifiers = member.getModifiers(); if (Modifier.isStatic(modifiers)) { @@ -74,6 +78,15 @@ public class SecurityMemberAccess extends DefaultMemberAccess { return isAcceptableProperty(propertyName); } + protected boolean isClassExcluded(Class targetClass, Class declaringClass) { + for (Class excludedClass : excludedClasses) { + if (targetClass.isAssignableFrom(excludedClass) || declaringClass.isAssignableFrom(excludedClass)) { + return true; + } + } + return false; + } + protected boolean isAcceptableProperty(String name) { return name == null || ((!isExcluded(name)) && isAccepted(name)); } @@ -115,4 +128,8 @@ public class SecurityMemberAccess extends DefaultMemberAccess { this.acceptProperties = acceptedProperties; } + public void setExcludedClasses(Set> excludedClasses) { + this.excludedClasses = excludedClasses; + } + } From d5bd607c6fd0cbbf12e75492e7333439758446ea Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:13:10 +0200 Subject: [PATCH 11/48] Renames excluded properties to excluded classes --- .../src/main/java/com/opensymphony/xwork2/XWorkConstants.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java index 1894372fc..dfbf6d594 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java @@ -17,6 +17,6 @@ public final class XWorkConstants { public static final String RELOAD_XML_CONFIGURATION = "reloadXmlConfiguration"; public static final String ALLOW_STATIC_METHOD_ACCESS = "allowStaticMethodAccess"; public static final String XWORK_LOGGER_FACTORY = "xwork.loggerFactory"; - public static final String OGNL_EXCLUDED_PROPERTIES = "ognlExcludedProperties"; + public static final String OGNL_EXCLUDED_CLASSES = "ognlExcludedClasses"; } From 279805721d6223673b5cb93e29fa91a4bbe0ea90 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:15:53 +0200 Subject: [PATCH 12/48] Creates default context with excluded classes --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 78 ++++++++++++------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 5e0697704..1c17ecac1 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -16,13 +16,18 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.XWorkConstants; +import com.opensymphony.xwork2.XWorkException; +import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; +import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.util.CompoundRoot; import com.opensymphony.xwork2.util.TextParseUtil; import com.opensymphony.xwork2.util.logging.Logger; import com.opensymphony.xwork2.util.logging.LoggerFactory; import com.opensymphony.xwork2.util.reflection.ReflectionException; +import ognl.ClassResolver; import ognl.Ognl; import ognl.OgnlContext; import ognl.OgnlException; @@ -61,7 +66,9 @@ public class OgnlUtil { private boolean enableExpressionCache = true; private boolean enableEvalExpression; - private Set excludedProperties = new HashSet(); + private Set> excludedClasses = new HashSet>(); + private Container container; + private boolean allowStaticMethodAccess; @Inject public void setXWorkConverter(XWorkConverter conv) { @@ -87,15 +94,32 @@ public class OgnlUtil { } } - @Inject(value = XWorkConstants.OGNL_EXCLUDED_PROPERTIES, required = false) - public void setExcludedProperties(String commaDelimitedProperties) { - Set props = TextParseUtil.commaDelimitedStringToSet(commaDelimitedProperties); - for (String prop : props) { - excludedProperties.add(prop); - excludedProperties.add(prop + "()"); + @Inject(value = XWorkConstants.OGNL_EXCLUDED_CLASSES, required = false) + public void setExcludedClasses(String commaDelimitedClasses) { + Set classes = TextParseUtil.commaDelimitedStringToSet(commaDelimitedClasses); + for (String className : classes) { + try { + excludedClasses.add(Class.forName(className)); + } catch (ClassNotFoundException e) { + throw new ConfigurationException("Cannot load excluded class: " + className, e); + } } } + public Set> getExcludedClasses() { + return excludedClasses; + } + + @Inject + public void setContainer(Container container) { + this.container = container; + } + + @Inject(value = XWorkConstants.ALLOW_STATIC_METHOD_ACCESS, required = false) + public void setAllowStaticMethodAccess(String allowStaticMethodAccess) { + this.allowStaticMethodAccess = Boolean.parseBoolean(allowStaticMethodAccess); + } + /** * Sets the object's properties using the default type converter, defaulting to not throw * exceptions for problems setting the properties. @@ -155,7 +179,7 @@ public class OgnlUtil { * problems setting the properties */ public void setProperties(Map properties, Object o, boolean throwPropertyExceptions) { - Map context = Ognl.createDefaultContext(o); + Map context = createDefaultContext(o, null); setProperties(properties, o, context, throwPropertyExceptions); } @@ -293,13 +317,11 @@ public class OgnlUtil { if (tree == null) { tree = Ognl.parseExpression(expression); checkEnableEvalExpression(tree, context); - checkExcludedPropertiesAccess(tree, null); expressions.putIfAbsent(expression, tree); } } else { tree = Ognl.parseExpression(expression); checkEnableEvalExpression(tree, context); - checkExcludedPropertiesAccess(tree, null); } @@ -309,20 +331,6 @@ public class OgnlUtil { return exec; } - private void checkExcludedPropertiesAccess(Object tree, SimpleNode parent) throws OgnlException { - if (tree instanceof SimpleNode) { - SimpleNode node = (SimpleNode) tree; - for (String excludedPattern : excludedProperties) { - if (excludedPattern.equalsIgnoreCase(node.toString())) { - throw new OgnlException("Tree [" + (parent != null ? parent : tree) + "] trying access excluded pattern [" + excludedPattern + "]"); - } - for (int i = 0; i < node.jjtGetNumChildren(); i++) { - checkExcludedPropertiesAccess(node.jjtGetChild(i), node); - } - } - } - } - public Object compile(String expression, Map context) throws OgnlException { return compileAndExecute(expression,context,new OgnlTask() { public Object execute(Object tree) throws OgnlException { @@ -359,9 +367,9 @@ public class OgnlUtil { } TypeConverter conv = getTypeConverterFromContext(context); - final Map contextFrom = Ognl.createDefaultContext(from); + final Map contextFrom = createDefaultContext(from, null); Ognl.setTypeConverter(contextFrom, conv); - final Map contextTo = Ognl.createDefaultContext(to); + final Map contextTo = createDefaultContext(to, null); Ognl.setTypeConverter(contextTo, conv); PropertyDescriptor[] fromPds; @@ -470,7 +478,7 @@ public class OgnlUtil { */ public Map getBeanMap(final Object source) throws IntrospectionException, OgnlException { Map beanMap = new HashMap(); - final Map sourceMap = Ognl.createDefaultContext(source); + final Map sourceMap = createDefaultContext(source, null); PropertyDescriptor[] propertyDescriptors = getPropertyDescriptors(source); for (PropertyDescriptor propertyDescriptor : propertyDescriptors) { final String propertyName = propertyDescriptor.getDisplayName(); @@ -548,6 +556,22 @@ public class OgnlUtil { return defaultConverter; } + protected Map createDefaultContext(Object root) { + return createDefaultContext(root, null); + } + + protected Map createDefaultContext(Object root, ClassResolver classResolver) { + ClassResolver resolver = classResolver; + if (resolver == null) { + resolver = container.getInstance(CompoundRootAccessor.class); + } + + SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticMethodAccess); + memberAccess.setExcludedClasses(excludedClasses); + + return Ognl.createDefaultContext(root, resolver, defaultConverter, memberAccess); + } + private interface OgnlTask { T execute(Object tree) throws OgnlException; } From 2180b06f7d1d38e7701e72123e57208feb4cb444 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:16:33 +0200 Subject: [PATCH 13/48] Sets excluded classes during injecting OgnlUtil --- .../main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 76f0d3fb9..83be3ed0a 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -79,6 +79,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS @Inject public void setOgnlUtil(OgnlUtil ognlUtil) { this.ognlUtil = ognlUtil; + securityMemberAccess.setExcludedClasses(ognlUtil.getExcludedClasses()); } protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, @@ -446,7 +447,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS XWorkConverter xworkConverter = cont.getInstance(XWorkConverter.class); CompoundRootAccessor accessor = (CompoundRootAccessor) cont.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()); TextProvider prov = cont.getInstance(TextProvider.class, "system"); - boolean allow = "true".equals(cont.getInstance(String.class, "allowStaticMethodAccess")); + boolean allow = "true".equals(cont.getInstance(String.class, XWorkConstants.ALLOW_STATIC_METHOD_ACCESS)); OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, allow); aStack.setOgnlUtil(cont.getInstance(OgnlUtil.class)); aStack.setRoot(xworkConverter, accessor, this.root, allow); From f0799fd99bff78f0c984922ac358d7cf3eede0ba Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:16:58 +0200 Subject: [PATCH 14/48] Adds mapping of excluded classes key --- .../org/apache/struts2/config/DefaultBeanSelectionProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index 4cc2d61fd..dedbce5ea 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -391,7 +391,7 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider convertIfExist(props, StrutsConstants.STRUTS_ENABLE_OGNL_EVAL_EXPRESSION, XWorkConstants.ENABLE_OGNL_EVAL_EXPRESSION); convertIfExist(props, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, XWorkConstants.ALLOW_STATIC_METHOD_ACCESS); convertIfExist(props, StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, XWorkConstants.RELOAD_XML_CONFIGURATION); - convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_PROPERTIES, XWorkConstants.OGNL_EXCLUDED_PROPERTIES); + convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_CLASSES, XWorkConstants.OGNL_EXCLUDED_CLASSES); LocalizedTextUtil.addDefaultResourceBundle("org/apache/struts2/struts-messages"); loadCustomResourceBundles(props); From afb5af1cc45aed1ee0404541279cb7f7853fc98b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:17:05 +0200 Subject: [PATCH 15/48] Uses excluded classes to --- core/src/main/java/org/apache/struts2/StrutsConstants.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 6be58ad36..d508373c4 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -282,7 +282,7 @@ public final class StrutsConstants { /** Allows override default DispatcherErrorHandler **/ public static final String STRUTS_DISPATCHER_ERROR_HANDLER = "struts.dispatcher.errorHandler"; - /** Comma delimited set of excluded properties which cannot be accessed via expressions **/ - public static final String STRUTS_EXCLUDED_PROPERTIES = "struts.excludedProperties"; + /** Comma delimited set of excluded classes which cannot be accessed via expressions **/ + public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses"; } From cdfb94d712e2b71bcf42f87f6c1b7d02d784dd87 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:17:19 +0200 Subject: [PATCH 16/48] Updates test to use new excluded classes --- .../impl/AnnotationXWorkConverterTest.java | 10 +- .../xwork2/ognl/OgnlUtilTest.java | 115 ++++++++---------- .../xwork2/ognl/OgnlValueStackTest.java | 1 + 3 files changed, 54 insertions(+), 72 deletions(-) diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/conversion/impl/AnnotationXWorkConverterTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/conversion/impl/AnnotationXWorkConverterTest.java index 4a7f5175b..14d9be186 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/conversion/impl/AnnotationXWorkConverterTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/conversion/impl/AnnotationXWorkConverterTest.java @@ -374,8 +374,8 @@ public class AnnotationXWorkConverterTest extends XWorkTestCase { stack.setValue("genericMap[456.12]", "42"); assertEquals(2, gb.getGenericMap().size()); - assertEquals(Integer.class, stack.findValue("genericMap.get(123.12).class")); - assertEquals(Integer.class, stack.findValue("genericMap.get(456.12).class")); + assertEquals("66", stack.findValue("genericMap.get(123.12).toString()")); + assertEquals("42", stack.findValue("genericMap.get(456.12).toString()")); assertEquals(66, stack.findValue("genericMap.get(123.12)")); assertEquals(42, stack.findValue("genericMap.get(456.12)")); assertEquals(true, stack.findValue("genericMap.containsValue(66)")); @@ -393,8 +393,8 @@ public class AnnotationXWorkConverterTest extends XWorkTestCase { stack.setValue("genericMap[456.12]", "42"); assertEquals(2, gb.getGenericMap().size()); - assertEquals(Integer.class, stack.findValue("genericMap.get(123.12).class")); - assertEquals(Integer.class, stack.findValue("genericMap.get(456.12).class")); + assertEquals("66", stack.findValue("genericMap.get(123.12).toString()")); + assertEquals("42", stack.findValue("genericMap.get(456.12).toString()")); assertEquals(66, stack.findValue("genericMap.get(123.12)")); assertEquals(42, stack.findValue("genericMap.get(456.12)")); assertEquals(true, stack.findValue("genericMap.containsValue(66)")); @@ -409,7 +409,7 @@ public class AnnotationXWorkConverterTest extends XWorkTestCase { stack.push(gb); assertEquals(1, gb.getGetterList().size()); - assertEquals(Double.class, stack.findValue("getterList.get(0).class")); + assertEquals("42.42", stack.findValue("getterList.get(0).toString()")); assertEquals(new Double(42.42), stack.findValue("getterList.get(0)")); assertEquals(new Double(42.42), gb.getGetterList().get(0)); diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index 98ff6719e..e8733d6b4 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -82,7 +82,7 @@ public class OgnlUtilTest extends XWorkTestCase { }); Owner owner = new Owner(); - Map context = Ognl.createDefaultContext(owner); + Map context = ognlUtil.createDefaultContext(owner); Map props = new HashMap(); props.put("dog.name", dogName); @@ -107,7 +107,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testCanSetDependentObjectArray() { EmailAction action = new EmailAction(); - Map context = Ognl.createDefaultContext(action); + Map context = ognlUtil.createDefaultContext(action); Map props = new HashMap(); props.put("email[0].address", "addr1"); @@ -125,7 +125,7 @@ public class OgnlUtilTest extends XWorkTestCase { Foo foo1 = new Foo(); Foo foo2 = new Foo(); - Map context = Ognl.createDefaultContext(foo1); + Map context = ognlUtil.createDefaultContext(foo1); Calendar cal = Calendar.getInstance(); cal.clear(); @@ -171,7 +171,7 @@ public class OgnlUtilTest extends XWorkTestCase { foo2.setTitle("foo2 title"); foo2.setNumber(2); - Map context = Ognl.createDefaultContext(foo1); + Map context = ognlUtil.createDefaultContext(foo1); List excludes = new ArrayList(); excludes.add("title"); @@ -200,7 +200,7 @@ public class OgnlUtilTest extends XWorkTestCase { b2.setTitle(""); b2.setId(new Long(2)); - context = Ognl.createDefaultContext(b1); + context = ognlUtil.createDefaultContext(b1); List includes = new ArrayList(); includes.add("title"); includes.add("somethingElse"); @@ -220,7 +220,7 @@ public class OgnlUtilTest extends XWorkTestCase { Foo foo = new Foo(); Bar bar = new Bar(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Calendar cal = Calendar.getInstance(); cal.clear(); @@ -244,7 +244,7 @@ public class OgnlUtilTest extends XWorkTestCase { Foo foo = new Foo(); foo.setBar(new Bar()); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("bar.title", "i am barbaz"); @@ -280,7 +280,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testOgnlHandlesCrapAtTheEndOfANumber() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("aLong", "123a"); @@ -317,7 +317,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testSetPropertiesBoolean() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("useful", "true"); @@ -338,7 +338,7 @@ public class OgnlUtilTest extends XWorkTestCase { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("birthday", "02/12/1982"); @@ -408,7 +408,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testSetPropertiesInt() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("number", "2"); @@ -420,7 +420,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testSetPropertiesLongArray() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("points", new String[]{"1", "2"}); @@ -435,7 +435,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testSetPropertiesString() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("title", "this is a title"); @@ -446,7 +446,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testSetProperty() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); assertFalse(123456 == foo.getNumber()); ognlUtil.setProperty("number", "123456", foo, context); assertEquals(123456, foo.getNumber()); @@ -457,7 +457,7 @@ public class OgnlUtilTest extends XWorkTestCase { ChainingInterceptor foo = new ChainingInterceptor(); ChainingInterceptor foo2 = new ChainingInterceptor(); - OgnlContext context = (OgnlContext) Ognl.createDefaultContext(null); + OgnlContext context = (OgnlContext) ognlUtil.createDefaultContext(null); SimpleNode expression = (SimpleNode) Ognl.parseExpression("{'a','ruby','b','tom'}"); @@ -499,7 +499,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testStringToLong() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); Map props = new HashMap(); props.put("aLong", "123"); @@ -518,7 +518,7 @@ public class OgnlUtilTest extends XWorkTestCase { Foo foo = new Foo(); foo.setALong(88); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); ognlUtil.setProperties(null, foo, context); assertEquals(88, foo.getALong()); @@ -531,7 +531,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testCopyNull() { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); ognlUtil.copy(null, null, context); ognlUtil.copy(foo, null, context); @@ -540,7 +540,7 @@ public class OgnlUtilTest extends XWorkTestCase { public void testGetTopTarget() throws Exception { Foo foo = new Foo(); - Map context = Ognl.createDefaultContext(foo); + Map context = ognlUtil.createDefaultContext(foo); CompoundRoot root = new CompoundRoot(); Object top = ognlUtil.getRealTarget("top", context, root); @@ -633,146 +633,127 @@ public class OgnlUtilTest extends XWorkTestCase { public void testAvoidCallingMethodsOnObjectClass() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("class"); Exception expected = null; try { - util.setValue("class.classLoader.defaultAssertionStatus", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setExcludedClasses(Object.class.getName()); + ognlUtil.setValue("class.classLoader.defaultAssertionStatus", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [class.classLoader.defaultAssertionStatus] trying access excluded pattern [class]"); + assertSame(NoSuchPropertyException.class, expected.getClass()); + assertEquals("com.opensymphony.xwork2.util.Foo.class", expected.getMessage()); } public void testAvoidCallingMethodsOnObjectClassUpperCased() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("class"); Exception expected = null; try { - util.setValue("Class.ClassLoader.DefaultAssertionStatus", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setExcludedClasses(Object.class.getName()); + ognlUtil.setValue("Class.ClassLoader.DefaultAssertionStatus", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [Class.ClassLoader.DefaultAssertionStatus] trying access excluded pattern [class]"); + assertSame(NoSuchPropertyException.class, expected.getClass()); + assertEquals("com.opensymphony.xwork2.util.Foo.Class", expected.getMessage()); } public void testAvoidCallingMethodsOnObjectClassAsMap() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("class"); Exception expected = null; try { - util.setValue("class['classLoader']['defaultAssertionStatus']", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setExcludedClasses(Object.class.getName()); + ognlUtil.setValue("class['classLoader']['defaultAssertionStatus']", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); + assertSame(NoSuchPropertyException.class, expected.getClass()); + assertEquals("com.opensymphony.xwork2.util.Foo.class", expected.getMessage()); } public void testAvoidCallingMethodsOnObjectClassAsMap2() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("class"); Exception expected = null; try { - util.setValue("model['class']['classLoader']['defaultAssertionStatus']", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setValue("foo['class']['classLoader']['defaultAssertionStatus']", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); + assertSame(NoSuchPropertyException.class, expected.getClass()); + assertEquals("com.opensymphony.xwork2.util.Foo.foo", expected.getMessage()); } public void testAvoidCallingMethodsOnObjectClassAsMapWithQuotes() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("class"); Exception expected = null; try { - util.setValue("class[\"classLoader\"]['defaultAssertionStatus']", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setExcludedClasses(Object.class.getName()); + ognlUtil.setValue("class[\"classLoader\"]['defaultAssertionStatus']", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [class[\"classLoader\"][\"defaultAssertionStatus\"]] trying access excluded pattern [class]"); + assertSame(NoSuchPropertyException.class, expected.getClass()); + assertEquals("com.opensymphony.xwork2.util.Foo.class", expected.getMessage()); } public void testAvoidCallingToString() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("toString"); Exception expected = null; try { - util.setValue("toString", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setValue("toString", ognlUtil.createDefaultContext(foo), foo, null); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [toString] trying access excluded pattern [toString]"); + assertSame(OgnlException.class, expected.getClass()); + assertEquals("toString", expected.getMessage()); } public void testAvoidCallingMethodsWithBraces() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("toString"); Exception expected = null; try { - util.setValue("toString()", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setValue("toString()", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [toString()] trying access excluded pattern [toString()]"); + assertSame(InappropriateExpressionException.class, expected.getClass()); + assertEquals(expected.getMessage(), "Inappropriate OGNL expression: toString()"); } public void testAvoidCallingSomeClasses() throws Exception { Foo foo = new Foo(); - OgnlUtil util = new OgnlUtil(); - util.setEnableExpressionCache("false"); - util.setExcludedProperties("Runtime"); Exception expected = null; try { - util.setValue("@java.lang.Runtime@getRuntime().exec('mate')", ActionContext.getContext().getContextMap(), foo, true); + ognlUtil.setExcludedClasses(Runtime.class.getName()); + ognlUtil.setValue("@java.lang.Runtime@getRuntime().exec('mate')", ognlUtil.createDefaultContext(foo), foo, true); fail(); } catch (OgnlException e) { expected = e; } assertNotNull(expected); - assertSame(expected.getClass(), OgnlException.class); - assertEquals(expected.getMessage(), "Tree [toString()] trying access excluded pattern [toString()]"); + assertSame(MethodFailedException.class, expected.getClass()); + assertEquals(expected.getMessage(), "Method \"getRuntime\" failed for object class java.lang.Runtime"); } public static class Email { diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index a4a153af4..cb7108134 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -58,6 +58,7 @@ public class OgnlValueStackTest extends XWorkTestCase { (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()), container.getInstance(TextProvider.class, "system"), allowStaticMethodAccess); container.inject(stack); + ognlUtil.setAllowStaticMethodAccess(Boolean.toString(allowStaticMethodAccess)); return stack; } From f84efa5f42a31ecbcbe3eba28653a57829e598b8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 3 May 2014 20:18:44 +0200 Subject: [PATCH 17/48] Defines excluded classes --- core/src/main/resources/struts-default.xml | 2 +- xwork-core/src/test/resources/xwork-param-test.xml | 2 +- xwork-core/src/test/resources/xwork-test-beans.xml | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 7cb687ef5..0e4c419c8 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -38,7 +38,7 @@ - + diff --git a/xwork-core/src/test/resources/xwork-param-test.xml b/xwork-core/src/test/resources/xwork-param-test.xml index 3ca616a5b..01787f70e 100644 --- a/xwork-core/src/test/resources/xwork-param-test.xml +++ b/xwork-core/src/test/resources/xwork-param-test.xml @@ -4,5 +4,5 @@ - + \ No newline at end of file diff --git a/xwork-core/src/test/resources/xwork-test-beans.xml b/xwork-core/src/test/resources/xwork-test-beans.xml index 3fa5b2838..7268ef70b 100644 --- a/xwork-core/src/test/resources/xwork-test-beans.xml +++ b/xwork-core/src/test/resources/xwork-test-beans.xml @@ -3,11 +3,11 @@ "http://struts.apache.org/dtds/xwork-2.0.dtd"> - - - - + + \ No newline at end of file From b3ca9ea5e31fc9b6c0a5e644e833874bb7cc62fa Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 4 May 2014 11:18:00 +0200 Subject: [PATCH 19/48] Adds special treatment of Object class and unit test --- .../xwork2/ognl/SecurityMemberAccess.java | 11 +- .../xwork2/ognl/SecurityMemberAccessTest.java | 139 ++++++++++++++++++ 2 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 9d84702bb..7fe77c338 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -21,6 +21,7 @@ import java.lang.reflect.Member; import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Collections; +import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; @@ -47,8 +48,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess { } @Override - public boolean isAccessible(Map context, Object target, Member member, - String propertyName) { + public boolean isAccessible(Map context, Object target, Member member, String propertyName) { if (isClassExcluded(target.getClass(), member.getDeclaringClass())) { return false; @@ -79,8 +79,11 @@ public class SecurityMemberAccess extends DefaultMemberAccess { } protected boolean isClassExcluded(Class targetClass, Class declaringClass) { - for (Class excludedClass : excludedClasses) { - if (targetClass.isAssignableFrom(excludedClass) || declaringClass.isAssignableFrom(excludedClass)) { + if (targetClass == Object.class || declaringClass == Object.class) { + return true; + } + for (Class excludedClass : excludedClasses) { + if (excludedClass.isAssignableFrom(targetClass) || declaringClass.isAssignableFrom(excludedClass)) { return true; } } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java new file mode 100644 index 000000000..4ccc831f2 --- /dev/null +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -0,0 +1,139 @@ +package com.opensymphony.xwork2.ognl; + +import junit.framework.TestCase; + +import java.lang.reflect.Member; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +public class SecurityMemberAccessTest extends TestCase { + + private Map context; + private FooBar target; + + @Override + public void setUp() throws Exception { + context = new HashMap(); + target = new FooBar(); + } + + public void testWithoutClassExclusion() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "stringField"; + Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue(accessible); + } + + public void testClassExclusion() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "stringField"; + Member member = FooBar.class.getDeclaredMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + + Set> excluded = new HashSet>(); + excluded.add(FooBar.class); + sma.setExcludedClasses(excluded); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse(accessible); + } + + public void testObjectClassExclusion() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "toString"; + Member member = FooBar.class.getMethod(propertyName); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse("toString() from Object is accessible!!!", accessible); + } + + public void testObjectOverwrittenMethodsExclusion() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "hashCode"; + Member member = FooBar.class.getMethod(propertyName); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue("hashCode() from FooBar isn't accessible!!!", accessible); + } + + public void testInterfaceInheritanceExclusion() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "barLogic"; + Member member = FooBar.class.getMethod("barLogic"); + + Set> excluded = new HashSet>(); + excluded.add(BarInterface.class); + sma.setExcludedClasses(excluded); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse("barLogic() from BarInterface is accessible!!!", accessible); + } + +} + +class FooBar implements FooInterface { + + private String stringField; + + public String getStringField() { + return stringField; + } + + public void setStringField(String stringField) { + this.stringField = stringField; + } + + public String fooLogic() { + return "fooLogic"; + } + + public String barLogic() { + return "barLogic"; + } + + @Override + public int hashCode() { + return 1; + } + +} + +interface FooInterface extends BarInterface { + + String fooLogic(); + +} + +interface BarInterface { + + String barLogic(); + +} From ba0ac0dfd47c768661fcd5fa12bb00af851eb548 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 4 May 2014 11:58:08 +0200 Subject: [PATCH 20/48] Adds more use cases --- .../xwork2/ognl/SecurityMemberAccess.java | 4 +- .../xwork2/ognl/SecurityMemberAccessTest.java | 84 ++++++++++++++++++- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 7fe77c338..a35f68bbf 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -49,10 +49,10 @@ public class SecurityMemberAccess extends DefaultMemberAccess { @Override public boolean isAccessible(Map context, Object target, Member member, String propertyName) { - if (isClassExcluded(target.getClass(), member.getDeclaringClass())) { return false; } + boolean allow = true; int modifiers = member.getModifiers(); if (Modifier.isStatic(modifiers)) { @@ -83,7 +83,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess { return true; } for (Class excludedClass : excludedClasses) { - if (excludedClass.isAssignableFrom(targetClass) || declaringClass.isAssignableFrom(excludedClass)) { + if (targetClass.isAssignableFrom(excludedClass) || declaringClass.isAssignableFrom(excludedClass)) { return true; } } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index 4ccc831f2..1c14cb265 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -84,7 +84,7 @@ public class SecurityMemberAccessTest extends TestCase { SecurityMemberAccess sma = new SecurityMemberAccess(false); String propertyName = "barLogic"; - Member member = FooBar.class.getMethod("barLogic"); + Member member = BarInterface.class.getMethod(propertyName); Set> excluded = new HashSet>(); excluded.add(BarInterface.class); @@ -97,9 +97,83 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("barLogic() from BarInterface is accessible!!!", accessible); } + public void testMiddleOfInheritanceExclusion1() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "fooLogic"; + Member member = FooBar.class.getMethod(propertyName); + + Set> excluded = new HashSet>(); + excluded.add(BarInterface.class); + sma.setExcludedClasses(excluded); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue("fooLogic() from FooInterface isn't accessible!!!", accessible); + } + + public void testMiddleOfInheritanceExclusion2() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "barLogic"; + Member member = BarInterface.class.getMethod(propertyName); + + Set> excluded = new HashSet>(); + excluded.add(BarInterface.class); + sma.setExcludedClasses(excluded); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse("barLogic() from BarInterface is accessible!!!", accessible); + } + + public void testMiddleOfInheritanceExclusion3() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "barLogic"; + Member member = BarInterface.class.getMethod(propertyName); + +/* + Set> excluded = new HashSet>(); + excluded.add(BarInterface.class); + sma.setExcludedClasses(excluded); +*/ + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue("barLogic() from BarInterface isn't accessible!!!", accessible); + } + + public void testMiddleOfInheritanceExclusion4() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + String propertyName = "barLogic"; + Member member = BarInterface.class.getMethod(propertyName); + + Set> excluded = new HashSet>(); + excluded.add(FooBarInterface.class); + sma.setExcludedClasses(excluded); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse("barLogic() from BarInterface is accessible!!!", accessible); + } + } -class FooBar implements FooInterface { +class FooBar implements FooBarInterface { private String stringField; @@ -126,7 +200,7 @@ class FooBar implements FooInterface { } -interface FooInterface extends BarInterface { +interface FooInterface { String fooLogic(); @@ -137,3 +211,7 @@ interface BarInterface { String barLogic(); } + +interface FooBarInterface extends FooInterface, BarInterface { + +} \ No newline at end of file From 62ee6b104ae871807ff073eb206b5f3ec549a302 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 5 May 2014 21:33:16 +0200 Subject: [PATCH 21/48] Adds logging of excluded classes --- .../opensymphony/xwork2/ognl/SecurityMemberAccess.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index a35f68bbf..c14d8b917 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -15,13 +15,14 @@ */ package com.opensymphony.xwork2.ognl; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; import ognl.DefaultMemberAccess; import java.lang.reflect.Member; import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Collections; -import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; @@ -33,6 +34,8 @@ import java.util.regex.Pattern; */ public class SecurityMemberAccess extends DefaultMemberAccess { + private static final Logger LOG = LoggerFactory.getLogger(SecurityMemberAccess.class); + private final boolean allowStaticMethodAccess; private Set excludeProperties = Collections.emptySet(); private Set acceptProperties = Collections.emptySet(); @@ -50,6 +53,9 @@ public class SecurityMemberAccess extends DefaultMemberAccess { @Override public boolean isAccessible(Map context, Object target, Member member, String propertyName) { if (isClassExcluded(target.getClass(), member.getDeclaringClass())) { + if (LOG.isDebugEnabled()) { + LOG.debug("Target class [#0] and member type [#1] are excluded!", target, member); + } return false; } From 7857b869a05b12779e35bfe8751828dfbf328fff Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 5 May 2014 21:33:44 +0200 Subject: [PATCH 22/48] Removes override which isn't used anymore --- .../interceptor/ParametersInterceptorTest.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 6ffb3ff2e..359618f0e 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -187,10 +187,6 @@ public class ParametersInterceptorTest extends XWorkTestCase { return result; } - @Override - protected void initializeHardCodedExcludePatterns() { - excludeParams = new HashSet(); - } }; container.inject(pi); @@ -306,11 +302,6 @@ public class ParametersInterceptorTest extends XWorkTestCase { final Map excluded = new HashMap(); ParametersInterceptor pi = new ParametersInterceptor() { - @Override - protected void initializeHardCodedExcludePatterns() { - this.excludeParams = new HashSet(); - } - @Override protected boolean isExcluded(String paramName) { boolean result = super.isExcluded(paramName); @@ -744,11 +735,6 @@ public class ParametersInterceptorTest extends XWorkTestCase { assertEquals(expected, actual); } - public void testExcludedPatternsGetInitialized() throws Exception { - ParametersInterceptor parametersInterceptor = new ParametersInterceptor(); - assertEquals(ExcludedPatterns.EXCLUDED_PATTERNS.length, parametersInterceptor.excludeParams.size()); - } - private ValueStack injectValueStack(Map actual) { ValueStack stack = createStubValueStack(actual); container.inject(stack); From 65c023b6f3e848fae13135ee90c101a0d0e2f262 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 12 May 2014 08:26:12 +0200 Subject: [PATCH 23/48] Converts class with patterns into Struts bean --- core/src/main/resources/struts-default.xml | 4 + .../opensymphony/xwork2/ExcludedPatterns.java | 22 --- .../xwork2/ExcludedPatternsChecker.java | 135 ++++++++++++++++++ 3 files changed, 139 insertions(+), 22 deletions(-) delete mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatterns.java create mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 1f37ea2f5..554a8ba03 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -144,6 +144,10 @@ + + + + diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatterns.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatterns.java deleted file mode 100644 index b618a52a0..000000000 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatterns.java +++ /dev/null @@ -1,22 +0,0 @@ -package com.opensymphony.xwork2; - -/** - * ExcludedPatterns contains hard-coded patterns that must be rejected by {@link com.opensymphony.xwork2.interceptor.ParametersInterceptor} - * and partially in CookInterceptor - */ -public class ExcludedPatterns { - - public static final String CLASS_ACCESS_PATTERN = "(.*\\.|^|.*|\\[('|\"))class(\\.|('|\")]|\\[).*"; - - public static final String[] EXCLUDED_PATTERNS = { - CLASS_ACCESS_PATTERN, - "^dojo\\..*", - "^struts\\..*", - "^session\\..*", - "^request\\..*", - "^application\\..*", - "^servlet(Request|Response)\\..*", - "^parameters\\..*" - }; - -} diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java new file mode 100644 index 000000000..ee3eea6e9 --- /dev/null +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java @@ -0,0 +1,135 @@ +package com.opensymphony.xwork2; + +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Pattern; + +/** + * Used across different interceptors to check if given string matches one of the excluded patterns. + * User has two options to change its behaviour: + * - define new set of patterns with + * - override this class and use then extension point + * to inject it in appropriated places + */ +public class ExcludedPatternsChecker { + + private static final Logger LOG = LoggerFactory.getLogger(ExcludedPatternsChecker.class); + + public static final String[] EXCLUDED_PATTERNS = { + "(.*\\.|^|.*|\\[('|\"))class(\\.|('|\")]|\\[).*", + "^dojo\\..*", + "^struts\\..*", + "^session\\..*", + "^request\\..*", + "^application\\..*", + "^servlet(Request|Response)\\..*", + "^parameters\\..*" + }; + + private Set excludedPatterns; + + public ExcludedPatternsChecker() { + excludedPatterns = new HashSet(); + for (String pattern : EXCLUDED_PATTERNS) { + excludedPatterns.add(Pattern.compile(pattern)); + } + } + + @Inject(value = XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, required = false) + public void setOverrideExcludePatterns(String excludePatterns) { + if (LOG.isWarnEnabled()) { + LOG.warn("Overriding [#0] with [#1], be aware that this can affect safety of your application!", + XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, excludePatterns); + } + excludedPatterns = new HashSet(); + for (String pattern : TextParseUtil.commaDelimitedStringToSet(excludePatterns)) { + excludedPatterns.add(Pattern.compile(pattern)); + } + } + + /** + * Allows add additional excluded patterns during runtime + * + * @param commaDelimitedPatterns comma delimited string with patterns + */ + public void addExcludedPatterns(String commaDelimitedPatterns) { + addExcludedPatterns(TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns)); + } + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns array of additional excluded patterns + */ + public void addExcludedPatterns(String[] additionalPatterns) { + addExcludedPatterns(new HashSet(Arrays.asList(additionalPatterns))); + } + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns set of additional patterns + */ + public void addExcludedPatterns(Set additionalPatterns) { + if (LOG.isTraceEnabled()) { + LOG.trace("Adding additional excluded patterns [#0]", additionalPatterns); + } + for (String pattern : additionalPatterns) { + excludedPatterns.add(Pattern.compile(pattern)); + } + } + + public IsExcluded isExcluded(String value) { + for (Pattern excludedPattern : excludedPatterns) { + if (excludedPattern.matcher(value).matches()) { + if (LOG.isTraceEnabled()) { + LOG.trace("[#0] matches excluded pattern [#1]", value, excludedPattern); + } + return IsExcluded.yes(excludedPattern); + } + } + return IsExcluded.no(); + } + + public final static class IsExcluded { + + private final boolean excluded; + private final Pattern excludedPattern; + + public static IsExcluded yes(Pattern excludedPattern) { + return new IsExcluded(true, excludedPattern); + } + + public static IsExcluded no() { + return new IsExcluded(false, null); + } + + private IsExcluded(boolean excluded, Pattern excludedPattern) { + this.excluded = excluded; + this.excludedPattern = excludedPattern; + } + + public boolean isExcluded() { + return excluded; + } + + public Pattern getExcludedPattern() { + return excludedPattern; + } + + @Override + public String toString() { + return "IsExcluded { " + + "excluded=" + excluded + + ", excludedPattern=" + excludedPattern + + " }"; + } + } + +} From 4577e5eefb057e80bbdd740b0c56120c15469827 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 12 May 2014 08:26:33 +0200 Subject: [PATCH 24/48] Defines new extension point --- core/src/main/java/org/apache/struts2/StrutsConstants.java | 6 ++++++ .../struts2/config/DefaultBeanSelectionProvider.java | 7 ++++++- .../main/java/com/opensymphony/xwork2/XWorkConstants.java | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index d508373c4..d173add5a 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -285,4 +285,10 @@ public final class StrutsConstants { /** Comma delimited set of excluded classes which cannot be accessed via expressions **/ public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses"; + /** Dedicated service to check if passed string is excluded or not **/ + public static final String STRUTS_EXCLUDED_PATTERNS_CHECKER = "struts.excludedPatterns.checker"; + + /** Constant is used to override framework's default excluded patterns **/ + public static final String STRUTS_OVERRIDE_EXCLUDED_PATTERNS = "struts.override.excludedPatterns"; + } diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index dedbce5ea..530491014 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -22,6 +22,7 @@ package org.apache.struts2.config; import com.opensymphony.xwork2.ActionProxyFactory; +import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.FileManager; import com.opensymphony.xwork2.FileManagerFactory; import com.opensymphony.xwork2.LocaleProvider; @@ -343,7 +344,7 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider alias(ResultFactory.class, StrutsConstants.STRUTS_OBJECTFACTORY_RESULTFACTORY, builder, props); alias(ConverterFactory.class, StrutsConstants.STRUTS_OBJECTFACTORY_CONVERTERFACTORY, builder, props); alias(InterceptorFactory.class, StrutsConstants.STRUTS_OBJECTFACTORY_INTERCEPTORFACTORY, builder, props); - alias(ValidatorFactory.class, StrutsConstants.STRUTS_OBJECTFACTORY_INTERCEPTORFACTORY, builder, props); + alias(ValidatorFactory.class, StrutsConstants.STRUTS_OBJECTFACTORY_VALIDATORFACTORY, builder, props); alias(FileManagerFactory.class, StrutsConstants.STRUTS_FILE_MANAGER_FACTORY, builder, props, Scope.SINGLETON); @@ -383,6 +384,9 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider alias(DispatcherErrorHandler.class, StrutsConstants.STRUTS_DISPATCHER_ERROR_HANDLER, builder, props); + /** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.REQUEST **/ + alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.REQUEST); + switchDevMode(props); // Convert Struts properties into XWork properties @@ -392,6 +396,7 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider convertIfExist(props, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, XWorkConstants.ALLOW_STATIC_METHOD_ACCESS); convertIfExist(props, StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, XWorkConstants.RELOAD_XML_CONFIGURATION); convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_CLASSES, XWorkConstants.OGNL_EXCLUDED_CLASSES); + convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_EXCLUDED_PATTERNS, XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS); LocalizedTextUtil.addDefaultResourceBundle("org/apache/struts2/struts-messages"); loadCustomResourceBundles(props); diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java index dfbf6d594..f2f03e78e 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java @@ -18,5 +18,6 @@ public final class XWorkConstants { public static final String ALLOW_STATIC_METHOD_ACCESS = "allowStaticMethodAccess"; public static final String XWORK_LOGGER_FACTORY = "xwork.loggerFactory"; public static final String OGNL_EXCLUDED_CLASSES = "ognlExcludedClasses"; + public static final String OVERRIDE_EXCLUDED_PATTERNS = "overrideExcludedPatterns"; } From 9884c49fd0d4683d3376070bc75d88a4afcb6a25 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 12 May 2014 08:26:50 +0200 Subject: [PATCH 25/48] Cleans up imports --- .../xwork2/interceptor/ParametersInterceptorTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 359618f0e..a2aa92bf6 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -18,7 +18,6 @@ package com.opensymphony.xwork2.interceptor; import com.opensymphony.xwork2.Action; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionProxy; -import com.opensymphony.xwork2.ExcludedPatterns; import com.opensymphony.xwork2.ModelDrivenAction; import com.opensymphony.xwork2.SimpleAction; import com.opensymphony.xwork2.TestBean; @@ -47,12 +46,10 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; -import java.util.regex.Pattern; /** From 735fd96114413181defb17cd49aa75da232a7040 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 12 May 2014 08:27:30 +0200 Subject: [PATCH 26/48] Uses newly defined Struts bean instead duplicating logic --- .../interceptor/CookieInterceptor.java | 49 +++++++------------ .../interceptor/CookieInterceptorTest.java | 11 +++++ 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index 340b57f81..8998c5cb1 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java @@ -23,17 +23,18 @@ package org.apache.struts2.interceptor; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.interceptor.AbstractInterceptor; -import com.opensymphony.xwork2.ExcludedPatterns; +import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.util.TextParseUtil; import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.util.logging.Logger; import com.opensymphony.xwork2.util.logging.LoggerFactory; import org.apache.struts2.ServletActionContext; +import org.apache.struts2.StrutsConstants; import javax.servlet.http.Cookie; import java.util.Collections; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; @@ -176,12 +177,12 @@ public class CookieInterceptor extends AbstractInterceptor { // Allowed names of cookies private Pattern acceptedPattern = Pattern.compile(ACCEPTED_PATTERN, Pattern.CASE_INSENSITIVE); - private Set excludedPatterns = new HashSet(); - public CookieInterceptor() { - for (String pattern : ExcludedPatterns.EXCLUDED_PATTERNS) { - excludedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); - } + private ExcludedPatternsChecker excludedPatternsChecker; + + @Inject(StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER) + public void setExcludedPatternsChecker(ExcludedPatternsChecker excludedPatternsChecker) { + this.excludedPatternsChecker = excludedPatternsChecker; } /** @@ -260,16 +261,7 @@ public class CookieInterceptor extends AbstractInterceptor { * @return true|false */ protected boolean isAcceptableValue(String value) { - for (Pattern excludedPattern : excludedPatterns) { - boolean matches = !excludedPattern.matcher(value).matches(); - if (!matches) { - if (LOG.isTraceEnabled()) { - LOG.trace("Cookie value [#0] matches excludedPattern [#1]", value, excludedPattern.toString()); - } - return false; - } - } - return true; + return !isExcluded(value) && isAccepted(value); } /** @@ -283,7 +275,7 @@ public class CookieInterceptor extends AbstractInterceptor { } /** - * Checks if name of Cookie match {@link #acceptedPattern} + * Checks if name/value of Cookie is acceptable * * @param name of Cookie * @return true|false @@ -303,24 +295,21 @@ public class CookieInterceptor extends AbstractInterceptor { } /** - * Checks if name of Cookie match {@link #excludedPatterns} + * Checks if name/value of Cookie is excluded * * @param name of Cookie * @return true|false */ protected boolean isExcluded(String name) { - for (Pattern excludedPattern : excludedPatterns) { - boolean matches = excludedPattern.matcher(name).matches(); - if (matches) { - if (LOG.isTraceEnabled()) { - LOG.trace("Cookie [#0] matches excludedPattern [#1]", name, excludedPattern.toString()); - } - return true; - } else { - if (LOG.isTraceEnabled()) { - LOG.trace("Cookie [#0] doesn't match excludedPattern [#1]", name, excludedPattern.toString()); - } + ExcludedPatternsChecker.IsExcluded excluded = excludedPatternsChecker.isExcluded(name); + if (excluded.isExcluded()) { + if (LOG.isTraceEnabled()) { + LOG.trace("Cookie [#0] matches excludedPattern [#1]", name, excluded.getExcludedPattern()); } + return true; + } + if (LOG.isTraceEnabled()) { + LOG.trace("Cookie [#0] doesn't match excludedPattern [#1]", name, excluded.getExcludedPattern()); } return false; } diff --git a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java index 99ba15164..2bbaef9bc 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java @@ -27,6 +27,7 @@ import java.util.Map; import javax.servlet.http.Cookie; +import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.mock.MockActionInvocation; import org.easymock.MockControl; import org.springframework.mock.web.MockHttpServletRequest; @@ -65,6 +66,8 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { // by default the interceptor doesn't accept any cookies CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.intercept(invocation); assertTrue(action.getCookiesMap().isEmpty()); @@ -99,6 +102,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("*"); interceptor.setCookiesValue("*"); interceptor.intercept(invocation); @@ -140,6 +144,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie2, cookie3"); interceptor.setCookiesValue("cookie1value, cookie2value, cookie3value"); interceptor.intercept(invocation); @@ -180,6 +185,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("cookie1value, cookie2value, cookie3value"); interceptor.intercept(invocation); @@ -220,6 +226,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("*"); interceptor.intercept(invocation); @@ -260,6 +267,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue(""); interceptor.intercept(invocation); @@ -301,6 +309,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("cookie1value"); interceptor.intercept(invocation); @@ -361,6 +370,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { return accepted; } }; + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); @@ -420,6 +430,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { return accepted; } }; + interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); From ba1850a1382765eb51c58103a8c5ee7c0d9417f4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 13 May 2014 20:28:26 +0200 Subject: [PATCH 27/48] Adds description about new extension point --- .../apache/struts2/config/DefaultBeanSelectionProvider.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index 530491014..5296b41b8 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -313,6 +313,12 @@ import java.util.StringTokenizer; * Used to parse expressions like ${foo.bar} or %{bar.foo} but it is up tp the TextParser's * implementation what kind of opening char to use (#, $, %, etc) * + * + * com.opensymphony.xwork2.ExcludedPatternsChecker + * struts.excludedPatterns.checker + * request + * Used across different interceptors to check if given string matches one of the excluded patterns + * * * * From bfbc4c04e007393986f374a02dfb7ded23bc9a05 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 13 May 2014 20:29:21 +0200 Subject: [PATCH 28/48] Extracts interface to simplify implementation by users --- core/src/main/resources/struts-default.xml | 2 +- .../interceptor/CookieInterceptorTest.java | 20 ++-- .../DefaultExcludedPatternsChecker.java | 93 +++++++++++++++++++ .../xwork2/ExcludedPatternsChecker.java | 92 +----------------- 4 files changed, 106 insertions(+), 101 deletions(-) create mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 554a8ba03..f2fb9222f 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -144,7 +144,7 @@ - + diff --git a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java index 2bbaef9bc..1f642f598 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java @@ -27,7 +27,7 @@ import java.util.Map; import javax.servlet.http.Cookie; -import com.opensymphony.xwork2.ExcludedPatternsChecker; +import com.opensymphony.xwork2.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.mock.MockActionInvocation; import org.easymock.MockControl; import org.springframework.mock.web.MockHttpServletRequest; @@ -66,7 +66,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { // by default the interceptor doesn't accept any cookies CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.intercept(invocation); @@ -102,7 +102,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("*"); interceptor.setCookiesValue("*"); interceptor.intercept(invocation); @@ -144,7 +144,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie2, cookie3"); interceptor.setCookiesValue("cookie1value, cookie2value, cookie3value"); interceptor.intercept(invocation); @@ -185,7 +185,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("cookie1value, cookie2value, cookie3value"); interceptor.intercept(invocation); @@ -226,7 +226,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("*"); interceptor.intercept(invocation); @@ -267,7 +267,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue(""); interceptor.intercept(invocation); @@ -309,7 +309,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { actionInvocationControl.replay(); CookieInterceptor interceptor = new CookieInterceptor(); - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("cookie1value"); interceptor.intercept(invocation); @@ -370,7 +370,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { return accepted; } }; - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); @@ -430,7 +430,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { return accepted; } }; - interceptor.setExcludedPatternsChecker(new ExcludedPatternsChecker()); + interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java new file mode 100644 index 000000000..3860e57d8 --- /dev/null +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java @@ -0,0 +1,93 @@ +package com.opensymphony.xwork2; + +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Pattern; + +public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { + + private static final Logger LOG = LoggerFactory.getLogger(DefaultExcludedPatternsChecker.class); + + public static final String[] EXCLUDED_PATTERNS = { + "(.*\\.|^|.*|\\[('|\"))class(\\.|('|\")]|\\[).*", + "^dojo\\..*", + "^struts\\..*", + "^session\\..*", + "^request\\..*", + "^application\\..*", + "^servlet(Request|Response)\\..*", + "^parameters\\..*" + }; + + private Set excludedPatterns; + + public DefaultExcludedPatternsChecker() { + excludedPatterns = new HashSet(); + for (String pattern : EXCLUDED_PATTERNS) { + excludedPatterns.add(Pattern.compile(pattern)); + } + } + + @Inject(value = XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, required = false) + public void setOverrideExcludePatterns(String excludePatterns) { + if (LOG.isWarnEnabled()) { + LOG.warn("Overriding [#0] with [#1], be aware that this can affect safety of your application!", + XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, excludePatterns); + } + excludedPatterns = new HashSet(); + for (String pattern : TextParseUtil.commaDelimitedStringToSet(excludePatterns)) { + excludedPatterns.add(Pattern.compile(pattern)); + } + } + + /** + * Allows add additional excluded patterns during runtime + * + * @param commaDelimitedPatterns comma delimited string with patterns + */ + public void addExcludedPatterns(String commaDelimitedPatterns) { + addExcludedPatterns(TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns)); + } + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns array of additional excluded patterns + */ + public void addExcludedPatterns(String[] additionalPatterns) { + addExcludedPatterns(new HashSet(Arrays.asList(additionalPatterns))); + } + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns set of additional patterns + */ + public void addExcludedPatterns(Set additionalPatterns) { + if (LOG.isTraceEnabled()) { + LOG.trace("Adding additional excluded patterns [#0]", additionalPatterns); + } + for (String pattern : additionalPatterns) { + excludedPatterns.add(Pattern.compile(pattern)); + } + } + + public IsExcluded isExcluded(String value) { + for (Pattern excludedPattern : excludedPatterns) { + if (excludedPattern.matcher(value).matches()) { + if (LOG.isTraceEnabled()) { + LOG.trace("[#0] matches excluded pattern [#1]", value, excludedPattern); + } + return IsExcluded.yes(excludedPattern); + } + } + return IsExcluded.no(); + } + +} diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java index ee3eea6e9..c4730ea73 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java @@ -1,101 +1,13 @@ package com.opensymphony.xwork2; -import com.opensymphony.xwork2.inject.Inject; -import com.opensymphony.xwork2.util.TextParseUtil; -import com.opensymphony.xwork2.util.logging.Logger; -import com.opensymphony.xwork2.util.logging.LoggerFactory; - -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; import java.util.regex.Pattern; /** * Used across different interceptors to check if given string matches one of the excluded patterns. - * User has two options to change its behaviour: - * - define new set of patterns with - * - override this class and use then extension point - * to inject it in appropriated places */ -public class ExcludedPatternsChecker { +public interface ExcludedPatternsChecker { - private static final Logger LOG = LoggerFactory.getLogger(ExcludedPatternsChecker.class); - - public static final String[] EXCLUDED_PATTERNS = { - "(.*\\.|^|.*|\\[('|\"))class(\\.|('|\")]|\\[).*", - "^dojo\\..*", - "^struts\\..*", - "^session\\..*", - "^request\\..*", - "^application\\..*", - "^servlet(Request|Response)\\..*", - "^parameters\\..*" - }; - - private Set excludedPatterns; - - public ExcludedPatternsChecker() { - excludedPatterns = new HashSet(); - for (String pattern : EXCLUDED_PATTERNS) { - excludedPatterns.add(Pattern.compile(pattern)); - } - } - - @Inject(value = XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, required = false) - public void setOverrideExcludePatterns(String excludePatterns) { - if (LOG.isWarnEnabled()) { - LOG.warn("Overriding [#0] with [#1], be aware that this can affect safety of your application!", - XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, excludePatterns); - } - excludedPatterns = new HashSet(); - for (String pattern : TextParseUtil.commaDelimitedStringToSet(excludePatterns)) { - excludedPatterns.add(Pattern.compile(pattern)); - } - } - - /** - * Allows add additional excluded patterns during runtime - * - * @param commaDelimitedPatterns comma delimited string with patterns - */ - public void addExcludedPatterns(String commaDelimitedPatterns) { - addExcludedPatterns(TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns)); - } - - /** - * Allows add additional excluded patterns during runtime - * - * @param additionalPatterns array of additional excluded patterns - */ - public void addExcludedPatterns(String[] additionalPatterns) { - addExcludedPatterns(new HashSet(Arrays.asList(additionalPatterns))); - } - - /** - * Allows add additional excluded patterns during runtime - * - * @param additionalPatterns set of additional patterns - */ - public void addExcludedPatterns(Set additionalPatterns) { - if (LOG.isTraceEnabled()) { - LOG.trace("Adding additional excluded patterns [#0]", additionalPatterns); - } - for (String pattern : additionalPatterns) { - excludedPatterns.add(Pattern.compile(pattern)); - } - } - - public IsExcluded isExcluded(String value) { - for (Pattern excludedPattern : excludedPatterns) { - if (excludedPattern.matcher(value).matches()) { - if (LOG.isTraceEnabled()) { - LOG.trace("[#0] matches excluded pattern [#1]", value, excludedPattern); - } - return IsExcluded.yes(excludedPattern); - } - } - return IsExcluded.no(); - } + public IsExcluded isExcluded(String value); public final static class IsExcluded { From 833a07e7fa1143f2a09786d561da70f144954c60 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 14 May 2014 08:24:03 +0200 Subject: [PATCH 29/48] Extends logging with more information --- .../main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 83be3ed0a..1e4a5768e 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -198,7 +198,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS throw new XWorkException(message, re); } else { if (LOG.isWarnEnabled()) { - LOG.warn("Error setting value", re); + LOG.warn("Error setting value [#0] with expression [#1]", re, value.toString(), expr); } } } From e8e5b51bc64e71cc7645c1083b33e1942bf4a03d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 14 May 2014 08:25:00 +0200 Subject: [PATCH 30/48] Cleans up new extension point --- .../apache/struts2/config/DefaultBeanSelectionProvider.java | 4 ++-- .../org/apache/struts2/interceptor/CookieInterceptor.java | 2 +- core/src/main/resources/struts-default.xml | 4 +--- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index 5296b41b8..5c29e78cb 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -390,8 +390,8 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider alias(DispatcherErrorHandler.class, StrutsConstants.STRUTS_DISPATCHER_ERROR_HANDLER, builder, props); - /** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.REQUEST **/ - alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.REQUEST); + /** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.DEFAULT **/ + alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.DEFAULT); switchDevMode(props); diff --git a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index 8998c5cb1..dbe47ce8c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java @@ -180,7 +180,7 @@ public class CookieInterceptor extends AbstractInterceptor { private ExcludedPatternsChecker excludedPatternsChecker; - @Inject(StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER) + @Inject public void setExcludedPatternsChecker(ExcludedPatternsChecker excludedPatternsChecker) { this.excludedPatternsChecker = excludedPatternsChecker; } diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index f2fb9222f..2d74b4fe7 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -144,9 +144,7 @@ - - - + From 3d77c348b15f438c5dcab9790daacfd4d43cd02b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 14 May 2014 08:25:22 +0200 Subject: [PATCH 31/48] Adds additional methods needed by ParametersInterceptor --- .../DefaultExcludedPatternsChecker.java | 19 +++------- .../xwork2/ExcludedPatternsChecker.java | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java index 3860e57d8..eabd62118 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java @@ -46,29 +46,14 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { } } - /** - * Allows add additional excluded patterns during runtime - * - * @param commaDelimitedPatterns comma delimited string with patterns - */ public void addExcludedPatterns(String commaDelimitedPatterns) { addExcludedPatterns(TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns)); } - /** - * Allows add additional excluded patterns during runtime - * - * @param additionalPatterns array of additional excluded patterns - */ public void addExcludedPatterns(String[] additionalPatterns) { addExcludedPatterns(new HashSet(Arrays.asList(additionalPatterns))); } - /** - * Allows add additional excluded patterns during runtime - * - * @param additionalPatterns set of additional patterns - */ public void addExcludedPatterns(Set additionalPatterns) { if (LOG.isTraceEnabled()) { LOG.trace("Adding additional excluded patterns [#0]", additionalPatterns); @@ -90,4 +75,8 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { return IsExcluded.no(); } + public Set getExcludedPatterns() { + return excludedPatterns; + } + } diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java index c4730ea73..ac0ff6e9d 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java @@ -1,5 +1,6 @@ package com.opensymphony.xwork2; +import java.util.Set; import java.util.regex.Pattern; /** @@ -7,8 +8,42 @@ import java.util.regex.Pattern; */ public interface ExcludedPatternsChecker { + /** + * Checks if value matches any of patterns on exclude list + * + * @param value to check + * @return object containing result of matched pattern and pattern itself + */ public IsExcluded isExcluded(String value); + /** + * Allows add additional excluded patterns during runtime + * + * @param commaDelimitedPatterns comma delimited string with patterns + */ + public void addExcludedPatterns(String commaDelimitedPatterns); + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns array of additional excluded patterns + */ + public void addExcludedPatterns(String[] additionalPatterns); + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns set of additional patterns + */ + public void addExcludedPatterns(Set additionalPatterns); + + /** + * Allow access list of all defined excluded patterns + * + * @return set of excluded patterns + */ + public Set getExcludedPatterns(); + public final static class IsExcluded { private final boolean excluded; From 5ec47b1e6df6c59ff3fa466d20f28fda46b60254 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 14 May 2014 08:25:50 +0200 Subject: [PATCH 32/48] Uses checker instead set of patterns to check if param is excluded --- .../interceptor/ParametersInterceptor.java | 43 +++++++------------ .../ParametersInterceptorTest.java | 4 +- 2 files changed, 16 insertions(+), 31 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java index 6de6aad3b..460aae2b9 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -17,6 +17,7 @@ package com.opensymphony.xwork2.interceptor; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.ValidationAware; import com.opensymphony.xwork2.XWorkConstants; import com.opensymphony.xwork2.conversion.impl.InstantiatingNullHandler; @@ -143,12 +144,13 @@ public class ParametersInterceptor extends MethodFilterInterceptor { protected static final int PARAM_NAME_MAX_LENGTH = 100; + private ExcludedPatternsChecker excludedPatterns; + private int paramNameMaxLength = PARAM_NAME_MAX_LENGTH; private boolean devMode = false; protected boolean ordered = false; - protected Set excludeParams = Collections.emptySet(); protected Set acceptParams = Collections.emptySet(); private ValueStackFactory valueStackFactory; @@ -163,7 +165,12 @@ public class ParametersInterceptor extends MethodFilterInterceptor { devMode = "true".equalsIgnoreCase(mode); } - /** + @Inject + public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { + this.excludedPatterns = excludedPatterns; + } + + /** * Sets a comma-delimited list of regular expressions to match * parameters that are allowed in the parameter map (aka whitelist). *

@@ -306,7 +313,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { //see WW-2761 for more details MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack; accessValueStack.setAcceptProperties(acceptParams); - accessValueStack.setExcludeProperties(excludeParams); + accessValueStack.setExcludeProperties(excludedPatterns.getExcludedPatterns()); } for (Map.Entry entry : acceptableParameters.entrySet()) { @@ -426,14 +433,10 @@ public class ParametersInterceptor extends MethodFilterInterceptor { } protected boolean isExcluded(String paramName) { - if (!this.excludeParams.isEmpty()) { - for (Pattern pattern : excludeParams) { - Matcher matcher = pattern.matcher(paramName); - if (matcher.matches()) { - notifyDeveloper("Parameter [#0] is on the excludeParams list of patterns!", paramName); - return true; - } - } + ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName); + if (result.isExcluded()) { + notifyDeveloper("Parameter [#0] is on the excludeParams list of patterns!", paramName); + return true; } return false; } @@ -466,16 +469,6 @@ public class ParametersInterceptor extends MethodFilterInterceptor { this.ordered = ordered; } - /** - * Gets a set of regular expressions of parameters to remove - * from the parameter map - * - * @return A set of compiled regular expression patterns - */ - protected Set getExcludeParamsSet() { - return excludeParams; - } - /** * Sets a comma-delimited list of regular expressions to match * parameters that should be removed from the parameter map. @@ -483,13 +476,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { * @param commaDelim A comma-delimited list of regular expressions */ public void setExcludeParams(String commaDelim) { - Collection excludePatterns = ArrayUtils.asCollection(commaDelim); - if (excludePatterns != null) { - excludeParams = new HashSet(); - for (String pattern : excludePatterns) { - excludeParams.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); - } - } + excludedPatterns.addExcludedPatterns(commaDelim); } } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index a2aa92bf6..156c0129d 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -145,7 +145,6 @@ public class ParametersInterceptorTest extends XWorkTestCase { }; - pi.setExcludeParams("(.*\\.|^)class\\..*"); container.inject(pi); ValueStack vs = ActionContext.getContext().getValueStack(); @@ -165,7 +164,7 @@ public class ParametersInterceptorTest extends XWorkTestCase { final String pollution2 = "model.class.classLoader.jarPath"; final String pollution3 = "class.classLoader.defaultAssertionStatus"; - loadConfigurationProviders(new XWorkConfigurationProvider(), new XmlConfigurationProvider("xwork-param-test.xml")); + loadConfigurationProviders(new XWorkConfigurationProvider(), new XmlConfigurationProvider("xwork-class-param-test.xml")); final Map params = new HashMap() { { put(pollution1, "bad"); @@ -308,7 +307,6 @@ public class ParametersInterceptorTest extends XWorkTestCase { }; - pi.setExcludeParams("(.*\\.|^|.*|\\[('|\"))class(\\.|('|\")]|\\[).*"); container.inject(pi); ValueStack vs = ActionContext.getContext().getValueStack(); From d1d81f8a77e05ade18d67571816510d6655cee1e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 14 May 2014 08:26:27 +0200 Subject: [PATCH 33/48] Adds new dependency to allow tests pass --- .../config/providers/XWorkConfigurationProvider.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java index 0d489994a..c341d9894 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java @@ -2,9 +2,11 @@ package com.opensymphony.xwork2.config.providers; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; +import com.opensymphony.xwork2.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.DefaultLocaleProvider; import com.opensymphony.xwork2.DefaultTextProvider; import com.opensymphony.xwork2.DefaultUnknownHandlerManager; +import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.FileManager; import com.opensymphony.xwork2.FileManagerFactory; import com.opensymphony.xwork2.LocaleProvider; @@ -168,7 +170,11 @@ public class XWorkConfigurationProvider implements ConfigurationProvider { .factory(ArrayConverter.class, Scope.SINGLETON) .factory(DateConverter.class, Scope.SINGLETON) .factory(NumberConverter.class, Scope.SINGLETON) - .factory(StringConverter.class, Scope.SINGLETON); + .factory(StringConverter.class, Scope.SINGLETON) + + .factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class, Scope.DEFAULT) + ; + props.setProperty(XWorkConstants.DEV_MODE, Boolean.FALSE.toString()); props.setProperty(XWorkConstants.LOG_MISSING_PROPERTIES, Boolean.FALSE.toString()); props.setProperty(XWorkConstants.ENABLE_OGNL_EXPRESSION_CACHE, Boolean.TRUE.toString()); From 83b76b0fe83411d93dc2c534c8c47dc53f0dca82 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 14 May 2014 08:26:43 +0200 Subject: [PATCH 34/48] Updates tests to match new requirements --- .../org/apache/struts2/TestConfigurationProvider.java | 5 +++++ .../src/test/resources/xwork-class-param-test.xml | 11 +++++++++++ 2 files changed, 16 insertions(+) create mode 100644 xwork-core/src/test/resources/xwork-class-param-test.xml diff --git a/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java b/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java index cd42ed557..9323f02e8 100644 --- a/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java +++ b/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java @@ -24,6 +24,8 @@ package org.apache.struts2; import com.opensymphony.xwork2.Action; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; +import com.opensymphony.xwork2.DefaultExcludedPatternsChecker; +import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.config.Configuration; import com.opensymphony.xwork2.config.ConfigurationException; @@ -164,5 +166,8 @@ public class TestConfigurationProvider implements ConfigurationProvider { if (!builder.contains(ActionProxyFactory.class)) { builder.factory(ActionProxyFactory.class, DefaultActionProxyFactory.class); } + if (!builder.contains(ExcludedPatternsChecker.class)) { + builder.factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class); + } } } diff --git a/xwork-core/src/test/resources/xwork-class-param-test.xml b/xwork-core/src/test/resources/xwork-class-param-test.xml new file mode 100644 index 000000000..f12c08384 --- /dev/null +++ b/xwork-core/src/test/resources/xwork-class-param-test.xml @@ -0,0 +1,11 @@ + + + + + + + + + \ No newline at end of file From 7faf91abe1987aa812655860b4e7ef1ad2f93644 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 19 May 2014 09:59:23 +0200 Subject: [PATCH 35/48] Moves security related classes to security package --- core/src/main/resources/struts-default.xml | 2 +- .../struts2/TestConfigurationProvider.java | 2 +- .../interceptor/CookieInterceptorTest.java | 2 +- .../providers/XWorkConfigurationProvider.java | 2 +- .../DefaultExcludedPatternsChecker.java | 5 +- .../security/ExcludedPatternsChecker.java | 82 +++++++++++++++++++ 6 files changed, 89 insertions(+), 6 deletions(-) rename xwork-core/src/main/java/com/opensymphony/xwork2/{ => security}/DefaultExcludedPatternsChecker.java (93%) create mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 2d74b4fe7..ecfa5cf69 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -144,7 +144,7 @@ - + diff --git a/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java b/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java index 9323f02e8..d9da6c48a 100644 --- a/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java +++ b/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java @@ -24,7 +24,7 @@ package org.apache.struts2; import com.opensymphony.xwork2.Action; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; -import com.opensymphony.xwork2.DefaultExcludedPatternsChecker; +import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.ExcludedPatternsChecker; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.config.Configuration; diff --git a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java index 1f642f598..a531a69d7 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java @@ -27,7 +27,7 @@ import java.util.Map; import javax.servlet.http.Cookie; -import com.opensymphony.xwork2.DefaultExcludedPatternsChecker; +import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.mock.MockActionInvocation; import org.easymock.MockControl; import org.springframework.mock.web.MockHttpServletRequest; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java index c341d9894..1a7220673 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java @@ -2,7 +2,7 @@ package com.opensymphony.xwork2.config.providers; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; -import com.opensymphony.xwork2.DefaultExcludedPatternsChecker; +import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.DefaultLocaleProvider; import com.opensymphony.xwork2.DefaultTextProvider; import com.opensymphony.xwork2.DefaultUnknownHandlerManager; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java similarity index 93% rename from xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java rename to xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java index eabd62118..f2abed691 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/DefaultExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java @@ -1,5 +1,6 @@ -package com.opensymphony.xwork2; +package com.opensymphony.xwork2.security; +import com.opensymphony.xwork2.*; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.TextParseUtil; import com.opensymphony.xwork2.util.logging.Logger; @@ -10,7 +11,7 @@ import java.util.HashSet; import java.util.Set; import java.util.regex.Pattern; -public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { +public class DefaultExcludedPatternsChecker implements com.opensymphony.xwork2.ExcludedPatternsChecker { private static final Logger LOG = LoggerFactory.getLogger(DefaultExcludedPatternsChecker.class); diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java new file mode 100644 index 000000000..51751e956 --- /dev/null +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java @@ -0,0 +1,82 @@ +package com.opensymphony.xwork2.security; + +import java.util.Set; +import java.util.regex.Pattern; + +/** + * Used across different interceptors to check if given string matches one of the excluded patterns. + */ +public interface ExcludedPatternsChecker { + + /** + * Checks if value matches any of patterns on exclude list + * + * @param value to check + * @return object containing result of matched pattern and pattern itself + */ + public IsExcluded isExcluded(String value); + + /** + * Allows add additional excluded patterns during runtime + * + * @param commaDelimitedPatterns comma delimited string with patterns + */ + public void addExcludedPatterns(String commaDelimitedPatterns); + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns array of additional excluded patterns + */ + public void addExcludedPatterns(String[] additionalPatterns); + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns set of additional patterns + */ + public void addExcludedPatterns(Set additionalPatterns); + + /** + * Allow access list of all defined excluded patterns + * + * @return set of excluded patterns + */ + public Set getExcludedPatterns(); + + public final static class IsExcluded { + + private final boolean excluded; + private final Pattern excludedPattern; + + public static IsExcluded yes(Pattern excludedPattern) { + return new IsExcluded(true, excludedPattern); + } + + public static IsExcluded no() { + return new IsExcluded(false, null); + } + + private IsExcluded(boolean excluded, Pattern excludedPattern) { + this.excluded = excluded; + this.excludedPattern = excludedPattern; + } + + public boolean isExcluded() { + return excluded; + } + + public Pattern getExcludedPattern() { + return excludedPattern; + } + + @Override + public String toString() { + return "IsExcluded { " + + "excluded=" + excluded + + ", excludedPattern=" + excludedPattern + + " }"; + } + } + +} From ec98c8a95beb58fface26371b5ae3829493259f5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 19 May 2014 10:08:30 +0200 Subject: [PATCH 36/48] Cleans up after moving to package --- .../xwork2/ExcludedPatternsChecker.java | 82 ------------------- .../DefaultExcludedPatternsChecker.java | 2 +- 2 files changed, 1 insertion(+), 83 deletions(-) delete mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java deleted file mode 100644 index ac0ff6e9d..000000000 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ExcludedPatternsChecker.java +++ /dev/null @@ -1,82 +0,0 @@ -package com.opensymphony.xwork2; - -import java.util.Set; -import java.util.regex.Pattern; - -/** - * Used across different interceptors to check if given string matches one of the excluded patterns. - */ -public interface ExcludedPatternsChecker { - - /** - * Checks if value matches any of patterns on exclude list - * - * @param value to check - * @return object containing result of matched pattern and pattern itself - */ - public IsExcluded isExcluded(String value); - - /** - * Allows add additional excluded patterns during runtime - * - * @param commaDelimitedPatterns comma delimited string with patterns - */ - public void addExcludedPatterns(String commaDelimitedPatterns); - - /** - * Allows add additional excluded patterns during runtime - * - * @param additionalPatterns array of additional excluded patterns - */ - public void addExcludedPatterns(String[] additionalPatterns); - - /** - * Allows add additional excluded patterns during runtime - * - * @param additionalPatterns set of additional patterns - */ - public void addExcludedPatterns(Set additionalPatterns); - - /** - * Allow access list of all defined excluded patterns - * - * @return set of excluded patterns - */ - public Set getExcludedPatterns(); - - public final static class IsExcluded { - - private final boolean excluded; - private final Pattern excludedPattern; - - public static IsExcluded yes(Pattern excludedPattern) { - return new IsExcluded(true, excludedPattern); - } - - public static IsExcluded no() { - return new IsExcluded(false, null); - } - - private IsExcluded(boolean excluded, Pattern excludedPattern) { - this.excluded = excluded; - this.excludedPattern = excludedPattern; - } - - public boolean isExcluded() { - return excluded; - } - - public Pattern getExcludedPattern() { - return excludedPattern; - } - - @Override - public String toString() { - return "IsExcluded { " + - "excluded=" + excluded + - ", excludedPattern=" + excludedPattern + - " }"; - } - } - -} diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java index f2abed691..53854d309 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java @@ -11,7 +11,7 @@ import java.util.HashSet; import java.util.Set; import java.util.regex.Pattern; -public class DefaultExcludedPatternsChecker implements com.opensymphony.xwork2.ExcludedPatternsChecker { +public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { private static final Logger LOG = LoggerFactory.getLogger(DefaultExcludedPatternsChecker.class); From 97ef7b50bbf12dcc3e4127c71487ec37f5b7132d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 19 May 2014 10:58:45 +0200 Subject: [PATCH 37/48] Cleans up after moving to package --- .../apache/struts2/config/DefaultBeanSelectionProvider.java | 2 +- .../java/org/apache/struts2/interceptor/CookieInterceptor.java | 3 +-- core/src/main/resources/struts-default.xml | 2 +- .../java/org/apache/struts2/TestConfigurationProvider.java | 2 +- .../src/main/java/com/opensymphony/xwork2/XWorkConstants.java | 2 ++ .../xwork2/config/providers/XWorkConfigurationProvider.java | 2 +- .../opensymphony/xwork2/interceptor/ParametersInterceptor.java | 2 +- 7 files changed, 8 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index 5c29e78cb..be4fa829d 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -22,7 +22,7 @@ package org.apache.struts2.config; import com.opensymphony.xwork2.ActionProxyFactory; -import com.opensymphony.xwork2.ExcludedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.FileManager; import com.opensymphony.xwork2.FileManagerFactory; import com.opensymphony.xwork2.LocaleProvider; diff --git a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index dbe47ce8c..ca195faa3 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java @@ -25,13 +25,12 @@ import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.interceptor.AbstractInterceptor; -import com.opensymphony.xwork2.ExcludedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.util.TextParseUtil; import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.util.logging.Logger; import com.opensymphony.xwork2.util.logging.LoggerFactory; import org.apache.struts2.ServletActionContext; -import org.apache.struts2.StrutsConstants; import javax.servlet.http.Cookie; import java.util.Collections; diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index ecfa5cf69..2fc16c9f0 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -144,7 +144,7 @@ - + diff --git a/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java b/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java index d9da6c48a..f9eb4c70b 100644 --- a/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java +++ b/core/src/test/java/org/apache/struts2/TestConfigurationProvider.java @@ -25,7 +25,7 @@ import com.opensymphony.xwork2.Action; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; -import com.opensymphony.xwork2.ExcludedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.config.Configuration; import com.opensymphony.xwork2.config.ConfigurationException; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java index f2f03e78e..b846ac0cf 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java @@ -18,6 +18,8 @@ public final class XWorkConstants { public static final String ALLOW_STATIC_METHOD_ACCESS = "allowStaticMethodAccess"; public static final String XWORK_LOGGER_FACTORY = "xwork.loggerFactory"; public static final String OGNL_EXCLUDED_CLASSES = "ognlExcludedClasses"; + public static final String OVERRIDE_EXCLUDED_PATTERNS = "overrideExcludedPatterns"; + public static final String OVERRIDE_ACCEPTED_PATTERNS = "overrideAcceptedPatterns"; } diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java index 1a7220673..9f28334df 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java @@ -6,7 +6,7 @@ import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.DefaultLocaleProvider; import com.opensymphony.xwork2.DefaultTextProvider; import com.opensymphony.xwork2.DefaultUnknownHandlerManager; -import com.opensymphony.xwork2.ExcludedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.FileManager; import com.opensymphony.xwork2.FileManagerFactory; import com.opensymphony.xwork2.LocaleProvider; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java index 460aae2b9..f1906b05c 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -17,7 +17,7 @@ package com.opensymphony.xwork2.interceptor; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionInvocation; -import com.opensymphony.xwork2.ExcludedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.ValidationAware; import com.opensymphony.xwork2.XWorkConstants; import com.opensymphony.xwork2.conversion.impl.InstantiatingNullHandler; From b140faad2813809c132ef75e4459f6dbbee664b8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 21 May 2014 09:03:30 +0200 Subject: [PATCH 38/48] Defines new service to check accepted patterns --- .../security/AcceptedPatternsChecker.java | 82 +++++++++++++++++ .../DefaultAcceptedPatternsChecker.java | 88 +++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java create mode 100644 xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java new file mode 100644 index 000000000..6ea9ec9ca --- /dev/null +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java @@ -0,0 +1,82 @@ +package com.opensymphony.xwork2.security; + +import java.util.Set; +import java.util.regex.Pattern; + +/** + * Used across different interceptors to check if given string matches one of the excluded patterns. + */ +public interface AcceptedPatternsChecker { + + /** + * Checks if value matches any of patterns on exclude list + * + * @param value to check + * @return object containing result of matched pattern and pattern itself + */ + public IsAccepted isAccepted(String value); + + /** + * Allows add additional excluded patterns during runtime + * + * @param commaDelimitedPatterns comma delimited string with patterns + */ + public void addAcceptedPatterns(String commaDelimitedPatterns); + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns array of additional excluded patterns + */ + public void addAcceptedPatterns(String[] additionalPatterns); + + /** + * Allows add additional excluded patterns during runtime + * + * @param additionalPatterns set of additional patterns + */ + public void addAcceptedPatterns(Set additionalPatterns); + + /** + * Allow access list of all defined excluded patterns + * + * @return set of excluded patterns + */ + public Set getAcceptedPatterns(); + + public final static class IsAccepted { + + private final boolean accepted; + private final Pattern acceptedPattern; + + public static IsAccepted yes(Pattern acceptedPattern) { + return new IsAccepted(true, acceptedPattern); + } + + public static IsAccepted no() { + return new IsAccepted(false, null); + } + + private IsAccepted(boolean accepted, Pattern acceptedPattern) { + this.accepted = accepted; + this.acceptedPattern = acceptedPattern; + } + + public boolean isAccepted() { + return accepted; + } + + public Pattern getAcceptedPattern() { + return acceptedPattern; + } + + @Override + public String toString() { + return "IsAccepted {" + + "accepted=" + accepted + + ", acceptedPattern=" + acceptedPattern + + " }"; + } + } + +} diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java new file mode 100644 index 000000000..fa1b8e141 --- /dev/null +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java @@ -0,0 +1,88 @@ +package com.opensymphony.xwork2.security; + +import com.opensymphony.xwork2.XWorkConstants; +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.logging.Logger; +import com.opensymphony.xwork2.util.logging.LoggerFactory; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Pattern; + +public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { + + private static final Logger LOG = LoggerFactory.getLogger(DefaultAcceptedPatternsChecker.class); + + public static final String[] ACCEPTED_PATTERNS = { + "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\\u9fa5])+'\\])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*" + }; + + private Set acceptedPatterns; + + public DefaultAcceptedPatternsChecker() { + acceptedPatterns = new HashSet(); + for (String pattern : ACCEPTED_PATTERNS) { + acceptedPatterns.add(Pattern.compile(pattern)); + } + } + + @Inject(value = XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, required = false) + public void setOverrideAcceptedPatterns(String acceptablePatterns) { + if (LOG.isWarnEnabled()) { + LOG.warn("Overriding [#0] with [#1], be aware that this can affect safety of your application!", + XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, acceptablePatterns); + } + acceptedPatterns = new HashSet(); + for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptablePatterns)) { + acceptedPatterns.add(Pattern.compile(pattern)); + } + } + + @Inject(value = XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, required = false) + public void setOverrideExcludePatterns(String acceptPatterns) { + if (LOG.isWarnEnabled()) { + LOG.warn("Overriding [#0] with [#1], be aware that this can affect safety of your application!", + XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, acceptedPatterns); + } + acceptedPatterns = new HashSet(); + for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptPatterns)) { + acceptedPatterns.add(Pattern.compile(pattern)); + } + } + + public void addAcceptedPatterns(String commaDelimitedPatterns) { + addAcceptedPatterns(TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns)); + } + + public void addAcceptedPatterns(String[] additionalPatterns) { + addAcceptedPatterns(new HashSet(Arrays.asList(additionalPatterns))); + } + + public void addAcceptedPatterns(Set additionalPatterns) { + if (LOG.isTraceEnabled()) { + LOG.trace("Adding additional excluded patterns [#0]", additionalPatterns); + } + for (String pattern : additionalPatterns) { + acceptedPatterns.add(Pattern.compile(pattern)); + } + } + + public IsAccepted isAccepted(String value) { + for (Pattern acceptedPattern : acceptedPatterns) { + if (acceptedPattern.matcher(value).matches()) { + if (LOG.isTraceEnabled()) { + LOG.trace("[#0] matches accepted pattern [#1]", value, acceptedPattern); + } + return IsAccepted.yes(acceptedPattern); + } + } + return IsAccepted.no(); + } + + public Set getAcceptedPatterns() { + return acceptedPatterns; + } + +} From 8a93df10c4f5f3f22f1837c47b4ca9b4facc4f94 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 21 May 2014 09:03:51 +0200 Subject: [PATCH 39/48] Uses new service to check if param matches accepted patterns --- .../org/apache/struts2/StrutsConstants.java | 4 +- .../config/DefaultBeanSelectionProvider.java | 3 + core/src/main/resources/struts-default.xml | 1 + .../providers/XWorkConfigurationProvider.java | 3 + .../interceptor/ParametersInterceptor.java | 56 +++++++++---------- .../ParametersInterceptorTest.java | 11 +--- 6 files changed, 37 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index d173add5a..8c0c5ce58 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -285,10 +285,12 @@ public final class StrutsConstants { /** Comma delimited set of excluded classes which cannot be accessed via expressions **/ public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses"; - /** Dedicated service to check if passed string is excluded or not **/ + /** Dedicated services to check if passed string is excluded/accepted **/ public static final String STRUTS_EXCLUDED_PATTERNS_CHECKER = "struts.excludedPatterns.checker"; + public static final String STRUTS_ACCEPTED_PATTERNS_CHECKER = "struts.acceptedPatterns.checker"; /** Constant is used to override framework's default excluded patterns **/ public static final String STRUTS_OVERRIDE_EXCLUDED_PATTERNS = "struts.override.excludedPatterns"; + public static final String STRUTS_OVERRIDE_ACCEPTED_PATTERNS = "struts.override.acceptedPatterns"; } diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index be4fa829d..4334d3c85 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -22,6 +22,7 @@ package org.apache.struts2.config; import com.opensymphony.xwork2.ActionProxyFactory; +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.FileManager; import com.opensymphony.xwork2.FileManagerFactory; @@ -392,6 +393,7 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider /** Checker is used mostly in interceptors, so there be one instance of checker per interceptor with Scope.DEFAULT **/ alias(ExcludedPatternsChecker.class, StrutsConstants.STRUTS_EXCLUDED_PATTERNS_CHECKER, builder, props, Scope.DEFAULT); + alias(AcceptedPatternsChecker.class, StrutsConstants.STRUTS_ACCEPTED_PATTERNS_CHECKER, builder, props, Scope.DEFAULT); switchDevMode(props); @@ -403,6 +405,7 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider convertIfExist(props, StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, XWorkConstants.RELOAD_XML_CONFIGURATION); convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_CLASSES, XWorkConstants.OGNL_EXCLUDED_CLASSES); convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_EXCLUDED_PATTERNS, XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS); + convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_ACCEPTED_PATTERNS, XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS); LocalizedTextUtil.addDefaultResourceBundle("org/apache/struts2/struts-messages"); loadCustomResourceBundles(props); diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 2fc16c9f0..a1aa63f5f 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -145,6 +145,7 @@ + diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java index 9f28334df..19e8e76a8 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java @@ -2,6 +2,8 @@ package com.opensymphony.xwork2.config.providers; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; +import com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker; import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; import com.opensymphony.xwork2.DefaultLocaleProvider; import com.opensymphony.xwork2.DefaultTextProvider; @@ -173,6 +175,7 @@ public class XWorkConfigurationProvider implements ConfigurationProvider { .factory(StringConverter.class, Scope.SINGLETON) .factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class, Scope.DEFAULT) + .factory(AcceptedPatternsChecker.class, DefaultAcceptedPatternsChecker.class, Scope.DEFAULT) ; props.setProperty(XWorkConstants.DEV_MODE, Boolean.FALSE.toString()); diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java index f1906b05c..c1b2f3dd1 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -17,6 +17,7 @@ package com.opensymphony.xwork2.interceptor; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.ValidationAware; import com.opensymphony.xwork2.XWorkConstants; @@ -151,9 +152,8 @@ public class ParametersInterceptor extends MethodFilterInterceptor { protected boolean ordered = false; - protected Set acceptParams = Collections.emptySet(); - private ValueStackFactory valueStackFactory; + private AcceptedPatternsChecker acceptedPatterns; @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { @@ -170,23 +170,9 @@ public class ParametersInterceptor extends MethodFilterInterceptor { this.excludedPatterns = excludedPatterns; } - /** - * Sets a comma-delimited list of regular expressions to match - * parameters that are allowed in the parameter map (aka whitelist). - *

- * Don't change the default unless you know what you are doing in terms - * of security implications. - * - * @param commaDelim A comma-delimited list of regular expressions - */ - public void setAcceptParamNames(String commaDelim) { - Collection acceptPatterns = ArrayUtils.asCollection(commaDelim); - if (acceptPatterns != null) { - acceptParams = new HashSet(); - for (String pattern : acceptPatterns) { - acceptParams.add(Pattern.compile(pattern)); - } - } + @Inject + public void setAcceptedPatterns(AcceptedPatternsChecker acceptedPatterns) { + this.acceptedPatterns = acceptedPatterns; } /** @@ -312,7 +298,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { //block or allow access to properties //see WW-2761 for more details MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack; - accessValueStack.setAcceptProperties(acceptParams); + accessValueStack.setAcceptProperties(acceptedPatterns.getAcceptedPatterns()); accessValueStack.setExcludeProperties(excludedPatterns.getExcludedPatterns()); } @@ -419,23 +405,18 @@ public class ParametersInterceptor extends MethodFilterInterceptor { } protected boolean isAccepted(String paramName) { - if (!this.acceptParams.isEmpty()) { - for (Pattern pattern : acceptParams) { - Matcher matcher = pattern.matcher(paramName); - if (matcher.matches()) { - return true; - } - } - notifyDeveloper("Parameter [#0] didn't match acceptParams list of patterns!", paramName); - return false; + AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName); + if (result.isAccepted()) { + return true; } - return true; + notifyDeveloper("Parameter [#0] didn't match accepted pattern [#1]!", paramName, String.valueOf(result.getAcceptedPattern())); + return false; } protected boolean isExcluded(String paramName) { ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName); if (result.isExcluded()) { - notifyDeveloper("Parameter [#0] is on the excludeParams list of patterns!", paramName); + notifyDeveloper("Parameter [#0] matches excluded pattern [#1]!", paramName, String.valueOf(result.getExcludedPattern())); return true; } return false; @@ -469,6 +450,19 @@ public class ParametersInterceptor extends MethodFilterInterceptor { this.ordered = ordered; } + /** + * Sets a comma-delimited list of regular expressions to match + * parameters that are allowed in the parameter map (aka whitelist). + *

+ * Don't change the default unless you know what you are doing in terms + * of security implications. + * + * @param commaDelim A comma-delimited list of regular expressions + */ + public void setAcceptParamNames(String commaDelim) { + acceptedPatterns.addAcceptedPatterns(commaDelim); + } + /** * Sets a comma-delimited list of regular expressions to match * parameters that should be removed from the parameter map. diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 156c0129d..ce86051b3 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -373,7 +373,7 @@ public class ParametersInterceptorTest extends XWorkTestCase { ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, null, extraContext); proxy.execute(); Map existingMap = ((SimpleAction) proxy.getAction()).getTheProtectedMap(); - assertEquals(4, existingMap.size()); + assertEquals(0, existingMap.size()); } public void testParametersWithChineseInTheName() throws Exception { @@ -479,7 +479,7 @@ public class ParametersInterceptorTest extends XWorkTestCase { proxy.execute(); SimpleAction action = (SimpleAction) proxy.getAction(); - assertNull(action.getName()); + assertEquals("try_1", action.getName()); assertEquals("This is blah", (action).getBlah()); assertEquals(123, action.getBaz()); } @@ -700,13 +700,6 @@ public class ParametersInterceptorTest extends XWorkTestCase { final Map expected = new HashMap() { { put("ordinary.bean", "value"); - put("#some.internal.object", "true"); - put("(bla)#some.internal.object", "true"); - put("#some.internal.object(bla)#some.internal.object", "true"); - put("#_some.internal.object", "true"); - put("\u0023_some.internal.object", "true"); - put("\u0023_some.internal.object,[dfd],bla(\u0023_some.internal.object)", "true"); - put("\\u0023_some.internal.object", "true"); } }; From dba9da3abf1b5e6f59251b5a6d948c5bc502c9af Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 23 May 2014 09:20:07 +0200 Subject: [PATCH 40/48] Adds ability to exclude whole packages based on regex --- .../xwork2/ognl/SecurityMemberAccess.java | 20 +++++++++++++++++++ .../xwork2/ognl/SecurityMemberAccessTest.java | 19 ++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index c14d8b917..39f882a5a 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -40,6 +40,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess { private Set excludeProperties = Collections.emptySet(); private Set acceptProperties = Collections.emptySet(); private Set> excludedClasses = Collections.emptySet(); + private Set excludedPackageNamePatterns = Collections.emptySet(); public SecurityMemberAccess(boolean method) { super(false); @@ -52,6 +53,13 @@ public class SecurityMemberAccess extends DefaultMemberAccess { @Override public boolean isAccessible(Map context, Object target, Member member, String propertyName) { + if (isPackageExcluded(target.getClass().getPackage(), member.getDeclaringClass().getPackage())) { + if (LOG.isDebugEnabled()) { + LOG.debug("Target package [#0] and member package [#1] are excluded!", target, member); + } + return false; + } + if (isClassExcluded(target.getClass(), member.getDeclaringClass())) { if (LOG.isDebugEnabled()) { LOG.debug("Target class [#0] and member type [#1] are excluded!", target, member); @@ -84,6 +92,15 @@ public class SecurityMemberAccess extends DefaultMemberAccess { return isAcceptableProperty(propertyName); } + protected boolean isPackageExcluded(Package targetPackage, Package memberPackage) { + for (Pattern pattern : excludedPackageNamePatterns) { + if (pattern.matcher(targetPackage.getName()).matches() || pattern.matcher(memberPackage.getName()).matches()) { + return true; + } + } + return false; + } + protected boolean isClassExcluded(Class targetClass, Class declaringClass) { if (targetClass == Object.class || declaringClass == Object.class) { return true; @@ -141,4 +158,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess { this.excludedClasses = excludedClasses; } + public void setExcludedPackageNamePatterns(Set excludedPackageNamePatterns) { + this.excludedPackageNamePatterns = excludedPackageNamePatterns; + } } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index 1c14cb265..748d5a959 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -7,6 +7,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; public class SecurityMemberAccessTest extends TestCase { @@ -171,6 +172,24 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("barLogic() from BarInterface is accessible!!!", accessible); } + public void testPackageExclusion() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + Set excluded = new HashSet(); + excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); + sma.setExcludedPackageNamePatterns(excluded); + + String propertyName = "stringField"; + Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + + // when + boolean actual = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse("stringField is accessible!", actual); + } + } class FooBar implements FooBarInterface { From 4ee18f96bc2d401f9007c5fd458c47b7ae4ff35d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 23 May 2014 09:58:33 +0200 Subject: [PATCH 41/48] Ties excluding packages into Struts DI mechanism --- .../org/apache/struts2/StrutsConstants.java | 3 ++- .../config/DefaultBeanSelectionProvider.java | 3 +++ core/src/main/resources/struts-default.xml | 2 ++ .../com/opensymphony/xwork2/XWorkConstants.java | 2 ++ .../com/opensymphony/xwork2/ognl/OgnlUtil.java | 17 ++++++++++++++++- .../xwork2/ognl/OgnlValueStack.java | 1 + 6 files changed, 26 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 8c0c5ce58..dd089936f 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -282,8 +282,9 @@ public final class StrutsConstants { /** Allows override default DispatcherErrorHandler **/ public static final String STRUTS_DISPATCHER_ERROR_HANDLER = "struts.dispatcher.errorHandler"; - /** Comma delimited set of excluded classes which cannot be accessed via expressions **/ + /** Comma delimited set of excluded classes and package names which cannot be accessed via expressions **/ public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses"; + public static final String STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS = "struts.excludedPackageNamePatterns"; /** Dedicated services to check if passed string is excluded/accepted **/ public static final String STRUTS_EXCLUDED_PATTERNS_CHECKER = "struts.excludedPatterns.checker"; diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index 4334d3c85..a671133df 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -403,7 +403,10 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider convertIfExist(props, StrutsConstants.STRUTS_ENABLE_OGNL_EVAL_EXPRESSION, XWorkConstants.ENABLE_OGNL_EVAL_EXPRESSION); convertIfExist(props, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, XWorkConstants.ALLOW_STATIC_METHOD_ACCESS); convertIfExist(props, StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, XWorkConstants.RELOAD_XML_CONFIGURATION); + convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_CLASSES, XWorkConstants.OGNL_EXCLUDED_CLASSES); + convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, XWorkConstants.OGNL_EXCLUDED_PACKAGE_NAME_PATTERNS); + convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_EXCLUDED_PATTERNS, XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS); convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_ACCEPTED_PATTERNS, XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS); diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index a1aa63f5f..0275a48fe 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -39,6 +39,8 @@ + + diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java index b846ac0cf..830df78bd 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java @@ -17,7 +17,9 @@ public final class XWorkConstants { public static final String RELOAD_XML_CONFIGURATION = "reloadXmlConfiguration"; public static final String ALLOW_STATIC_METHOD_ACCESS = "allowStaticMethodAccess"; public static final String XWORK_LOGGER_FACTORY = "xwork.loggerFactory"; + public static final String OGNL_EXCLUDED_CLASSES = "ognlExcludedClasses"; + public static final String OGNL_EXCLUDED_PACKAGE_NAME_PATTERNS = "ognlExcludedPackageNamePatterns"; public static final String OVERRIDE_EXCLUDED_PATTERNS = "overrideExcludedPatterns"; public static final String OVERRIDE_ACCEPTED_PATTERNS = "overrideAcceptedPatterns"; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 1c17ecac1..b0345fc89 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -16,7 +16,6 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.XWorkConstants; -import com.opensymphony.xwork2.XWorkException; import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Container; @@ -47,6 +46,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.regex.Pattern; /** @@ -67,6 +67,8 @@ public class OgnlUtil { private boolean enableEvalExpression; private Set> excludedClasses = new HashSet>(); + private Set excludedPackageNamePatterns = new HashSet(); + private Container container; private boolean allowStaticMethodAccess; @@ -106,10 +108,22 @@ public class OgnlUtil { } } + @Inject(value = XWorkConstants.OGNL_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + public void setExcludedPackageName(String commaDelimitedPackagePatterns) { + Set packagePatterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackagePatterns); + for (String pattern : packagePatterns) { + excludedPackageNamePatterns.add(Pattern.compile(pattern)); + } + } + public Set> getExcludedClasses() { return excludedClasses; } + public Set getExcludedPackageNamePatterns() { + return excludedPackageNamePatterns; + } + @Inject public void setContainer(Container container) { this.container = container; @@ -568,6 +582,7 @@ public class OgnlUtil { SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticMethodAccess); memberAccess.setExcludedClasses(excludedClasses); + memberAccess.setExcludedPackageNamePatterns(excludedPackageNamePatterns); return Ognl.createDefaultContext(root, resolver, defaultConverter, memberAccess); } diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 1e4a5768e..acf54c4c8 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -80,6 +80,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS public void setOgnlUtil(OgnlUtil ognlUtil) { this.ognlUtil = ognlUtil; securityMemberAccess.setExcludedClasses(ognlUtil.getExcludedClasses()); + securityMemberAccess.setExcludedPackageNamePatterns(ognlUtil.getExcludedPackageNamePatterns()); } protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, From 5a5af1b5879a9865aca03c70ae5bd6f7a3473f7b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 23 May 2014 09:58:52 +0200 Subject: [PATCH 42/48] Uses WARN to report if package or class is excluded --- .../opensymphony/xwork2/ognl/SecurityMemberAccess.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 39f882a5a..d0862e7de 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -54,15 +54,15 @@ public class SecurityMemberAccess extends DefaultMemberAccess { @Override public boolean isAccessible(Map context, Object target, Member member, String propertyName) { if (isPackageExcluded(target.getClass().getPackage(), member.getDeclaringClass().getPackage())) { - if (LOG.isDebugEnabled()) { - LOG.debug("Target package [#0] and member package [#1] are excluded!", target, member); + if (LOG.isWarnEnabled()) { + LOG.warn("Package of target [#0] or package of member [#1] are excluded!", target, member); } return false; } if (isClassExcluded(target.getClass(), member.getDeclaringClass())) { - if (LOG.isDebugEnabled()) { - LOG.debug("Target class [#0] and member type [#1] are excluded!", target, member); + if (LOG.isWarnEnabled()) { + LOG.warn("Target class [#0] or declaring class of member type [#1] are excluded!", target, member); } return false; } From 2df72b941186b9c0a2a7fdc84cbf6d3001ec30e9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 23 May 2014 17:36:45 +0200 Subject: [PATCH 43/48] Adds javax.* to excluded packages --- core/src/main/resources/struts-default.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 0275a48fe..0fe8e68ce 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -40,7 +40,7 @@ - + From 89cbe13853a849340d740d45685e6fd14da93d9b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 1 Jun 2014 10:33:39 +0200 Subject: [PATCH 44/48] Adds option to define additional accepted/excluded patterns Also all patterns are by default case insensitive --- .../org/apache/struts2/StrutsConstants.java | 3 + .../config/DefaultBeanSelectionProvider.java | 2 + .../opensymphony/xwork2/XWorkConstants.java | 3 + .../DefaultAcceptedPatternsChecker.java | 18 +++--- .../DefaultExcludedPatternsChecker.java | 28 +++++++--- .../DefaultAcceptedPatternsCheckerTest.java | 56 +++++++++++++++++++ .../DefaultExcludedPatternsCheckerTest.java | 56 +++++++++++++++++++ 7 files changed, 147 insertions(+), 19 deletions(-) create mode 100644 xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsCheckerTest.java create mode 100644 xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index dd089936f..918f91bc1 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -294,4 +294,7 @@ public final class StrutsConstants { public static final String STRUTS_OVERRIDE_EXCLUDED_PATTERNS = "struts.override.excludedPatterns"; public static final String STRUTS_OVERRIDE_ACCEPTED_PATTERNS = "struts.override.acceptedPatterns"; + public static final String STRUTS_ADDITIONAL_EXCLUDED_PATTERNS = "struts.additional.excludedPatterns"; + public static final String STRUTS_ADDITIONAL_ACCEPTED_PATTERNS = "struts.additional.acceptedPatterns"; + } diff --git a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java index a671133df..06b730290 100644 --- a/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java @@ -407,6 +407,8 @@ public class DefaultBeanSelectionProvider extends AbstractBeanSelectionProvider convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_CLASSES, XWorkConstants.OGNL_EXCLUDED_CLASSES); convertIfExist(props, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, XWorkConstants.OGNL_EXCLUDED_PACKAGE_NAME_PATTERNS); + convertIfExist(props, StrutsConstants.STRUTS_ADDITIONAL_EXCLUDED_PATTERNS, XWorkConstants.ADDITIONAL_EXCLUDED_PATTERNS); + convertIfExist(props, StrutsConstants.STRUTS_ADDITIONAL_ACCEPTED_PATTERNS, XWorkConstants.ADDITIONAL_ACCEPTED_PATTERNS); convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_EXCLUDED_PATTERNS, XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS); convertIfExist(props, StrutsConstants.STRUTS_OVERRIDE_ACCEPTED_PATTERNS, XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS); diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java index 830df78bd..433b005ef 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/XWorkConstants.java @@ -21,6 +21,9 @@ public final class XWorkConstants { public static final String OGNL_EXCLUDED_CLASSES = "ognlExcludedClasses"; public static final String OGNL_EXCLUDED_PACKAGE_NAME_PATTERNS = "ognlExcludedPackageNamePatterns"; + public static final String ADDITIONAL_EXCLUDED_PATTERNS = "additionalExcludedPatterns"; + public static final String ADDITIONAL_ACCEPTED_PATTERNS = "additionalAcceptedPatterns"; + public static final String OVERRIDE_EXCLUDED_PATTERNS = "overrideExcludedPatterns"; public static final String OVERRIDE_ACCEPTED_PATTERNS = "overrideAcceptedPatterns"; diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java index fa1b8e141..970a52cc5 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java @@ -24,7 +24,7 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { public DefaultAcceptedPatternsChecker() { acceptedPatterns = new HashSet(); for (String pattern : ACCEPTED_PATTERNS) { - acceptedPatterns.add(Pattern.compile(pattern)); + acceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } @@ -36,19 +36,17 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { } acceptedPatterns = new HashSet(); for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptablePatterns)) { - acceptedPatterns.add(Pattern.compile(pattern)); + acceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } - @Inject(value = XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, required = false) - public void setOverrideExcludePatterns(String acceptPatterns) { - if (LOG.isWarnEnabled()) { - LOG.warn("Overriding [#0] with [#1], be aware that this can affect safety of your application!", - XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, acceptedPatterns); + @Inject(value = XWorkConstants.ADDITIONAL_ACCEPTED_PATTERNS, required = false) + public void setAdditionalAcceptedPatterns(String acceptablePatterns) { + if (LOG.isDebugEnabled()) { + LOG.warn("Adding additional patterns [#0] to accepted patterns!", acceptablePatterns); } - acceptedPatterns = new HashSet(); - for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptPatterns)) { - acceptedPatterns.add(Pattern.compile(pattern)); + for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptablePatterns)) { + acceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java index 53854d309..f0a3d6248 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java @@ -17,13 +17,13 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { public static final String[] EXCLUDED_PATTERNS = { "(.*\\.|^|.*|\\[('|\"))class(\\.|('|\")]|\\[).*", - "^dojo\\..*", - "^struts\\..*", - "^session\\..*", - "^request\\..*", - "^application\\..*", - "^servlet(Request|Response)\\..*", - "^parameters\\..*" + "(^|.*#)dojo(\\.|\\[).*", + "(^|.*#)struts(\\.|\\[).*", + "(^|.*#)session(\\.|\\[).*", + "(^|.*#)request(\\.|\\[).*", + "(^|.*#)application(\\.|\\[).*", + "(^|.*#)servlet(Request|Response)(\\.|\\[).*", + "(^|.*#)parameters(\\.|\\[).*" }; private Set excludedPatterns; @@ -31,7 +31,7 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { public DefaultExcludedPatternsChecker() { excludedPatterns = new HashSet(); for (String pattern : EXCLUDED_PATTERNS) { - excludedPatterns.add(Pattern.compile(pattern)); + excludedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } @@ -43,7 +43,17 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { } excludedPatterns = new HashSet(); for (String pattern : TextParseUtil.commaDelimitedStringToSet(excludePatterns)) { - excludedPatterns.add(Pattern.compile(pattern)); + excludedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); + } + } + + @Inject(value = XWorkConstants.ADDITIONAL_EXCLUDED_PATTERNS, required = false) + public void setAdditionalExcludePatterns(String excludePatterns) { + if (LOG.isDebugEnabled()) { + LOG.debug("Adding additional patterns [#0] to excluded patterns!", excludePatterns); + } + for (String pattern : TextParseUtil.commaDelimitedStringToSet(excludePatterns)) { + excludedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsCheckerTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsCheckerTest.java new file mode 100644 index 000000000..c2c079b04 --- /dev/null +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsCheckerTest.java @@ -0,0 +1,56 @@ +package com.opensymphony.xwork2.security; + +import com.opensymphony.xwork2.XWorkTestCase; + +import java.util.ArrayList; +import java.util.List; + +public class DefaultAcceptedPatternsCheckerTest extends XWorkTestCase { + + public void testHardcodedAcceptedPatterns() throws Exception { + // given + List params = new ArrayList() { + { + add("%{#application['test']}"); + add("%{#application.test}"); + add("%{#Application['test']}"); + add("%{#Application.test}"); + add("%{#session['test']}"); + add("%{#session.test}"); + add("%{#Session['test']}"); + add("%{#Session.test}"); + add("%{#struts['test']}"); + add("%{#struts.test}"); + add("%{#Struts['test']}"); + add("%{#Struts.test}"); + add("%{#request['test']}"); + add("%{#request.test}"); + add("%{#Request['test']}"); + add("%{#Request.test}"); + add("%{#servletRequest['test']}"); + add("%{#servletRequest.test}"); + add("%{#ServletRequest['test']}"); + add("%{#ServletRequest.test}"); + add("%{#servletResponse['test']}"); + add("%{#servletResponse.test}"); + add("%{#ServletResponse['test']}"); + add("%{#ServletResponse.test}"); + add("%{#parameters['test']}"); + add("%{#parameters.test}"); + add("%{#Parameters['test']}"); + add("%{#Parameters.test}"); + } + }; + + AcceptedPatternsChecker checker = new DefaultAcceptedPatternsChecker(); + + for (String param : params) { + // when + AcceptedPatternsChecker.IsAccepted actual = checker.isAccepted(param); + + // then + assertFalse("Access to " + param + " is possible!", actual.isAccepted()); + } + } + +} \ No newline at end of file diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java new file mode 100644 index 000000000..32121b964 --- /dev/null +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java @@ -0,0 +1,56 @@ +package com.opensymphony.xwork2.security; + +import com.opensymphony.xwork2.XWorkTestCase; + +import java.util.ArrayList; +import java.util.List; + +public class DefaultExcludedPatternsCheckerTest extends XWorkTestCase { + + public void testHardcodedPatterns() throws Exception { + // given + List params = new ArrayList() { + { + add("%{#application['test']}"); + add("%{#application.test}"); + add("%{#Application['test']}"); + add("%{#Application.test}"); + add("%{#session['test']}"); + add("%{#session.test}"); + add("%{#Session['test']}"); + add("%{#Session.test}"); + add("%{#struts['test']}"); + add("%{#struts.test}"); + add("%{#Struts['test']}"); + add("%{#Struts.test}"); + add("%{#request['test']}"); + add("%{#request.test}"); + add("%{#Request['test']}"); + add("%{#Request.test}"); + add("%{#servletRequest['test']}"); + add("%{#servletRequest.test}"); + add("%{#ServletRequest['test']}"); + add("%{#ServletRequest.test}"); + add("%{#servletResponse['test']}"); + add("%{#servletResponse.test}"); + add("%{#ServletResponse['test']}"); + add("%{#ServletResponse.test}"); + add("%{#parameters['test']}"); + add("%{#parameters.test}"); + add("%{#Parameters['test']}"); + add("%{#Parameters.test}"); + } + }; + + ExcludedPatternsChecker checker = new DefaultExcludedPatternsChecker(); + + for (String param : params) { + // when + ExcludedPatternsChecker.IsExcluded actual = checker.isExcluded(param); + + // then + assertTrue("Access to " + param + " is possible!", actual.isExcluded()); + } + } + +} \ No newline at end of file From 5ebc0643b55d728a6713a82559a594d875452cd8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 1 Jun 2014 10:49:20 +0200 Subject: [PATCH 45/48] Adds additional method to check if value of param isn't excluded --- .../interceptor/ParametersInterceptor.java | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java index c1b2f3dd1..d95c2a78c 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -273,7 +273,8 @@ public class ParametersInterceptor extends MethodFilterInterceptor { for (Map.Entry entry : params.entrySet()) { String name = entry.getKey(); - if (isAcceptableParameter(name, action)) { + Object value = entry.getValue(); + if (isAcceptableParameter(name, action) && isAcceptableValue(value)) { acceptableParameters.put(name, entry.getValue()); } } @@ -348,6 +349,33 @@ public class ParametersInterceptor extends MethodFilterInterceptor { return acceptableName(name) && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name)); } + /** + * Checks if given value doesn't match global excluded patterns to avoid passing malicious code + * + * @param value incoming parameter's value + * @return true if value is safe + * + * FIXME: can be removed when parameters won't be represented as simple Strings + */ + protected boolean isAcceptableValue(Object value) { + if (value == null) { + return true; + } + Object[] values; + if (value.getClass().isArray()) { + values = (Object[]) value; + } else { + values = new Object[] { value }; + } + boolean result = true; + for (Object obj : values) { + if (isExcluded(obj.toString())) { + result = false; + } + } + return result; + } + /** * Gets an instance of the comparator to use for the ordered sorting. Override this * method to customize the ordering of the parameters as they are set to the From eb8aae87521e627d3cd333e4dc351390bf1e80dc Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 5 Jun 2014 08:25:24 +0200 Subject: [PATCH 46/48] Adds additional default exclude patterns to avoid access to #context --- .../xwork2/security/DefaultExcludedPatternsChecker.java | 4 +++- .../xwork2/interceptor/ParametersInterceptorTest.java | 6 ++---- .../xwork2/security/DefaultExcludedPatternsCheckerTest.java | 4 ++++ 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java index f0a3d6248..983ce630b 100644 --- a/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java +++ b/xwork-core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java @@ -23,7 +23,9 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { "(^|.*#)request(\\.|\\[).*", "(^|.*#)application(\\.|\\[).*", "(^|.*#)servlet(Request|Response)(\\.|\\[).*", - "(^|.*#)parameters(\\.|\\[).*" + "(^|.*#)parameters(\\.|\\[).*", + "(^|.*#)context(\\.|\\[).*", + "(^|.*#)_memberAccess(\\.|\\[).*" }; private Set excludedPatterns; diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index ce86051b3..d6fc7c546 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -110,13 +110,11 @@ public class ParametersInterceptorTest extends XWorkTestCase { pi.setParameters(action, vs, params); // then - assertEquals(2, action.getActionMessages().size()); + assertEquals(1, action.getActionMessages().size()); String msg1 = action.getActionMessage(0); - String msg2 = action.getActionMessage(1); - assertTrue(msg1.contains("Error setting expression 'name' with value '(#context[\"xwork.MethodAccessor.denyMethodExecution\"]= new java.lang.Boolean(false), #_memberAccess[\"allowStaticMethodAccess\"]= new java.lang.Boolean(true), @java.lang.Runtime@getRuntime().exec('mkdir /tmp/PWNAGE'))(meh)'")); - assertTrue(msg2.contains("Error setting expression 'top['name'](0)' with value 'true'")); + assertTrue(msg1.contains("Error setting expression 'top['name'](0)' with value 'true'")); assertNull(action.getName()); } diff --git a/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java b/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java index 32121b964..612552187 100644 --- a/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java +++ b/xwork-core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java @@ -39,6 +39,10 @@ public class DefaultExcludedPatternsCheckerTest extends XWorkTestCase { add("%{#parameters.test}"); add("%{#Parameters['test']}"); add("%{#Parameters.test}"); + add("#context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse')"); + add("%{#context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse')}"); + add("#_memberAccess[\"allowStaticMethodAccess\"]= new java.lang.Boolean(true)"); + add("%{#_memberAccess[\"allowStaticMethodAccess\"]= new java.lang.Boolean(true)}"); } }; From bbcc6014f61e4d751114051605e8041474e5b496 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 5 Jun 2014 08:25:44 +0200 Subject: [PATCH 47/48] Excludes ActionContext from Ognl evaluation --- core/src/main/resources/struts-default.xml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 0fe8e68ce..49eba9095 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -38,7 +38,15 @@ - + From 965428711572ad52d3713b3432ff38e1dd3e9dae Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 18 Jun 2014 08:45:56 +0200 Subject: [PATCH 48/48] Adds additional classes to be excluded --- core/src/main/resources/struts-default.xml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 49eba9095..ea2a631c5 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -42,13 +42,17 @@ value=" java.lang.Object, java.lang.Runtime, + java.lang.System, + java.lang.Class, + java.lang.ClassLoader, + java.lang.Shutdown, ognl.OgnlContext, ognl.MemberAccess, ognl.ClassResolver, ognl.TypeConverter, com.opensymphony.xwork2.ActionContext" /> - +