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/28] 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 939b3bddb..64d539280 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 efc4a7b04..b6dc5c87e 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 class ParametersInterceptor extends MethodFilterInterceptor { 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { * @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 000000000..2a19aa83f --- /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/28] 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 3a3674a70..7bf0e7c77 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.conversion.StrutsTypeConverterHolder; 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 class DefaultConfiguration implements Configuration { .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 0cd5bf789..510a65c60 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.LogManager; 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 class SecurityMemberAccess implements MemberAccess { * - 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 @@ public class SecurityMemberAccess implements MemberAccess { 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 b6dc5c87e..7d8cba359 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.ParameterNameAware; 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 class ParametersInterceptor extends MethodFilterInterceptor { 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 000000000..e07cae64e --- /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 93614fa0b..5c3121f77 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 7e5a22bcc..03bad82e4 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 com.opensymphony.xwork2.util.Foo; 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/28] 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 7d8cba359..08c956f21 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.inject.Inject; 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.HttpParameters; 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { } 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { * @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/28] 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 08c956f21..f1651cd86 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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/28] 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 000000000..7b6bbd260 --- /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/28] 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 0896fec82..4d2caa594 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.LogManager; 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 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { newAcceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); } } finally { - acceptedPatterns = Collections.unmodifiableSet(newAcceptedPatterns); + acceptedPatterns = unmodifiableSet(newAcceptedPatterns); } } @@ -85,7 +91,7 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { @Override public void setAcceptedPatterns(String[] additionalPatterns) { - setAcceptedPatterns(new HashSet<>(Arrays.asList(additionalPatterns))); + setAcceptedPatterns(new HashSet<>(asList(additionalPatterns))); } @Override @@ -97,7 +103,7 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { 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 f1651cd86..6833a796b 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.Set; 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { * 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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 7b6bbd260..839456e43 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 class StrutsParameterAnnotationTest { 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 class StrutsParameterAnnotationTest { 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/28] 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 ca5aecbb7..1e87b2193 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.ActionSupport; 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 class UITagExample extends ActionSupport implements Validateable { return leftSideCartoonCharacters; } + @StrutsParameter public void setLeftSideCartoonCharacters(List leftSideCartoonCharacters) { this.leftSideCartoonCharacters = leftSideCartoonCharacters; } @@ -98,6 +105,7 @@ public class UITagExample extends ActionSupport implements Validateable { return rightSideCartoonCharacters; } + @StrutsParameter public void setRightSideCartoonCharacters(List rightSideCartoonCharacters) { this.rightSideCartoonCharacters = rightSideCartoonCharacters; } @@ -107,6 +115,7 @@ public class UITagExample extends ActionSupport implements Validateable { return favouriteVehicalType; } + @StrutsParameter public void setFavouriteVehicalType(String favouriteVehicalType) { this.favouriteVehicalType = favouriteVehicalType; } @@ -115,6 +124,7 @@ public class UITagExample extends ActionSupport implements Validateable { return favouriteVehicalSpecific; } + @StrutsParameter public void setFavouriteVehicalSpecific(String favouriteVehicalSpecific) { this.favouriteVehicalSpecific = favouriteVehicalSpecific; } @@ -145,6 +155,7 @@ public class UITagExample extends ActionSupport implements Validateable { return name; } + @StrutsParameter public void setName(String name) { this.name = name; } @@ -153,6 +164,7 @@ public class UITagExample extends ActionSupport implements Validateable { return birthday; } + @StrutsParameter public void setBirthday(Date birthday) { this.birthday = birthday; } @@ -161,6 +173,7 @@ public class UITagExample extends ActionSupport implements Validateable { return bio; } + @StrutsParameter public void setBio(String bio) { this.bio = bio; } @@ -169,6 +182,7 @@ public class UITagExample extends ActionSupport implements Validateable { return favouriteColor; } + @StrutsParameter public void setFavouriteColor(String favoriteColor) { this.favouriteColor = favoriteColor; } @@ -177,6 +191,7 @@ public class UITagExample extends ActionSupport implements Validateable { return friends; } + @StrutsParameter public void setFriends(List friends) { this.friends = friends; } @@ -193,6 +208,7 @@ public class UITagExample extends ActionSupport implements Validateable { return legalAge; } + @StrutsParameter public void setLegalAge(boolean legalAge) { this.legalAge = legalAge; } @@ -201,6 +217,7 @@ public class UITagExample extends ActionSupport implements Validateable { return state; } + @StrutsParameter public void setState(String state) { this.state = state; } @@ -209,6 +226,7 @@ public class UITagExample extends ActionSupport implements Validateable { return region; } + @StrutsParameter public void setRegion(String region) { this.region = region; } @@ -229,6 +247,7 @@ public class UITagExample extends ActionSupport implements Validateable { this.pictureFileName = pictureFileName; } + @StrutsParameter public void setFavouriteLanguage(String favouriteLanguage) { this.favouriteLanguage = favouriteLanguage; } @@ -237,7 +256,7 @@ public class UITagExample extends ActionSupport implements Validateable { return favouriteLanguage; } - + @StrutsParameter public void setThoughts(String thoughts) { this.thoughts = thoughts; } @@ -250,6 +269,7 @@ public class UITagExample extends ActionSupport implements Validateable { 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 1c96d20dc..6ba209691 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 @@ package org.apache.struts2.showcase.action; 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 @@ public class SkillAction extends AbstractCRUDAction implements Preparable { 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 5877fe6e1..67b27e3b6 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 66e9c2746..0f764b787 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 @@ import java.util.Set; */ 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 class AddressAction extends ActionSupport { 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 a8f8cd624..272c8b6cc 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 class OperationsEnumAction extends ActionSupport { 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 70307db80..27df30a97 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 class PersonAction extends ActionSupport { 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 c9fab7f46..5f23c19d7 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 @@ package org.apache.struts2.showcase.filedownload; 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 class FileDownloadAction implements Action { 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 c2ac471f4..279c1e928 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 @@ package org.apache.struts2.showcase.fileupload; 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 class FileUploadAction extends ActionSupport implements UploadedFilesAwar 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 a9e02deab..9639bacd1 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 class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return dateValidatorField; } + @StrutsParameter public void setDateValidatorField(Date dateValidatorField) { this.dateValidatorField = dateValidatorField; } @@ -52,6 +55,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return emailValidatorField; } + @StrutsParameter public void setEmailValidatorField(String emailValidatorField) { this.emailValidatorField = emailValidatorField; } @@ -60,6 +64,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return integerValidatorField; } + @StrutsParameter public void setIntegerValidatorField(Integer integerValidatorField) { this.integerValidatorField = integerValidatorField; } @@ -68,6 +73,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return regexValidatorField; } + @StrutsParameter public void setRegexValidatorField(String regexValidatorField) { this.regexValidatorField = regexValidatorField; } @@ -76,6 +82,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return requiredStringValidatorField; } + @StrutsParameter public void setRequiredStringValidatorField(String requiredStringValidatorField) { this.requiredStringValidatorField = requiredStringValidatorField; } @@ -84,6 +91,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return requiredValidatorField; } + @StrutsParameter public void setRequiredValidatorField(String requiredValidatorField) { this.requiredValidatorField = requiredValidatorField; } @@ -92,6 +100,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return stringLengthValidatorField; } + @StrutsParameter public void setStringLengthValidatorField(String stringLengthValidatorField) { this.stringLengthValidatorField = stringLengthValidatorField; } @@ -100,6 +109,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor return fieldExpressionValidatorField; } + @StrutsParameter public void setFieldExpressionValidatorField( String fieldExpressionValidatorField) { this.fieldExpressionValidatorField = fieldExpressionValidatorField; @@ -109,6 +119,7 @@ public class FieldValidatorsExampleAction extends AbstractValidationActionSuppor 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 8ff6859bb..cd14a3674 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 class LongProcessAction extends ActionSupport { 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 a9fe3c9dc..373cf5f7b 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/28] 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 3947d89d2..4a8f7215c 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.ActionMapper; 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 class Dispatcher { return actionMapper; } + @Inject + public void setThreadAllowlist(ThreadAllowlist threadAllowlist) { + this.threadAllowlist = threadAllowlist; + } + /** * Releases all instances bound to this dispatcher instance. */ @@ -712,6 +719,8 @@ public class Dispatcher { } 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 6833a796b..1443fddeb 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { } 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/28] 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 373cf5f7b..eca6c6271 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/28] 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 eca6c6271..9f94eec1d 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 2e8ee4ae6..7ff83bd45 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 @@ import java.util.List; * * *

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 class IteratorComponent extends ContextBean { 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 4a8f7215c..70b85e1b7 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 class Dispatcher { } else { throw new ServletException(e); } - } finally { - threadAllowlist.clearAllowlist(); } } @@ -1051,6 +1049,7 @@ public class Dispatcher { */ 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 73bba2ea6..c2208982c 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 abstract class ComponentTagSupport extends StrutsBodyTagSupport { 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 abstract class ComponentTagSupport extends StrutsBodyTagSupport { /** * 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 @@ public abstract class ComponentTagSupport extends StrutsBodyTagSupport { /** * 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 d6cc5305d..065a42ae9 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 @@ package org.apache.struts2.components; 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 @@ import java.util.List; 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 class IteratorComponentTest extends StrutsInternalTestCase { 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 class IteratorComponentTest extends StrutsInternalTestCase { 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 class IteratorComponentTest extends StrutsInternalTestCase { 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/28] 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 4d2caa594..be803b7e8 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 java.util.regex.Pattern; 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 1443fddeb..71c84d195 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.TreeMap; 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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/28] WW-5352 Auto allowlist parameterized types! --- apps/showcase/src/main/resources/struts.xml | 5 -- .../parameter/ParametersInterceptor.java | 79 ++++++++++--------- 2 files changed, 41 insertions(+), 43 deletions(-) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index 9f94eec1d..373cf5f7b 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 71c84d195..2a500d668 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.AnnotatedElement; 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { } 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()); + } + if (paramDepth >= 2) { + allowlistReturnTypeIfParameterized(relevantMethod); + } + 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 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; + 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; } - return null; + if (paramDepth >= 1) { + threadAllowlist.allowClass(field.getType()); + } + 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/28] 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 2a500d668..ca67fc275 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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/28] 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 839456e43..8c30ec857 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 org.junit.Test; 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 class StrutsParameterAnnotationTest { 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 @@ public class StrutsParameterAnnotationTest { @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 class StrutsParameterAnnotationTest { 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/28] 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 64d539280..3d0d1a00d 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 ca67fc275..53180df6e 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 class ParametersInterceptor extends MethodFilterInterceptor { 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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 8c30ec857..1a2909948 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 class StrutsParameterAnnotationTest { 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/28] 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 53180df6e..c91aea87a 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.ValueStack; 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { return false; } if (paramDepth >= 1) { - threadAllowlist.allowClass(relevantMethod.getReturnType()); + allowlistClass(relevantMethod.getReturnType()); } if (paramDepth >= 2) { allowlistReturnTypeIfParameterized(relevantMethod); @@ -429,10 +430,16 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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 1a2909948..53fa14717 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 @@ package org.apache.struts2.interceptor.parameter; 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.HashMap; 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 @@ public class StrutsParameterAnnotationTest { } } + 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 class StrutsParameterAnnotationTest { @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 class StrutsParameterAnnotationTest { @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 class StrutsParameterAnnotationTest { @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 class StrutsParameterAnnotationTest { @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 class StrutsParameterAnnotationTest { @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 class StrutsParameterAnnotationTest { @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/28] 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 c91aea87a..9c7013b7c 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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/28] 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 000000000..0c3ff4f7c --- /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 000000000..7abee847e --- /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 000000000..db3992884 --- /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 373cf5f7b..33095326c 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 000000000..a0c4efefc --- /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 000000000..417909544 --- /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/28] 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 9c7013b7c..e9215e533 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 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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); } /** From 775febbdfaeb31b1ee5f3f7daf6984f0e87640b8 Mon Sep 17 00:00:00 2001 From: Sebastian Peters Date: Sat, 20 Jan 2024 23:25:52 +0100 Subject: [PATCH 20/28] Upgrade maven to 3.9.6 and wrapper to 3.2.0 * `mvn wrapper:wrapper -Dmaven=3.9.6` * remove outdated MavenWrapperDownloader from takari * remove duplicate definition for maven-wrapper-plugin b/c it is already defined under pluginManagement (cherry picked from commit 54a7c7094f69d7cf5a7aa422e2782a8c2f506c9e) --- .mvn/wrapper/MavenWrapperDownloader.java | 117 ------------ .mvn/wrapper/maven-wrapper.properties | 8 +- mvnw | 218 +++++++++++------------ mvnw.cmd | 31 +++- pom.xml | 7 +- 5 files changed, 134 insertions(+), 247 deletions(-) delete mode 100644 .mvn/wrapper/MavenWrapperDownloader.java diff --git a/.mvn/wrapper/MavenWrapperDownloader.java b/.mvn/wrapper/MavenWrapperDownloader.java deleted file mode 100644 index b901097f2..000000000 --- a/.mvn/wrapper/MavenWrapperDownloader.java +++ /dev/null @@ -1,117 +0,0 @@ -/* - * Copyright 2007-present the original author or authors. - * - * Licensed 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. - */ -import java.net.*; -import java.io.*; -import java.nio.channels.*; -import java.util.Properties; - -public class MavenWrapperDownloader { - - private static final String WRAPPER_VERSION = "0.5.6"; - /** - * Default URL to download the maven-wrapper.jar from, if no 'downloadUrl' is provided. - */ - private static final String DEFAULT_DOWNLOAD_URL = "https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/" - + WRAPPER_VERSION + "/maven-wrapper-" + WRAPPER_VERSION + ".jar"; - - /** - * Path to the maven-wrapper.properties file, which might contain a downloadUrl property to - * use instead of the default one. - */ - private static final String MAVEN_WRAPPER_PROPERTIES_PATH = - ".mvn/wrapper/maven-wrapper.properties"; - - /** - * Path where the maven-wrapper.jar will be saved to. - */ - private static final String MAVEN_WRAPPER_JAR_PATH = - ".mvn/wrapper/maven-wrapper.jar"; - - /** - * Name of the property which should be used to override the default download url for the wrapper. - */ - private static final String PROPERTY_NAME_WRAPPER_URL = "wrapperUrl"; - - public static void main(String args[]) { - System.out.println("- Downloader started"); - File baseDirectory = new File(args[0]); - System.out.println("- Using base directory: " + baseDirectory.getAbsolutePath()); - - // If the maven-wrapper.properties exists, read it and check if it contains a custom - // wrapperUrl parameter. - File mavenWrapperPropertyFile = new File(baseDirectory, MAVEN_WRAPPER_PROPERTIES_PATH); - String url = DEFAULT_DOWNLOAD_URL; - if(mavenWrapperPropertyFile.exists()) { - FileInputStream mavenWrapperPropertyFileInputStream = null; - try { - mavenWrapperPropertyFileInputStream = new FileInputStream(mavenWrapperPropertyFile); - Properties mavenWrapperProperties = new Properties(); - mavenWrapperProperties.load(mavenWrapperPropertyFileInputStream); - url = mavenWrapperProperties.getProperty(PROPERTY_NAME_WRAPPER_URL, url); - } catch (IOException e) { - System.out.println("- ERROR loading '" + MAVEN_WRAPPER_PROPERTIES_PATH + "'"); - } finally { - try { - if(mavenWrapperPropertyFileInputStream != null) { - mavenWrapperPropertyFileInputStream.close(); - } - } catch (IOException e) { - // Ignore ... - } - } - } - System.out.println("- Downloading from: " + url); - - File outputFile = new File(baseDirectory.getAbsolutePath(), MAVEN_WRAPPER_JAR_PATH); - if(!outputFile.getParentFile().exists()) { - if(!outputFile.getParentFile().mkdirs()) { - System.out.println( - "- ERROR creating output directory '" + outputFile.getParentFile().getAbsolutePath() + "'"); - } - } - System.out.println("- Downloading to: " + outputFile.getAbsolutePath()); - try { - downloadFileFromURL(url, outputFile); - System.out.println("Done"); - System.exit(0); - } catch (Throwable e) { - System.out.println("- Error downloading"); - e.printStackTrace(); - System.exit(1); - } - } - - private static void downloadFileFromURL(String urlString, File destination) throws Exception { - if (System.getenv("MVNW_USERNAME") != null && System.getenv("MVNW_PASSWORD") != null) { - String username = System.getenv("MVNW_USERNAME"); - char[] password = System.getenv("MVNW_PASSWORD").toCharArray(); - Authenticator.setDefault(new Authenticator() { - @Override - protected PasswordAuthentication getPasswordAuthentication() { - return new PasswordAuthentication(username, password); - } - }); - } - URL website = new URL(urlString); - ReadableByteChannel rbc; - rbc = Channels.newChannel(website.openStream()); - FileOutputStream fos = new FileOutputStream(destination); - fos.getChannel().transferFrom(rbc, 0, Long.MAX_VALUE); - fos.close(); - rbc.close(); - } - -} diff --git a/.mvn/wrapper/maven-wrapper.properties b/.mvn/wrapper/maven-wrapper.properties index 57bb58438..346d645fd 100644 --- a/.mvn/wrapper/maven-wrapper.properties +++ b/.mvn/wrapper/maven-wrapper.properties @@ -5,14 +5,14 @@ # 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. -distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.8.6/apache-maven-3.8.6-bin.zip -wrapperUrl=https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar +distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip +wrapperUrl=https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar diff --git a/mvnw b/mvnw index 5643201c7..8d937f4c1 100755 --- a/mvnw +++ b/mvnw @@ -19,7 +19,7 @@ # ---------------------------------------------------------------------------- # ---------------------------------------------------------------------------- -# Maven Start Up Batch script +# Apache Maven Wrapper startup batch script, version 3.2.0 # # Required ENV vars: # ------------------ @@ -27,7 +27,6 @@ # # Optional ENV vars # ----------------- -# M2_HOME - location of maven2's installed home dir # MAVEN_OPTS - parameters passed to the Java VM when running Maven # e.g. to debug Maven itself, use # set MAVEN_OPTS=-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=8000 @@ -54,7 +53,7 @@ fi cygwin=false; darwin=false; mingw=false -case "`uname`" in +case "$(uname)" in CYGWIN*) cygwin=true ;; MINGW*) mingw=true;; Darwin*) darwin=true @@ -62,9 +61,9 @@ case "`uname`" in # See https://developer.apple.com/library/mac/qa/qa1170/_index.html if [ -z "$JAVA_HOME" ]; then if [ -x "/usr/libexec/java_home" ]; then - export JAVA_HOME="`/usr/libexec/java_home`" + JAVA_HOME="$(/usr/libexec/java_home)"; export JAVA_HOME else - export JAVA_HOME="/Library/Java/Home" + JAVA_HOME="/Library/Java/Home"; export JAVA_HOME fi fi ;; @@ -72,68 +71,38 @@ esac if [ -z "$JAVA_HOME" ] ; then if [ -r /etc/gentoo-release ] ; then - JAVA_HOME=`java-config --jre-home` + JAVA_HOME=$(java-config --jre-home) fi fi -if [ -z "$M2_HOME" ] ; then - ## resolve links - $0 may be a link to maven's home - PRG="$0" - - # need this for relative symlinks - while [ -h "$PRG" ] ; do - ls=`ls -ld "$PRG"` - link=`expr "$ls" : '.*-> \(.*\)$'` - if expr "$link" : '/.*' > /dev/null; then - PRG="$link" - else - PRG="`dirname "$PRG"`/$link" - fi - done - - saveddir=`pwd` - - M2_HOME=`dirname "$PRG"`/.. - - # make it fully qualified - M2_HOME=`cd "$M2_HOME" && pwd` - - cd "$saveddir" - # echo Using m2 at $M2_HOME -fi - # For Cygwin, ensure paths are in UNIX format before anything is touched if $cygwin ; then - [ -n "$M2_HOME" ] && - M2_HOME=`cygpath --unix "$M2_HOME"` [ -n "$JAVA_HOME" ] && - JAVA_HOME=`cygpath --unix "$JAVA_HOME"` + JAVA_HOME=$(cygpath --unix "$JAVA_HOME") [ -n "$CLASSPATH" ] && - CLASSPATH=`cygpath --path --unix "$CLASSPATH"` + CLASSPATH=$(cygpath --path --unix "$CLASSPATH") fi # For Mingw, ensure paths are in UNIX format before anything is touched if $mingw ; then - [ -n "$M2_HOME" ] && - M2_HOME="`(cd "$M2_HOME"; pwd)`" - [ -n "$JAVA_HOME" ] && - JAVA_HOME="`(cd "$JAVA_HOME"; pwd)`" + [ -n "$JAVA_HOME" ] && [ -d "$JAVA_HOME" ] && + JAVA_HOME="$(cd "$JAVA_HOME" || (echo "cannot cd into $JAVA_HOME."; exit 1); pwd)" fi if [ -z "$JAVA_HOME" ]; then - javaExecutable="`which javac`" - if [ -n "$javaExecutable" ] && ! [ "`expr \"$javaExecutable\" : '\([^ ]*\)'`" = "no" ]; then + javaExecutable="$(which javac)" + if [ -n "$javaExecutable" ] && ! [ "$(expr "\"$javaExecutable\"" : '\([^ ]*\)')" = "no" ]; then # readlink(1) is not available as standard on Solaris 10. - readLink=`which readlink` - if [ ! `expr "$readLink" : '\([^ ]*\)'` = "no" ]; then + readLink=$(which readlink) + if [ ! "$(expr "$readLink" : '\([^ ]*\)')" = "no" ]; then if $darwin ; then - javaHome="`dirname \"$javaExecutable\"`" - javaExecutable="`cd \"$javaHome\" && pwd -P`/javac" + javaHome="$(dirname "\"$javaExecutable\"")" + javaExecutable="$(cd "\"$javaHome\"" && pwd -P)/javac" else - javaExecutable="`readlink -f \"$javaExecutable\"`" + javaExecutable="$(readlink -f "\"$javaExecutable\"")" fi - javaHome="`dirname \"$javaExecutable\"`" - javaHome=`expr "$javaHome" : '\(.*\)/bin'` + javaHome="$(dirname "\"$javaExecutable\"")" + javaHome=$(expr "$javaHome" : '\(.*\)/bin') JAVA_HOME="$javaHome" export JAVA_HOME fi @@ -149,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then JAVACMD="$JAVA_HOME/bin/java" fi else - JAVACMD="`\\unset -f command; \\command -v java`" + JAVACMD="$(\unset -f command 2>/dev/null; \command -v java)" fi fi @@ -163,12 +132,9 @@ if [ -z "$JAVA_HOME" ] ; then echo "Warning: JAVA_HOME environment variable is not set." fi -CLASSWORLDS_LAUNCHER=org.codehaus.plexus.classworlds.launcher.Launcher - # traverses directory structure from process work directory to filesystem root # first directory with .mvn subdirectory is considered project base directory find_maven_basedir() { - if [ -z "$1" ] then echo "Path not specified to find_maven_basedir" @@ -184,96 +150,99 @@ find_maven_basedir() { fi # workaround for JBEAP-8937 (on Solaris 10/Sparc) if [ -d "${wdir}" ]; then - wdir=`cd "$wdir/.."; pwd` + wdir=$(cd "$wdir/.." || exit 1; pwd) fi # end of workaround done - echo "${basedir}" + printf '%s' "$(cd "$basedir" || exit 1; pwd)" } # concatenates all lines of a file concat_lines() { if [ -f "$1" ]; then - echo "$(tr -s '\n' ' ' < "$1")" + # Remove \r in case we run on Windows within Git Bash + # and check out the repository with auto CRLF management + # enabled. Otherwise, we may read lines that are delimited with + # \r\n and produce $'-Xarg\r' rather than -Xarg due to word + # splitting rules. + tr -s '\r\n' ' ' < "$1" fi } -BASE_DIR=`find_maven_basedir "$(pwd)"` +log() { + if [ "$MVNW_VERBOSE" = true ]; then + printf '%s\n' "$1" + fi +} + +BASE_DIR=$(find_maven_basedir "$(dirname "$0")") if [ -z "$BASE_DIR" ]; then exit 1; fi +MAVEN_PROJECTBASEDIR=${MAVEN_BASEDIR:-"$BASE_DIR"}; export MAVEN_PROJECTBASEDIR +log "$MAVEN_PROJECTBASEDIR" + ########################################################################################## # Extension to allow automatically downloading the maven-wrapper.jar from Maven-central # This allows using the maven wrapper in projects that prohibit checking in binary data. ########################################################################################## -if [ -r "$BASE_DIR/.mvn/wrapper/maven-wrapper.jar" ]; then - if [ "$MVNW_VERBOSE" = true ]; then - echo "Found .mvn/wrapper/maven-wrapper.jar" - fi +wrapperJarPath="$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.jar" +if [ -r "$wrapperJarPath" ]; then + log "Found $wrapperJarPath" else - if [ "$MVNW_VERBOSE" = true ]; then - echo "Couldn't find .mvn/wrapper/maven-wrapper.jar, downloading it ..." - fi + log "Couldn't find $wrapperJarPath, downloading it ..." + if [ -n "$MVNW_REPOURL" ]; then - jarUrl="$MVNW_REPOURL/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar" + wrapperUrl="$MVNW_REPOURL/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar" else - jarUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar" + wrapperUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar" fi - while IFS="=" read key value; do - case "$key" in (wrapperUrl) jarUrl="$value"; break ;; + while IFS="=" read -r key value; do + # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' ) + safeValue=$(echo "$value" | tr -d '\r') + case "$key" in (wrapperUrl) wrapperUrl="$safeValue"; break ;; esac - done < "$BASE_DIR/.mvn/wrapper/maven-wrapper.properties" - if [ "$MVNW_VERBOSE" = true ]; then - echo "Downloading from: $jarUrl" - fi - wrapperJarPath="$BASE_DIR/.mvn/wrapper/maven-wrapper.jar" + done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties" + log "Downloading from: $wrapperUrl" + if $cygwin; then - wrapperJarPath=`cygpath --path --windows "$wrapperJarPath"` + wrapperJarPath=$(cygpath --path --windows "$wrapperJarPath") fi if command -v wget > /dev/null; then - if [ "$MVNW_VERBOSE" = true ]; then - echo "Found wget ... using wget" - fi + log "Found wget ... using wget" + [ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--quiet" if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then - wget "$jarUrl" -O "$wrapperJarPath" || rm -f "$wrapperJarPath" + wget $QUIET "$wrapperUrl" -O "$wrapperJarPath" || rm -f "$wrapperJarPath" else - wget --http-user=$MVNW_USERNAME --http-password=$MVNW_PASSWORD "$jarUrl" -O "$wrapperJarPath" || rm -f "$wrapperJarPath" + wget $QUIET --http-user="$MVNW_USERNAME" --http-password="$MVNW_PASSWORD" "$wrapperUrl" -O "$wrapperJarPath" || rm -f "$wrapperJarPath" fi elif command -v curl > /dev/null; then - if [ "$MVNW_VERBOSE" = true ]; then - echo "Found curl ... using curl" - fi + log "Found curl ... using curl" + [ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--silent" if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then - curl -o "$wrapperJarPath" "$jarUrl" -f + curl $QUIET -o "$wrapperJarPath" "$wrapperUrl" -f -L || rm -f "$wrapperJarPath" else - curl --user $MVNW_USERNAME:$MVNW_PASSWORD -o "$wrapperJarPath" "$jarUrl" -f + curl $QUIET --user "$MVNW_USERNAME:$MVNW_PASSWORD" -o "$wrapperJarPath" "$wrapperUrl" -f -L || rm -f "$wrapperJarPath" fi - else - if [ "$MVNW_VERBOSE" = true ]; then - echo "Falling back to using Java to download" - fi - javaClass="$BASE_DIR/.mvn/wrapper/MavenWrapperDownloader.java" + log "Falling back to using Java to download" + javaSource="$MAVEN_PROJECTBASEDIR/.mvn/wrapper/MavenWrapperDownloader.java" + javaClass="$MAVEN_PROJECTBASEDIR/.mvn/wrapper/MavenWrapperDownloader.class" # For Cygwin, switch paths to Windows format before running javac if $cygwin; then - javaClass=`cygpath --path --windows "$javaClass"` + javaSource=$(cygpath --path --windows "$javaSource") + javaClass=$(cygpath --path --windows "$javaClass") fi - if [ -e "$javaClass" ]; then - if [ ! -e "$BASE_DIR/.mvn/wrapper/MavenWrapperDownloader.class" ]; then - if [ "$MVNW_VERBOSE" = true ]; then - echo " - Compiling MavenWrapperDownloader.java ..." - fi - # Compiling the Java class - ("$JAVA_HOME/bin/javac" "$javaClass") + if [ -e "$javaSource" ]; then + if [ ! -e "$javaClass" ]; then + log " - Compiling MavenWrapperDownloader.java ..." + ("$JAVA_HOME/bin/javac" "$javaSource") fi - if [ -e "$BASE_DIR/.mvn/wrapper/MavenWrapperDownloader.class" ]; then - # Running the downloader - if [ "$MVNW_VERBOSE" = true ]; then - echo " - Running MavenWrapperDownloader.java ..." - fi - ("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader "$MAVEN_PROJECTBASEDIR") + if [ -e "$javaClass" ]; then + log " - Running MavenWrapperDownloader.java ..." + ("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader "$wrapperUrl" "$wrapperJarPath") || rm -f "$wrapperJarPath" fi fi fi @@ -282,35 +251,58 @@ fi # End of extension ########################################################################################## -export MAVEN_PROJECTBASEDIR=${MAVEN_BASEDIR:-"$BASE_DIR"} -if [ "$MVNW_VERBOSE" = true ]; then - echo $MAVEN_PROJECTBASEDIR +# If specified, validate the SHA-256 sum of the Maven wrapper jar file +wrapperSha256Sum="" +while IFS="=" read -r key value; do + case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;; + esac +done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties" +if [ -n "$wrapperSha256Sum" ]; then + wrapperSha256Result=false + if command -v sha256sum > /dev/null; then + if echo "$wrapperSha256Sum $wrapperJarPath" | sha256sum -c > /dev/null 2>&1; then + wrapperSha256Result=true + fi + elif command -v shasum > /dev/null; then + if echo "$wrapperSha256Sum $wrapperJarPath" | shasum -a 256 -c > /dev/null 2>&1; then + wrapperSha256Result=true + fi + else + echo "Checksum validation was requested but neither 'sha256sum' or 'shasum' are available." + echo "Please install either command, or disable validation by removing 'wrapperSha256Sum' from your maven-wrapper.properties." + exit 1 + fi + if [ $wrapperSha256Result = false ]; then + echo "Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised." >&2 + echo "Investigate or delete $wrapperJarPath to attempt a clean download." >&2 + echo "If you updated your Maven version, you need to update the specified wrapperSha256Sum property." >&2 + exit 1 + fi fi + MAVEN_OPTS="$(concat_lines "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS" # For Cygwin, switch paths to Windows format before running java if $cygwin; then - [ -n "$M2_HOME" ] && - M2_HOME=`cygpath --path --windows "$M2_HOME"` [ -n "$JAVA_HOME" ] && - JAVA_HOME=`cygpath --path --windows "$JAVA_HOME"` + JAVA_HOME=$(cygpath --path --windows "$JAVA_HOME") [ -n "$CLASSPATH" ] && - CLASSPATH=`cygpath --path --windows "$CLASSPATH"` + CLASSPATH=$(cygpath --path --windows "$CLASSPATH") [ -n "$MAVEN_PROJECTBASEDIR" ] && - MAVEN_PROJECTBASEDIR=`cygpath --path --windows "$MAVEN_PROJECTBASEDIR"` + MAVEN_PROJECTBASEDIR=$(cygpath --path --windows "$MAVEN_PROJECTBASEDIR") fi # Provide a "standardized" way to retrieve the CLI args that will # work with both Windows and non-Windows executions. -MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $@" +MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $*" export MAVEN_CMD_LINE_ARGS WRAPPER_LAUNCHER=org.apache.maven.wrapper.MavenWrapperMain +# shellcheck disable=SC2086 # safe args exec "$JAVACMD" \ $MAVEN_OPTS \ $MAVEN_DEBUG_OPTS \ -classpath "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.jar" \ - "-Dmaven.home=${M2_HOME}" \ "-Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR}" \ ${WRAPPER_LAUNCHER} $MAVEN_CONFIG "$@" diff --git a/mvnw.cmd b/mvnw.cmd index 8a15b7f31..c4586b564 100644 --- a/mvnw.cmd +++ b/mvnw.cmd @@ -18,13 +18,12 @@ @REM ---------------------------------------------------------------------------- @REM ---------------------------------------------------------------------------- -@REM Maven Start Up Batch script +@REM Apache Maven Wrapper startup batch script, version 3.2.0 @REM @REM Required ENV vars: @REM JAVA_HOME - location of a JDK home dir @REM @REM Optional ENV vars -@REM M2_HOME - location of maven2's installed home dir @REM MAVEN_BATCH_ECHO - set to 'on' to enable the echoing of the batch commands @REM MAVEN_BATCH_PAUSE - set to 'on' to wait for a keystroke before ending @REM MAVEN_OPTS - parameters passed to the Java VM when running Maven @@ -120,10 +119,10 @@ SET MAVEN_JAVA_EXE="%JAVA_HOME%\bin\java.exe" set WRAPPER_JAR="%MAVEN_PROJECTBASEDIR%\.mvn\wrapper\maven-wrapper.jar" set WRAPPER_LAUNCHER=org.apache.maven.wrapper.MavenWrapperMain -set DOWNLOAD_URL="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar" +set WRAPPER_URL="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar" FOR /F "usebackq tokens=1,2 delims==" %%A IN ("%MAVEN_PROJECTBASEDIR%\.mvn\wrapper\maven-wrapper.properties") DO ( - IF "%%A"=="wrapperUrl" SET DOWNLOAD_URL=%%B + IF "%%A"=="wrapperUrl" SET WRAPPER_URL=%%B ) @REM Extension to allow automatically downloading the maven-wrapper.jar from Maven-central @@ -134,11 +133,11 @@ if exist %WRAPPER_JAR% ( ) ) else ( if not "%MVNW_REPOURL%" == "" ( - SET DOWNLOAD_URL="%MVNW_REPOURL%/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar" + SET WRAPPER_URL="%MVNW_REPOURL%/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar" ) if "%MVNW_VERBOSE%" == "true" ( echo Couldn't find %WRAPPER_JAR%, downloading it ... - echo Downloading from: %DOWNLOAD_URL% + echo Downloading from: %WRAPPER_URL% ) powershell -Command "&{"^ @@ -146,7 +145,7 @@ if exist %WRAPPER_JAR% ( "if (-not ([string]::IsNullOrEmpty('%MVNW_USERNAME%') -and [string]::IsNullOrEmpty('%MVNW_PASSWORD%'))) {"^ "$webclient.Credentials = new-object System.Net.NetworkCredential('%MVNW_USERNAME%', '%MVNW_PASSWORD%');"^ "}"^ - "[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12; $webclient.DownloadFile('%DOWNLOAD_URL%', '%WRAPPER_JAR%')"^ + "[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12; $webclient.DownloadFile('%WRAPPER_URL%', '%WRAPPER_JAR%')"^ "}" if "%MVNW_VERBOSE%" == "true" ( echo Finished downloading %WRAPPER_JAR% @@ -154,6 +153,24 @@ if exist %WRAPPER_JAR% ( ) @REM End of extension +@REM If specified, validate the SHA-256 sum of the Maven wrapper jar file +SET WRAPPER_SHA_256_SUM="" +FOR /F "usebackq tokens=1,2 delims==" %%A IN ("%MAVEN_PROJECTBASEDIR%\.mvn\wrapper\maven-wrapper.properties") DO ( + IF "%%A"=="wrapperSha256Sum" SET WRAPPER_SHA_256_SUM=%%B +) +IF NOT %WRAPPER_SHA_256_SUM%=="" ( + powershell -Command "&{"^ + "$hash = (Get-FileHash \"%WRAPPER_JAR%\" -Algorithm SHA256).Hash.ToLower();"^ + "If('%WRAPPER_SHA_256_SUM%' -ne $hash){"^ + " Write-Output 'Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised.';"^ + " Write-Output 'Investigate or delete %WRAPPER_JAR% to attempt a clean download.';"^ + " Write-Output 'If you updated your Maven version, you need to update the specified wrapperSha256Sum property.';"^ + " exit 1;"^ + "}"^ + "}" + if ERRORLEVEL 1 goto error +) + @REM Provide a "standardized" way to retrieve the CLI args that will @REM work with both Windows and non-Windows executions. set MAVEN_CMD_LINE_ARGS=%* diff --git a/pom.xml b/pom.xml index 08d577794..84f11fd73 100644 --- a/pom.xml +++ b/pom.xml @@ -379,7 +379,7 @@ org.apache.maven.plugins maven-wrapper-plugin - 3.1.0 + 3.2.0 @@ -448,11 +448,6 @@ - - org.apache.maven.plugins - maven-wrapper-plugin - 3.1.0 - install From cde86457ab87ed9996dd0f68e0dd55c813bdc918 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 22 Jan 2024 01:38:19 +0000 Subject: [PATCH 21/28] Bump actions/upload-artifact from 4.1.0 to 4.2.0 Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.1.0 to 4.2.0. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](https://github.com/actions/upload-artifact/compare/1eb3cb2b3e0f29609092a73eb033bb759a334595...694cdabd8bdb0f10b2cea11669e1bf5453eed0a6) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/scorecards-analysis.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/scorecards-analysis.yaml b/.github/workflows/scorecards-analysis.yaml index 0fde102be..4e3f1b29c 100644 --- a/.github/workflows/scorecards-analysis.yaml +++ b/.github/workflows/scorecards-analysis.yaml @@ -57,7 +57,7 @@ jobs: publish_results: true - name: "Upload artifact" - uses: actions/upload-artifact@1eb3cb2b3e0f29609092a73eb033bb759a334595 # 4.1.0 + uses: actions/upload-artifact@694cdabd8bdb0f10b2cea11669e1bf5453eed0a6 # 4.2.0 with: name: SARIF file path: results.sarif From cf74a4450c60c616a283ccadd96cae8410fa9e05 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 22 Jan 2024 09:28:14 +0100 Subject: [PATCH 22/28] Fixes excluding Plexus container in OWASP scan --- src/etc/project-suppression.xml | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/etc/project-suppression.xml b/src/etc/project-suppression.xml index be1c572b1..7b2a1c5fb 100644 --- a/src/etc/project-suppression.xml +++ b/src/etc/project-suppression.xml @@ -132,21 +132,16 @@ ^pkg:maven/org\.codehaus\.plexus/plexus\-utils@.*$ cpe:/a:plexus-utils_project:plexus-utils + CVE-2022-4244 + CVE-2022-4245 + CVE-2017-1000487 - - ^pkg:maven/org\.codehaus\.plexus/plexus\-utils@.*$ - CVE-2017-1000487 - - - - ^pkg:maven/org\.codehaus\.plexus/plexus\-utils@.*$ - Directory traversal in org.codehaus.plexus.util.Expand - - - - ^pkg:maven/org\.codehaus\.plexus/plexus\-utils@.*$ - Possible XML Injection + + ^pkg:maven/org\.codehaus\.plexus\/plexus\-container\-default@.*$ + cpe:/a:plexus-utils_project:plexus-utils + CVE-2022-4244 + CVE-2022-4245 From 9f4b67a9a243fbd17de8232aacf56a92b493f5f4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 22 Jan 2024 10:08:59 +0100 Subject: [PATCH 23/28] Drops JDK11 build and fixes duplicated steps --- Jenkinsfile | 62 ++++++++++------------------------------------------- 1 file changed, 11 insertions(+), 51 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index 1c62cdb5a..1d8aa072a 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -67,14 +67,9 @@ pipeline { MAVEN_OPTS = "-Xmx1024m" } stages { - stage('Build') { + stage('Test & Coverage') { steps { - sh './mvnw -B -DskipAssembly verify --no-transfer-progress' - } - } - stage('Test') { - steps { - sh './mvnw -B verify -Pcoverage -DskipAssembly' + sh './mvnw -B verify -Pcoverage -DskipAssembly --no-transfer-progress' } post { always { @@ -91,7 +86,7 @@ pipeline { } steps { withCredentials([string(credentialsId: 'asf-struts-sonarcloud', variable: 'SONARCLOUD_TOKEN')]) { - sh './mvnw -B -Pcoverage -DskipAssembly -Dsonar.login=${SONARCLOUD_TOKEN} verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar' + sh './mvnw -B -Pcoverage -DskipAssembly -Dsonar.login=${SONARCLOUD_TOKEN} verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar --no-transfer-progress' } } } @@ -103,7 +98,7 @@ pipeline { dir("local-snapshots-dir/") { deleteDir() } - sh './mvnw -B source:jar javadoc:jar -DskipTests -DskipAssembly' + sh './mvnw -B source:jar javadoc:jar -DskipTests -DskipAssembly --no-transfer-progress' } } stage('Deploy Snapshot') { @@ -112,7 +107,7 @@ pipeline { } steps { withCredentials([file(credentialsId: 'lukaszlenart-repository-access-token', variable: 'CUSTOM_SETTINGS')]) { - sh './mvnw -s \${CUSTOM_SETTINGS} deploy -DskipTests -DskipAssembly' + sh './mvnw -s \${CUSTOM_SETTINGS} deploy -DskipTests -DskipAssembly --no-transfer-progress' } } } @@ -121,7 +116,7 @@ pipeline { branch 'release/struts-7-0-x' } steps { - sh './mvnw -B package -DskipTests' + sh './mvnw -B package -DskipTests --no-transfer-progress' sshPublisher(publishers: [ sshPublisherDesc( configName: 'Nightlies', @@ -145,41 +140,6 @@ pipeline { } } } - stage('JDK 11') { - agent { - label 'ubuntu' - } - tools { - jdk 'jdk_11_latest' - maven 'maven_3_latest' - } - environment { - MAVEN_OPTS = "-Xmx1024m" - } - stages { - stage('Build') { - steps { - sh './mvnw -B -DskipAssembly verify --no-transfer-progress' - } - } - stage('Test') { - steps { - sh './mvnw -B test' - } - post { - always { - junit(testResults: '**/surefire-reports/*.xml', allowEmptyResults: true) - junit(testResults: '**/failsafe-reports/*.xml', allowEmptyResults: true) - } - } - } - } - post { - always { - cleanWs deleteDirs: true, patterns: [[pattern: '**/target/**', type: 'INCLUDE']] - } - } - } stage('JDK 8') { agent { label 'ubuntu' @@ -194,12 +154,12 @@ pipeline { stages { stage('Build') { steps { - sh './mvnw -B clean install -DskipTests -DskipAssembly' + sh './mvnw -B clean install -DskipTests -DskipAssembly --no-transfer-progress' } } stage('Test') { steps { - sh './mvnw -B test' + sh './mvnw -B verify --no-transfer-progress' } post { always { @@ -216,7 +176,7 @@ pipeline { dir("local-snapshots-dir/") { deleteDir() } - sh './mvnw -B source:jar javadoc:jar -DskipTests -DskipAssembly' + sh './mvnw -B source:jar javadoc:jar -DskipTests -DskipAssembly --no-transfer-progress' } } stage('Deploy Snapshot') { @@ -225,7 +185,7 @@ pipeline { } steps { withCredentials([file(credentialsId: 'lukaszlenart-repository-access-token', variable: 'CUSTOM_SETTINGS')]) { - sh './mvnw -s \${CUSTOM_SETTINGS} deploy -DskipTests -DskipAssembly' + sh './mvnw -s \${CUSTOM_SETTINGS} deploy -DskipTests -DskipAssembly --no-transfer-progress' } } } @@ -234,7 +194,7 @@ pipeline { branch 'master' } steps { - sh './mvnw -B package -DskipTests' + sh './mvnw -B package -DskipTests --no-transfer-progress' sshPublisher(publishers: [ sshPublisherDesc( configName: 'Nightlies', From e7a13b9637a148d76aa2546e356df0349fb31872 Mon Sep 17 00:00:00 2001 From: Sebastian Peters Date: Sun, 21 Jan 2024 12:10:36 +0100 Subject: [PATCH 24/28] Small spelling and MD fixes (IntelliJ assisted) --- SECURITY.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index eca65f01b..a81895f4d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,11 +2,11 @@ ## Supported Versions -Please vist the [Releases](https://struts.apache.org/releases.html#prior-releases) page to see full information about each version +Please visit the [Releases](https://struts.apache.org/releases.html#prior-releases) page to see full information about each version and what potential vulnerability it can have: | Version | Supported | -| ------- | ------------------ | +|---------|--------------------| | 6.0.0 | :white_check_mark: | | 2.5.30 | :white_check_mark: | @@ -28,8 +28,8 @@ All mail sent to this address that does not relate to security problems in the A ``` Note that all networked servers are subject to denial of service attacks, and we cannot promise magic -workarounds to generic problems (such as a client streaming lots of data to your server, or re-requesting -the same URL repeatedly). In general our philosophy is to avoid any attacks which can cause the server +workarounds to generic problems (such as a client streaming lots of data to your server, or requesting +the same URL repeatedly). In general, our philosophy is to avoid any attacks that can cause the server to consume resources in a non-linear relationship to the size of inputs. The mailing address is: [security@struts.apache.org](mailto:security@struts.apache.org) From 644bd1f8ca4080544f118faeb0d13176152b0ad8 Mon Sep 17 00:00:00 2001 From: Sebastian Peters Date: Wed, 24 Jan 2024 15:14:09 +0100 Subject: [PATCH 25/28] Mention just the maintenance branches for supported versions b/c https://struts.apache.org/releases.html#prior-releases has further details. --- SECURITY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index a81895f4d..7908f9a34 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -7,8 +7,8 @@ and what potential vulnerability it can have: | Version | Supported | |---------|--------------------| -| 6.0.0 | :white_check_mark: | -| 2.5.30 | :white_check_mark: | +| 6.x | :white_check_mark: | +| 2.5.x | :white_check_mark: | ## Reporting New Security Issues with thr Apache Struts From 2513fcb292ea1fae754b9ff4d3b84ac778e9ac66 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 24 Jan 2024 17:28:52 +0100 Subject: [PATCH 26/28] Stops running sonar.yml on forks --- .github/workflows/sonar.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 836d50d20..4a3667e4c 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -31,6 +31,7 @@ jobs: sonarcloud: name: Scan runs-on: ubuntu-latest + if: ${{ !github.event.pull_request.head.repo.fork }} steps: - uses: actions/checkout@v4 with: From a358db585326e6e1cddd5442b0f3e1fec0d01ed7 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 21 Jan 2024 10:39:49 +0100 Subject: [PATCH 27/28] WW-5360 Introduces additional countStr & indexStr to allow to ignore conversion --- .../struts2/views/jsp/IteratorStatus.java | 20 +- .../com/opensymphony/xwork2/test/User.java | 6 + .../java/org/apache/struts2/TestAction.java | 9 + .../components/IteratorComponentTest.java | 192 +++++++++++++++- .../struts2/views/jsp/IteratorTagTest.java | 205 +++++++++--------- 5 files changed, 314 insertions(+), 118 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/views/jsp/IteratorStatus.java b/core/src/main/java/org/apache/struts2/views/jsp/IteratorStatus.java index 13022f856..fedef363e 100644 --- a/core/src/main/java/org/apache/struts2/views/jsp/IteratorStatus.java +++ b/core/src/main/java/org/apache/struts2/views/jsp/IteratorStatus.java @@ -26,23 +26,29 @@ package org.apache.struts2.views.jsp; *
  • count: iterations so far, starts on 1. count is always index + 1
    • *
  • first: true if index == 0
    • *
  • even: true if (index + 1) % 2 == 0
    • - *
  • last: true if current iteration is the last iteration
    • + *
  • last: true if current iteration is the last iteration
    • *
  • odd: true if (index + 1) % 2 == 1
    • * *

    Example

    * 
      *   <s:iterator status="status" value='{0, 1}'>
  *      Index: <s:property value="%{#status.index}" /> <br />
- *      Count: <s:property value="%{#status.count}" /> <br />  
+ *      Index Str: <s:property value="%{#status.indexStr}" /> <br />
+ *      Count: <s:property value="%{#status.count}" /> <br />
+ *      Count Str: <s:property value="%{#status.countStr}" /> <br />
  *   </s:iterator>
  *
    - * + * *

    will print

    * 
      *      Index: 0
+ *      Index Str: 0
  *      Count: 1
+ *      Count Str: 1
  *      Index: 1
+ *      Index Str: 1
  *      Count: 2
+ *      Count Str: 2
  *
    */ public class IteratorStatus { @@ -56,6 +62,10 @@ public class IteratorStatus { return state.index + 1; } + public String getCountStr() { + return String.valueOf(state.index + 1); + } + public boolean isEven() { return ((state.index + 1) % 2) == 0; } @@ -68,6 +78,10 @@ public class IteratorStatus { return state.index; } + public String getIndexStr() { + return String.valueOf(state.index); + } + public boolean isLast() { return state.last; } diff --git a/core/src/test/java/com/opensymphony/xwork2/test/User.java b/core/src/test/java/com/opensymphony/xwork2/test/User.java index d377fe018..e6c5d2af4 100644 --- a/core/src/test/java/com/opensymphony/xwork2/test/User.java +++ b/core/src/test/java/com/opensymphony/xwork2/test/User.java @@ -37,6 +37,12 @@ public class User implements UserMarker { private String email2; private String name; + public User() { + } + + public User(String name) { + this.name = name; + } public void setCollection(Collection collection) { this.collection = collection; diff --git a/core/src/test/java/org/apache/struts2/TestAction.java b/core/src/test/java/org/apache/struts2/TestAction.java index 77f784a61..b9595de11 100644 --- a/core/src/test/java/org/apache/struts2/TestAction.java +++ b/core/src/test/java/org/apache/struts2/TestAction.java @@ -45,6 +45,7 @@ public class TestAction extends ActionSupport { private String result; private User user; private String[] array; + private Object[] objectArray; private String[][] list; private List list2; private List list3; @@ -135,6 +136,14 @@ public class TestAction extends ActionSupport { this.array = array; } + public Object[] getObjectArray() { + return objectArray; + } + + public void setObjectArray(Object[] arrayObject) { + this.objectArray = arrayObject; + } + public String[][] getList() { return list; } 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 065a42ae9..7f08ef64e 100644 --- a/core/src/test/java/org/apache/struts2/components/IteratorComponentTest.java +++ b/core/src/test/java/org/apache/struts2/components/IteratorComponentTest.java @@ -19,26 +19,29 @@ package org.apache.struts2.components; import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.test.User; import com.opensymphony.xwork2.util.ValueStack; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.ognl.ThreadAllowlist; +import org.apache.struts2.TestAction; import java.io.StringWriter; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Locale; 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(); + ThreadAllowlist threadAllowlist = new ThreadAllowlist(); ic.setThreadAllowlist(threadAllowlist); } @@ -74,7 +77,48 @@ public class IteratorComponentTest extends StrutsInternalTestCase { assertEquals("item1 item2 item3 item4 ", out.getBuffer().toString()); } - public void testIteratorWithBegin() throws Exception { + public void testSimpleIterator() { + // given + stack.push(new FooAction()); + + StringWriter out = new StringWriter(); + + ic.setBegin("1"); + ic.setEnd("8"); + ic.setStep("2"); + ic.setStatus("status"); + + Property prop = new Property(stack); + Property status = new Property(stack); + status.setValue("#status.index"); + + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + + String body = " "; + + // when + assertTrue(ic.start(out)); + + for (int i = 0; i < 4; i++) { + status.start(out); + status.end(out, body); + prop.start(out); + prop.end(out, body); + ic.end(out, null); + } + + // then + assertEquals("0 1 1 3 2 5 3 7 ", out.getBuffer().toString()); + } + + public void testIteratorWithBegin() { // given stack.push(new FooAction()); @@ -104,12 +148,12 @@ public class IteratorComponentTest extends StrutsInternalTestCase { assertEquals("item2 item3 item4 ", out.getBuffer().toString()); } - public void testIteratorWithNulls() throws Exception { + public void testIteratorWithNulls() { // given stack.push(new FooAction() { - private List items = Arrays.asList("1", "2", null, "4"); + private final List items = Arrays.asList("1", "2", null, "4"); - public List getItems() { + public List getItems() { return items; } }); @@ -140,15 +184,147 @@ public class IteratorComponentTest extends StrutsInternalTestCase { assertEquals("1, 2, , 4, ", out.getBuffer().toString()); } + public void testIteratorWithDifferentLocale() { + // given + ActionContext.getContext().withLocale(new Locale("fa_IR")); + stack.push(new FooAction()); + + StringWriter out = new StringWriter(); + + ic.setBegin("1"); + ic.setEnd("3"); + ic.setStatus("status"); + + Property prop = new Property(stack); + Property status = new Property(stack); + status.setValue("#status.count"); + + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + + String body = ","; + + // when + assertTrue(ic.start(out)); + + for (int i = 0; i < 3; i++) { + status.start(out); + status.end(out, body); + + prop.start(out); + prop.end(out, body); + ic.end(out, null); + } + + // then + assertEquals("1,1,2,2,3,3,", out.getBuffer().toString()); + } + + public void testListOfBeansIterator() { + // given + TestAction action = new TestAction(); + action.setList2(new ArrayList() {{ + add(new User("Anton")); + add(new User("Tym")); + add(new User("Luk")); + }}); + stack.push(action); + + StringWriter out = new StringWriter(); + + ic.setValue("list2"); + ic.setStatus("status"); + + Property prop = new Property(stack); + prop.setValue("name"); + Property status = new Property(stack); + status.setValue("#status.indexStr"); + + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + + String body = ","; + + // when + assertTrue(ic.start(out)); + + for (int i = 0; i < 3; i++) { + status.start(out); + status.end(out, body); + + prop.start(out); + prop.end(out, body); + + ic.end(out, null); + } + + // then + assertEquals("0,Anton,1,Tym,2,Luk,", out.getBuffer().toString()); + } + + public void testArrayOfBeansIterator() { + // given + TestAction action = new TestAction(); + action.setObjectArray(new ArrayList() {{ + add(new User("Anton")); + add(new User("Tym")); + add(new User("Luk")); + }}.toArray()); + stack.push(action); + + StringWriter out = new StringWriter(); + + ic.setValue("objectArray"); + ic.setStatus("status"); + + Property prop = new Property(stack); + prop.setValue("name"); + Property status = new Property(stack); + status.setValue("#status.countStr"); + + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + ic.getComponentStack().push(status); + ic.getComponentStack().push(prop); + + String body = " "; + + // when + assertTrue(ic.start(out)); + + for (int i = 0; i < 3; i++) { + status.start(out); + status.end(out, body); + + prop.start(out); + prop.end(out, body); + + ic.end(out, null); + } + + // then + assertEquals("1 Anton 2 Tym 3 Luk ", out.getBuffer().toString()); + } + static class FooAction { - private List items; + private final List items; public FooAction() { items = Arrays.asList("item1", "item2", "item3", "item4"); } - public List getItems() { + public List getItems() { return items; } } diff --git a/core/src/test/java/org/apache/struts2/views/jsp/IteratorTagTest.java b/core/src/test/java/org/apache/struts2/views/jsp/IteratorTagTest.java index df5022c0a..fd3fc9587 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/IteratorTagTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/IteratorTagTest.java @@ -20,6 +20,7 @@ package org.apache.struts2.views.jsp; import com.mockobjects.servlet.MockBodyContent; import com.mockobjects.servlet.MockJspWriter; +import com.opensymphony.xwork2.ActionContext; import org.apache.commons.collections.ListUtils; import javax.servlet.jsp.JspException; @@ -29,20 +30,15 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; - -/** - * Test Case for Iterator Tag - * - */ public class IteratorTagTest extends AbstractUITagTest { - IteratorTag tag; - + private IteratorTag tag; public void testIteratingWithIdSpecified() throws Exception { - List list = new ArrayList(); + List list = new ArrayList<>(); list.add("one"); list.add("two"); list.add("three"); @@ -104,12 +100,12 @@ public class IteratorTagTest extends AbstractUITagTest { IteratorTag freshTag = new IteratorTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testIteratingWithIdSpecified_clearTagStateSet() throws Exception { - List list = new ArrayList(); + List list = new ArrayList<>(); list.add("one"); list.add("two"); list.add("three"); @@ -174,12 +170,12 @@ public class IteratorTagTest extends AbstractUITagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testIteratingWithIdSpecifiedAndNullElementOnCollection() throws Exception { - List list = new ArrayList(); + List list = new ArrayList<>(); list.add("one"); list.add(null); list.add("three"); @@ -224,12 +220,12 @@ public class IteratorTagTest extends AbstractUITagTest { IteratorTag freshTag = new IteratorTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testIteratingWithIdSpecifiedAndNullElementOnCollection_clearTagStateSet() throws Exception { - List list = new ArrayList(); + List list = new ArrayList<>(); list.add("one"); list.add(null); list.add("three"); @@ -277,7 +273,7 @@ public class IteratorTagTest extends AbstractUITagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -294,7 +290,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCollectionIterator() { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + List list = new ArrayList<>(); list.add("test1"); list.add("test2"); list.add("test3"); @@ -314,7 +310,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testMapIterator() { Foo foo = new Foo(); - HashMap map = new HashMap(); + HashMap map = new HashMap<>(); map.put("test1", "123"); map.put("test2", "456"); map.put("test3", "789"); @@ -329,8 +325,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doStartTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); @@ -340,8 +335,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -351,8 +345,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -362,8 +355,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.SKIP_BODY, result); @@ -372,8 +364,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doEndTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_PAGE, result); @@ -382,13 +373,13 @@ public class IteratorTagTest extends AbstractUITagTest { IteratorTag freshTag = new IteratorTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } public void testMapIterator_clearTagStateSet() { Foo foo = new Foo(); - HashMap map = new HashMap(); + HashMap map = new HashMap<>(); map.put("test1", "123"); map.put("test2", "456"); map.put("test3", "789"); @@ -405,8 +396,7 @@ public class IteratorTagTest extends AbstractUITagTest { result = tag.doStartTag(); setComponentTagClearTagState(tag, true); // Ensure component tag state clearing is set true (to match tag). } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); @@ -416,8 +406,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -427,8 +416,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -438,8 +426,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.SKIP_BODY, result); @@ -448,8 +435,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doEndTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_PAGE, result); @@ -459,7 +445,7 @@ public class IteratorTagTest extends AbstractUITagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -477,8 +463,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doStartTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); @@ -490,15 +475,16 @@ public class IteratorTagTest extends AbstractUITagTest { assertFalse(status.isLast()); assertTrue(status.isFirst()); assertEquals(0, status.getIndex()); + assertEquals("0", status.getIndexStr()); assertEquals(1, status.getCount()); + assertEquals("1", status.getCountStr()); assertTrue(status.isOdd()); assertFalse(status.isEven()); try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -517,8 +503,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -537,8 +522,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doEndTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_PAGE, result); @@ -547,7 +531,7 @@ public class IteratorTagTest extends AbstractUITagTest { IteratorTag freshTag = new IteratorTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -567,8 +551,7 @@ public class IteratorTagTest extends AbstractUITagTest { result = tag.doStartTag(); setComponentTagClearTagState(tag, true); // Ensure component tag state clearing is set true (to match tag). } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); @@ -587,8 +570,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -607,8 +589,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -627,8 +608,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doEndTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_PAGE, result); @@ -638,7 +618,7 @@ public class IteratorTagTest extends AbstractUITagTest { freshTag.setPerformClearTagStateForTagPoolingServers(true); freshTag.setPageContext(pageContext); assertTrue("Tag state after doEndTag() and explicit tag state clearing is inequal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } @@ -666,7 +646,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testEmptyCollection() { Foo foo = new Foo(); - foo.setList(new ArrayList()); + foo.setList(new ArrayList<>()); stack.push(foo); @@ -692,17 +672,41 @@ public class IteratorTagTest extends AbstractUITagTest { validateCounter(new Integer[]{0, 1, 2, 3, 4, 5}); } - public void testCounterWithStackValues() throws JspException { + public void testCounterWithDifferentLocale() throws JspException { + stack.getActionContext().withLocale(new Locale("fa_IR")); + tag.setVar("it"); + tag.setBegin("0"); + tag.setEnd("5"); + List expectedValues = Arrays.asList("0", "1", "2", "3", "4", "5"); + + ArrayList values = new ArrayList<>(); + try { + int result = tag.doStartTag(); + assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); + values.add((String) stack.findValue("it", String.class)); + } catch (JspException e) { + fail(e.getMessage()); + } + + while (tag.doAfterBody() == TagSupport.EVAL_BODY_AGAIN) { + values.add((String) stack.findValue("top", String.class)); + } + + assertEquals(expectedValues.size(), values.size()); + assertEquals(expectedValues, values); + } + + public void testCounterWithStackValues() throws JspException { stack.getContext().put("begin", 0); stack.getContext().put("end", 5); - tag.setBegin("%{#begin}"); - tag.setEnd("%{#end}"); + tag.setBegin("begin"); + tag.setEnd("end"); validateCounter(new Integer[]{0, 1, 2, 3, 4, 5}); } public void testCounterWithList() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -720,7 +724,6 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithArray() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); foo.setArray(new String[]{"a", "b", "c", "d"}); stack.push(foo); @@ -735,7 +738,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithListNoEnd() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -752,7 +755,6 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithArrayNoEnd() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); foo.setArray(new String[]{"a", "b", "c", "d"}); stack.push(foo); @@ -765,7 +767,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithList2() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -783,7 +785,6 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithArray2() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); foo.setArray(new String[]{"a", "b", "c", "d"}); stack.push(foo); @@ -797,7 +798,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithListNoEnd2() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -812,9 +813,8 @@ public class IteratorTagTest extends AbstractUITagTest { validateCounter(new String[]{"c", "d"}); } - public void testCounterWithArrayNoEnd2() throws JspException { + public void testCounterWithArrayNoEnd2() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); foo.setArray(new String[]{"a", "b", "c", "d"}); stack.push(foo); @@ -838,9 +838,9 @@ public class IteratorTagTest extends AbstractUITagTest { validateCounter(new Integer[]{0, 2, 4}); } - public void testCounterWithListAndStep() throws JspException { + public void testCounterWithListAndStep() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -858,9 +858,8 @@ public class IteratorTagTest extends AbstractUITagTest { validateCounter(new String[]{"a", "c"}); } - public void testCounterWithArrayAndStep() throws JspException { + public void testCounterWithArrayAndStep() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); foo.setArray(new String[]{"a", "b", "c", "d"}); stack.push(foo); @@ -876,7 +875,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithListAndStepNoEnd() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -895,7 +894,6 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithArrayAndStepNoEnd() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); foo.setArray(new String[]{"a", "b", "c", "d"}); stack.push(foo); @@ -917,7 +915,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithListAndNegativeStep() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -937,7 +935,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithListAndNegativeStepNoEnd() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -954,9 +952,9 @@ public class IteratorTagTest extends AbstractUITagTest { validateCounter(new String[]{"d", "c", "b", "a"}); } - public void testCounterWithArrayAndNegativeStep() throws JspException { + public void testCounterWithArrayAndNegativeStep() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -976,7 +974,7 @@ public class IteratorTagTest extends AbstractUITagTest { public void testCounterWithArrayAndNegativeStepNoEnd() throws JspException { Foo foo = new Foo(); - ArrayList list = new ArrayList(); + ArrayList list = new ArrayList<>(); list.add("a"); list.add("b"); list.add("c"); @@ -994,14 +992,13 @@ public class IteratorTagTest extends AbstractUITagTest { } protected void validateCounter(Object[] expectedValues) throws JspException { - List values = new ArrayList(); + ArrayList values = new ArrayList<>(); try { int result = tag.doStartTag(); assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); values.add(stack.getRoot().peek()); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } while (tag.doAfterBody() == TagSupport.EVAL_BODY_AGAIN) { @@ -1009,7 +1006,7 @@ public class IteratorTagTest extends AbstractUITagTest { } assertEquals(expectedValues.length, values.size()); - ListUtils.isEqualList(Arrays.asList(expectedValues), values); + assertTrue(ListUtils.isEqualList(Arrays.asList(expectedValues), values)); } @Override @@ -1033,8 +1030,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doStartTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_INCLUDE, result); @@ -1044,8 +1040,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -1055,8 +1050,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_BODY_AGAIN, result); @@ -1066,8 +1060,7 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doAfterBody(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.SKIP_BODY, result); @@ -1080,16 +1073,14 @@ public class IteratorTagTest extends AbstractUITagTest { try { result = tag.doStartTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.SKIP_BODY, result); try { result = tag.doEndTag(); } catch (JspException e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertEquals(TagSupport.EVAL_PAGE, result); @@ -1098,13 +1089,13 @@ public class IteratorTagTest extends AbstractUITagTest { IteratorTag freshTag = new IteratorTag(); freshTag.setPageContext(pageContext); assertFalse("Tag state after doEndTag() under default tag clear state is equal to new Tag with pageContext/parent set. " + - "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", + "May indicate that clearTagStateForTagPoolingServers() calls are not working properly.", strutsBodyTagsAreReflectionEqual(tag, freshTag)); } - class Foo { - private Collection list; - private Map map; + static class Foo { + private Collection list; + private Map map; private String[] array; public void setArray(String[] array) { @@ -1115,24 +1106,24 @@ public class IteratorTagTest extends AbstractUITagTest { return array; } - public void setList(Collection list) { + public void setList(Collection list) { this.list = list; } - public Collection getList() { + public Collection getList() { return list; } - public void setMap(Map map) { + public void setMap(Map map) { this.map = map; } - public Map getMap() { + public Map getMap() { return map; } } - class TestMockBodyContent extends MockBodyContent { + static class TestMockBodyContent extends MockBodyContent { public String getString() { return ".-."; } From 372aad2c6ce1e8ff07e2c880c93b35bc5c0ecf05 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 29 Jan 2024 01:48:45 +0000 Subject: [PATCH 28/28] Bump actions/upload-artifact from 4.2.0 to 4.3.0 Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.2.0 to 4.3.0. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](https://github.com/actions/upload-artifact/compare/694cdabd8bdb0f10b2cea11669e1bf5453eed0a6...26f96dfa697d77e81fd5907df203aa23a56210a8) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/scorecards-analysis.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/scorecards-analysis.yaml b/.github/workflows/scorecards-analysis.yaml index 4e3f1b29c..192f6cda6 100644 --- a/.github/workflows/scorecards-analysis.yaml +++ b/.github/workflows/scorecards-analysis.yaml @@ -57,7 +57,7 @@ jobs: publish_results: true - name: "Upload artifact" - uses: actions/upload-artifact@694cdabd8bdb0f10b2cea11669e1bf5453eed0a6 # 4.2.0 + uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 # 4.3.0 with: name: SARIF file path: results.sarif