From e83487f0e19f4000cf46a88fc6f06bd549ad9d7b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 18 May 2026 13:52:17 +0200 Subject: [PATCH] WW-5627 Gate CookieInterceptor through ParameterAuthorizer (#1681) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * WW-5627 add ParameterAllowlister interface and STRUTS_PARAMETER_ALLOWLISTER constant Co-Authored-By: Claude Sonnet 4.6 * WW-5627 add OgnlParameterAllowlister default implementation Co-Authored-By: Claude Sonnet 4.6 * WW-5627 register ParameterAllowlister bean in struts-default DI * 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 * WW-5627 test(cookie): failing test for unannotated setter skip Co-Authored-By: Claude Sonnet 4.6 * 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. * WW-5627 cover CookieInterceptor authorization matrix in CookieInterceptorAnnotationTest * WW-5627 docs(cookie): document new 5-arg extension hook and deprecation * WW-5627 wire OgnlParameterAllowlister in StrutsParameterAnnotationTest fixture * WW-5627 address SonarCloud findings on PR #1681 - 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. * WW-5627 clarify ParameterAllowlister contract and tidy ParametersInterceptor Rename `ParameterAllowlister#allowlistAuthorizedPath` to `primeAllowlistForPath` to make the contract explicit: the SAM is a side-effect-only priming hook that runs after `ParameterAuthorizer#isAuthorized` has already decided. A no-op return means "no priming needed or possible", never "rejected". The interface name stays channel-agnostic; only the impl class (`OgnlParameterAllowlister`) binds the priming to OGNL's `ThreadAllowlist`. Add a `LOG.debug` in `OgnlParameterAllowlister` for the case where authorization passed but no `@StrutsParameter` could be located on the root property (e.g. `ModelDriven` models without per-property annotations) so the authorize-vs-prime gap is observable instead of surfacing later as an opaque OGNL traversal failure. Drop the dead `performOgnlAllowlisting` pass-through and its unused `paramDepth` parameter from `ParametersInterceptor` — the depth check is already enforced inside `OgnlParameterAllowlister.primeAllowlistForPath`, so the outer guard was a redundant computation. No behavior change. --------- Co-authored-by: Claude Sonnet 4.6 --- .../org/apache/struts2/StrutsConstants.java | 8 + .../config/StrutsBeanSelectionProvider.java | 2 + .../config/impl/DefaultConfiguration.java | 3 + .../interceptor/CookieInterceptor.java | 63 ++++- .../parameter/OgnlParameterAllowlister.java | 192 +++++++++++++++ .../parameter/ParameterAllowlister.java | 44 ++++ .../parameter/ParametersInterceptor.java | 56 +---- core/src/main/resources/struts-beans.xml | 3 + .../CookieInterceptorAnnotationTest.java | 219 ++++++++++++++++++ .../interceptor/CookieInterceptorTest.java | 18 ++ .../OgnlParameterAllowlisterTest.java | 154 ++++++++++++ .../StrutsParameterAnnotationTest.java | 6 + 12 files changed, 714 insertions(+), 54 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorAnnotationTest.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/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index eb925b422..13fc2d3d2 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/config/StrutsBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java index f169b67f1..e3f632eda 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.UrlDecoder; 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 class StrutsBeanSelectionProvider extends AbstractBeanSelectionProvider { 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/java/org/apache/struts2/config/impl/DefaultConfiguration.java b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java index 9eb009592..dcf4f1602 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.SecurityMemberAccess; 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 class DefaultConfiguration implements Configuration { .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/CookieInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java index 6ca7ecb0a..4d9685543 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.ActionInvocation; 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; @@ -99,8 +101,16 @@ import java.util.Set; * *
    *
  • - * 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) @@ -187,6 +197,8 @@ public class CookieInterceptor extends AbstractInterceptor { private ExcludedPatternsChecker excludedPatternsChecker; private AcceptedPatternsChecker acceptedPatternsChecker; + private transient ParameterAuthorizer parameterAuthorizer; + private transient ParameterAllowlister parameterAllowlister; @Inject public void setExcludedPatternsChecker(ExcludedPatternsChecker excludedPatternsChecker) { @@ -199,6 +211,16 @@ public class CookieInterceptor extends AbstractInterceptor { 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 +256,8 @@ public class CookieInterceptor extends AbstractInterceptor { 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 +272,9 @@ public class CookieInterceptor extends AbstractInterceptor { 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 +283,7 @@ public class CookieInterceptor extends AbstractInterceptor { } // inject the cookiesMap, even if we don't have any cookies - injectIntoCookiesAwareAction(invocation.getAction(), cookiesMap); + injectIntoCookiesAwareAction(action, cookiesMap); return invocation.invoke(); } @@ -314,6 +338,30 @@ public class CookieInterceptor extends AbstractInterceptor { 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 + */ + @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)) { + LOG.debug("Cookie [{}] rejected by @StrutsParameter authorization on target [{}]", cookieName, target.getClass().getSimpleName()); + return; + } + parameterAllowlister.primeAllowlistForPath(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 +370,12 @@ public class CookieInterceptor extends AbstractInterceptor { * @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/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 000000000..0a24d8577 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java @@ -0,0 +1,192 @@ +/* + * 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}. Registers the root property's class (and generic type args for {@code depth >= 2}) + * into the OGNL {@link ThreadAllowlist} so OGNL may introspect and traverse a nested path on the value stack. Logic is + * extracted verbatim from {@code ParametersInterceptor.performOgnlAllowlisting} so the OGNL parameter and cookie + * channels share a single implementation. + * + *

    No-ops when: + *

      + *
    • {@code paramDepth == 0} — shallow setter; OGNL does not need to traverse
      • + *
    • the root property has no {@code @StrutsParameter} annotation reachable via {@link java.beans.PropertyDescriptor} + * or as a public field (e.g. a {@code ModelDriven} model whose properties are not individually annotated). A + * {@code LOG.debug} surfaces this case so the gap between authorization and OGNL traversal is observable.
      • + *
    + * + * @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 primeAllowlistForPath(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); + + if (allowlistViaPropertyDescriptor(target, normalisedRootProperty, paramDepth)) { + return; + } + if (allowlistViaPublicField(target, normalisedRootProperty, paramDepth)) { + return; + } + // Authorization passed but no @StrutsParameter on the root property — e.g. ModelDriven model with no + // per-property annotations. OGNL won't be able to walk this nested path; surface the gap in logs. + LOG.debug("Parameter [{}] authorized but no @StrutsParameter on root property [{}] of [{}]; " + + "OGNL allowlist not primed and nested traversal may be blocked", + parameterName, normalisedRootProperty, ultimateClass(target).getSimpleName()); + } + + private boolean allowlistViaPropertyDescriptor(Object target, String rootProperty, long paramDepth) { + BeanInfo beanInfo = getBeanInfo(target); + 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 boolean allowlistViaPublicField(Object target, String rootProperty, long paramDepth) { + Class targetClass = ultimateClass(target); + Field field; + try { + field = targetClass.getDeclaredField(rootProperty); + } catch (NoSuchFieldException e) { + return false; + } + if (!Modifier.isPublic(field.getModifiers()) || getPermittedInjectionDepth(field) < paramDepth) { + return false; + } + allowlistClass(field.getType()); + if (paramDepth >= 2) { + allowlistParameterizedTypeArg(field.getGenericType()); + } + return true; + } + + 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/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 000000000..846498c71 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.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; + +/** + * Primes channel-specific runtime state required for an already-authorized parameter path to be walked by the + * value-stack — for example, registering the path's classes into the OGNL {@link org.apache.struts2.ognl.ThreadAllowlist} + * so OGNL may traverse them. Separated from {@link ParameterAuthorizer} so the authorization decision can remain + * side-effect-free and reusable from non-OGNL channels (Jackson, Juneau). + * + *

    Implementations MUST NOT repeat the authorization decision — that is owned by + * {@link ParameterAuthorizer#isAuthorized}. A no-op return (e.g. shallow paths, unannotated root) means "no priming + * needed or possible" and never "rejected": callers must not treat the absence of priming as a negative authorization + * signal.

    + * + * @since 7.2.0 + */ +public interface ParameterAllowlister { + + /** + * Primes the channel-specific allowlist for an authorized parameter path. Side-effect-only; no return value + * because a no-op is a valid outcome (see class-level javadoc). + * + * @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 primeAllowlistForPath(String parameterName, Object target); +} 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 5491b585f..365868a2b 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,7 @@ import java.util.regex.Pattern; 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 +97,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { private Set excludedValuePatterns = null; private Set acceptedValuePatterns = null; private ParameterAuthorizer parameterAuthorizer; + private transient ParameterAllowlister parameterAllowlister; @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { @@ -126,6 +124,11 @@ public class ParametersInterceptor extends MethodFilterInterceptor { 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); @@ -375,55 +378,10 @@ public class ParametersInterceptor extends MethodFilterInterceptor { return false; } - // OGNL-specific allowlisting: only needed for nested params (depth >= 1) - long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); - if (paramDepth >= 1) { - performOgnlAllowlisting(name, target, paramDepth); - } + parameterAllowlister.primeAllowlistForPath(name, target); return true; } - /** - * Performs OGNL ThreadAllowlist side effects for an authorized parameter. This is specific to OGNL-based parameter - * 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 - } - } - /** * 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 diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 232f0f4a4..21c4ec7e6 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -248,6 +248,9 @@ + + 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)); + 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; + private NestedBean nested; + private NestedBean unannotatedNested; + + @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; } + + @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; } + } +} 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 8189f5ce8..8bcfe704c 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 @@ import static org.easymock.EasyMock.verify; 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 class CookieInterceptorTest extends StrutsInternalTestCase { CookieInterceptor interceptor = new CookieInterceptor(); interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.intercept(invocation); @@ -105,6 +115,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { 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 class CookieInterceptorTest extends StrutsInternalTestCase { 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 class CookieInterceptorTest extends StrutsInternalTestCase { 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 class CookieInterceptorTest extends StrutsInternalTestCase { 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 class CookieInterceptorTest extends StrutsInternalTestCase { 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 class CookieInterceptorTest extends StrutsInternalTestCase { 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 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { excludedPatternsChecker.setAdditionalExcludePatterns(".*(^|\\.|\\[|'|\")class(\\.|\\[|'|\").*"); interceptor.setExcludedPatternsChecker(excludedPatternsChecker); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); @@ -441,6 +458,7 @@ public class CookieInterceptorTest extends StrutsInternalTestCase { }; interceptor.setExcludedPatternsChecker(new DefaultExcludedPatternsChecker()); interceptor.setAcceptedPatternsChecker(new DefaultAcceptedPatternsChecker()); + disableAuthorizationGate(interceptor); interceptor.setCookiesName("*"); MockActionInvocation invocation = new MockActionInvocation(); 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 000000000..7066a9bee --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlisterTest.java @@ -0,0 +1,154 @@ +/* + * 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.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.primeAllowlistForPath("simple", target); + assertThat(threadAllowlist.classes).isEmpty(); + } + + @Test + public void nestedProperty_allowlistsPropertyType() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.primeAllowlistForPath("nested.field", target); + assertThat(threadAllowlist.classes).contains(NestedBean.class); + } + + @Test + public void parameterizedReturn_allowlistsTypeArguments() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.primeAllowlistForPath("things[0].field", target); + assertThat(threadAllowlist.classes).contains(List.class, NestedBean.class); + } + + @Test + public void publicField_isAllowlistedWhenNoGetter() { + var target = new TargetWithAnnotatedPublicField(); + allowlister.primeAllowlistForPath("publicNested.field", target); + assertThat(threadAllowlist.classes).contains(NestedBean.class); + } + + @Test + public void unmatchedRoot_isNoOp() { + var target = new TargetWithAnnotatedNestedBean(); + allowlister.primeAllowlistForPath("unknownRoot.field", target); + assertThat(threadAllowlist.classes).isEmpty(); + } + + @Test + public void unannotatedNested_isNoOp() { + var target = new TargetWithUnannotatedNested(); + allowlister.primeAllowlistForPath("unannotated.field", target); + assertThat(threadAllowlist.classes).isEmpty(); + } + + public static class TargetWithAnnotatedNestedBean { + private NestedBean nested; + private List things; + + @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(); + } + } +} 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 803e0dad5..8ec445253 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 class StrutsParameterAnnotationTest { 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()));