[
https://issues.apache.org/jira/browse/WW-5675?focusedWorklogId=1035825&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1035825
]
ASF GitHub Bot logged work on WW-5675:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Aug/26 15:09
Start Date: 14/Aug/26 15:09
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #1847:
URL: https://github.com/apache/struts/pull/1847#discussion_r3784928094
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.ognl;
+
+import org.apache.commons.lang3.BooleanUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.StrutsConstants;
+import org.apache.struts2.inject.Inject;
+import org.apache.struts2.inject.Initializable;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.regex.Pattern;
+
+import static java.util.Collections.emptySet;
+import static java.util.Collections.unmodifiableSet;
+import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
+import static
org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES;
+import static org.apache.struts2.util.ConfigParseUtil.toClassObjectsSet;
+import static org.apache.struts2.util.ConfigParseUtil.toClassesSet;
+import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet;
+import static org.apache.struts2.util.ConfigParseUtil.toNewPackageNamesSet;
+import static org.apache.struts2.util.ConfigParseUtil.toNewPatternsSet;
+import static org.apache.struts2.util.ConfigParseUtil.toPackageNamesSet;
+import static org.apache.struts2.util.DebugUtils.logWarningForFirstOccurrence;
+
+/**
+ * Holds the parsed OGNL security configuration for one container.
+ * <p>
+ * {@link SecurityMemberAccess} is a {@code Scope.PROTOTYPE} bean, constructed
once per value stack and
+ * again for each OGNL context. Parsing the roughly ninety configuration
entries on every one of those
+ * was the dominant cost identified by WW-5667. This bean is a {@code
Scope.SINGLETON}, so the parsing
+ * happens once per container and each {@code SecurityMemberAccess} merely
copies immutable references.
+ * <p>
+ * Dev-mode is resolved in {@link #init()} rather than in a setter, because
the container iterates
+ * {@code getDeclaredMethods()}, whose order the JDK leaves unspecified. If
{@code init()} never runs,
+ * the normal production exclusions stay in force, which fails closed.
+ *
+ * @since Struts 7.4.0
+ */
+public class SecurityMemberAccessConfig implements Initializable {
+
+ private static final Logger LOG =
LogManager.getLogger(SecurityMemberAccessConfig.class);
+
+ /**
+ * Struts' own component packages, which must always be allowlisted
regardless of what an
+ * application configures via {@code struts.allowlist.packageNames}. Lives
here, alongside
+ * {@link #union(Set, Set)}, because this is the single place that computes
+ * {@code allowlistPackageNamesUnion}; {@link SecurityMemberAccess}
references both statically for
+ * its default field value and its deprecated {@code
useAllowlistPackageNames} setter, so the
+ * computation is never duplicated.
+ */
+ static final Set<String> ALLOWLIST_REQUIRED_PACKAGES = Set.of(
+ "org.apache.struts2.validator.validators",
+ "org.apache.struts2.components",
+ "org.apache.struts2.views.jsp"
+ );
+
+ private boolean allowStaticFieldAccess = true;
+
+ private Set<String> excludedClasses = Set.of(Object.class.getName());
+ private Set<Pattern> excludedPackageNamePatterns = emptySet();
+ private Set<String> excludedPackageNames = emptySet();
+ private Set<String> excludedPackageExemptClasses = emptySet();
+
+ private boolean isDevMode;
+ private Set<String> devModeExcludedClasses =
Set.of(Object.class.getName());
+ private Set<Pattern> devModeExcludedPackageNamePatterns = emptySet();
+ private Set<String> devModeExcludedPackageNames = emptySet();
+ private Set<String> devModeExcludedPackageExemptClasses = emptySet();
+
+ private boolean enforceAllowlistEnabled = false;
+ private Set<Class<?>> allowlistClasses = emptySet();
+ private Set<String> allowlistPackageNames = emptySet();
+ private Set<String> allowlistPackageNamesUnion =
ALLOWLIST_REQUIRED_PACKAGES;
+
+ private boolean disallowProxyObjectAccess = false;
+ private boolean disallowProxyMemberAccess = false;
+ private boolean disallowDefaultPackageAccess = false;
+
+ @Override
+ public void init() {
+ if (!isDevMode) {
+ return;
+ }
+ logWarningForFirstOccurrence("devMode", LOG,
+ "DevMode enabled, using DevMode excluded classes and packages
for OGNL security enforcement!");
+ excludedClasses = devModeExcludedClasses;
+ excludedPackageNamePatterns = devModeExcludedPackageNamePatterns;
+ excludedPackageNames = devModeExcludedPackageNames;
+ excludedPackageExemptClasses = devModeExcludedPackageExemptClasses;
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required
= false)
+ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) {
+ this.allowStaticFieldAccess =
BooleanUtils.toBoolean(allowStaticFieldAccess);
+ if (!this.allowStaticFieldAccess) {
+ useExcludedClasses(Class.class.getName());
+ }
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false)
+ public void useExcludedClasses(String commaDelimitedClasses) {
+ this.excludedClasses = toNewClassesSet(excludedClasses,
commaDelimitedClasses);
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS,
required = false)
+ public void useExcludedPackageNamePatterns(String
commaDelimitedPackagePatterns) {
+ this.excludedPackageNamePatterns =
toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns);
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required =
false)
+ public void useExcludedPackageNames(String commaDelimitedPackageNames) {
+ this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames,
commaDelimitedPackageNames);
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES,
required = false)
+ public void useExcludedPackageExemptClasses(String commaDelimitedClasses) {
+ this.excludedPackageExemptClasses =
toClassesSet(commaDelimitedClasses);
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false)
+ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) {
+ this.enforceAllowlistEnabled =
BooleanUtils.toBoolean(enforceAllowlistEnabled);
+ if (!this.enforceAllowlistEnabled) {
+ String msg = "OGNL allowlist is disabled!" +
+ " We strongly recommend keeping it enabled to protect
against critical vulnerabilities." +
+ " Set the configuration `{}=true` to enable it." +
+ " Please refer to the Struts 7.0 migration guide and
security documentation for further information.";
+ logWarningForFirstOccurrence("allowlist", LOG, msg,
StrutsConstants.STRUTS_ALLOWLIST_ENABLE);
+ }
+ }
+
+ @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false)
+ public void useAllowlistClasses(String commaDelimitedClasses) {
+ this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses);
+ }
+
+ @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false)
+ public void useAllowlistPackageNames(String commaDelimitedPackageNames) {
+ this.allowlistPackageNames =
toPackageNamesSet(commaDelimitedPackageNames);
+ this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES,
allowlistPackageNames);
+ }
+
+ /**
+ * The only place in the codebase that computes the allowlist package
union. Both
+ * {@link #useAllowlistPackageNames(String)} above and {@link
SecurityMemberAccess}'s deprecated
+ * setter path call this method, so {@code ALLOWLIST_REQUIRED_PACKAGES}
can never silently drop out
+ * of the union through a second, drifted implementation.
+ * <p>
+ * The early return aliases {@code required} directly into the result,
which is safe only because
+ * every caller passes an immutable {@code Set.of(...)} for that argument;
a mutable set must not be
+ * passed as {@code required}.
+ */
+ static Set<String> union(Set<String> required, Set<String> configured) {
+ if (configured.isEmpty()) {
+ return required;
+ }
+ Set<String> union = new HashSet<>(required);
+ union.addAll(configured);
+ return unmodifiableSet(union);
+ }
Review Comment:
Good catch — fixed in b8cd7d128.
You're right that a documented contract is the wrong strength for a
package-visible method whose result is shared across every
`SecurityMemberAccess` in the container. I took a slightly different route to
it than `unmodifiableSet`, though: that would wrap an already-immutable
`Set.of(...)` in a redundant wrapper on every call, since
`ALLOWLIST_REQUIRED_PACKAGES` is the only value any current caller passes.
`Set.copyOf` gets the same guarantee for free. I verified the behaviour
rather than assuming it:
```
Set.copyOf(Set.of) -> same instance (no allocation)
Set.copyOf(unmodifiableSet) -> new instance (defensive copy)
copy isolated from later mutation of the source -> true
copy rejects mutation -> true
```
So the real path allocates nothing, and any future caller passing a mutable
set gets a defensive copy instead of aliasing it into shared state. The javadoc
now states the result is always immutable, which is enforced rather than merely
asserted.
Full core suite remains 3181 tests, 0 failures, 0 errors.
Issue Time Tracking
-------------------
Worklog Id: (was: 1035825)
Time Spent: 50m (was: 40m)
> Stop re-parsing OGNL security config on every SecurityMemberAccess
> instantiation
> --------------------------------------------------------------------------------
>
> Key: WW-5675
> URL: https://issues.apache.org/jira/browse/WW-5675
> Project: Struts 2
> Issue Type: Sub-task
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Sub-task of WW-5667.
> {{SecurityMemberAccess}} is a {{Scope.PROTOTYPE}} bean
> ({{StrutsBeanSelectionProvider}}, since WW-5343). Every
> {{container.getInstance(SecurityMemberAccess.class)}} therefore constructs a
> fresh instance and re-runs all of its {{@Inject}} setters, each of which
> re-parses a raw comma-delimited configuration string from scratch.
> With the stock {{struts-excluded-classes.xml}}, a single instantiation
> re-parses roughly:
> * 16 excluded class names from {{struts.excludedClasses}} and another 16 from
> {{struts.devMode.excludedClasses}} — comma split plus classloader validation
> * 29 excluded package names from {{struts.excludedPackageNames}} and another
> 29 from {{struts.devMode.excludedPackageNames}} — comma split, {{strip}}, and
> {{validatePackageNames}}
> * the allowlist class and package sets
> * any configured excluded-package-name patterns, which are re-compiled via
> {{Pattern.compile}}
> New instances are created on the request path from at least:
> * {{OgnlValueStackFactory.createValueStack(...)}} — once per value stack;
> {{ParametersInterceptor.toNewStack}} creates an additional stack per request
> * {{OgnlUtil.createDefaultContext(Object, ClassResolver)}} — reached from
> {{setProperties}}, {{copy}}, {{getBeanMap}} and friends
> so the full configuration is rebuilt several times per request.
> This matches JFR sample 2 on the parent ticket:
> {noformat}
> HashMap.put / HashSet.add
> ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
> SecurityMemberAccess.useExcludedPackageNames(String) :446
> {noformat}
> Note that the fix proposed on the parent ticket — caching the parsed set in a
> {{SecurityMemberAccess}} field — does not address this, because the instance
> holding the field is itself discarded and rebuilt each time.
> h2. Also in scope: validatePackageNames recompiles a constant pattern per
> entry
> {{ConfigParseUtil.validatePackageNames}} evaluates {{Pattern.compile("\\s")}}
> once per package name rather than once overall:
> {code:java}
> public static void validatePackageNames(Collection packageNames) {
> if (packageNames.stream().anyMatch(s ->
> Pattern.compile("\\s").matcher(s).find())) {
> throw new ConfigurationException("Excluded package names could not be
> parsed due to erroneous whitespace characters: " + packageNames);
> }
> }
> {code}
> With the default configuration that is roughly 58 recompiles of a trivial
> pattern per {{SecurityMemberAccess}} instantiation, on top of the parsing
> work above. Hoisting the pattern to a static constant — or replacing the
> check with a plain character scan — is a small, self-contained fix, and it
> shares this ticket's root cause of doing per-instantiation work that should
> be done once.
> h2. Design note: this ticket unblocks collapsing the allowlist two-set walk
> WW-5674 merged the two allowlist walks in {{isClassAllowlisted}} by adding a
> two-set helper:
> {code:java}
> static boolean isPackageBelongsToPackages(String packageName, Set first, Set
> second)
> {code}
> The cleaner shape is a single set holding the union of
> {{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}}, precomputed
> once. That would delete the two-set parameter and the three-argument overload
> entirely, leaving one set and one walk.
> It was rejected in WW-5674 for two reasons, and _this ticket removes the
> first and largely removes the second_:
> h1. Under the current prototype scope, a precomputed union field is rebuilt
> on every instantiation — several times per request — adding to exactly the
> cost this ticket is about. Once the parsed configuration is shared rather
> than per-instance, the union becomes free.
> h1. The union would need initialising in two places: at the field
> declaration, for the case where the {{@Inject}} setter never fires because no
> allowlist is configured, and again in the setter. Duplicated initialisation
> here is a _fail-open_ hazard — get it wrong and
> {{ALLOWLIST_REQUIRED_PACKAGES}} silently drops out of the allowlist with
> nothing failing loudly. Whatever this ticket does to give the configuration a
> single well-defined construction point makes that much safer to get right.
> Worth doing as part of this ticket rather than leaving it to WW-5678, since
> the enabling change lands here. Note that it would also resolve WW-5678's
> first item for free, by removing the package-private overload that shares a
> name with a public method.
> h2. Constraints
> The OGNL allow/deny semantics must not change. The dev-mode configuration
> switchover in {{useDevModeConfiguration()}} must keep working. Per-instance
> mutable state set at request time by
> {{ParametersInterceptor.applyMemberAccessProperties}}
> ({{useAcceptProperties}} / {{useExcludeProperties}}) must stay per-instance —
> this is why the bean was made prototype-scoped in the first place, so any
> move back towards sharing has to account for it.
> h2. Related
> * WW-5674 — the per-access half of the parent report
> ({{isClassBelongsToPackages}} allocations). Handled separately; it does not
> move this cost.
> * WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access
> path. Same file, different root cause.
> * WW-5678 — helper naming and visibility cleanup for 8.0.0. Its first item
> may be resolved for free here, see the design note above.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
