Return-Path: X-Original-To: apmail-struts-commits-archive@minotaur.apache.org Delivered-To: apmail-struts-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 3FFBA117E1 for ; Fri, 27 Jun 2014 11:23:42 +0000 (UTC) Received: (qmail 98067 invoked by uid 500); 27 Jun 2014 11:23:40 -0000 Delivered-To: apmail-struts-commits-archive@struts.apache.org Received: (qmail 97995 invoked by uid 500); 27 Jun 2014 11:23:39 -0000 Mailing-List: contact commits-help@struts.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@struts.apache.org Delivered-To: mailing list commits@struts.apache.org Received: (qmail 97938 invoked by uid 99); 27 Jun 2014 11:23:39 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 27 Jun 2014 11:23:39 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id 6F38198D6D8; Fri, 27 Jun 2014 11:23:38 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: lukaszlenart@apache.org To: commits@struts.apache.org Date: Fri, 27 Jun 2014 11:24:06 -0000 Message-Id: <80968f401625407d98bbe88d36da4c71@git.apache.org> In-Reply-To: References: X-Mailer: ASF-Git Admin Mailer Subject: [38/50] git commit: Uses new service to check if param matches accepted patterns Uses new service to check if param matches accepted patterns Project: http://git-wip-us.apache.org/repos/asf/struts/repo Commit: http://git-wip-us.apache.org/repos/asf/struts/commit/8a93df10 Tree: http://git-wip-us.apache.org/repos/asf/struts/tree/8a93df10 Diff: http://git-wip-us.apache.org/repos/asf/struts/diff/8a93df10 Branch: refs/heads/feature/WW-4295-localization Commit: 8a93df10c4f5f3f22f1837c47b4ca9b4facc4f94 Parents: b140faa Author: Lukasz Lenart Authored: Wed May 21 09:03:51 2014 +0200 Committer: Lukasz Lenart Committed: Wed May 21 09:03:51 2014 +0200 ---------------------------------------------------------------------- .../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 +++++++++----------- .../interceptor/ParametersInterceptorTest.java | 11 +--- 6 files changed, 37 insertions(+), 41 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/struts/blob/8a93df10/core/src/main/java/org/apache/struts2/StrutsConstants.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index d173add..8c0c5ce 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"; } http://git-wip-us.apache.org/repos/asf/struts/blob/8a93df10/core/src/main/java/org/apache/struts2/config/DefaultBeanSelectionProvider.java ---------------------------------------------------------------------- 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 be4fa82..4334d3c 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); http://git-wip-us.apache.org/repos/asf/struts/blob/8a93df10/core/src/main/resources/struts-default.xml ---------------------------------------------------------------------- diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 2fc16c9..a1aa63f 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -145,6 +145,7 @@ + http://git-wip-us.apache.org/repos/asf/struts/blob/8a93df10/xwork-core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java ---------------------------------------------------------------------- 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 9f28334..19e8e76 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()); http://git-wip-us.apache.org/repos/asf/struts/blob/8a93df10/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java ---------------------------------------------------------------------- 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 f1906b0..c1b2f3d 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; @@ -471,6 +452,19 @@ public class ParametersInterceptor extends MethodFilterInterceptor { /** * 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. * * @param commaDelim A comma-delimited list of regular expressions http://git-wip-us.apache.org/repos/asf/struts/blob/8a93df10/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java ---------------------------------------------------------------------- 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 156c012..ce86051 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"); } };