From ed33f13f2d08a7239a69e62de1019df0a47f350a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 17:06:47 +0200 Subject: [PATCH 01/10] WW-5627 add ParameterAllowlister interface and STRUTS_PARAMETER_ALLOWLISTER constant Co-Authored-By: Claude Sonnet 4.6 --- .../org/apache/struts2/StrutsConstants.java | 8 ++++ .../parameter/ParameterAllowlister.java | 41 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index eb925b422a..13fc2d3d29 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -558,6 +558,14 @@ public final class StrutsConstants { */ public static final String STRUTS_PARAMETER_AUTHORIZER = "struts.parameterAuthorizer"; + /** + * The {@link org.apache.struts2.interceptor.parameter.ParameterAllowlister} implementation class. + * Override to provide a custom allowlister for non-OGNL parameter targets. + * + * @since 7.2.0 + */ + public static final String STRUTS_PARAMETER_ALLOWLISTER = "struts.parameterAllowlister"; + /** * Enables evaluation of OGNL expressions * diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java new file mode 100644 index 0000000000..3749bc4f59 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java @@ -0,0 +1,41 @@ +/* + * 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; + +/** + * Service for priming downstream allowlists (e.g. the OGNL {@link org.apache.struts2.ognl.ThreadAllowlist}) for a + * parameter path that has already been authorized by {@link ParameterAuthorizer}. Separated from the authorizer so + * that the authorizer can remain side-effect-free and reusable from non-OGNL channels (Jackson, Juneau). + * + *

Implementations are expected to no-op when {@code parameterName} is depth-0 if their downstream engine does not + * require root-level priming. Callers must have already verified authorization via + * {@link ParameterAuthorizer#isAuthorized}; this service does NOT enforce annotations.

+ * + * @since 7.2.0 + */ +public interface ParameterAllowlister { + + /** + * Primes the underlying allowlist for an authorized parameter path. + * + * @param parameterName the parameter name (e.g. {@code "user.role"}, {@code "items[0].name"}) + * @param target the object receiving the parameter value (the action, or the model for ModelDriven actions) + */ + void allowlistAuthorizedPath(String parameterName, Object target); +} From 17c076e2a269704aff7e38a3b55860d8e9ae8558 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 17:10:33 +0200 Subject: [PATCH 02/10] WW-5627 add OgnlParameterAllowlister default implementation Co-Authored-By: Claude Sonnet 4.6 --- .../parameter/OgnlParameterAllowlister.java | 163 ++++++++++++++++++ .../OgnlParameterAllowlisterTest.java | 156 +++++++++++++++++ 2 files changed, 319 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java new file mode 100644 index 0000000000..c38015cfa8 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java @@ -0,0 +1,163 @@ +/* + * 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 org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.inject.Inject; +import org.apache.struts2.ognl.OgnlUtil; +import org.apache.struts2.ognl.ThreadAllowlist; +import org.apache.struts2.util.ProxyService; + +import java.beans.BeanInfo; +import java.beans.IntrospectionException; +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.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.util.Arrays; +import java.util.Optional; + +import static org.apache.commons.lang3.StringUtils.indexOfAny; +import static org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS; +import static org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS_STR; + +/** + * Default {@link ParameterAllowlister} that primes OGNL {@link ThreadAllowlist} for nested-path writes. Logic is + * extracted verbatim from {@code ParametersInterceptor.performOgnlAllowlisting} so the OGNL parameter and cookie + * channels share a single implementation. + * + *

No-ops for depth-0 paths — root-level setters do not need allowlisting.

+ * + * @since 7.2.0 + */ +public class OgnlParameterAllowlister implements ParameterAllowlister { + + private static final Logger LOG = LogManager.getLogger(OgnlParameterAllowlister.class); + + private OgnlUtil ognlUtil; + private ProxyService proxyService; + private ThreadAllowlist threadAllowlist; + + @Inject + public void setOgnlUtil(OgnlUtil ognlUtil) { + this.ognlUtil = ognlUtil; + } + + @Inject + public void setProxyService(ProxyService proxyService) { + this.proxyService = proxyService; + } + + @Inject + public void setThreadAllowlist(ThreadAllowlist threadAllowlist) { + this.threadAllowlist = threadAllowlist; + } + + @Override + public void allowlistAuthorizedPath(String parameterName, Object target) { + if (parameterName == null || parameterName.isEmpty() || target == null) { + return; + } + long paramDepth = parameterName.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + if (paramDepth == 0) { + return; + } + + int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR); + String rootProperty = nestingIndex == -1 ? parameterName : parameterName.substring(0, nestingIndex); + String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1); + + BeanInfo beanInfo = getBeanInfo(target); + if (beanInfo != null) { + Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) + .filter(desc -> desc.getName().equals(normalisedRootProperty)).findFirst(); + if (propDescOpt.isPresent()) { + PropertyDescriptor propDesc = propDescOpt.get(); + Method relevantMethod = propDesc.getReadMethod(); + if (relevantMethod != null && getPermittedInjectionDepth(relevantMethod) >= paramDepth) { + allowlistClass(propDesc.getPropertyType()); + if (paramDepth >= 2) { + allowlistParameterizedTypeArg(relevantMethod.getGenericReturnType()); + } + return; + } + } + } + + Class targetClass = ultimateClass(target); + try { + Field field = targetClass.getDeclaredField(normalisedRootProperty); + if (Modifier.isPublic(field.getModifiers()) && getPermittedInjectionDepth(field) >= paramDepth) { + allowlistClass(field.getType()); + if (paramDepth >= 2) { + allowlistParameterizedTypeArg(field.getGenericType()); + } + } + } catch (NoSuchFieldException e) { + // No public field by that name — nothing to allowlist. + } + } + + private void allowlistClass(Class clazz) { + threadAllowlist.allowClassHierarchy(clazz); + } + + private void allowlistParameterizedTypeArg(Type genericType) { + if (!(genericType instanceof ParameterizedType pType)) { + return; + } + Type[] paramTypes = pType.getActualTypeArguments(); + allowlistParamType(paramTypes[0]); + if (paramTypes.length > 1) { + allowlistParamType(paramTypes[1]); + } + } + + private void allowlistParamType(Type paramType) { + if (paramType instanceof Class clazz) { + allowlistClass(clazz); + } + } + + private int getPermittedInjectionDepth(AnnotatedElement element) { + StrutsParameter annotation = element.getAnnotation(StrutsParameter.class); + return annotation == null ? -1 : annotation.depth(); + } + + private Class ultimateClass(Object target) { + if (proxyService.isProxy(target)) { + return proxyService.ultimateTargetClass(target); + } + return target.getClass(); + } + + private BeanInfo getBeanInfo(Object target) { + Class targetClass = ultimateClass(target); + try { + return ognlUtil.getBeanInfo(targetClass); + } catch (IntrospectionException e) { + LOG.warn("Error introspecting target {} for OGNL allowlisting", targetClass, e); + return null; + } + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java new file mode 100644 index 0000000000..0376da80bc --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java @@ -0,0 +1,156 @@ +/* + * 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 org.apache.struts2.ognl.DefaultOgnlBeanInfoCacheFactory; +import org.apache.struts2.ognl.DefaultOgnlExpressionCacheFactory; +import org.apache.struts2.ognl.OgnlUtil; +import org.apache.struts2.ognl.StrutsOgnlGuard; +import org.apache.struts2.ognl.StrutsProxyCacheFactory; +import org.apache.struts2.ognl.ThreadAllowlist; +import org.apache.struts2.util.StrutsProxyService; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU; +import static org.assertj.core.api.Assertions.assertThat; + +public class OgnlParameterAllowlisterTest { + + private OgnlParameterAllowlister allowlister; + private RecordingThreadAllowlist threadAllowlist; + + @Before + public void setUp() { + threadAllowlist = new RecordingThreadAllowlist(); + allowlister = new OgnlParameterAllowlister(); + var ognlUtil = new OgnlUtil( + new DefaultOgnlExpressionCacheFactory<>(String.valueOf(1000), LRU.toString()), + new DefaultOgnlBeanInfoCacheFactory<>(String.valueOf(1000), LRU.toString()), + new StrutsOgnlGuard()); + allowlister.setOgnlUtil(ognlUtil); + allowlister.setProxyService(new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic"))); + allowlister.setThreadAllowlist(threadAllowlist); + } + + @After + public void tearDown() { + threadAllowlist.clear(); + } + + @Test + public void depthZero_isNoOp() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.allowlistAuthorizedPath("simple", target); + assertThat(threadAllowlist.classes).isEmpty(); + } + + @Test + public void nestedProperty_allowlistsPropertyType() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.allowlistAuthorizedPath("nested.field", target); + assertThat(threadAllowlist.classes).contains(NestedBean.class); + } + + @Test + public void parameterizedReturn_allowlistsTypeArguments() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.allowlistAuthorizedPath("things[0].field", target); + assertThat(threadAllowlist.classes).contains(List.class, NestedBean.class); + } + + @Test + public void publicField_isAllowlistedWhenNoGetter() { + var target = new TargetWithAnnotatedPublicField(); + allowlister.allowlistAuthorizedPath("publicNested.field", target); + assertThat(threadAllowlist.classes).contains(NestedBean.class); + } + + @Test + public void unmatchedRoot_isNoOp() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.allowlistAuthorizedPath("unknownRoot.field", target); + assertThat(threadAllowlist.classes).isEmpty(); + } + + @Test + public void unannotatedNested_isNoOp() { + var target = new TargetWithUnannotatedNested(); + allowlister.allowlistAuthorizedPath("unannotated.field", target); + assertThat(threadAllowlist.classes).isEmpty(); + } + + public static class TargetWithAnnotatedNestedBean { + private NestedBean nested; + private List things; + private Map mapping; + + @StrutsParameter(depth = 1) + public NestedBean getNested() { return nested; } + public void setNested(NestedBean nested) { this.nested = nested; } + + @StrutsParameter(depth = 2) + public List getThings() { return things; } + public void setThings(List things) { this.things = things; } + } + + public static class TargetWithAnnotatedPublicField { + @StrutsParameter(depth = 1) + public NestedBean publicNested; + } + + public static class TargetWithUnannotatedNested { + private NestedBean unannotated; + public NestedBean getUnannotated() { return unannotated; } + public void setUnannotated(NestedBean v) { this.unannotated = v; } + } + + public static class NestedBean { + private String field; + public String getField() { return field; } + public void setField(String f) { this.field = f; } + } + + private static final class RecordingThreadAllowlist extends ThreadAllowlist { + final Set> classes = new HashSet<>(); + + @Override + public void allowClass(Class clazz) { + classes.add(clazz); + super.allowClass(clazz); + } + + @Override + public void allowClassHierarchy(Class clazz) { + classes.add(clazz); + super.allowClassHierarchy(clazz); + } + + void clear() { + classes.clear(); + clearAllowlist(); + } + } +} From 09e3e30bb99bffdf582dbb4e164e794f7f19ddad Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 17:15:57 +0200 Subject: [PATCH 03/10] WW-5627 register ParameterAllowlister bean in struts-default DI --- .../org/apache/struts2/config/StrutsBeanSelectionProvider.java | 2 ++ core/src/main/resources/struts-beans.xml | 3 +++ 2 files changed, 5 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java index f169b67f1c..e3f632eda0 100644 --- a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java @@ -73,6 +73,7 @@ import org.apache.struts2.url.UrlEncoder; import org.apache.struts2.util.ContentTypeMatcher; import org.apache.struts2.util.PatternMatcher; +import org.apache.struts2.interceptor.parameter.ParameterAllowlister; import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.util.ProxyService; import org.apache.struts2.util.TextParser; @@ -448,6 +449,7 @@ public void register(ContainerBuilder builder, LocatableProperties props) { alias(ProxyCacheFactory.class, StrutsConstants.STRUTS_PROXY_CACHE_FACTORY, builder, props, Scope.SINGLETON); alias(ProxyService.class, StrutsConstants.STRUTS_PROXYSERVICE, builder, props, Scope.SINGLETON); alias(ParameterAuthorizer.class, StrutsConstants.STRUTS_PARAMETER_AUTHORIZER, builder, props, Scope.SINGLETON); + alias(ParameterAllowlister.class, StrutsConstants.STRUTS_PARAMETER_ALLOWLISTER, builder, props, Scope.SINGLETON); alias(SecurityMemberAccess.class, StrutsConstants.STRUTS_MEMBER_ACCESS, builder, props, Scope.PROTOTYPE); alias(OgnlGuard.class, StrutsConstants.STRUTS_OGNL_GUARD, builder, props, Scope.SINGLETON); diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 232f0f4a42..21c4ec7e68 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -248,6 +248,9 @@ + + Date: Sat, 9 May 2026 18:52:31 +0200 Subject: [PATCH 04/10] WW-5627 delegate ParametersInterceptor OGNL allowlisting to OgnlParameterAllowlister Also register ParameterAllowlister in DefaultConfiguration bootstrap factories so it is available in test containers (parallel to how ParameterAuthorizer was already registered there). Co-Authored-By: Claude Sonnet 4.6 --- .../config/impl/DefaultConfiguration.java | 3 ++ .../parameter/ParametersInterceptor.java | 43 +++---------------- 2 files changed, 10 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java index 9eb0095928..dcf4f1602e 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java @@ -92,6 +92,8 @@ import org.apache.struts2.ognl.accessor.CompoundRootAccessor; import org.apache.struts2.ognl.accessor.RootAccessor; import org.apache.struts2.ognl.accessor.XWorkMethodAccessor; +import org.apache.struts2.interceptor.parameter.OgnlParameterAllowlister; +import org.apache.struts2.interceptor.parameter.ParameterAllowlister; import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.util.StrutsProxyService; @@ -409,6 +411,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { .factory(ProxyCacheFactory.class, StrutsProxyCacheFactory.class, Scope.SINGLETON) .factory(ProxyService.class, StrutsProxyService.class, Scope.SINGLETON) .factory(ParameterAuthorizer.class, StrutsParameterAuthorizer.class, Scope.SINGLETON) + .factory(ParameterAllowlister.class, OgnlParameterAllowlister.class, Scope.SINGLETON) .factory(OgnlUtil.class, Scope.SINGLETON) .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) 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 5491b585f2..6b7df1ff87 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 @@ -65,10 +65,8 @@ import static java.lang.String.format; 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; import static org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS; -import static org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS_STR; import static org.apache.struts2.util.DebugUtils.logWarningForFirstOccurrence; import static org.apache.struts2.util.DebugUtils.notifyDeveloperOfError; @@ -100,6 +98,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { private Set excludedValuePatterns = null; private Set acceptedValuePatterns = null; private ParameterAuthorizer parameterAuthorizer; + private ParameterAllowlister parameterAllowlister; @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { @@ -126,6 +125,11 @@ public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { this.parameterAuthorizer = parameterAuthorizer; } + @Inject + public void setParameterAllowlister(ParameterAllowlister parameterAllowlister) { + this.parameterAllowlister = parameterAllowlister; + } + @Inject(StrutsConstants.STRUTS_DEVMODE) public void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); @@ -388,40 +392,7 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { * injection and must NOT be shared with other input channels (JSON, REST). */ private void performOgnlAllowlisting(String name, Object target, long paramDepth) { - 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); - - BeanInfo beanInfo = getBeanInfo(target); - if (beanInfo != null) { - Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) - .filter(desc -> desc.getName().equals(normalisedRootProperty)).findFirst(); - if (propDescOpt.isPresent()) { - PropertyDescriptor propDesc = propDescOpt.get(); - Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); - if (relevantMethod != null && getPermittedInjectionDepth(relevantMethod) >= paramDepth) { - allowlistClass(propDesc.getPropertyType()); - if (paramDepth >= 2) { - allowlistReturnTypeIfParameterized(relevantMethod); - } - return; - } - } - } - - // Fallback: check public field - Class targetClass = ultimateClass(target); - try { - Field field = targetClass.getDeclaredField(normalisedRootProperty); - if (Modifier.isPublic(field.getModifiers()) && getPermittedInjectionDepth(field) >= paramDepth) { - allowlistClass(field.getType()); - if (paramDepth >= 2) { - allowlistFieldIfParameterized(field); - } - } - } catch (NoSuchFieldException e) { - // No field to allowlist - } + parameterAllowlister.allowlistAuthorizedPath(name, target); } /** From c9a69f63cf0cf4dc5460934571c0bc6e6f8b0546 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 19:00:07 +0200 Subject: [PATCH 05/10] WW-5627 test(cookie): failing test for unannotated setter skip Co-Authored-By: Claude Sonnet 4.6 --- .../CookieInterceptorAnnotationTest.java | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java diff --git a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java new file mode 100644 index 0000000000..dc965a029b --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java @@ -0,0 +1,92 @@ +/* + * 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; + +import jakarta.servlet.http.Cookie; +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionSupport; +import org.apache.struts2.ServletActionContext; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.action.Action; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; +import org.apache.struts2.interceptor.parameter.StrutsParameter; +import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; +import org.apache.struts2.mock.MockActionInvocation; +import org.springframework.mock.web.MockHttpServletRequest; + +public class CookieInterceptorAnnotationTest extends StrutsInternalTestCase { + + private CookieInterceptor interceptor; + + @Override + protected void setUp() throws Exception { + super.setUp(); + interceptor = container.inject(CookieInterceptor.class); + interceptor.setCookiesName("*"); + } + + @Override + protected void tearDown() throws Exception { + // Reset shared singleton state — flags flipped on the container's StrutsParameterAuthorizer + // would otherwise leak across tests in the same JVM run. + configureRequireAnnotations(false, false); + super.tearDown(); + } + + public void testRequireAnnotations_unannotatedSetter_isSkipped() throws Exception { + configureRequireAnnotations(true, false); + AnnotatedAction action = new AnnotatedAction(); + invokeWithCookies(action, new Cookie("unannotated", "v")); + + assertNull("unannotated setter must not be populated", action.getUnannotated()); + assertNull(ActionContext.getContext().getValueStack().findValue("unannotated")); + } + + private void configureRequireAnnotations(boolean require, boolean transitionMode) { + StrutsParameterAuthorizer authorizer = (StrutsParameterAuthorizer) container.getInstance(ParameterAuthorizer.class); + authorizer.setRequireAnnotations(Boolean.toString(require)); + authorizer.setRequireAnnotationsTransitionMode(Boolean.toString(transitionMode)); + } + + private void invokeWithCookies(Object action, Cookie... cookies) throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setCookies(cookies); + ServletActionContext.setRequest(request); + ActionContext.getContext().getValueStack().push(action); + + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setInvocationContext(ActionContext.getContext()); + invocation.setResultCode(Action.SUCCESS); + + interceptor.intercept(invocation); + } + + public static class AnnotatedAction extends ActionSupport { + private String annotated; + private String unannotated; + + @StrutsParameter + public void setAnnotated(String v) { this.annotated = v; } + public String getAnnotated() { return annotated; } + + public void setUnannotated(String v) { this.unannotated = v; } + public String getUnannotated() { return unannotated; } + } +} From 39b8fe92964325324602384fa0bee85dfa4884e5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 19:13:51 +0200 Subject: [PATCH 06/10] WW-5627 gate CookieInterceptor cookie injection through ParameterAuthorizer Adds a 5-arg `populateCookieValueIntoStack(name, value, map, stack, action)` hook that runs cookie writes through `ParameterAuthorizer.isAuthorized` and primes `ThreadAllowlist` via `ParameterAllowlister` for nested paths, then delegates to the legacy 4-arg form. The 4-arg form is `@Deprecated(since="7.2.0")` but its body is unchanged, so existing subclass overrides automatically receive only authorized cookies. Default-config behavior is preserved because the authorizer short-circuits when `requireAnnotations=false`. Existing `CookieInterceptorTest` instantiates `new CookieInterceptor()` rather than going through the container, leaving the new injected fields null. Wires explicit pass-through lambdas through a `disableAuthorizationGate(...)` helper so those tests continue to exercise default-config behavior. --- .../interceptor/CookieInterceptor.java | 50 +++++++++++++++++-- .../interceptor/CookieInterceptorTest.java | 18 +++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index 6ca7ecb0ad..7a3d7fa292 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java @@ -26,6 +26,8 @@ import org.apache.struts2.ServletActionContext; import org.apache.struts2.action.CookiesAware; import org.apache.struts2.inject.Inject; +import org.apache.struts2.interceptor.parameter.ParameterAllowlister; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.security.AcceptedPatternsChecker; import org.apache.struts2.security.ExcludedPatternsChecker; import org.apache.struts2.util.TextParseUtil; @@ -187,6 +189,8 @@ public class CookieInterceptor extends AbstractInterceptor { private ExcludedPatternsChecker excludedPatternsChecker; private AcceptedPatternsChecker acceptedPatternsChecker; + private ParameterAuthorizer parameterAuthorizer; + private ParameterAllowlister parameterAllowlister; @Inject public void setExcludedPatternsChecker(ExcludedPatternsChecker excludedPatternsChecker) { @@ -199,6 +203,16 @@ public void setAcceptedPatternsChecker(AcceptedPatternsChecker acceptedPatternsC this.acceptedPatternsChecker.setAcceptedPatterns(ACCEPTED_PATTERN); } + @Inject + public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { + this.parameterAuthorizer = parameterAuthorizer; + } + + @Inject + public void setParameterAllowlister(ParameterAllowlister parameterAllowlister) { + this.parameterAllowlister = parameterAllowlister; + } + /** * @param cookiesName the cookiesName which if matched will allow the cookie * to be injected into action, could be comma-separated string. @@ -234,6 +248,8 @@ public void setAcceptCookieNames(String commaDelimitedPattern) { public String intercept(ActionInvocation invocation) throws Exception { LOG.debug("start interception"); + final Object action = invocation.getAction(); + // contains selected cookies final Map cookiesMap = new LinkedHashMap<>(); @@ -248,9 +264,9 @@ public String intercept(ActionInvocation invocation) throws Exception { if (isAcceptableName(name)) { if (cookiesNameSet.contains("*")) { LOG.debug("Contains cookie name [*] in configured cookies name set, cookie with name [{}] with value [{}] will be injected", name, value); - populateCookieValueIntoStack(name, value, cookiesMap, stack); + populateCookieValueIntoStack(name, value, cookiesMap, stack, action); } else if (cookiesNameSet.contains(cookie.getName())) { - populateCookieValueIntoStack(name, value, cookiesMap, stack); + populateCookieValueIntoStack(name, value, cookiesMap, stack, action); } } else { LOG.warn("Cookie name [{}] with value [{}] was rejected!", name, value); @@ -259,7 +275,7 @@ public String intercept(ActionInvocation invocation) throws Exception { } // inject the cookiesMap, even if we don't have any cookies - injectIntoCookiesAwareAction(invocation.getAction(), cookiesMap); + injectIntoCookiesAwareAction(action, cookiesMap); return invocation.invoke(); } @@ -314,6 +330,29 @@ protected boolean isExcluded(String name) { return false; } + /** + * Authorizes the cookie against {@link ParameterAuthorizer}, primes OGNL allowlist for any nested path via + * {@link ParameterAllowlister}, then delegates to the legacy {@link #populateCookieValueIntoStack(String, String, + * Map, ValueStack)} hook so existing subclass overrides continue to participate. Override this method to customize + * the authorization behavior itself. + * + * @param cookieName cookie name (potentially an OGNL path; {@code ACCEPTED_PATTERN} restricts the character set) + * @param cookieValue cookie value + * @param cookiesMap map of cookies populated for {@link org.apache.struts2.action.CookiesAware} + * @param stack current request value stack + * @param action the action instance from {@link ActionInvocation#getAction()}; used for {@code @StrutsParameter} target resolution + * @since 7.2.0 + */ + protected void populateCookieValueIntoStack(String cookieName, String cookieValue, Map cookiesMap, ValueStack stack, Object action) { + Object target = parameterAuthorizer.resolveTarget(action); + if (!parameterAuthorizer.isAuthorized(cookieName, target, action)) { + LOG.debug("Cookie [{}] rejected by @StrutsParameter authorization on target [{}]", cookieName, target.getClass().getSimpleName()); + return; + } + parameterAllowlister.allowlistAuthorizedPath(cookieName, target); + populateCookieValueIntoStack(cookieName, cookieValue, cookiesMap, stack); + } + /** * Hook that populate cookie value into value stack (hence the action) * if the criteria is satisfied (if the cookie value matches with those configured). @@ -322,7 +361,12 @@ protected boolean isExcluded(String name) { * @param cookieValue cookie value * @param cookiesMap map of cookies * @param stack value stack + * @deprecated since 7.2.0. Override + * {@link #populateCookieValueIntoStack(String, String, Map, ValueStack, Object)} instead so cookie writes are + * authorized by {@link ParameterAuthorizer}. The default 5-arg implementation calls this method after the + * authorization gate, so existing overrides continue to receive only authorized cookies. */ + @Deprecated(since = "7.2.0") protected void populateCookieValueIntoStack(String cookieName, String cookieValue, Map cookiesMap, ValueStack stack) { if (cookiesValueSet.isEmpty() || cookiesValueSet.contains("*")) { // If the interceptor is configured to accept any cookie value diff --git a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java index 8189f5ce8e..8bcfe704c9 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java @@ -43,6 +43,15 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { + /** + * These tests construct {@link CookieInterceptor} via {@code new} rather than the DI container, so the + * {@code @StrutsParameter} authorization gate added in WW-5627 has no injected services. We supply explicit + * pass-through lambdas to mirror the default-config behavior these tests assume ({@code requireAnnotations=false}). + */ + private static void disableAuthorizationGate(CookieInterceptor interceptor) { + interceptor.setParameterAuthorizer((name, target, action) -> true); + interceptor.setParameterAllowlister((name, target) -> {}); + } public void testIntercepDefault() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest(); @@ -68,6 +77,7 @@ public void testIntercepDefault() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.intercept(invocation); @@ -105,6 +115,7 @@ public void testInterceptAll1() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("*"); interceptor.setCookiesValue("*"); interceptor.intercept(invocation); @@ -147,6 +158,7 @@ public void testInterceptAll2() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("cookie1, cookie2, cookie3"); interceptor.setCookiesValue("cookie1value, cookie2value, cookie3value"); interceptor.intercept(invocation); @@ -188,6 +200,7 @@ public void testInterceptSelectedCookiesNameOnly1() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("cookie1value, cookie2value, cookie3value"); interceptor.intercept(invocation); @@ -230,6 +243,7 @@ public void testInterceptSelectedCookiesNameOnly2() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("*"); interceptor.intercept(invocation); @@ -271,6 +285,7 @@ public void testInterceptSelectedCookiesNameOnly3() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue(""); interceptor.intercept(invocation); @@ -313,6 +328,7 @@ public void testInterceptSelectedCookiesNameAndValue() throws Exception { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("cookie1, cookie3"); interceptor.setCookiesValue("cookie1value"); interceptor.intercept(invocation); @@ -395,6 +411,7 @@ protected boolean isAcceptableName(String name) { excludedPatternsChecker.setAdditionalExcludePatterns(".*(^|\\.|\\[|'|\")class(\\.|\\[|'|\").*"); interceptor.setExcludedPatternsChecker(excludedPatternsChecker); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); @@ -441,6 +458,7 @@ protected boolean isAcceptableName(String name) { }; interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); From cea95636907c0fa852731a93f3f0dc5ec5fdd3ab Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 19:15:49 +0200 Subject: [PATCH 07/10] WW-5627 cover CookieInterceptor authorization matrix in CookieInterceptorAnnotationTest --- .../CookieInterceptorAnnotationTest.java | 127 ++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java index dc965a029b..4d794ecbf4 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.java @@ -21,6 +21,7 @@ import jakarta.servlet.http.Cookie; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionSupport; +import org.apache.struts2.ModelDriven; import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.action.Action; @@ -28,8 +29,12 @@ import org.apache.struts2.interceptor.parameter.StrutsParameter; import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; import org.apache.struts2.mock.MockActionInvocation; +import org.apache.struts2.util.ValueStack; import org.springframework.mock.web.MockHttpServletRequest; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + public class CookieInterceptorAnnotationTest extends StrutsInternalTestCase { private CookieInterceptor interceptor; @@ -58,6 +63,101 @@ public void testRequireAnnotations_unannotatedSetter_isSkipped() throws Exceptio assertNull(ActionContext.getContext().getValueStack().findValue("unannotated")); } + public void testRequireAnnotations_annotatedSetter_isInjected() throws Exception { + configureRequireAnnotations(true, false); + AnnotatedAction action = new AnnotatedAction(); + invokeWithCookies(action, new Cookie("annotated", "v")); + + assertEquals("v", action.getAnnotated()); + assertEquals("v", ActionContext.getContext().getValueStack().findValue("annotated")); + } + + public void testRequireAnnotations_annotatedNestedPath_isInjected() throws Exception { + configureRequireAnnotations(true, false); + AnnotatedAction action = new AnnotatedAction(); + action.setNested(new NestedBean()); + invokeWithCookies(action, new Cookie("nested.field", "v")); + + assertEquals("v", action.getNested().getField()); + } + + public void testRequireAnnotations_unannotatedNestedPath_isSkipped() throws Exception { + configureRequireAnnotations(true, false); + AnnotatedAction action = new AnnotatedAction(); + action.setUnannotatedNested(new NestedBean()); + invokeWithCookies(action, new Cookie("unannotatedNested.field", "v")); + + assertNull(action.getUnannotatedNested().getField()); + } + + public void testRequireAnnotations_transitionMode_exemptsDepthZero() throws Exception { + configureRequireAnnotations(true, true); + AnnotatedAction action = new AnnotatedAction(); + invokeWithCookies(action, new Cookie("unannotated", "v")); + + assertEquals("v", action.getUnannotated()); + } + + public void testDefaultConfig_unannotatedSetter_stillInjected() throws Exception { + configureRequireAnnotations(false, false); + AnnotatedAction action = new AnnotatedAction(); + invokeWithCookies(action, new Cookie("unannotated", "v")); + + assertEquals("v", action.getUnannotated()); + } + + public void testRequireAnnotations_modelDriven_exemptsModel() throws Exception { + configureRequireAnnotations(true, false); + ModelDrivenAction action = new ModelDrivenAction(); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setCookies(new Cookie("name", "v")); + ServletActionContext.setRequest(request); + // ModelDriven contract: the model is pushed on top of the action. + ActionContext.getContext().getValueStack().push(action); + ActionContext.getContext().getValueStack().push(action.getModel()); + + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setInvocationContext(ActionContext.getContext()); + invocation.setResultCode(Action.SUCCESS); + + interceptor.intercept(invocation); + + assertEquals("v", action.getModel().getName()); + } + + public void testSubclassOverridingDeprecatedHook_stillSeesAuthorizationGate() throws Exception { + configureRequireAnnotations(true, false); + AtomicInteger calls = new AtomicInteger(); + @SuppressWarnings("deprecation") + CookieInterceptor subclass = new CookieInterceptor() { + @Override + protected void populateCookieValueIntoStack(String name, String value, Map map, ValueStack stack) { + calls.incrementAndGet(); + super.populateCookieValueIntoStack(name, value, map, stack); + } + }; + container.inject(subclass); + subclass.setCookiesName("*"); + + AnnotatedAction action = new AnnotatedAction(); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setCookies(new Cookie("annotated", "ok"), new Cookie("unannotated", "blocked")); + ServletActionContext.setRequest(req); + ActionContext.getContext().getValueStack().push(action); + + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setInvocationContext(ActionContext.getContext()); + invocation.setResultCode(Action.SUCCESS); + subclass.intercept(invocation); + + assertEquals("ok", action.getAnnotated()); + assertNull(action.getUnannotated()); + assertEquals("4-arg hook should be invoked exactly once (only for the authorized cookie)", 1, calls.get()); + } + private void configureRequireAnnotations(boolean require, boolean transitionMode) { StrutsParameterAuthorizer authorizer = (StrutsParameterAuthorizer) container.getInstance(ParameterAuthorizer.class); authorizer.setRequireAnnotations(Boolean.toString(require)); @@ -81,6 +181,8 @@ private void invokeWithCookies(Object action, Cookie... cookies) throws Exceptio public static class AnnotatedAction extends ActionSupport { private String annotated; private String unannotated; + private NestedBean nested; + private NestedBean unannotatedNested; @StrutsParameter public void setAnnotated(String v) { this.annotated = v; } @@ -88,5 +190,30 @@ public static class AnnotatedAction extends ActionSupport { public void setUnannotated(String v) { this.unannotated = v; } public String getUnannotated() { return unannotated; } + + @StrutsParameter(depth = 1) + public NestedBean getNested() { return nested; } + public void setNested(NestedBean nested) { this.nested = nested; } + + public NestedBean getUnannotatedNested() { return unannotatedNested; } + public void setUnannotatedNested(NestedBean v) { this.unannotatedNested = v; } + } + + public static class NestedBean { + private String field; + public String getField() { return field; } + public void setField(String f) { this.field = f; } + } + + public static class ModelDrivenAction extends ActionSupport implements ModelDriven { + private final Model model = new Model(); + @Override + public Model getModel() { return model; } + } + + public static class Model { + private String name; + public String getName() { return name; } + public void setName(String n) { this.name = n; } } } From ee23865e6a545d76e9d9a67954788243ab2c36dc Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 19:16:19 +0200 Subject: [PATCH 08/10] WW-5627 docs(cookie): document new 5-arg extension hook and deprecation --- .../struts2/interceptor/CookieInterceptor.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index 7a3d7fa292..78cc84597b 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java @@ -101,8 +101,16 @@ * *
    *
  • - * populateCookieValueIntoStack - this method will decide if this cookie value is qualified - * to be populated into the value stack (hence into the action itself) + * populateCookieValueIntoStack(name, value, map, stack, action) - the preferred extension point + * since 7.2.0. The default implementation gates the cookie write through + * {@link org.apache.struts2.interceptor.parameter.ParameterAuthorizer} and primes the OGNL allowlist via + * {@link org.apache.struts2.interceptor.parameter.ParameterAllowlister} before delegating to the legacy + * 4-arg {@code populateCookieValueIntoStack}. Override here to customize the authorization behavior itself. + *
    • + *
  • + * populateCookieValueIntoStack(name, value, map, stack) - deprecated since 7.2.0. The legacy + * hook that performs the actual {@code stack.setValue}. Existing overrides continue to work and + * automatically receive only authorized cookies via the 5-arg default. *
    • *
  • * injectIntoCookiesAwareAction - this method will inject selected cookies (as a java.util.Map) From 87803213f148405c97c0f5f66b5e25cb02ad0cd3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 19:19:22 +0200 Subject: [PATCH 09/10] WW-5627 wire OgnlParameterAllowlister in StrutsParameterAnnotationTest fixture --- .../parameter/StrutsParameterAnnotationTest.java | 6 ++++++ 1 file changed, 6 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 803e0dad5a..8ec445253c 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 @@ -84,6 +84,12 @@ public void setUp() throws Exception { this.parameterAuthorizer = parameterAuthorizer; parametersInterceptor.setParameterAuthorizer(parameterAuthorizer); + var parameterAllowlister = new OgnlParameterAllowlister(); + parameterAllowlister.setOgnlUtil(ognlUtil); + parameterAllowlister.setProxyService(proxyService); + parameterAllowlister.setThreadAllowlist(threadAllowlist); + parametersInterceptor.setParameterAllowlister(parameterAllowlister); + NotExcludedAcceptedPatternsChecker checker = mock(NotExcludedAcceptedPatternsChecker.class); when(checker.isAccepted(anyString())).thenReturn(IsAccepted.yes("")); when(checker.isExcluded(anyString())).thenReturn(IsExcluded.no(Set.of())); From 73f456b8db7cf24a09e61bba22b17c2859724dc9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 May 2026 19:44:40 +0200 Subject: [PATCH 10/10] WW-5627 address SonarCloud findings on PR #1681 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - S1948: mark transient on the new ParameterAuthorizer/ParameterAllowlister fields in CookieInterceptor and ParametersInterceptor (the host classes are Serializable; the injected services are not). - S1874: suppress the deprecation warning on the new 5-arg populateCookieValueIntoStack — the delegation to the deprecated 4-arg form is the contract that lets existing subclass overrides participate. - S3776: extract `allowlistViaPropertyDescriptor` and `allowlistViaPublicField` from `OgnlParameterAllowlister.allowlistAuthorizedPath` to drop cognitive complexity below the threshold. - S1068: remove the unused `mapping` test fixture field. --- .../interceptor/CookieInterceptor.java | 5 +- .../parameter/OgnlParameterAllowlister.java | 58 ++++++++++++------- .../parameter/ParametersInterceptor.java | 2 +- .../OgnlParameterAllowlisterTest.java | 2 - 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index 78cc84597b..aa64523959 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java @@ -197,8 +197,8 @@ public class CookieInterceptor extends AbstractInterceptor { private ExcludedPatternsChecker excludedPatternsChecker; private AcceptedPatternsChecker acceptedPatternsChecker; - private ParameterAuthorizer parameterAuthorizer; - private ParameterAllowlister parameterAllowlister; + private transient ParameterAuthorizer parameterAuthorizer; + private transient ParameterAllowlister parameterAllowlister; @Inject public void setExcludedPatternsChecker(ExcludedPatternsChecker excludedPatternsChecker) { @@ -351,6 +351,7 @@ protected boolean isExcluded(String name) { * @param action the action instance from {@link ActionInvocation#getAction()}; used for {@code @StrutsParameter} target resolution * @since 7.2.0 */ + @SuppressWarnings("deprecation") // intentional: delegating to the deprecated 4-arg form is the contract that lets existing subclass overrides participate protected void populateCookieValueIntoStack(String cookieName, String cookieValue, Map cookiesMap, ValueStack stack, Object action) { Object target = parameterAuthorizer.resolveTarget(action); if (!parameterAuthorizer.isAuthorized(cookieName, target, action)) { diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java index c38015cfa8..51a37cfc13 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java @@ -87,34 +87,48 @@ public void allowlistAuthorizedPath(String parameterName, Object target) { String rootProperty = nestingIndex == -1 ? parameterName : parameterName.substring(0, nestingIndex); String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1); + if (allowlistViaPropertyDescriptor(target, normalisedRootProperty, paramDepth)) { + return; + } + allowlistViaPublicField(target, normalisedRootProperty, paramDepth); + } + + private boolean allowlistViaPropertyDescriptor(Object target, String rootProperty, long paramDepth) { BeanInfo beanInfo = getBeanInfo(target); - if (beanInfo != null) { - Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) - .filter(desc -> desc.getName().equals(normalisedRootProperty)).findFirst(); - if (propDescOpt.isPresent()) { - PropertyDescriptor propDesc = propDescOpt.get(); - Method relevantMethod = propDesc.getReadMethod(); - if (relevantMethod != null && getPermittedInjectionDepth(relevantMethod) >= paramDepth) { - allowlistClass(propDesc.getPropertyType()); - if (paramDepth >= 2) { - allowlistParameterizedTypeArg(relevantMethod.getGenericReturnType()); - } - return; - } - } + if (beanInfo == null) { + return false; + } + Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) + .filter(desc -> desc.getName().equals(rootProperty)).findFirst(); + if (propDescOpt.isEmpty()) { + return false; + } + PropertyDescriptor propDesc = propDescOpt.get(); + Method relevantMethod = propDesc.getReadMethod(); + if (relevantMethod == null || getPermittedInjectionDepth(relevantMethod) < paramDepth) { + return false; } + allowlistClass(propDesc.getPropertyType()); + if (paramDepth >= 2) { + allowlistParameterizedTypeArg(relevantMethod.getGenericReturnType()); + } + return true; + } + private void allowlistViaPublicField(Object target, String rootProperty, long paramDepth) { Class targetClass = ultimateClass(target); + Field field; try { - Field field = targetClass.getDeclaredField(normalisedRootProperty); - if (Modifier.isPublic(field.getModifiers()) && getPermittedInjectionDepth(field) >= paramDepth) { - allowlistClass(field.getType()); - if (paramDepth >= 2) { - allowlistParameterizedTypeArg(field.getGenericType()); - } - } + field = targetClass.getDeclaredField(rootProperty); } catch (NoSuchFieldException e) { - // No public field by that name — nothing to allowlist. + return; + } + if (!Modifier.isPublic(field.getModifiers()) || getPermittedInjectionDepth(field) < paramDepth) { + return; + } + allowlistClass(field.getType()); + if (paramDepth >= 2) { + allowlistParameterizedTypeArg(field.getGenericType()); } } 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 6b7df1ff87..3146cd455c 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 @@ -98,7 +98,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { private Set excludedValuePatterns = null; private Set acceptedValuePatterns = null; private ParameterAuthorizer parameterAuthorizer; - private ParameterAllowlister parameterAllowlister; + private transient ParameterAllowlister parameterAllowlister; @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java index 0376da80bc..f02ee1c420 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java @@ -31,7 +31,6 @@ import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU; @@ -105,7 +104,6 @@ public void unannotatedNested_isNoOp() { public static class TargetWithAnnotatedNestedBean { private NestedBean nested; private List things; - private Map mapping; @StrutsParameter(depth = 1) public NestedBean getNested() { return nested; }