lukaszlenart opened a new pull request, #1847: URL: https://github.com/apache/struts/pull/1847
Fixes [WW-5675](https://issues.apache.org/jira/browse/WW-5675) — sub-task of [WW-5667](https://issues.apache.org/jira/browse/WW-5667). ## The problem `SecurityMemberAccess` is a `Scope.PROTOTYPE` bean, so a fresh instance is constructed once per value stack and again for every OGNL context — several times per request. Each construction re-ran all sixteen `@Inject` configuration setters, each re-parsing a raw comma-delimited string from scratch: comma splitting, `strip`, classloader validation, `Pattern.compile`, and `HashSet` accumulation. With the stock `struts-excluded-classes.xml` that is roughly 90 configuration entries rebuilt per instantiation. `OgnlUtil.copy` alone calls `createDefaultContext` twice, so a single copy paid for two full configuration rebuilds. This is the dominant half of WW-5667. The sibling ticket WW-5674 (merged as `81b34c295`) addressed the per-*access* allocations; **this** is the one expected to move the reporter's 9%. Note that the fix originally proposed on WW-5667 — caching the parsed set in a `SecurityMemberAccess` field — cannot work, because the instance holding that field is itself discarded and rebuilt each time. ## The change A new container-singleton bean, `SecurityMemberAccessConfig`, owns all configuration parsing and does it once per container. `SecurityMemberAccess` stays `Scope.PROTOTYPE` and receives that bean through a single `@Inject` setter, copying immutable set references — no parsing, no `HashSet` construction, no `Pattern.compile` per instantiation. Also in this PR: - The lazy dev-mode flip (`useDevModeConfiguration()` plus a `volatile` guard) is gone from the OGNL access path. Dev-mode now resolves once, in the config bean's `Initializable.init()`. - The two-set allowlist walk added by WW-5674 collapses into a single precomputed union, resolving WW-5678's first item. - `ConfigParseUtil.validatePackageNames` no longer recompiles `Pattern.compile("\\s")` once per package name (~58 recompiles per instantiation under the default config). ### Why setter injection rather than constructor injection Deliberate, and load-bearing. `ContainerImpl.addInjectors` recurses into superclasses first, so an existing user subclass calling `super(providerAllowlist, threadAllowlist)` keeps compiling *and* still receives the configuration. Had the configuration arrived via the constructor with a deprecated two-argument overload retained, such a subclass would have compiled cleanly and run with empty exclusions — a silent fail-open. Setter injection removes that failure mode rather than documenting it. `useConfig` is a mandatory `@Inject`, so a container missing the binding throws at build time instead of running with weaker exclusions. ### Registration in two places `SecurityMemberAccessConfig` is registered in both `DefaultConfiguration.bootstrapFactories` and `struts-beans.xml`, mirroring `ProviderAllowlist` and `ThreadAllowlist`. Both are load-bearing: - `Dispatcher.init()` installs its own provider list and never adds `StrutsDefaultConfigurationProvider`, so the main container is built from `StrutsBeanSelectionProvider` plus `struts-beans.xml`. - `DefaultConfiguration.reloadContainer` builds a bootstrap container from `bootstrapFactories` and instantiates `SecurityMemberAccess` through it before the main container exists. Production throws at startup without either. ## ⚠️ Breaking change in a minor release **Five `public` methods are deleted from `SecurityMemberAccess`:** `useDevMode`, `useDevModeExcludedClasses`, `useDevModeExcludedPackageNamePatterns`, `useDevModeExcludedPackageNames`, `useDevModeExcludedPackageExemptClasses`. They are only ever container-injected — a repo-wide search finds no direct caller in core, plugins, apps, or tests. Retaining them faithfully would mean keeping `isDevMode` plus the four dev-mode set fields on the instance and reinstating the lazy flip, i.e. keeping precisely the code this change exists to delete. Retaining them in simplified form was rejected because the current semantics are subtle enough that any simplification would silently change them: a manual `useDevModeExcludedClasses` call accumulates into the dev-mode set, which then *replaces* — rather than unions with — `excludedClasses` on first access. The failure mode for anyone affected is a compile error on upgrade, which is loud and has an obvious fix. **This needs a Version Notes and Migration Guide entry for the release. That entry does not exist yet.** The other eleven configuration setters are *not* removed. They keep their exact bodies and still mutate the instance, so the ~110 existing direct call sites are unaffected. They are annotated `@Deprecated(since = "7.4.0", forRemoval = true)` and scheduled for removal in [WW-5682](https://issues.apache.org/jira/browse/WW-5682) (8.0.0). ## Behaviour OGNL allow/deny semantics are unchanged. One visible difference: the `"DevMode enabled, using DevMode excluded classes and packages..."` warning now fires when the configuration singleton is built rather than on the first OGNL access — a deterministic startup signal instead of one contingent on traffic. ## Testing Full `core` suite: **3181 tests, 0 failures, 0 errors**. `plugins/spring` 61/61, `plugins/cdi` 17/17. New coverage, beyond the existing suites passing untouched: - **Sharing proof** — several `SecurityMemberAccess` instances from one container hold reference-identical configuration sets (`assertSame`, since any re-parse necessarily allocates fresh). The container is loaded with every relevant constant set away from its default, so the assertions cannot pass on shared defaults. - **Instance isolation** — a deprecated setter call on one instance perturbs neither a sibling nor the singleton. - **Subclass injection** — a subclass using the two-argument constructor still receives the configuration, guarding the fail-open hole described above. - **Production registration** — a `StrutsInternalTestCase` boot (real `Dispatcher.init()`, loads `struts-beans.xml`) asserts the bean is a singleton on the path production actually uses. Verified by temporarily setting `scope="prototype"` and confirming the test fails. - **Differential parsing** — the config bean's output is compared against a frozen copy of the accumulation logic it replaces, including the `useAllowStaticFieldAccess` → `useExcludedClasses` side effect and the commutativity that makes the setters safe against the container's unspecified `getDeclaredMethods()` order. The design document is included in the diff at `docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
