From eca0666f0af5a38a6d422afaecf4467b4e32a0db Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 3 Jan 2024 22:12:17 +1100 Subject: [PATCH 01/19] WW-5352 Introduce StrutsParameter annotation --- .../org/apache/struts2/StrutsConstants.java | 2 + .../parameter/ParametersInterceptor.java | 16 ++++++- .../parameter/StrutsParameter.java | 44 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameter.java diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 939b3bddb0..64d539280b 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -469,6 +469,8 @@ public final class StrutsConstants { public static final String STRUTS_ADDITIONAL_EXCLUDED_PATTERNS = "struts.additional.excludedPatterns"; public static final String STRUTS_ADDITIONAL_ACCEPTED_PATTERNS = "struts.additional.acceptedPatterns"; + public static final String STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS = "struts.parameters.requireAnnotations"; + public static final String STRUTS_CONTENT_TYPE_MATCHER = "struts.contentTypeMatcher"; public static final String STRUTS_SMI_METHOD_REGEX = "struts.strictMethodInvocation.methodRegex"; diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index efc4a7b04e..b6dc5c87e5 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -70,6 +70,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { private boolean dmiEnabled = false; protected boolean ordered = false; + protected boolean requireAnnotations = false; private ValueStackFactory valueStackFactory; private ExcludedPatternsChecker excludedPatterns; @@ -87,6 +88,11 @@ public void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); } + @Inject(value = StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, required = false) + public void setRequireAnnotations(String requireAnnotations) { + this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations); + } + @Inject public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { this.excludedPatterns = excludedPatterns; @@ -295,13 +301,21 @@ protected void notifyDeveloperParameterException(Object action, String property, * @return true if parameter is accepted */ protected boolean isAcceptableParameter(String name, Object action) { - return acceptableName(name) && isAcceptableParameterNameAware(name, action); + return acceptableName(name) && isAcceptableParameterNameAware(name, action) && isParameterAnnotated(name, action); } protected boolean isAcceptableParameterNameAware(String name, Object action) { return !(action instanceof ParameterNameAware) || ((ParameterNameAware) action).acceptableParameterName(name); } + protected boolean isParameterAnnotated(String name, Object action) { + if (!requireAnnotations) { + return true; + } + // TODO: Implement + return true; + } + /** * Checks if parameter value can be accepted or thrown away * diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameter.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameter.java new file mode 100644 index 0000000000..2a19aa83fa --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameter.java @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Used to annotate public getter/setter methods or fields on {@link com.opensymphony.xwork2.Action} classes that are + * intended for parameter injection by the {@link ParametersInterceptor}. + * + * @since 6.4.0 + */ +@Target({ElementType.METHOD, ElementType.FIELD}) +@Retention(RetentionPolicy.RUNTIME) +public @interface StrutsParameter { + + /** + * The depth to which parameter injection is permitted, where a depth of 0 only allows setters/fields directly on + * the action class. Setting within a POJO on an action will require a depth of 1 or more depending on the level of + * nesting within the POJO. + *

+ * In a practical sense, the depth dictates the number of periods or brackets that can appear in the parameter name. + */ + int depth() default 0; +} From ad576f0fd59212bf5d7f4cf70001ad787ee9b746 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 3 Jan 2024 22:12:37 +1100 Subject: [PATCH 02/19] WW-5352 Introduce ThreadAllowlist bean --- .../config/impl/DefaultConfiguration.java | 2 + .../xwork2/ognl/SecurityMemberAccess.java | 10 ++-- .../parameter/ParametersInterceptor.java | 7 +++ .../apache/struts2/ognl/ThreadAllowlist.java | 51 +++++++++++++++++++ core/src/main/resources/struts-beans.xml | 1 + .../xwork2/ognl/SecurityMemberAccessTest.java | 6 ++- 6 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java index 3a3674a708..7bf0e7c779 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java @@ -112,6 +112,7 @@ import org.apache.struts2.ognl.OgnlGuard; import org.apache.struts2.ognl.ProviderAllowlist; import org.apache.struts2.ognl.StrutsOgnlGuard; +import org.apache.struts2.ognl.ThreadAllowlist; import java.util.ArrayList; import java.util.Collections; @@ -395,6 +396,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) .factory(ProviderAllowlist.class, Scope.SINGLETON) + .factory(ThreadAllowlist.class, Scope.SINGLETON) .factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); } diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 0cd5bf7896..510a65c605 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -26,6 +26,7 @@ import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.ognl.ThreadAllowlist; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Field; @@ -75,6 +76,7 @@ public class SecurityMemberAccess implements MemberAccess { ))); private final ProviderAllowlist providerAllowlist; + private final ThreadAllowlist threadAllowlist; private boolean allowStaticFieldAccess = true; private Set excludeProperties = emptySet(); private Set acceptProperties = emptySet(); @@ -89,8 +91,9 @@ public class SecurityMemberAccess implements MemberAccess { private boolean disallowDefaultPackageAccess = false; @Inject - public SecurityMemberAccess(@Inject ProviderAllowlist providerAllowlist) { + public SecurityMemberAccess(@Inject ProviderAllowlist providerAllowlist, @Inject ThreadAllowlist threadAllowlist) { this.providerAllowlist = providerAllowlist; + this.threadAllowlist = threadAllowlist; } /** @@ -99,11 +102,11 @@ public SecurityMemberAccess(@Inject ProviderAllowlist providerAllowlist) { * - block or allow access to properties (configurable-after-construction) * * @param allowStaticFieldAccess if set to true static fields (constants) will be accessible - * @deprecated since 6.4.0, use {@link #SecurityMemberAccess(ProviderAllowlist)} instead. + * @deprecated since 6.4.0, use {@link #SecurityMemberAccess(ProviderAllowlist, ThreadAllowlist)} instead. */ @Deprecated public SecurityMemberAccess(boolean allowStaticFieldAccess) { - this(null); + this(null, null); useAllowStaticFieldAccess(String.valueOf(allowStaticFieldAccess)); } @@ -223,6 +226,7 @@ protected boolean isClassAllowlisted(Class clazz) { return allowlistClasses.contains(clazz) || ALLOWLIST_REQUIRED_CLASSES.contains(clazz) || (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz)) + || (threadAllowlist != null && threadAllowlist.getAllowlist().contains(clazz)) || isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES) || isClassBelongsToPackages(clazz, allowlistPackageNames); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index b6dc5c87e5..7d8cba359b 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -41,6 +41,7 @@ import org.apache.struts2.action.ParameterValueAware; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.Parameter; +import org.apache.struts2.ognl.ThreadAllowlist; import java.util.Collection; import java.util.Comparator; @@ -73,6 +74,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { protected boolean requireAnnotations = false; private ValueStackFactory valueStackFactory; + protected ThreadAllowlist threadAllowlist; private ExcludedPatternsChecker excludedPatterns; private AcceptedPatternsChecker acceptedPatterns; private Set excludedValuePatterns = null; @@ -83,6 +85,11 @@ public void setValueStackFactory(ValueStackFactory valueStackFactory) { this.valueStackFactory = valueStackFactory; } + @Inject + public void setThreadAllowlist(ThreadAllowlist threadAllowlist) { + this.threadAllowlist = threadAllowlist; + } + @Inject(StrutsConstants.STRUTS_DEVMODE) public void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); diff --git a/core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java b/core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java new file mode 100644 index 0000000000..e07cae64e9 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.ognl; + +import java.util.HashSet; +import java.util.Set; + +import static java.util.Collections.emptySet; +import static java.util.Collections.unmodifiableSet; + +/** + * Allows any bean to allowlist a class for use in OGNL expressions, for the current thread only. The allowlist can be + * cleared once any desired OGNL expressions have been evaluated. + * + * @since 6.4.0 + */ +public class ThreadAllowlist { + + private final ThreadLocal>> allowlist = new ThreadLocal<>(); + + public void allowClass(Class clazz) { + if (allowlist.get() == null) { + allowlist.set(new HashSet<>()); + } + allowlist.get().add(clazz); + } + + public void clearAllowlist() { + allowlist.remove(); + } + + public Set> getAllowlist() { + return allowlist.get() != null ? unmodifiableSet(allowlist.get()) : emptySet(); + } +} diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 93614fa0b5..5c3121f77f 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -170,6 +170,7 @@ + diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index 7e5a22bcc9..03bad82e41 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -25,6 +25,7 @@ import ognl.MemberAccess; import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.ognl.ThreadAllowlist; import org.junit.Before; import org.junit.Test; @@ -54,18 +55,21 @@ public class SecurityMemberAccessTest { private FooBar target; protected SecurityMemberAccess sma; private ProviderAllowlist mockedProviderAllowlist; + private ThreadAllowlist mockedThreadAllowlist; @Before public void setUp() throws Exception { context = new HashMap<>(); target = new FooBar(); mockedProviderAllowlist = mock(ProviderAllowlist.class); + mockedThreadAllowlist = mock(ThreadAllowlist.class); assignNewSma(true); } protected void assignNewSma(boolean allowStaticFieldAccess) { when(mockedProviderAllowlist.getProviderAllowlist()).thenReturn(new HashSet<>()); - sma = new SecurityMemberAccess(mockedProviderAllowlist); + when(mockedThreadAllowlist.getAllowlist()).thenReturn(new HashSet<>()); + sma = new SecurityMemberAccess(mockedProviderAllowlist, mockedThreadAllowlist); sma.useAllowStaticFieldAccess(String.valueOf(allowStaticFieldAccess)); } From 4255da3ee9590b38c2ab1b221f49034076b91a33 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 4 Jan 2024 00:53:12 +1100 Subject: [PATCH 03/19] WW-5352 First draft implementation --- .../parameter/ParametersInterceptor.java | 146 ++++++++++++++++-- 1 file changed, 137 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 7d8cba359b..08c956f21a 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -25,6 +25,7 @@ import com.opensymphony.xwork2.interceptor.MethodFilterInterceptor; import com.opensymphony.xwork2.interceptor.ValidationAware; import com.opensymphony.xwork2.security.AcceptedPatternsChecker; +import com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker; import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.util.ClearableValueStack; import com.opensymphony.xwork2.util.MemberAccessValueStack; @@ -43,16 +44,27 @@ import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.ognl.ThreadAllowlist; +import java.beans.BeanInfo; +import java.beans.IntrospectionException; +import java.beans.Introspector; +import java.beans.PropertyDescriptor; +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.TreeMap; import java.util.regex.Pattern; import static java.util.Collections.unmodifiableSet; import static java.util.stream.Collectors.joining; +import static org.apache.commons.lang3.StringUtils.indexOfAny; import static org.apache.commons.lang3.StringUtils.normalizeSpace; /** @@ -204,13 +216,19 @@ protected void setParameters(final Object action, ValueStack stack, HttpParamete } protected void applyParameters(final Object action, ValueStack stack, HttpParameters parameters) { - Map acceptableParameters = toAcceptableParameters(parameters, action); + Map acceptableParameters; + ValueStack newStack; + try { + acceptableParameters = toAcceptableParameters(parameters, action); // Side-effect: Allowlist required types - ValueStack newStack = toNewStack(stack); - batchApplyReflectionContextState(newStack.getContext(), true); - applyMemberAccessProperties(newStack); + newStack = toNewStack(stack); + batchApplyReflectionContextState(newStack.getContext(), true); + applyMemberAccessProperties(newStack); - applyParametersOnStack(newStack, acceptableParameters, action); + applyParametersOnStack(newStack, acceptableParameters, action); + } finally { + threadAllowlist.clearAllowlist(); + } if (newStack instanceof ClearableValueStack) { stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors()); @@ -308,19 +326,129 @@ protected void notifyDeveloperParameterException(Object action, String property, * @return true if parameter is accepted */ protected boolean isAcceptableParameter(String name, Object action) { - return acceptableName(name) && isAcceptableParameterNameAware(name, action) && isParameterAnnotated(name, action); + return acceptableName(name) && isAcceptableParameterNameAware(name, action) && isParameterAnnotatedAndAllowlist(name, action); } protected boolean isAcceptableParameterNameAware(String name, Object action) { return !(action instanceof ParameterNameAware) || ((ParameterNameAware) action).acceptableParameterName(name); } - protected boolean isParameterAnnotated(String name, Object action) { + /** + * Checks if the Action class member corresponding to a parameter is appropriately annotated with + * {@link StrutsParameter} and OGNL allowlists any necessary classes. + *

+ * Note that this logic relies on the use of {@link DefaultAcceptedPatternsChecker#ACCEPTED_PATTERNS} and may also + * be adversely impacted by the use of custom OGNL property accessors. + */ + protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { if (!requireAnnotations) { return true; } - // TODO: Implement - return true; + + int nestingIndex = indexOfAny(name, ".["); + String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); + long paramDepth = name.chars().filter(ch -> ch == '.' || ch == '[').count(); + + return hasValidAnnotatedMember(rootProperty, action, paramDepth); + } + + /** + * Note that we check for a public field last or only if there is no valid, annotated property descriptor. This is + * because this check is likely to fail more often than not, as the relative use of public fields is low - so we + * save computation by checking this last. + */ + protected boolean hasValidAnnotatedMember(String rootProperty, Object action, long paramDepth) { + BeanInfo beanInfo = getBeanInfo(action); + if (beanInfo == null) { + return hasValidAnnotatedField(action, rootProperty, paramDepth); + } + + Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) + .filter(desc -> desc.getName().equals(rootProperty)).findFirst(); + if (!propDescOpt.isPresent()) { + return hasValidAnnotatedField(action, rootProperty, paramDepth); + } + + if (hasValidAnnotatedPropertyDescriptor(propDescOpt.get(), paramDepth)) { + return true; + } + + return hasValidAnnotatedField(action, rootProperty, paramDepth); + } + + protected boolean hasValidAnnotatedPropertyDescriptor(PropertyDescriptor propDesc, long paramDepth) { + Class rootType = getValidAnnotatedPropertyDescriptorType(propDesc, paramDepth); + if (rootType != null) { + if (paramDepth > 0) { + threadAllowlist.allowClass(rootType); + } + return true; + } + return false; + } + + /** + * @return getter return type or setter parameter type, if one corresponding to the paramDepth exists + * with a valid annotation + */ + protected Class getValidAnnotatedPropertyDescriptorType(PropertyDescriptor propDesc, long paramDepth) { + Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); + if (relevantMethod == null) { + return null; + } + StrutsParameter annotation = getParameterAnnotation(relevantMethod); + if (annotation != null && annotation.depth() >= paramDepth) { + return paramDepth == 0 ? relevantMethod.getParameterTypes()[0] : relevantMethod.getReturnType(); + } + return null; + } + + protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) { + Class rootType = getValidAnnotatedFieldType(action, fieldName, paramDepth); + if (rootType != null) { + if (paramDepth > 0) { + threadAllowlist.allowClass(rootType); + } + return true; + } + return false; + } + + /** + * @return field type if a public field exists on the action with a valid annotation + */ + protected Class getValidAnnotatedFieldType(Object action, String fieldName, long paramDepth) { + Field field; + try { + field = action.getClass().getDeclaredField(fieldName); + } catch (NoSuchFieldException e) { + return null; + } + if (!Modifier.isPublic(field.getModifiers())) { + return null; + } + StrutsParameter annotation = getParameterAnnotation(field); + if (annotation != null && annotation.depth() >= paramDepth) { + return field.getType(); + } + return null; + } + + /** + * Annotation retrieval logic. Can be overridden to support extending annotations or some other form of annotation + * inheritance. + */ + protected StrutsParameter getParameterAnnotation(AnnotatedElement element) { + return element.getAnnotation(StrutsParameter.class); + } + + protected BeanInfo getBeanInfo(Object action) { + try { + return Introspector.getBeanInfo(action.getClass()); + } catch (IntrospectionException e) { + LOG.warn("Error introspecting Action {} for parameter injection validation", action.getClass(), e); + return null; + } } /** From bf3f407b5fffac1d74142c52fa28fda91a542f8f Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 15:48:35 +1100 Subject: [PATCH 04/19] WW-5352 Ensure allowlist is cleared if in unexpected state --- .../struts2/interceptor/parameter/ParametersInterceptor.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 08c956f21a..f1651cd86c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -219,6 +219,10 @@ protected void applyParameters(final Object action, ValueStack stack, HttpParame Map acceptableParameters; ValueStack newStack; try { + if (!threadAllowlist.getAllowlist().isEmpty()) { + LOG.error("Thread allowlist was utilised but not cleared", new IllegalStateException()); + threadAllowlist.clearAllowlist(); + } acceptableParameters = toAcceptableParameters(parameters, action); // Side-effect: Allowlist required types newStack = toNewStack(stack); From 4c5f2b02666628c4a144964bca8a75bc5cbbb8f7 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 15:48:53 +1100 Subject: [PATCH 05/19] WW-5352 Add full unit test coverage --- .../StrutsParameterAnnotationTest.java | 240 ++++++++++++++++++ 1 file changed, 240 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java new file mode 100644 index 0000000000..7b6bbd2607 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -0,0 +1,240 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; +import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; +import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.dispatcher.Parameter; +import org.apache.struts2.ognl.ThreadAllowlist; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class StrutsParameterAnnotationTest { + + private ParametersInterceptor parametersInterceptor; + + private ThreadAllowlist threadAllowlist; + + @Before + public void setUp() throws Exception { + parametersInterceptor = new ParametersInterceptor(); + parametersInterceptor.setRequireAnnotations(Boolean.TRUE.toString()); + + threadAllowlist = new ThreadAllowlist(); + parametersInterceptor.setThreadAllowlist(threadAllowlist); + + NotExcludedAcceptedPatternsChecker checker = mock(NotExcludedAcceptedPatternsChecker.class); + when(checker.isAccepted(anyString())).thenReturn(AcceptedPatternsChecker.IsAccepted.yes("")); + when(checker.isExcluded(anyString())).thenReturn(NotExcludedAcceptedPatternsChecker.IsExcluded.no(new HashSet<>())); + parametersInterceptor.setAcceptedPatterns(checker); + parametersInterceptor.setExcludedPatterns(checker); + } + + @After + public void tearDown() throws Exception { + threadAllowlist.clearAllowlist(); + } + + private void testParameter(Object action, String paramName, boolean shouldContain) { + Map requestParamMap = new HashMap<>(); + requestParamMap.put(paramName, new String[]{"value"}); + HttpParameters httpParameters = HttpParameters.create(requestParamMap).build(); + + Map acceptedParameters = parametersInterceptor.toAcceptableParameters(httpParameters, action); + + if (shouldContain) { + assertThat(acceptedParameters).containsOnlyKeys(paramName); + } else { + assertThat(acceptedParameters).isEmpty(); + assertThat(threadAllowlist.getAllowlist()).isEmpty(); + } + } + + @Test + public void privateStrAnnotated() { + testParameter(new FieldAction(), "privateStr", false); + } + + @Test + public void publicStrAnnotated() { + testParameter(new FieldAction(), "publicStr", true); + assertThat(threadAllowlist.getAllowlist()).isEmpty(); + } + + @Test + public void publicStrNotAnnotated() { + testParameter(new FieldAction(), "publicStrNotAnnotated", false); + } + + @Test + public void privatePojoAnnotated() { + testParameter(new FieldAction(), "privatePojo.key", false); + } + + @Test + public void publicPojoDepthZero() { + testParameter(new FieldAction(), "publicPojoDepthZero.key", false); + } + + @Test + public void publicPojoDepthOne() { + testParameter(new FieldAction(), "publicPojoDepthOne.key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void publicNestedPojoDepthOne() { + testParameter(new FieldAction(), "publicPojoDepthOne.key.key", false); + } + + @Test + public void publicPojoDepthTwo() { + testParameter(new FieldAction(), "publicPojoDepthTwo.key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void publicNestedPojoDepthTwo() { + testParameter(new FieldAction(), "publicPojoDepthTwo.key.key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void privateStrAnnotatedMethod() { + testParameter(new MethodAction(), "privateStr", false); + } + + @Test + public void publicStrAnnotatedMethod() { + testParameter(new MethodAction(), "publicStr", true); + assertThat(threadAllowlist.getAllowlist()).isEmpty(); + } + + @Test + public void publicStrNotAnnotatedMethod() { + testParameter(new MethodAction(), "publicStrNotAnnotated", false); + } + + @Test + public void privatePojoAnnotatedMethod() { + testParameter(new MethodAction(), "privatePojo.key", false); + } + + @Test + public void publicPojoDepthZeroMethod() { + testParameter(new MethodAction(), "publicPojoDepthZero.key", false); + } + + @Test + public void publicPojoDepthOneMethod() { + testParameter(new MethodAction(), "publicPojoDepthOne.key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void publicNestedPojoDepthOneMethod() { + testParameter(new MethodAction(), "publicPojoDepthOne.key.key", false); + } + + @Test + public void publicPojoDepthTwoMethod() { + testParameter(new MethodAction(), "publicPojoDepthTwo.key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void publicNestedPojoDepthTwoMethod() { + testParameter(new MethodAction(), "publicPojoDepthTwo.key.key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + class FieldAction { + @StrutsParameter + private String privateStr; + + @StrutsParameter + public String publicStr; + + public String publicStrNotAnnotated; + + @StrutsParameter(depth = 1) + private Pojo privatePojo; + + @StrutsParameter + public Pojo publicPojoDepthZero; + + @StrutsParameter(depth = 1) + public Pojo publicPojoDepthOne ; + + @StrutsParameter(depth = 2) + public Pojo publicPojoDepthTwo; + } + + class MethodAction { + + @StrutsParameter + private void setPrivateStr(String str) { + } + + @StrutsParameter + public void setPublicStr(String str) { + } + + public void setPublicStrNotAnnotated(String str) { + } + + @StrutsParameter(depth = 1) + private Pojo getPrivatePojo() { + return null; + } + + @StrutsParameter + public Pojo getPublicPojoDepthZero() { + return null; + } + + @StrutsParameter + public void setPublicPojoDepthZero() { + } + + @StrutsParameter(depth = 1) + public Pojo getPublicPojoDepthOne() { + return null; + } + + @StrutsParameter(depth = 2) + public Pojo getPublicPojoDepthTwo() { + return null; + } + } + + class Pojo { + } +} From 5d793012350e96184b01079eff9ccbd024d7fc8b Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 17:21:02 +1100 Subject: [PATCH 06/19] WW-5352 Fix missing curved bracket --- .../DefaultAcceptedPatternsChecker.java | 16 +++++++++---- .../parameter/ParametersInterceptor.java | 7 +++--- .../StrutsParameterAnnotationTest.java | 24 +++++++++++++++++++ 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java index 0896fec826..4d2caa594f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java +++ b/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java @@ -25,12 +25,13 @@ import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; -import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.regex.Pattern; +import static java.util.Arrays.asList; +import static java.util.Collections.unmodifiableSet; + public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { private static final Logger LOG = LogManager.getLogger(DefaultAcceptedPatternsChecker.class); @@ -39,6 +40,11 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { "\\w+((\\.\\w+)|(\\[\\d+])|(\\(\\d+\\))|(\\['(\\w-?|[\\u4e00-\\u9fa5]-?)+'])|(\\('(\\w-?|[\\u4e00-\\u9fa5]-?)+'\\)))*" }; + /** + * Must match {@link #ACCEPTED_PATTERNS} RegEx. Signifies characters which result in a nested lookup via OGNL. + */ + public static final Set NESTING_CHARS = unmodifiableSet(new HashSet<>(asList('.', '[', '('))); + public static final String[] DMI_AWARE_ACCEPTED_PATTERNS = { "\\w+([:]?\\w+)?((\\.\\w+)|(\\[\\d+])|(\\(\\d+\\))|(\\['(\\w-?|[\\u4e00-\\u9fa5]-?)+'])|(\\('(\\w-?|[\\u4e00-\\u9fa5]-?)+'\\)))*([!]?\\w+)?" }; @@ -74,7 +80,7 @@ protected void setAdditionalAcceptedPatterns(String acceptablePatterns) { newAcceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } finally { - acceptedPatterns = Collections.unmodifiableSet(newAcceptedPatterns); + acceptedPatterns = unmodifiableSet(newAcceptedPatterns); } } @@ -85,7 +91,7 @@ public void setAcceptedPatterns(String commaDelimitedPatterns) { @Override public void setAcceptedPatterns(String[] additionalPatterns) { - setAcceptedPatterns(new HashSet<>(Arrays.asList(additionalPatterns))); + setAcceptedPatterns(new HashSet<>(asList(additionalPatterns))); } @Override @@ -97,7 +103,7 @@ public void setAcceptedPatterns(Set patterns) { newAcceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } finally { - acceptedPatterns = Collections.unmodifiableSet(newAcceptedPatterns); + acceptedPatterns = unmodifiableSet(newAcceptedPatterns); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index f1651cd86c..6833a796b7 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -62,6 +62,7 @@ import java.util.TreeMap; import java.util.regex.Pattern; +import static com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS; import static java.util.Collections.unmodifiableSet; import static java.util.stream.Collectors.joining; import static org.apache.commons.lang3.StringUtils.indexOfAny; @@ -341,7 +342,7 @@ protected boolean isAcceptableParameterNameAware(String name, Object action) { * Checks if the Action class member corresponding to a parameter is appropriately annotated with * {@link StrutsParameter} and OGNL allowlists any necessary classes. *

- * Note that this logic relies on the use of {@link DefaultAcceptedPatternsChecker#ACCEPTED_PATTERNS} and may also + * Note that this logic relies on the use of {@link DefaultAcceptedPatternsChecker#NESTING_CHARS} and may also * be adversely impacted by the use of custom OGNL property accessors. */ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { @@ -349,9 +350,9 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { return true; } - int nestingIndex = indexOfAny(name, ".["); + int nestingIndex = indexOfAny(name, NESTING_CHARS.stream().map(String::valueOf).collect(joining())); String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); - long paramDepth = name.chars().filter(ch -> ch == '.' || ch == '[').count(); + long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); return hasValidAnnotatedMember(rootProperty, action, paramDepth); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 7b6bbd2607..839456e435 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -109,6 +109,18 @@ public void publicPojoDepthOne() { assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); } + @Test + public void publicPojoDepthOne_sqrBracket() { + testParameter(new FieldAction(), "publicPojoDepthOne['key']", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void publicPojoDepthOne_bracket() { + testParameter(new FieldAction(), "publicPojoDepthOne('key')", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + @Test public void publicNestedPojoDepthOne() { testParameter(new FieldAction(), "publicPojoDepthOne.key.key", false); @@ -126,6 +138,18 @@ public void publicNestedPojoDepthTwo() { assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); } + @Test + public void publicNestedPojoDepthTwo_sqrBracket() { + testParameter(new FieldAction(), "publicPojoDepthTwo['key']['key']", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + + @Test + public void publicNestedPojoDepthTwo_bracket() { + testParameter(new FieldAction(), "publicPojoDepthTwo('key')('key')", true); + assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + } + @Test public void privateStrAnnotatedMethod() { testParameter(new MethodAction(), "privateStr", false); From 4c60f39c7acb0bfbc802be6a4f179a8113ca283f Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 17:21:21 +1100 Subject: [PATCH 07/19] WW-5352 Enable annotations for showcase --- .../apache/struts2/showcase/UITagExample.java | 24 +++++++++++++++++-- .../struts2/showcase/action/SkillAction.java | 2 ++ .../showcase/async/ChatRoomAction.java | 3 +++ .../showcase/conversion/AddressAction.java | 4 +++- .../conversion/OperationsEnumAction.java | 2 ++ .../showcase/conversion/PersonAction.java | 2 ++ .../filedownload/FileDownloadAction.java | 2 ++ .../showcase/fileupload/FileUploadAction.java | 2 ++ .../FieldValidatorsExampleAction.java | 11 +++++++++ .../showcase/wait/LongProcessAction.java | 2 ++ apps/showcase/src/main/resources/struts.xml | 12 +--------- 11 files changed, 52 insertions(+), 14 deletions(-) diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/UITagExample.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/UITagExample.java index ca5aecbb7a..1e87b2193d 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/UITagExample.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/UITagExample.java @@ -24,9 +24,15 @@ import com.opensymphony.xwork2.Validateable; import com.opensymphony.xwork2.util.ValueStack; import org.apache.struts2.ServletActionContext; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.io.File; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; /** */ @@ -89,6 +95,7 @@ public List getLeftSideCartoonCharacters() { return leftSideCartoonCharacters; } + @StrutsParameter public void setLeftSideCartoonCharacters(List leftSideCartoonCharacters) { this.leftSideCartoonCharacters = leftSideCartoonCharacters; } @@ -98,6 +105,7 @@ public List getRightSideCartoonCharacters() { return rightSideCartoonCharacters; } + @StrutsParameter public void setRightSideCartoonCharacters(List rightSideCartoonCharacters) { this.rightSideCartoonCharacters = rightSideCartoonCharacters; } @@ -107,6 +115,7 @@ public String getFavouriteVehicalType() { return favouriteVehicalType; } + @StrutsParameter public void setFavouriteVehicalType(String favouriteVehicalType) { this.favouriteVehicalType = favouriteVehicalType; } @@ -115,6 +124,7 @@ public String getFavouriteVehicalSpecific() { return favouriteVehicalSpecific; } + @StrutsParameter public void setFavouriteVehicalSpecific(String favouriteVehicalSpecific) { this.favouriteVehicalSpecific = favouriteVehicalSpecific; } @@ -145,6 +155,7 @@ public String getName() { return name; } + @StrutsParameter public void setName(String name) { this.name = name; } @@ -153,6 +164,7 @@ public Date getBirthday() { return birthday; } + @StrutsParameter public void setBirthday(Date birthday) { this.birthday = birthday; } @@ -161,6 +173,7 @@ public String getBio() { return bio; } + @StrutsParameter public void setBio(String bio) { this.bio = bio; } @@ -169,6 +182,7 @@ public String getFavouriteColor() { return favouriteColor; } + @StrutsParameter public void setFavouriteColor(String favoriteColor) { this.favouriteColor = favoriteColor; } @@ -177,6 +191,7 @@ public List getFriends() { return friends; } + @StrutsParameter public void setFriends(List friends) { this.friends = friends; } @@ -193,6 +208,7 @@ public boolean isLegalAge() { return legalAge; } + @StrutsParameter public void setLegalAge(boolean legalAge) { this.legalAge = legalAge; } @@ -201,6 +217,7 @@ public String getState() { return state; } + @StrutsParameter public void setState(String state) { this.state = state; } @@ -209,6 +226,7 @@ public String getRegion() { return region; } + @StrutsParameter public void setRegion(String region) { this.region = region; } @@ -229,6 +247,7 @@ public void setPictureFileName(String pictureFileName) { this.pictureFileName = pictureFileName; } + @StrutsParameter public void setFavouriteLanguage(String favouriteLanguage) { this.favouriteLanguage = favouriteLanguage; } @@ -237,7 +256,7 @@ public String getFavouriteLanguage() { return favouriteLanguage; } - + @StrutsParameter public void setThoughts(String thoughts) { this.thoughts = thoughts; } @@ -250,6 +269,7 @@ public Date getWakeup() { return wakeup; } + @StrutsParameter public void setWakeup(Date wakeup) { this.wakeup = wakeup; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/action/SkillAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/action/SkillAction.java index 1c96d20dca..6ba2096915 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/action/SkillAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/action/SkillAction.java @@ -21,6 +21,7 @@ import com.opensymphony.xwork2.Preparable; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import org.apache.struts2.showcase.dao.Dao; import org.apache.struts2.showcase.dao.SkillDao; import org.apache.struts2.showcase.model.Skill; @@ -71,6 +72,7 @@ protected Dao getDao() { return skillDao; } + @StrutsParameter(depth = 1) public Skill getCurrentSkill() { return currentSkill; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/async/ChatRoomAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/async/ChatRoomAction.java index 5877fe6e19..67b27e3b6c 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/async/ChatRoomAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/async/ChatRoomAction.java @@ -19,6 +19,7 @@ package org.apache.struts2.showcase.async; import com.opensymphony.xwork2.ActionSupport; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.util.ArrayList; import java.util.List; @@ -34,10 +35,12 @@ public class ChatRoomAction extends ActionSupport { private static final List messages = new ArrayList<>(); + @StrutsParameter public void setMessage(String message) { this.message = message; } + @StrutsParameter public void setLastIndex(Integer lastIndex) { this.lastIndex = lastIndex; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/AddressAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/AddressAction.java index 66e9c27467..0f764b787b 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/AddressAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/AddressAction.java @@ -21,6 +21,7 @@ package org.apache.struts2.showcase.conversion; import com.opensymphony.xwork2.ActionSupport; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.util.LinkedHashSet; import java.util.Set; @@ -30,7 +31,7 @@ */ public class AddressAction extends ActionSupport { - private Set

addresses = new LinkedHashSet
(); + private Set
addresses = new LinkedHashSet<>(); public String input() throws Exception { return SUCCESS; @@ -41,6 +42,7 @@ public String submit() throws Exception { return SUCCESS; } + @StrutsParameter(depth = 2) public Set
getAddresses() { return addresses; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/OperationsEnumAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/OperationsEnumAction.java index a8f8cd624b..272c8b6cc2 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/OperationsEnumAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/OperationsEnumAction.java @@ -21,6 +21,7 @@ package org.apache.struts2.showcase.conversion; import com.opensymphony.xwork2.ActionSupport; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.util.Arrays; import java.util.LinkedList; @@ -47,6 +48,7 @@ public List getSelectedOperations() { return this.selectedOperations; } + @StrutsParameter public void setSelectedOperations(List selectedOperations) { this.selectedOperations = selectedOperations; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/PersonAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/PersonAction.java index 70307db80e..27df30a974 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/PersonAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/conversion/PersonAction.java @@ -21,6 +21,7 @@ package org.apache.struts2.showcase.conversion; import com.opensymphony.xwork2.ActionSupport; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.util.List; @@ -36,6 +37,7 @@ public String submit() throws Exception { return SUCCESS; } + @StrutsParameter(depth = 2) public List getPersons() { return persons; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/filedownload/FileDownloadAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/filedownload/FileDownloadAction.java index c9fab7f468..5f23c19d7e 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/filedownload/FileDownloadAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/filedownload/FileDownloadAction.java @@ -22,6 +22,7 @@ import com.opensymphony.xwork2.Action; import org.apache.struts2.ServletActionContext; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.io.InputStream; @@ -38,6 +39,7 @@ public String execute() throws Exception { return SUCCESS; } + @StrutsParameter public void setInputPath(String value) { inputPath = sanitizeInputPath(value); } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/fileupload/FileUploadAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/fileupload/FileUploadAction.java index c2ac471f45..279c1e928e 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/fileupload/FileUploadAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/fileupload/FileUploadAction.java @@ -23,6 +23,7 @@ import com.opensymphony.xwork2.ActionSupport; import org.apache.struts2.action.UploadedFilesAware; import org.apache.struts2.dispatcher.multipart.UploadedFile; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.util.List; @@ -65,6 +66,7 @@ public String getCaption() { return caption; } + @StrutsParameter public void setCaption(String caption) { this.caption = caption; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/validation/FieldValidatorsExampleAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/validation/FieldValidatorsExampleAction.java index a9e02deabf..9639bacd17 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/validation/FieldValidatorsExampleAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/validation/FieldValidatorsExampleAction.java @@ -20,6 +20,8 @@ */ package org.apache.struts2.showcase.validation; +import org.apache.struts2.interceptor.parameter.StrutsParameter; + import java.sql.Date; /** @@ -44,6 +46,7 @@ public Date getDateValidatorField() { return dateValidatorField; } + @StrutsParameter public void setDateValidatorField(Date dateValidatorField) { this.dateValidatorField = dateValidatorField; } @@ -52,6 +55,7 @@ public String getEmailValidatorField() { return emailValidatorField; } + @StrutsParameter public void setEmailValidatorField(String emailValidatorField) { this.emailValidatorField = emailValidatorField; } @@ -60,6 +64,7 @@ public Integer getIntegerValidatorField() { return integerValidatorField; } + @StrutsParameter public void setIntegerValidatorField(Integer integerValidatorField) { this.integerValidatorField = integerValidatorField; } @@ -68,6 +73,7 @@ public String getRegexValidatorField() { return regexValidatorField; } + @StrutsParameter public void setRegexValidatorField(String regexValidatorField) { this.regexValidatorField = regexValidatorField; } @@ -76,6 +82,7 @@ public String getRequiredStringValidatorField() { return requiredStringValidatorField; } + @StrutsParameter public void setRequiredStringValidatorField(String requiredStringValidatorField) { this.requiredStringValidatorField = requiredStringValidatorField; } @@ -84,6 +91,7 @@ public String getRequiredValidatorField() { return requiredValidatorField; } + @StrutsParameter public void setRequiredValidatorField(String requiredValidatorField) { this.requiredValidatorField = requiredValidatorField; } @@ -92,6 +100,7 @@ public String getStringLengthValidatorField() { return stringLengthValidatorField; } + @StrutsParameter public void setStringLengthValidatorField(String stringLengthValidatorField) { this.stringLengthValidatorField = stringLengthValidatorField; } @@ -100,6 +109,7 @@ public String getFieldExpressionValidatorField() { return fieldExpressionValidatorField; } + @StrutsParameter public void setFieldExpressionValidatorField( String fieldExpressionValidatorField) { this.fieldExpressionValidatorField = fieldExpressionValidatorField; @@ -109,6 +119,7 @@ public String getUrlValidatorField() { return urlValidatorField; } + @StrutsParameter public void setUrlValidatorField(String urlValidatorField) { this.urlValidatorField = urlValidatorField; } diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/wait/LongProcessAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/wait/LongProcessAction.java index 8ff6859bb0..cd14a36746 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/wait/LongProcessAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/wait/LongProcessAction.java @@ -21,6 +21,7 @@ package org.apache.struts2.showcase.wait; import com.opensymphony.xwork2.ActionSupport; +import org.apache.struts2.interceptor.parameter.StrutsParameter; /** * Example to illustrate the execAndWait interceptor. @@ -41,6 +42,7 @@ public int getTime() { return time; } + @StrutsParameter public void setTime(int time) { this.time = time; } diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index a9fe3c9dca..373cf5f7b2 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -35,17 +35,7 @@ - - + From b2c75422659499622a6af838e91e9feb9e56d350 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 17:52:09 +1100 Subject: [PATCH 08/19] WW-5352 Dispatcher should up thread allowlist --- .../apache/struts2/dispatcher/Dispatcher.java | 9 +++++++++ .../parameter/ParametersInterceptor.java | 20 +++++-------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java index 3947d89d25..4a8f7215c5 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java @@ -68,6 +68,7 @@ import org.apache.struts2.dispatcher.mapper.ActionMapping; import org.apache.struts2.dispatcher.multipart.MultiPartRequest; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; +import org.apache.struts2.ognl.ThreadAllowlist; import org.apache.struts2.util.ObjectFactoryDestroyable; import org.apache.struts2.util.fs.JBossFileManager; @@ -199,6 +200,7 @@ public class Dispatcher { private LocaleProviderFactory localeProviderFactory; private StaticContentLoader staticContentLoader; private ActionMapper actionMapper; + private ThreadAllowlist threadAllowlist; /** * Provide the dispatcher instance for the current thread. @@ -404,6 +406,11 @@ public ActionMapper getActionMapper() { return actionMapper; } + @Inject + public void setThreadAllowlist(ThreadAllowlist threadAllowlist) { + this.threadAllowlist = threadAllowlist; + } + /** * Releases all instances bound to this dispatcher instance. */ @@ -712,6 +719,8 @@ public void serviceAction(HttpServletRequest request, HttpServletResponse respon } else { throw new ServletException(e); } + } finally { + threadAllowlist.clearAllowlist(); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 6833a796b7..1443fddebb 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -217,23 +217,13 @@ protected void setParameters(final Object action, ValueStack stack, HttpParamete } protected void applyParameters(final Object action, ValueStack stack, HttpParameters parameters) { - Map acceptableParameters; - ValueStack newStack; - try { - if (!threadAllowlist.getAllowlist().isEmpty()) { - LOG.error("Thread allowlist was utilised but not cleared", new IllegalStateException()); - threadAllowlist.clearAllowlist(); - } - acceptableParameters = toAcceptableParameters(parameters, action); // Side-effect: Allowlist required types + Map acceptableParameters = toAcceptableParameters(parameters, action); - newStack = toNewStack(stack); - batchApplyReflectionContextState(newStack.getContext(), true); - applyMemberAccessProperties(newStack); + ValueStack newStack = toNewStack(stack); + batchApplyReflectionContextState(newStack.getContext(), true); + applyMemberAccessProperties(newStack); - applyParametersOnStack(newStack, acceptableParameters, action); - } finally { - threadAllowlist.clearAllowlist(); - } + applyParametersOnStack(newStack, acceptableParameters, action); if (newStack instanceof ClearableValueStack) { stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors()); From a57c2882e7fd0d3fcbfdc6442b0516a9b14ed3a9 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 17:52:37 +1100 Subject: [PATCH 09/19] WW-5352 Reinstate manual allowlist for generic types --- apps/showcase/src/main/resources/struts.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index 373cf5f7b2..eca6c62712 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -36,6 +36,12 @@ + From 0a71e2c3b92d2d58fda40f252a6a5a4392fa58b7 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 19:24:18 +1100 Subject: [PATCH 10/19] WW-5352 Implement auto-allowlisting for Iterator component --- apps/showcase/src/main/resources/struts.xml | 1 - .../struts2/components/IteratorComponent.java | 13 ++++++++++-- .../apache/struts2/dispatcher/Dispatcher.java | 3 +-- .../views/jsp/ComponentTagSupport.java | 19 ++++++++---------- .../components/IteratorComponentTest.java | 20 +++++++++++++------ 5 files changed, 34 insertions(+), 22 deletions(-) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index eca6c62712..9f94eec1db 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -38,7 +38,6 @@ diff --git a/core/src/main/java/org/apache/struts2/components/IteratorComponent.java b/core/src/main/java/org/apache/struts2/components/IteratorComponent.java index 2e8ee4ae6a..7ff83bd458 100644 --- a/core/src/main/java/org/apache/struts2/components/IteratorComponent.java +++ b/core/src/main/java/org/apache/struts2/components/IteratorComponent.java @@ -18,9 +18,11 @@ */ package org.apache.struts2.components; +import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.ValueStack; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.ognl.ThreadAllowlist; import org.apache.struts2.util.MakeIterator; import org.apache.struts2.views.annotations.StrutsTag; import org.apache.struts2.views.annotations.StrutsTagAttribute; @@ -188,8 +190,8 @@ * * *

Another way to create a simple loop, similar to JSTL's - * <c:forEach begin="..." end="..." ...> is to use some - * OGNL magic, which provides some under-the-covers magic to + * <c:forEach begin="..." end="..." ...> is to use some + * OGNL magic, which provides some under-the-covers magic to * make 0-n loops trivial. This example also loops five times.

* * @@ -237,11 +239,17 @@ public class IteratorComponent extends ContextBean { protected Integer end; protected String stepStr; protected Integer step; + private ThreadAllowlist threadAllowlist; public IteratorComponent(ValueStack stack) { super(stack); } + @Inject + public void setThreadAllowlist(ThreadAllowlist threadAllowlist) { + this.threadAllowlist = threadAllowlist; + } + public boolean start(Writer writer) { //Create an iterator status if the status attribute was set. if (statusAttr != null) { @@ -298,6 +306,7 @@ public boolean start(Writer writer) { if ((iterator != null) && iterator.hasNext()) { Object currentValue = iterator.next(); stack.push(currentValue); + threadAllowlist.allowClass(currentValue.getClass()); String var = getVar(); diff --git a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java index 4a8f7215c5..70b85e1b79 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java @@ -719,8 +719,6 @@ public void serviceAction(HttpServletRequest request, HttpServletResponse respon } else { throw new ServletException(e); } - } finally { - threadAllowlist.clearAllowlist(); } } @@ -1051,6 +1049,7 @@ protected MultiPartRequest getMultiPartRequest() { */ public void cleanUpRequest(HttpServletRequest request) { ContainerHolder.clear(); + threadAllowlist.clearAllowlist(); if (!(request instanceof MultiPartRequestWrapper)) { return; } diff --git a/core/src/main/java/org/apache/struts2/views/jsp/ComponentTagSupport.java b/core/src/main/java/org/apache/struts2/views/jsp/ComponentTagSupport.java index 73bba2ea6a..c2208982c6 100644 --- a/core/src/main/java/org/apache/struts2/views/jsp/ComponentTagSupport.java +++ b/core/src/main/java/org/apache/struts2/views/jsp/ComponentTagSupport.java @@ -18,16 +18,14 @@ */ package org.apache.struts2.views.jsp; +import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.util.ValueStack; +import org.apache.struts2.components.Component; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.jsp.JspException; -import com.opensymphony.xwork2.ActionContext; -import org.apache.struts2.components.Component; - -import com.opensymphony.xwork2.inject.Container; -import com.opensymphony.xwork2.util.ValueStack; - /** */ public abstract class ComponentTagSupport extends StrutsBodyTagSupport { @@ -39,8 +37,7 @@ public abstract class ComponentTagSupport extends StrutsBodyTagSupport { public int doEndTag() throws JspException { component.end(pageContext.getOut(), getBody()); component = null; // Always clear component reference (since clearTagStateForTagPoolingServers() is conditional). - clearTagStateForTagPoolingServers(); - return EVAL_PAGE; + return super.doEndTag(); } @Override @@ -49,7 +46,7 @@ public int doStartTag() throws JspException { component = getBean(stack, (HttpServletRequest) pageContext.getRequest(), (HttpServletResponse) pageContext.getResponse()); Container container = stack.getActionContext().getContainer(); container.inject(component); - + populateParams(); boolean evalBody = component.start(pageContext.getOut()); @@ -62,7 +59,7 @@ public int doStartTag() throws JspException { /** * Define method to populate component state based on the Tag parameters. - * + *

* Descendants should override this method for custom behaviour, but should always call the ancestor method when doing so. */ protected void populateParams() { @@ -71,7 +68,7 @@ protected void populateParams() { /** * Specialized method to populate the performClearTagStateForTagPoolingServers state of the Component to match the value set in the Tag. - * + *

* Generally only unit tests would call this method directly, to avoid calling the whole populateParams() chain again after doStartTag() * has been called. Doing that can break tag / component state behaviour, but unit tests still need a way to set the * performClearTagStateForTagPoolingServers state for the component (which only comes into being after doStartTag() is called). diff --git a/core/src/test/java/org/apache/struts2/components/IteratorComponentTest.java b/core/src/test/java/org/apache/struts2/components/IteratorComponentTest.java index d6cc5305de..065a42ae92 100644 --- a/core/src/test/java/org/apache/struts2/components/IteratorComponentTest.java +++ b/core/src/test/java/org/apache/struts2/components/IteratorComponentTest.java @@ -21,6 +21,7 @@ import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.util.ValueStack; import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.ognl.ThreadAllowlist; import java.io.StringWriter; import java.util.Arrays; @@ -28,14 +29,25 @@ public class IteratorComponentTest extends StrutsInternalTestCase { + private ValueStack stack; + private IteratorComponent ic; + private ThreadAllowlist threadAllowlist; + + @Override + public void setUp() throws Exception { + super.setUp(); + stack = ActionContext.getContext().getValueStack(); + ic = new IteratorComponent(stack); + threadAllowlist = new ThreadAllowlist(); + ic.setThreadAllowlist(threadAllowlist); + } + public void testIterator() throws Exception { // given - final ValueStack stack = ActionContext.getContext().getValueStack(); stack.push(new FooAction()); StringWriter out = new StringWriter(); - IteratorComponent ic = new IteratorComponent(stack); ic.setValue("items"); ic.setVar("val"); @@ -64,12 +76,10 @@ public void testIterator() throws Exception { public void testIteratorWithBegin() throws Exception { // given - final ValueStack stack = ActionContext.getContext().getValueStack(); stack.push(new FooAction()); StringWriter out = new StringWriter(); - IteratorComponent ic = new IteratorComponent(stack); ic.setValue("items"); ic.setVar("val"); ic.setBegin("1"); @@ -96,7 +106,6 @@ public void testIteratorWithBegin() throws Exception { public void testIteratorWithNulls() throws Exception { // given - final ValueStack stack = ActionContext.getContext().getValueStack(); stack.push(new FooAction() { private List items = Arrays.asList("1", "2", null, "4"); @@ -107,7 +116,6 @@ public List getItems() { StringWriter out = new StringWriter(); - IteratorComponent ic = new IteratorComponent(stack); ic.setValue("items"); ic.setVar("val"); Property prop = new Property(stack); From 770d311105840d69673c9c279bb9361385f6839b Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 20:14:14 +1100 Subject: [PATCH 11/19] WW-5352 Mild optimisation --- .../xwork2/security/DefaultAcceptedPatternsChecker.java | 2 ++ .../struts2/interceptor/parameter/ParametersInterceptor.java | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java index 4d2caa594f..be803b7e84 100644 --- a/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java +++ b/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java @@ -31,6 +31,7 @@ import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableSet; +import static java.util.stream.Collectors.joining; public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { @@ -44,6 +45,7 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { * Must match {@link #ACCEPTED_PATTERNS} RegEx. Signifies characters which result in a nested lookup via OGNL. */ public static final Set NESTING_CHARS = unmodifiableSet(new HashSet<>(asList('.', '[', '('))); + public static final String NESTING_CHARS_STR = NESTING_CHARS.stream().map(String::valueOf).collect(joining()); public static final String[] DMI_AWARE_ACCEPTED_PATTERNS = { "\\w+([:]?\\w+)?((\\.\\w+)|(\\[\\d+])|(\\(\\d+\\))|(\\['(\\w-?|[\\u4e00-\\u9fa5]-?)+'])|(\\('(\\w-?|[\\u4e00-\\u9fa5]-?)+'\\)))*([!]?\\w+)?" diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 1443fddebb..71c84d1952 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -63,6 +63,7 @@ import java.util.regex.Pattern; import static com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS; +import static com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS_STR; import static java.util.Collections.unmodifiableSet; import static java.util.stream.Collectors.joining; import static org.apache.commons.lang3.StringUtils.indexOfAny; @@ -340,7 +341,7 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { return true; } - int nestingIndex = indexOfAny(name, NESTING_CHARS.stream().map(String::valueOf).collect(joining())); + int nestingIndex = indexOfAny(name, NESTING_CHARS_STR); String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); From 6df80041e32199b5b98099b480af2519e89ff85c Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 20:54:15 +1100 Subject: [PATCH 12/19] WW-5352 Auto allowlist parameterized types! --- apps/showcase/src/main/resources/struts.xml | 5 -- .../parameter/ParametersInterceptor.java | 75 ++++++++++--------- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index 9f94eec1db..373cf5f7b2 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -36,11 +36,6 @@ - diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 71c84d1952..2a500d6681 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -52,6 +52,8 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; import java.util.Arrays; import java.util.Collection; import java.util.Comparator; @@ -373,61 +375,62 @@ protected boolean hasValidAnnotatedMember(String rootProperty, Object action, lo } protected boolean hasValidAnnotatedPropertyDescriptor(PropertyDescriptor propDesc, long paramDepth) { - Class rootType = getValidAnnotatedPropertyDescriptorType(propDesc, paramDepth); - if (rootType != null) { - if (paramDepth > 0) { - threadAllowlist.allowClass(rootType); - } - return true; - } - return false; - } - - /** - * @return getter return type or setter parameter type, if one corresponding to the paramDepth exists - * with a valid annotation - */ - protected Class getValidAnnotatedPropertyDescriptorType(PropertyDescriptor propDesc, long paramDepth) { Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); if (relevantMethod == null) { - return null; + return false; } StrutsParameter annotation = getParameterAnnotation(relevantMethod); - if (annotation != null && annotation.depth() >= paramDepth) { - return paramDepth == 0 ? relevantMethod.getParameterTypes()[0] : relevantMethod.getReturnType(); + if (annotation == null || annotation.depth() < paramDepth) { + return false; + } + if (paramDepth >= 1) { + threadAllowlist.allowClass(relevantMethod.getReturnType()); } - return null; + if (paramDepth >= 2) { + allowlistReturnTypeIfParameterized(relevantMethod); + } + return true; } - protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) { - Class rootType = getValidAnnotatedFieldType(action, fieldName, paramDepth); - if (rootType != null) { - if (paramDepth > 0) { - threadAllowlist.allowClass(rootType); - } - return true; + protected void allowlistReturnTypeIfParameterized(Method method) { + allowlistParameterizedTypeArg(method.getGenericReturnType()); + } + + protected void allowlistParameterizedTypeArg(Type genericType) { + if (!(genericType instanceof ParameterizedType)) { + return; + } + Type paramType = ((ParameterizedType) genericType).getActualTypeArguments()[0]; + if (paramType instanceof Class) { + threadAllowlist.allowClass((Class) paramType); } - return false; } - /** - * @return field type if a public field exists on the action with a valid annotation - */ - protected Class getValidAnnotatedFieldType(Object action, String fieldName, long paramDepth) { + protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) { Field field; try { field = action.getClass().getDeclaredField(fieldName); } catch (NoSuchFieldException e) { - return null; + return false; } if (!Modifier.isPublic(field.getModifiers())) { - return null; + return false; } StrutsParameter annotation = getParameterAnnotation(field); - if (annotation != null && annotation.depth() >= paramDepth) { - return field.getType(); + if (annotation == null || annotation.depth() < paramDepth) { + return false; + } + if (paramDepth >= 1) { + threadAllowlist.allowClass(field.getType()); } - return null; + if (paramDepth >= 2) { + allowlistFieldIfParameterized(field); + } + return true; + } + + protected void allowlistFieldIfParameterized(Field field) { + allowlistParameterizedTypeArg(field.getGenericType()); } /** From f106b20983caf2668ff6adf8fb1df553df67f28a Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 21:23:18 +1100 Subject: [PATCH 13/19] WW-5352 Map-like type support --- .../interceptor/parameter/ParametersInterceptor.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 2a500d6681..ca67fc2755 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -400,7 +400,15 @@ protected void allowlistParameterizedTypeArg(Type genericType) { if (!(genericType instanceof ParameterizedType)) { return; } - Type paramType = ((ParameterizedType) genericType).getActualTypeArguments()[0]; + Type[] paramTypes = ((ParameterizedType) genericType).getActualTypeArguments(); + allowlistParamType(paramTypes[0]); + if (paramTypes.length > 1) { + // Probably useful for Map or Map-like classes + allowlistParamType(paramTypes[1]); + } + } + + protected void allowlistParamType(Type paramType) { if (paramType instanceof Class) { threadAllowlist.allowClass((Class) paramType); } From bf7737fa07d262eb5025cec6e59836928e1a06b2 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 22:30:40 +1100 Subject: [PATCH 14/19] WW-5352 Add unit test coverage for generics --- .../StrutsParameterAnnotationTest.java | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 839456e435..8c30ec857f 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -29,6 +29,7 @@ import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; @@ -199,6 +200,41 @@ public void publicNestedPojoDepthTwoMethod() { assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); } + @Test + public void publicPojoListDepthOne() { + testParameter(new FieldAction(), "publicPojoListDepthOne[0].key", false); + } + + @Test + public void publicPojoListDepthTwo() { + testParameter(new FieldAction(), "publicPojoListDepthTwo[0].key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(List.class, Pojo.class); + } + + @Test + public void publicPojoMapDepthTwo() { + testParameter(new FieldAction(), "publicPojoMapDepthTwo['a'].key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(Map.class, String.class, Pojo.class); + } + + @Test + public void publicPojoListDepthOneMethod() { + testParameter(new MethodAction(), "publicPojoListDepthOne[0].key", false); + } + + @Test + public void publicPojoListDepthTwoMethod() { + testParameter(new MethodAction(), "publicPojoListDepthTwo[0].key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(List.class, Pojo.class); + } + + @Test + public void publicPojoMapDepthTwoMethod() { + testParameter(new MethodAction(), "publicPojoMapDepthTwo['a'].key", true); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(Map.class, String.class, Pojo.class); + } + + class FieldAction { @StrutsParameter private String privateStr; @@ -219,6 +255,15 @@ class FieldAction { @StrutsParameter(depth = 2) public Pojo publicPojoDepthTwo; + + @StrutsParameter(depth = 1) + public List publicPojoListDepthOne; + + @StrutsParameter(depth = 2) + public List publicPojoListDepthTwo; + + @StrutsParameter(depth = 2) + public Map publicPojoMapDepthTwo; } class MethodAction { @@ -257,6 +302,21 @@ public Pojo getPublicPojoDepthOne() { public Pojo getPublicPojoDepthTwo() { return null; } + + @StrutsParameter(depth = 1) + public List getPublicPojoListDepthOne() { + return null; + } + + @StrutsParameter(depth = 2) + public List getPublicPojoListDepthTwo() { + return null; + } + + @StrutsParameter(depth = 2) + public Map getPublicPojoMapDepthTwo() { + return null; + } } class Pojo { From 56d8361b415696cee6eb6f8cc6b373b6d3e41e62 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 9 Jan 2024 22:49:56 +1100 Subject: [PATCH 15/19] WW-5352 Implement transition mode --- .../org/apache/struts2/StrutsConstants.java | 1 + .../parameter/ParametersInterceptor.java | 21 ++++++++++++++++++- .../StrutsParameterAnnotationTest.java | 12 +++++++++++ 3 files changed, 33 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 64d539280b..3d0d1a00dc 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -470,6 +470,7 @@ public final class StrutsConstants { public static final String STRUTS_ADDITIONAL_ACCEPTED_PATTERNS = "struts.additional.acceptedPatterns"; public static final String STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS = "struts.parameters.requireAnnotations"; + public static final String STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS_TRANSITION = "struts.parameters.requireAnnotations.transitionMode"; public static final String STRUTS_CONTENT_TYPE_MATCHER = "struts.contentTypeMatcher"; diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index ca67fc2755..53180df6e1 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -88,6 +88,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { protected boolean ordered = false; protected boolean requireAnnotations = false; + protected boolean requireAnnotationsTransitionMode = false; private ValueStackFactory valueStackFactory; protected ThreadAllowlist threadAllowlist; @@ -116,6 +117,20 @@ public void setRequireAnnotations(String requireAnnotations) { this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations); } + /** + * When 'Transition Mode' is enabled, parameters that are not 'nested' will be accepted without annotations. What + * this means in practice is that all public setters on an Action will be exposed for parameter injection again, and + * only 'nested' parameters, i.e. public getters on an Action, will require annotations. + *

+ * In this mode, the OGNL auto-allowlisting capability is not degraded in any way, and as such, it offers a + * convenient option for applications to enable the OGNL allowlist capability whilst they work through the process + * of annotating all their Action parameters. + */ + @Inject(value = StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS_TRANSITION, required = false) + public void setRequireAnnotationsTransitionMode(String transitionMode) { + this.requireAnnotationsTransitionMode = BooleanUtils.toBoolean(transitionMode); + } + @Inject public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { this.excludedPatterns = excludedPatterns; @@ -343,9 +358,13 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { return true; } + long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + if (requireAnnotationsTransitionMode && paramDepth == 0) { + return true; + } + int nestingIndex = indexOfAny(name, NESTING_CHARS_STR); String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); - long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); return hasValidAnnotatedMember(rootProperty, action, paramDepth); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 8c30ec857f..1a29099488 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -234,6 +234,18 @@ public void publicPojoMapDepthTwoMethod() { assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(Map.class, String.class, Pojo.class); } + @Test + public void publicStrNotAnnotated_transitionMode() { + parametersInterceptor.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); + testParameter(new FieldAction(), "publicStrNotAnnotated", true); + } + + @Test + public void publicStrNotAnnotatedMethod_transitionMode() { + parametersInterceptor.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); + testParameter(new MethodAction(), "publicStrNotAnnotated", true); + } + class FieldAction { @StrutsParameter From 49b9c0c78cca0dd7b990dc7e210ac9f8980d1a94 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sun, 14 Jan 2024 16:15:46 +1100 Subject: [PATCH 16/19] WW-5352 Ensure superclasses and interfaces allowlisted --- .../parameter/ParametersInterceptor.java | 13 ++++-- .../StrutsParameterAnnotationTest.java | 40 ++++++++++++------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 53180df6e1..c91aea87af 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -34,6 +34,7 @@ import com.opensymphony.xwork2.util.ValueStackFactory; import com.opensymphony.xwork2.util.reflection.ReflectionContextState; import org.apache.commons.lang3.BooleanUtils; +import org.apache.commons.lang3.ClassUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; @@ -403,7 +404,7 @@ protected boolean hasValidAnnotatedPropertyDescriptor(PropertyDescriptor propDes return false; } if (paramDepth >= 1) { - threadAllowlist.allowClass(relevantMethod.getReturnType()); + allowlistClass(relevantMethod.getReturnType()); } if (paramDepth >= 2) { allowlistReturnTypeIfParameterized(relevantMethod); @@ -429,10 +430,16 @@ protected void allowlistParameterizedTypeArg(Type genericType) { protected void allowlistParamType(Type paramType) { if (paramType instanceof Class) { - threadAllowlist.allowClass((Class) paramType); + allowlistClass((Class) paramType); } } + protected void allowlistClass(Class clazz) { + threadAllowlist.allowClass(clazz); + ClassUtils.getAllSuperclasses(clazz).forEach(threadAllowlist::allowClass); + ClassUtils.getAllInterfaces(clazz).forEach(threadAllowlist::allowClass); + } + protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) { Field field; try { @@ -448,7 +455,7 @@ protected boolean hasValidAnnotatedField(Object action, String fieldName, long p return false; } if (paramDepth >= 1) { - threadAllowlist.allowClass(field.getType()); + allowlistClass(field.getType()); } if (paramDepth >= 2) { allowlistFieldIfParameterized(field); diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 1a29099488..53fa147170 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -20,6 +20,7 @@ import com.opensymphony.xwork2.security.AcceptedPatternsChecker; import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; +import org.apache.commons.lang3.ClassUtils; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.ognl.ThreadAllowlist; @@ -31,6 +32,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyString; @@ -78,6 +80,16 @@ private void testParameter(Object action, String paramName, boolean shouldContai } } + private Set> getParentClasses(Class ...clazzes) { + Set> set = new HashSet<>(); + for (Class clazz : clazzes) { + set.add(clazz); + set.addAll(ClassUtils.getAllSuperclasses(clazz)); + set.addAll(ClassUtils.getAllInterfaces(clazz)); + } + return set; + } + @Test public void privateStrAnnotated() { testParameter(new FieldAction(), "privateStr", false); @@ -107,19 +119,19 @@ public void publicPojoDepthZero() { @Test public void publicPojoDepthOne() { testParameter(new FieldAction(), "publicPojoDepthOne.key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test public void publicPojoDepthOne_sqrBracket() { testParameter(new FieldAction(), "publicPojoDepthOne['key']", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test public void publicPojoDepthOne_bracket() { testParameter(new FieldAction(), "publicPojoDepthOne('key')", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test @@ -130,25 +142,25 @@ public void publicNestedPojoDepthOne() { @Test public void publicPojoDepthTwo() { testParameter(new FieldAction(), "publicPojoDepthTwo.key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test public void publicNestedPojoDepthTwo() { testParameter(new FieldAction(), "publicPojoDepthTwo.key.key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test public void publicNestedPojoDepthTwo_sqrBracket() { testParameter(new FieldAction(), "publicPojoDepthTwo['key']['key']", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test public void publicNestedPojoDepthTwo_bracket() { testParameter(new FieldAction(), "publicPojoDepthTwo('key')('key')", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test @@ -180,7 +192,7 @@ public void publicPojoDepthZeroMethod() { @Test public void publicPojoDepthOneMethod() { testParameter(new MethodAction(), "publicPojoDepthOne.key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test @@ -191,13 +203,13 @@ public void publicNestedPojoDepthOneMethod() { @Test public void publicPojoDepthTwoMethod() { testParameter(new MethodAction(), "publicPojoDepthTwo.key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test public void publicNestedPojoDepthTwoMethod() { testParameter(new MethodAction(), "publicPojoDepthTwo.key.key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactly(Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class)); } @Test @@ -208,13 +220,13 @@ public void publicPojoListDepthOne() { @Test public void publicPojoListDepthTwo() { testParameter(new FieldAction(), "publicPojoListDepthTwo[0].key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(List.class, Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(List.class, Pojo.class)); } @Test public void publicPojoMapDepthTwo() { testParameter(new FieldAction(), "publicPojoMapDepthTwo['a'].key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(Map.class, String.class, Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Map.class, String.class, Pojo.class)); } @Test @@ -225,13 +237,13 @@ public void publicPojoListDepthOneMethod() { @Test public void publicPojoListDepthTwoMethod() { testParameter(new MethodAction(), "publicPojoListDepthTwo[0].key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(List.class, Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(List.class, Pojo.class)); } @Test public void publicPojoMapDepthTwoMethod() { testParameter(new MethodAction(), "publicPojoMapDepthTwo['a'].key", true); - assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrder(Map.class, String.class, Pojo.class); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Map.class, String.class, Pojo.class)); } @Test From 728d695ce14174b0e4cdb116ba3d3f9260fae2cb Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 17 Jan 2024 19:26:03 +1100 Subject: [PATCH 17/19] WW-5352 Add debug logging for parameter rejections --- .../parameter/ParametersInterceptor.java | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index c91aea87af..9c7013b7c8 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -399,8 +399,11 @@ protected boolean hasValidAnnotatedPropertyDescriptor(PropertyDescriptor propDes if (relevantMethod == null) { return false; } - StrutsParameter annotation = getParameterAnnotation(relevantMethod); - if (annotation == null || annotation.depth() < paramDepth) { + if (getPermittedInjectionDepth(relevantMethod) < paramDepth) { + LOG.debug( + "Parameter injection for method [{}] on action [{}] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + relevantMethod.getName(), + relevantMethod.getDeclaringClass().getName()); return false; } if (paramDepth >= 1) { @@ -450,8 +453,11 @@ protected boolean hasValidAnnotatedField(Object action, String fieldName, long p if (!Modifier.isPublic(field.getModifiers())) { return false; } - StrutsParameter annotation = getParameterAnnotation(field); - if (annotation == null || annotation.depth() < paramDepth) { + if (getPermittedInjectionDepth(field) < paramDepth) { + LOG.debug( + "Parameter injection for field [{}] on action [{}] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + fieldName, + action.getClass().getName()); return false; } if (paramDepth >= 1) { @@ -467,6 +473,17 @@ protected void allowlistFieldIfParameterized(Field field) { allowlistParameterizedTypeArg(field.getGenericType()); } + /** + * @return permitted injection depth where -1 indicates not permitted + */ + protected int getPermittedInjectionDepth(AnnotatedElement element) { + StrutsParameter annotation = getParameterAnnotation(element); + if (annotation == null) { + return -1; + } + return annotation.depth(); + } + /** * Annotation retrieval logic. Can be overridden to support extending annotations or some other form of annotation * inheritance. From b50616942b81de3b0d1c02b77787134b6fccee2e Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 18 Jan 2024 19:54:54 +1100 Subject: [PATCH 18/19] WW-5352 Acceptance test coverage --- .../action/ParamsAnnotationAction.java | 133 ++++++++++ .../apache/struts2/showcase/model/MyDto.java | 38 +++ .../resources/struts-params-annotation.xml | 32 +++ apps/showcase/src/main/resources/struts.xml | 2 + .../main/webapp/WEB-INF/paramsannotation.vm | 19 ++ .../showcase/StrutsParametersTest.java | 239 ++++++++++++++++++ 6 files changed, 463 insertions(+) create mode 100644 apps/showcase/src/main/java/org/apache/struts2/showcase/action/ParamsAnnotationAction.java create mode 100644 apps/showcase/src/main/java/org/apache/struts2/showcase/model/MyDto.java create mode 100644 apps/showcase/src/main/resources/struts-params-annotation.xml create mode 100644 apps/showcase/src/main/webapp/WEB-INF/paramsannotation.vm create mode 100644 apps/showcase/src/test/java/it/org/apache/struts2/showcase/StrutsParametersTest.java diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/action/ParamsAnnotationAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/action/ParamsAnnotationAction.java new file mode 100644 index 0000000000..0c3ff4f7c4 --- /dev/null +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/action/ParamsAnnotationAction.java @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.showcase.action; + +import com.opensymphony.xwork2.ActionSupport; +import org.apache.struts2.interceptor.parameter.StrutsParameter; +import org.apache.struts2.showcase.model.MyDto; + +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; + +/** + * This class supports {@link com.atlassian.confluence.stateless.webdriver.selenium3.security.StrutsParametersTest} + * which prevents critical security regressions. Do NOT modify without understanding the motivation behind the tests and + * the implications of any changes. + */ +public class ParamsAnnotationAction extends ActionSupport { + + @StrutsParameter + public String varToPrint; + + public String publicField = "no"; + + @StrutsParameter + public String publicFieldAnnotated = "no"; + + private String privateField = "no"; + + public int[] publicArray = new int[]{0}; + + @StrutsParameter(depth = 1) + public int[] publicArrayAnnotated = new int[]{0}; + + public List publicList = new ArrayList<>(singletonList("no")); + + @StrutsParameter(depth = 1) + public List publicListAnnotated = new ArrayList<>(singletonList("no")); + + private List privateList = new ArrayList<>(singletonList("no")); + + public Map publicMap = new HashMap<>(singletonMap("key", "no")); + + @StrutsParameter(depth = 1) + public Map publicMapAnnotated = new HashMap<>(singletonMap("key", "no")); + + public MyDto publicMyDto = new MyDto(); + + @StrutsParameter(depth = 2) + public MyDto publicMyDtoAnnotated = new MyDto(); + + @StrutsParameter(depth = 1) + public MyDto publicMyDtoAnnotatedDepthOne = new MyDto(); + + private MyDto privateMyDto = new MyDto(); + + public void setPrivateFieldMethod(String privateField) { + this.privateField = privateField; + } + + @StrutsParameter + public void setPrivateFieldMethodAnnotated(String privateField) { + this.privateField = privateField; + } + + public List getPrivateListMethod() { + return privateList; + } + + @StrutsParameter(depth = 1) + public List getPrivateListMethodAnnotated() { + return privateList; + } + + public MyDto getUnsafeMethodMyDto() { + return privateMyDto; + } + + @StrutsParameter(depth = 2) + public MyDto getSafeMethodMyDto() { + return privateMyDto; + } + + @StrutsParameter(depth = 1) + public MyDto getSafeMethodMyDtoDepthOne() { + return privateMyDto; + } + + public String renderVarToPrint() throws ReflectiveOperationException { + if (varToPrint == null) { + return "null"; + } + Field field = this.getClass().getDeclaredField(varToPrint); + field.setAccessible(true); + try { + return String.format("%s{%s}", varToPrint, + field.getType().isArray() ? stringifyArray(field.get(this)) : field.get(this)); + } finally { + field.setAccessible(false); + } + } + + private String stringifyArray(Object array) { + switch (array.getClass().getComponentType().getName()) { + case "int": + return Arrays.toString((int[]) array); + default: + return "TODO"; + } + } +} diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/model/MyDto.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/model/MyDto.java new file mode 100644 index 0000000000..7abee847e7 --- /dev/null +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/model/MyDto.java @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.showcase.model; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + +import static java.util.Collections.singletonMap; + +public class MyDto { + + public String str = "no"; + + public Map map = new HashMap<>(singletonMap("key", "no")); + public int[] array = new int[]{0}; + + @Override + public String toString() { + return "str=" + str + ", map=" + map + ", array=" + Arrays.toString(array); + } +} diff --git a/apps/showcase/src/main/resources/struts-params-annotation.xml b/apps/showcase/src/main/resources/struts-params-annotation.xml new file mode 100644 index 0000000000..db39928844 --- /dev/null +++ b/apps/showcase/src/main/resources/struts-params-annotation.xml @@ -0,0 +1,32 @@ + + + + + + + + /WEB-INF/paramsannotation.vm + + + diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index 373cf5f7b2..33095326c2 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -83,6 +83,8 @@ + + diff --git a/apps/showcase/src/main/webapp/WEB-INF/paramsannotation.vm b/apps/showcase/src/main/webapp/WEB-INF/paramsannotation.vm new file mode 100644 index 0000000000..a0c4efefc4 --- /dev/null +++ b/apps/showcase/src/main/webapp/WEB-INF/paramsannotation.vm @@ -0,0 +1,19 @@ +#* +* Licensed to the Apache Software Foundation (ASF) under one +* or more contributor license agreements. See the NOTICE file +* distributed with this work for additional information +* regarding copyright ownership. The ASF licenses this file +* to you under the Apache License, Version 2.0 (the +* "License"); you may not use this file except in compliance +* with the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, +* software distributed under the License is distributed on an +* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +* KIND, either express or implied. See the License for the +* specific language governing permissions and limitations +* under the License. +*# +

$action.renderVarToPrint()
diff --git a/apps/showcase/src/test/java/it/org/apache/struts2/showcase/StrutsParametersTest.java b/apps/showcase/src/test/java/it/org/apache/struts2/showcase/StrutsParametersTest.java new file mode 100644 index 0000000000..4179095448 --- /dev/null +++ b/apps/showcase/src/test/java/it/org/apache/struts2/showcase/StrutsParametersTest.java @@ -0,0 +1,239 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package it.org.apache.struts2.showcase; + +import com.gargoylesoftware.htmlunit.WebClient; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.web.util.UriComponentsBuilder; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; + +public class StrutsParametersTest { + + private WebClient webClient; + + @Before + public void setUp() throws Exception { + webClient = new WebClient(); + } + + @After + public void tearDown() throws Exception { + webClient.close(); + } + + @Test + public void public_StringField_WithoutGetterSetter_FieldNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicField", "yes"); + params.put("varToPrint", "publicField"); + assertText(params, "publicField{no}"); + } + + @Test + public void public_StringField_WithoutGetterSetter_FieldAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicFieldAnnotated", "yes"); + params.put("varToPrint", "publicFieldAnnotated"); + assertText(params, "publicFieldAnnotated{yes}"); + } + + @Test + public void private_StringField_WithSetter_MethodNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("privateFieldMethod", "yes"); + params.put("varToPrint", "privateField"); + assertText(params, "privateField{no}"); + } + + @Test + public void private_StringField_WithSetter_MethodAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("privateFieldMethodAnnotated", "yes"); + params.put("varToPrint", "privateField"); + assertText(params, "privateField{yes}"); + } + + @Test + public void public_ArrayField_WithoutGetterSetter_FieldNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicArray[0]", "1"); + params.put("varToPrint", "publicArray"); + assertText(params, "publicArray{[0]}"); + } + + @Test + public void public_ArrayField_WithoutGetterSetter_FieldAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicArrayAnnotated[0]", "1"); + params.put("varToPrint", "publicArrayAnnotated"); + assertText(params, "publicArrayAnnotated{[1]}"); + } + + @Test + public void public_ListField_WithoutGetterSetter_FieldNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicList[0]", "yes"); + params.put("varToPrint", "publicList"); + assertText(params, "publicList{[no]}"); + } + + @Test + public void public_ListField_WithoutGetterSetter_FieldAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicListAnnotated[0]", "yes"); + params.put("varToPrint", "publicListAnnotated"); + assertText(params, "publicListAnnotated{[yes]}"); + } + + @Test + public void private_ListField_WithGetterNoSetter_MethodNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("privateListMethod[0]", "yes"); + params.put("varToPrint", "privateList"); + assertText(params, "privateList{[no]}"); + } + + @Test + public void private_ListField_WithGetterNoSetter_MethodAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("privateListMethodAnnotated[0]", "yes"); + params.put("varToPrint", "privateList"); + assertText(params, "privateList{[yes]}"); + } + + @Test + public void public_MapField_WithoutGetterSetter_FieldNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicMap['key']", "yes"); + params.put("varToPrint", "publicMap"); + assertText(params, "publicMap{{key=no}}"); + } + + @Test + public void public_MapField_WithoutGetterSetter_FieldAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicMapAnnotated['key']", "yes"); + params.put("varToPrint", "publicMapAnnotated"); + assertText(params, "publicMapAnnotated{{key=yes}}"); + } + + @Test + public void public_MapField_Insert_WithoutGetterSetter_FieldNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicMap[999]", "yes"); + params.put("varToPrint", "publicMap"); + assertText(params, "publicMap{{key=no}}"); + } + + @Test + public void public_MapField_Insert_WithoutGetterSetter_FieldAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicMapAnnotated[999]", "yes"); + params.put("varToPrint", "publicMapAnnotated"); + assertText(params, "publicMapAnnotated{{999=yes, key=no}}"); + } + + @Test + public void public_MyDtoField_WithoutGetter_FieldNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicMyDto.str", "yes"); + params.put("publicMyDto.map['key']", "yes"); + params.put("publicMyDto.array[0]", "1"); + params.put("varToPrint", "publicMyDto"); + assertText(params, "publicMyDto{str=no, map={key=no}, array=[0]}"); + } + + @Test + public void public_MyDtoField_WithoutGetter_FieldAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("publicMyDtoAnnotated.str", "yes"); + params.put("publicMyDtoAnnotated.map['key']", "yes"); + params.put("publicMyDtoAnnotated.array[0]", "1"); + params.put("varToPrint", "publicMyDtoAnnotated"); + assertText(params, "publicMyDtoAnnotated{str=yes, map={key=yes}, array=[1]}"); + } + + @Test + public void public_MyDtoField_WithoutGetter_FieldAnnotatedDepthOne() throws Exception { + Map params = new HashMap<>(); + params.put("publicMyDtoAnnotatedDepthOne.str", "yes"); + params.put("publicMyDtoAnnotatedDepthOne.map['key']", "yes"); + params.put("publicMyDtoAnnotatedDepthOne.array[0]", "1"); + params.put("varToPrint", "publicMyDtoAnnotatedDepthOne"); + assertText(params, "publicMyDtoAnnotatedDepthOne{str=yes, map={key=no}, array=[0]}"); + } + + @Test + public void private_MyDtoField_WithGetter_MethodNotAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("unsafeMethodMyDto.str", "yes"); + params.put("unsafeMethodMyDto.map['key']", "yes"); + params.put("unsafeMethodMyDto.array[0]", "1"); + params.put("varToPrint", "privateMyDto"); + assertText(params, "privateMyDto{str=no, map={key=no}, array=[0]}"); + } + + @Test + public void private_MyDtoField_WithGetter_MethodNotAnnotated_Alternate() throws Exception { + Map params = new HashMap<>(); + params.put("unsafeMethodMyDto['str']", "yes"); + params.put("unsafeMethodMyDto['map']['key']", "yes"); + params.put("unsafeMethodMyDto['map'][999]", "yes"); + params.put("unsafeMethodMyDto['array'][0]", "1"); + params.put("varToPrint", "privateMyDto"); + assertText(params, "privateMyDto{str=no, map={key=no}, array=[0]}"); + } + + @Test + public void private_MyDtoField_WithGetter_MethodAnnotated() throws Exception { + Map params = new HashMap<>(); + params.put("safeMethodMyDto.str", "yes"); + params.put("safeMethodMyDto.map['key']", "yes"); + params.put("safeMethodMyDto.array[0]", "1"); + params.put("varToPrint", "privateMyDto"); + assertText(params, "privateMyDto{str=yes, map={key=yes}, array=[1]}"); + } + + @Test + public void private_MyDtoField_WithGetter_MethodAnnotatedDepthOne() throws Exception { + Map params = new HashMap<>(); + params.put("safeMethodMyDtoDepthOne.str", "yes"); + params.put("safeMethodMyDtoDepthOne.map['key']", "yes"); + params.put("safeMethodMyDtoDepthOne.array[0]", "1"); + params.put("varToPrint", "privateMyDto"); + assertText(params, "privateMyDto{str=yes, map={key=no}, array=[0]}"); + } + + private void assertText(Map params, String text) throws IOException { + UriComponentsBuilder builder = UriComponentsBuilder.fromHttpUrl(ParameterUtils.getBaseUrl()).path("/paramsannotation/test.action"); + params.forEach(builder::queryParam); + String url = builder.toUriString(); + HtmlPage page = webClient.getPage(url); + String output = page.getElementById("output").asNormalizedText(); + assertEquals(text, output); + } +} From 71d77df3f30fb2898da0ea003934460dc4167ac9 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 18 Jan 2024 21:24:36 +1100 Subject: [PATCH 19/19] WW-5352 Normalise parameter name --- .../struts2/interceptor/parameter/ParametersInterceptor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 9c7013b7c8..e9215e5339 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -366,8 +366,9 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { int nestingIndex = indexOfAny(name, NESTING_CHARS_STR); String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); + String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1); - return hasValidAnnotatedMember(rootProperty, action, paramDepth); + return hasValidAnnotatedMember(normalisedRootProperty, action, paramDepth); } /**