WW-5627 Gate CookieInterceptor through ParameterAuthorizer (#1681)

* WW-5627 add ParameterAllowlister interface and STRUTS_PARAMETER_ALLOWLISTER constant

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* WW-5627 add OgnlParameterAllowlister default implementation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* WW-5627 test(cookie): failing test for unannotated setter skip

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
2026-05-18 13:52:17 +02:00
committed by GitHub
parent 3432433e38
commit e83487f0e1
12 changed files with 714 additions and 54 deletions
@@ -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
*
@@ -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);
@@ -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)
@@ -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;
*
* <ul>
* <li>
* 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.
* </li>
* <li>
* populateCookieValueIntoStack(name, value, map, stack) - <em>deprecated since 7.2.0</em>. 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.
* </li>
* <li>
* 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 <code>cookiesName</code> 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<String, String> 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<String, String> 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<String, String> cookiesMap, ValueStack stack) {
if (cookiesValueSet.isEmpty() || cookiesValueSet.contains("*")) {
// If the interceptor is configured to accept any cookie value
@@ -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.
*
* <p>No-ops when:
* <ul>
* <li>{@code paramDepth == 0} — shallow setter; OGNL does not need to traverse</li>
* <li>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.</li>
* </ul>
*
* @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<PropertyDescriptor> 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;
}
}
}
@@ -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).
*
* <p>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.</p>
*
* @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);
}
@@ -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<Pattern> excludedValuePatterns = null;
private Set<Pattern> 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<PropertyDescriptor> 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
+3
View File
@@ -248,6 +248,9 @@
<bean type="org.apache.struts2.interceptor.parameter.ParameterAuthorizer" name="struts"
class="org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer" scope="singleton"/>
<bean type="org.apache.struts2.interceptor.parameter.ParameterAllowlister" name="struts"
class="org.apache.struts2.interceptor.parameter.OgnlParameterAllowlister" scope="singleton"/>
<bean type="org.apache.struts2.url.QueryStringBuilder" name="strutsQueryStringBuilder"
class="org.apache.struts2.url.StrutsQueryStringBuilder" scope="singleton"/>
<bean type="org.apache.struts2.url.QueryStringParser" name="strutsQueryStringParser"
@@ -0,0 +1,219 @@
/*
* 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.ModelDriven;
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.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;
@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"));
}
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<String, String> 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<Model> {
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; }
}
}
@@ -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();
@@ -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<NestedBean> things;
@StrutsParameter(depth = 1)
public NestedBean getNested() { return nested; }
public void setNested(NestedBean nested) { this.nested = nested; }
@StrutsParameter(depth = 2)
public List<NestedBean> getThings() { return things; }
public void setThings(List<NestedBean> 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<Class<?>> 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();
}
}
}
@@ -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()));