From b10154e1702f88cccc4b9107bb6fc9e52dcb1e6b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 13:45:30 +0200 Subject: [PATCH 01/18] WW-5675 docs(ognl): add design for sharing parsed OGNL security config Co-Authored-By: Claude Opus 5 --- ...ity-member-access-config-sharing-design.md | 286 ++++++++++++++++++ 1 file changed, 286 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md new file mode 100644 index 0000000000..b5435fe330 --- /dev/null +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -0,0 +1,286 @@ +# WW-5675 — Share parsed OGNL security configuration across `SecurityMemberAccess` instances + +**Ticket:** [WW-5675](https://issues.apache.org/jira/browse/WW-5675) (sub-task of [WW-5667](https://issues.apache.org/jira/browse/WW-5667)) +**Target:** 7.4.0 +**Date:** 2026-08-14 +**Status:** Design approved, pending implementation plan +**Open decision:** whether the five dev-mode setters are deleted or deprecated — see "`SecurityMemberAccess` +changes" + +## Problem + +`SecurityMemberAccess` is a `Scope.PROTOTYPE` bean, registered in two places: + +- `DefaultConfiguration.java:416` — `.factory(SecurityMemberAccess.class, Scope.PROTOTYPE)` +- `StrutsBeanSelectionProvider.java:456` — aliased to `STRUTS_MEMBER_ACCESS`, making it user-overridable + +Every `container.getInstance(SecurityMemberAccess.class)` therefore constructs a fresh instance and re-runs all +sixteen `@Inject` configuration setters, each of which re-parses a raw comma-delimited string from scratch. With +the stock `struts-excluded-classes.xml` that is roughly 90 configuration entries per instantiation: comma +splitting, `strip`, classloader validation, `Pattern.compile`, and `HashSet` accumulation. + +New instances are created on the request path from at least: + +- `OgnlValueStackFactory.createValueStack(...)` — once per value stack, and `ParametersInterceptor` creates an + additional stack per request +- `OgnlUtil.createDefaultContext(Object, ClassResolver)` at `OgnlUtil.java:738` — reached from `setProperties`, + `copy`, `getBeanMap` and friends. Note `OgnlUtil.copy` calls it **twice** (`OgnlUtil.java:551-552`), so a single + copy costs two full configuration rebuilds. + +This is the dominant half of the parent report. The sibling ticket WW-5674 (merged as `81b34c295`) addressed the +per-*access* allocations; this ticket addresses the per-*instantiation* cost, which is where the reported 9% lives. + +The fix proposed on the parent ticket — caching the parsed set in a `SecurityMemberAccess` field — cannot work, +because the instance holding the field is itself discarded and rebuilt each time. + +### Also in scope + +`ConfigParseUtil.validatePackageNames` (`ConfigParseUtil.java:143`) evaluates `Pattern.compile("\\s")` once per +package name rather than once overall — roughly 58 recompiles of a trivial pattern per instantiation under the +default configuration. Hoist it to a static constant. + +## Goals + +- Parse the OGNL security configuration once per container instead of once per `SecurityMemberAccess`. +- Preserve OGNL allow/deny semantics exactly. No configuration may become more permissive. +- Keep source compatibility for 7.4.0: existing subclasses and direct setter callers must continue to compile and + behave identically. +- Collapse the two-set allowlist walk introduced by WW-5674 into a single precomputed set. + +## Non-goals + +- Changing array/primitive package-resolution semantics — that is WW-5676, deliberately separate because it is a + security-semantics decision rather than a performance fix. +- Removing the residual per-access `getPackage()` lookups — that is WW-5677. +- Removing the deprecated setters. They are scheduled for 8.0.0 (see Follow-ups). +- Adding JMH or any benchmarking infrastructure to the build. + +## Approach + +Introduce a container-singleton configuration bean that owns all parsing. `SecurityMemberAccess` stays +`Scope.PROTOTYPE` and copies immutable set *references* out of that bean. + +Two alternatives were considered and rejected: + +**Memoize parsing inside `ConfigParseUtil`** (keyed by raw config string, following the existing Caffeine +precedent in that file). Smallest possible diff and no API change, but it recovers the least: every instantiation +still invokes sixteen setters, still builds the accumulated `HashSet` copies, and still runs the lazy dev-mode +flip. It also does not unblock the allowlist union collapse. + +**Revert `SecurityMemberAccess` to `Scope.SINGLETON`**, relocating `acceptProperties`/`excludeProperties` into the +OGNL context. Largest theoretical win, but it reverses a deliberate WW-5343 decision, converts two fields into +shared mutable state requiring thread-safety on the OGNL security gate, and changes the `MemberAccessValueStack` +contract that `ParametersInterceptor` depends on. Under the chosen approach the per-instantiation cost is already +about a dozen reference copies, so this buys very little for substantially more risk. + +## Design + +### New bean: `SecurityMemberAccessConfig` + +Registered in `DefaultConfiguration` beside the existing internal singletons: + +```java +.factory(SecurityMemberAccessConfig.class, Scope.SINGLETON) +``` + +Concrete class, no interface, **not** aliased in `StrutsBeanSelectionProvider`. It is internal plumbing, following +the shape of `ProviderAllowlist` and `ThreadAllowlist` (`DefaultConfiguration.java:418-419`), not a user extension +point. + +It takes over these sixteen `@Inject` setters from `SecurityMemberAccess`: + +| Setter | Constant | +|---|---| +| `useAllowStaticFieldAccess` | `STRUTS_ALLOW_STATIC_FIELD_ACCESS` | +| `useExcludedClasses` | `STRUTS_EXCLUDED_CLASSES` | +| `useExcludedPackageNamePatterns` | `STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS` | +| `useExcludedPackageNames` | `STRUTS_EXCLUDED_PACKAGE_NAMES` | +| `useExcludedPackageExemptClasses` | `STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES` | +| `useEnforceAllowlistEnabled` | `STRUTS_ALLOWLIST_ENABLE` | +| `useAllowlistClasses` | `STRUTS_ALLOWLIST_CLASSES` | +| `useAllowlistPackageNames` | `STRUTS_ALLOWLIST_PACKAGE_NAMES` | +| `useDisallowProxyObjectAccess` | `STRUTS_DISALLOW_PROXY_OBJECT_ACCESS` | +| `useDisallowProxyMemberAccess` | `STRUTS_DISALLOW_PROXY_MEMBER_ACCESS` | +| `useDisallowDefaultPackageAccess` | `STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS` | +| `useDevMode` | `STRUTS_DEVMODE` | +| `useDevModeExcludedClasses` | `STRUTS_DEV_MODE_EXCLUDED_CLASSES` | +| `useDevModeExcludedPackageNamePatterns` | `STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS` | +| `useDevModeExcludedPackageNames` | `STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES` | +| `useDevModeExcludedPackageExemptClasses` | `STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES` | + +`setProxyService` and the `@Inject` constructor stay on `SecurityMemberAccess` — those inject collaborators, not +configuration. + +The bean implements `Initializable`. Dev-mode resolution cannot happen inside any individual setter, because +`ContainerImpl.addInjectorsForMembers` iterates `getDeclaredMethods()`, whose order the JDK explicitly leaves +unspecified. `Initializable.init()` runs after the whole dependency graph is built +(`InitializableFactory.wrapIfNeeded`, applied from `Scope` for singleton scope; `DefaultValidatorFactory` is the +existing precedent). `init()` therefore: + +1. Selects the effective excluded sets — dev-mode variants when `struts.devMode=true`, otherwise the normal ones. +2. Precomputes the allowlist package union. + +The bean exposes only immutable getters, and publishes the **effective** excluded sets with dev-mode already +applied, so nothing downstream needs to know dev-mode exists. + +### `SecurityMemberAccess` changes + +Gains exactly one injected member: + +```java +@Inject +public void useConfig(SecurityMemberAccessConfig config) { … } +``` + +which seeds its fields by copying immutable set references — no parsing, no `HashSet` construction, no +`Pattern.compile`. + +**Fields removed:** `isDevModeInit` (volatile), `isDevMode`, `devModeExcludedClasses`, +`devModeExcludedPackageNamePatterns`, `devModeExcludedPackageNames`, `devModeExcludedPackageExemptClasses`. + +**Method removed:** `useDevModeConfiguration()`, along with its call from `checkExclusionList` +(`SecurityMemberAccess.java:264`). The lazy dev-mode flip disappears from the access path entirely. + +**Field added:** `allowlistPackageNamesUnion`. + +The five dev-mode setters are proposed for **deletion outright rather than deprecation**. This is a deliberate +deviation from the "additive and deprecate, no breakage in a minor" policy chosen for the rest of this change, and +needs explicit sign-off. + +The case for deleting them: they are `public`, but only ever container-injected, with no direct caller anywhere in +core, plugins, or tests. Preserving them faithfully would mean keeping `isDevMode` plus the four dev-mode set +fields on the instance and reinstating some form of the lazy flip — that is, keeping precisely the code this +change exists to delete, to serve a caller that does not demonstrably exist. + +The case against: they are public methods on a user-overridable bean, so a deployment could in principle call +them, and removal in a minor release would break it at compile time. Deprecating them while preserving exact +semantics is not cheap, because today's semantics are subtle — a manual `useDevModeExcludedClasses` call +accumulates into the dev-mode set, which then *replaces* (not unions with) `excludedClasses` on first access. Any +simplified retention would silently change that. + +The remaining eleven configuration setters stay as `@Deprecated` methods with their `@Inject` annotations removed. +They keep mutating that instance exactly as they do now. Deprecation is by annotation only — no runtime warnings, +which would flood test output given roughly 110 direct call sites across core and plugins. + +`useAllowStaticFieldAccess` retains its side effect of calling `useExcludedClasses(Class.class.getName())`, and +the configuration bean must reproduce that accumulation exactly. + +### Why setter injection rather than constructor injection + +Constructor injection would be the obvious way to guarantee ordering, but it forces a constructor signature +change. A user subclass calling `super(providerAllowlist, threadAllowlist)` — precisely the shape of the existing +`ExternalSecurityMemberAccess` test fixture — would then either fail to compile, or, if a deprecated 2-arg +overload were retained, compile cleanly and silently run with empty exclusions. **That is a fail-open hole**, and +the kind that fails silently rather than loudly. + +Setter injection avoids it: `ContainerImpl.addInjectors` recurses into superclasses first +(`ContainerImpl.java:97`), so inherited `@Inject` setters are injected on subclass instances. Existing subclasses +keep compiling *and* receive the configuration. + +Injection ordering is safe by construction. Today the setters survive unspecified ordering only because they +*accumulate* rather than assign, making them commutative — a subtlety that is easy to destroy accidentally. After +this change `SecurityMemberAccess` has exactly one injected member touching those fields, so ordering stops +mattering at all. + +A null configuration is also safe: the eight direct `new SecurityMemberAccess(null, null)` test sites never have +the setter called, so their fields keep today's hardcoded defaults. Reads only ever touch fields, never the +configuration object, so there is no null path on the access path. + +### Allowlist union + +With the sets precomputed per container, `isClassAllowlisted` collapses to a single set and a single walk: + +```java +|| isClassBelongsToPackages(clazz, allowlistPackageNamesUnion); +``` + +This deletes the three-argument `isClassBelongsToPackages` overload and the two-set parameters on +`isPackageBelongsToPackages`, resolving WW-5678's first item as a side effect. + +The ticket flagged this as a fail-open hazard: if the union were computed in two places — once seeded from +configuration, once when the deprecated `useAllowlistPackageNames` setter fires — the two could drift, silently +dropping `ALLOWLIST_REQUIRED_PACKAGES` from the allowlist with nothing failing loudly. Both routes therefore +funnel through one private method, so exactly one line in the codebase computes the union: + +```java +private void applyAllowlistPackageNames(Set names) { + this.allowlistPackageNames = names; + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, names); +} +``` + +`isPackageBelongsToPackages` currently early-returns on `first.isEmpty() && second.isEmpty()`. Since +`ALLOWLIST_REQUIRED_PACKAGES` is never empty, that guard simply stops firing on the allowlist path; the exclusion +path, where both sets genuinely can be empty, keeps it. The guard was only ever an optimization, so this is not a +semantic change. + +## Data flow + +| Tier | Frequency | Work | +|---|---|---| +| `SecurityMemberAccessConfig` construction | Once per container | All parsing, class validation, pattern compilation, dev-mode resolution, union precomputation | +| `useConfig` | Once per `SecurityMemberAccess` | About a dozen immutable reference copies | +| `ParametersInterceptor` | Once per request | Sets `acceptProperties`/`excludeProperties` on the instance (unchanged) | +| `isAccessible` | Per OGNL member access | Field reads only | + +## Error handling + +Parsing failures — `ConfigurationException` for an unloadable class, an invalid regex, or whitespace in a package +name — move from being thrown on every instantiation to being thrown once, when the singleton is first built. +Still fatal, still loud, just earlier and once. Nothing degrades to a warning. + +The `struts.allowlist.enable=false` warning already dedupes via `logWarningForFirstOccurrence`; moving it to the +configuration bean makes it a genuine once-per-container event. + +## Behaviour changes + +One, accepted during design review: the `"DevMode enabled, using DevMode excluded classes and packages for OGNL +security enforcement!"` warning currently fires on the first OGNL access and will now fire when the configuration +singleton is built. This makes it a deterministic startup signal rather than one contingent on traffic. + +No other externally visible behaviour changes. OGNL allow/deny semantics are identical. + +## Testing + +Core tests are JUnit 4 or extend `XWorkTestCase`. A JUnit 5 `@Test` added to these suites silently never runs. + +1. **Sharing proof.** Request several `SecurityMemberAccess` instances from one container and assert their + configuration-derived sets are reference-identical (`assertSame`, not `assertEquals`). Reference identity is a + dependency-free proof that no re-parsing occurred, since any re-parse necessarily produces a fresh set. Backed + by a counting probe asserting exactly one parse per container. +2. **Instance isolation.** Calling a deprecated setter on one instance must not perturb a sibling instance or the + singleton. The sets are `unmodifiableSet`, so an in-place mutation bug would throw rather than corrupt + silently, but this invariant deserves an explicit assertion. +3. **Subclass injection.** A subclass declaring the 2-arg constructor and calling + `super(providerAllowlist, threadAllowlist)` must receive the configuration through the inherited setter. This + is the test that would catch a future refactor to constructor injection reintroducing the fail-open hole. +4. **Behaviour preservation.** Following WW-5674's differential pattern: for default, dev-mode, and custom + configurations, the sets the new bean publishes must equal what a legacy-style accumulation produces. This is + where the `useAllowStaticFieldAccess` → `useExcludedClasses` side effect gets pinned down. +5. **Dev-mode.** With `struts.devMode=true` the effective sets are the dev-mode ones from the start, with no OGNL + access required to trigger the switch. + +The principal safety net is the existing suite. `SecurityMemberAccessTest` and its siblings drive these setters +directly from roughly 110 call sites across core and plugins and must pass untouched. If the deprecated setters +have kept their exact semantics, that suite cannot tell the difference — the strongest available evidence that +OGNL allow/deny semantics are unchanged. + +## Risks + +| Risk | Mitigation | +|---|---| +| Shared sets mutated in place, poisoning every instance in the container | Sets are already `unmodifiableSet`; test 2 asserts isolation explicitly | +| Allowlist union drifts from `ALLOWLIST_REQUIRED_PACKAGES` (fail-open) | Single computation site; test 4 covers custom allowlist configurations | +| Configuration bean fails to reproduce the accumulate-not-assign semantics | Test 4 is differential against the legacy accumulation, not against hand-written expectations | +| A future refactor moves configuration to constructor injection, reintroducing the silent fail-open | Test 3 encodes the subclass contract; the rationale is recorded above and in the class Javadoc | +| `Initializable` is documented "should be only used internally" | The bean is internal and unaliased; `DefaultValidatorFactory` is the existing precedent | + +## Follow-ups + +- **8.0.0 — remove the deprecated configuration setters.** The eleven methods left on `SecurityMemberAccess` + should be removed once the major version allows it. To be filed as its own ticket, cross-referencing WW-5675 and + WW-5678. +- **WW-5678** — its first item (the package-private overload sharing a name with a public method) is resolved for + free here by the union collapse. The remaining visibility narrowing stays with that ticket. +- **WW-5667** — the parent should be updated to note that this ticket, not WW-5674, is the one expected to move + the reported 9%. From 6216411b4def47b1ac76e374dab0f3db29d1e782 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 13:49:00 +0200 Subject: [PATCH 02/18] WW-5675 docs(ognl): record that bootstrapFactories is on the production path Co-Authored-By: Claude Opus 5 --- ...security-member-access-config-sharing-design.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md index b5435fe330..3523beb019 100644 --- a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -87,6 +87,20 @@ Concrete class, no interface, **not** aliased in `StrutsBeanSelectionProvider`. the shape of `ProviderAllowlist` and `ThreadAllowlist` (`DefaultConfiguration.java:418-419`), not a user extension point. +`bootstrapFactories` is on the production path, not test-only: `ConfigurationManager.addDefaultContainerProviders` +(`ConfigurationManager.java:94`) registers `StrutsDefaultConfigurationProvider`, which calls it at +`StrutsDefaultConfigurationProvider.java:116`, and `Dispatcher` drives `ConfigurationManager`. It reads as +test-oriented in a grep only because a dozen tests name the provider explicitly and `XWorkTestCaseHelper` — test +scaffolding that lives in `core/src/main` — registers it too. + +The method serves both the bootstrap container (`DefaultConfiguration.java:360`) and the main container, so each +gets its own configuration singleton. The bootstrap container carries only `BOOTSTRAP_CONSTANTS`, so most security +constants are absent there, the `required = false` setters do not fire, and the bean falls back to defaults — +exactly as a `SecurityMemberAccess` constructed in that container behaves today. + +The `TODO: SpringObjectFactoryTest fails when these are SINGLETON` comment at the top of `bootstrapFactories` +applies to the `*Factory` beans in the first block, not to this region, where singletons are already the norm. + It takes over these sixteen `@Inject` setters from `SecurityMemberAccess`: | Setter | Constant | From e91a14870290f870ab503e763b0cd3fcbcb66763 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 13:51:27 +0200 Subject: [PATCH 03/18] WW-5675 docs(ognl): settle the dev-mode setter removal as decided Co-Authored-By: Claude Opus 5 --- ...ity-member-access-config-sharing-design.md | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md index 3523beb019..50ecb29627 100644 --- a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -4,8 +4,6 @@ **Target:** 7.4.0 **Date:** 2026-08-14 **Status:** Design approved, pending implementation plan -**Open decision:** whether the five dev-mode setters are deleted or deprecated — see "`SecurityMemberAccess` -changes" ## Problem @@ -44,7 +42,8 @@ default configuration. Hoist it to a static constant. - Parse the OGNL security configuration once per container instead of once per `SecurityMemberAccess`. - Preserve OGNL allow/deny semantics exactly. No configuration may become more permissive. - Keep source compatibility for 7.4.0: existing subclasses and direct setter callers must continue to compile and - behave identically. + behave identically. The five dev-mode setters are the one signed-off exception — see "`SecurityMemberAccess` + changes". - Collapse the two-set allowlist walk introduced by WW-5674 into a single precomputed set. ## Non-goals @@ -157,20 +156,21 @@ which seeds its fields by copying immutable set references — no parsing, no `H **Field added:** `allowlistPackageNamesUnion`. -The five dev-mode setters are proposed for **deletion outright rather than deprecation**. This is a deliberate -deviation from the "additive and deprecate, no breakage in a minor" policy chosen for the rest of this change, and -needs explicit sign-off. +The five dev-mode setters are **deleted outright rather than deprecated** — decided 2026-08-14. This is a +deliberate, signed-off deviation from the "additive and deprecate, no breakage in a minor" policy that governs the +rest of this change. -The case for deleting them: they are `public`, but only ever container-injected, with no direct caller anywhere in -core, plugins, or tests. Preserving them faithfully would mean keeping `isDevMode` plus the four dev-mode set -fields on the instance and reinstating some form of the lazy flip — that is, keeping precisely the code this -change exists to delete, to serve a caller that does not demonstrably exist. +They are `public`, but only ever container-injected, with no direct caller anywhere in core, plugins, or tests. +Preserving them faithfully would mean keeping `isDevMode` plus the four dev-mode set fields on the instance and +reinstating some form of the lazy flip — that is, keeping precisely the code this change exists to delete, to +serve a caller that does not demonstrably exist. Retention in simplified form was rejected because today's +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 case against: they are public methods on a user-overridable bean, so a deployment could in principle call -them, and removal in a minor release would break it at compile time. Deprecating them while preserving exact -semantics is not cheap, because today's semantics are subtle — a manual `useDevModeExcludedClasses` call -accumulates into the dev-mode set, which then *replaces* (not unions with) `excludedClasses` on first access. Any -simplified retention would silently change that. +The accepted risk is that a deployment calling these methods directly breaks at compile time on upgrade to 7.4.0. +This is a loud, immediate failure with an obvious fix, not a silent behavioural change, which is what makes it +acceptable where the constructor break discussed below was not. The remaining eleven configuration setters stay as `@Deprecated` methods with their `@Inject` annotations removed. They keep mutating that instance exactly as they do now. Deprecation is by annotation only — no runtime warnings, @@ -298,3 +298,6 @@ OGNL allow/deny semantics are unchanged. free here by the union collapse. The remaining visibility narrowing stays with that ticket. - **WW-5667** — the parent should be updated to note that this ticket, not WW-5674, is the one expected to move the reported 9%. +- **Migration guide entry for 7.4.0** — the removal of the five dev-mode setters is a source-breaking change in a + minor release and must be called out in the Version Notes and Migration Guide, however narrow the affected + audience. From d8a4e3003cbb45ff0f7d0cbc58cdad3d174da9ce Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 13:56:51 +0200 Subject: [PATCH 04/18] WW-5675 docs(ognl): add implementation plan for sharing parsed security config Co-Authored-By: Claude Opus 5 --- ...-5675-share-parsed-ognl-security-config.md | 1096 +++++++++++++++++ 1 file changed, 1096 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md diff --git a/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md b/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md new file mode 100644 index 0000000000..4ce8dbc8e0 --- /dev/null +++ b/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md @@ -0,0 +1,1096 @@ +# WW-5675 Share Parsed OGNL Security Configuration — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Parse the OGNL security configuration once per container instead of once per `SecurityMemberAccess` instantiation, without changing OGNL allow/deny semantics. + +**Architecture:** A new `Scope.SINGLETON` bean, `SecurityMemberAccessConfig`, takes over all sixteen `@Inject` configuration setters and does all parsing once per container, resolving dev-mode in `Initializable.init()`. `SecurityMemberAccess` stays `Scope.PROTOTYPE` and receives that bean through a single `@Inject` setter, copying immutable set references. The dev-mode lazy flip is deleted from the access path, and the allowlist two-set walk collapses into one precomputed union. + +**Tech Stack:** Java 17 (`maven.compiler.release=17`), Maven, JUnit 4 (`org.junit.Test`), AssertJ, Mockito, Log4j2, Caffeine. + +**Spec:** `docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md` + +## Global Constraints + +- **Branch:** `WW-5675-share-parsed-ognl-security-config`. Never push to `main`; finish via a PR. +- **Commit format:** `WW-5675 (): `, e.g. `WW-5675 perf(ognl): share parsed config across instances`. Every commit ends with the `Co-Authored-By: Claude Opus 5 ` trailer. +- **Never `git add -A` or `git add .`** in this repo — the tree carries roughly twenty long-lived untracked files. Stage explicit paths and verify with `git diff --cached --name-only` before every commit. +- **Core tests are JUnit 4** (`org.junit.Test`, `org.junit.Before`) or extend `XWorkTestCase`. A JUnit 5 `@Test` added to these suites silently never runs. +- **OGNL allow/deny semantics must not change.** No configuration may become more permissive. This is a security gate. +- **Test command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` +- **Full module suite:** `mvn test -DskipAssembly -pl core` +- Target version 7.4.0. + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java` | Modify: hoist the whitespace `Pattern` to a constant | +| `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java` | **Create**: owns all config parsing and dev-mode resolution, one instance per container | +| `core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java` | Modify: register the new bean as `Scope.SINGLETON` | +| `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java` | Modify: receive config by setter, deprecate eleven setters, delete dev-mode state, collapse the allowlist union | +| `core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java` | Test: whitespace validation behaviour preserved | +| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java` | **Create**: differential parsing + dev-mode resolution | +| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` | **Create**: sharing proof, instance isolation, subclass injection | + +--- + +### Task 1: Hoist the whitespace pattern in `ConfigParseUtil` + +`validatePackageNames` currently calls `Pattern.compile("\\s")` once per package name — roughly 58 recompiles of a trivial pattern per `SecurityMemberAccess` instantiation under the default configuration. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java:142-146` +- Test: `core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: no signature change. `public static void validatePackageNames(Collection packageNames)` keeps its exact behaviour and throws `ConfigurationException` on any whitespace. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java` if it does not exist; otherwise append these methods to the existing class. If creating it, use the standard ASF license header copied verbatim from `ConfigParseUtil.java` lines 1-18. + +```java +package org.apache.struts2.util; + +import org.apache.struts2.config.ConfigurationException; +import org.junit.Test; + +import java.util.List; +import java.util.Set; + +import static org.junit.Assert.assertThrows; + +public class ConfigParseUtilTest { + + @Test + public void validatePackageNamesAcceptsNamesWithoutWhitespace() { + ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache.struts2", "")); + } + + @Test + public void validatePackageNamesRejectsSpace() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache struts2"))); + } + + @Test + public void validatePackageNamesRejectsTab() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java\tlang"))); + } + + @Test + public void validatePackageNamesRejectsNewline() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java\nlang"))); + } + + @Test + public void validatePackageNamesAcceptsEmptyCollection() { + ConfigParseUtil.validatePackageNames(List.of()); + } +} +``` + +- [ ] **Step 2: Run the tests to verify they pass against the current implementation** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ConfigParseUtilTest` +Expected: PASS. These tests characterise existing behaviour before the refactor — they are the guard, not a red test. Do not proceed if any fails; that would mean the characterisation is wrong. + +- [ ] **Step 3: Hoist the pattern** + +In `core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java`, add the constant next to the existing cache constants near line 45: + +```java + private static final Pattern WHITESPACE = Pattern.compile("\\s"); +``` + +Then replace the body of `validatePackageNames`: + +```java + public static void validatePackageNames(Collection packageNames) { + if (packageNames.stream().anyMatch(s -> WHITESPACE.matcher(s).find())) { + throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames); + } + } +``` + +- [ ] **Step 4: Run the tests again** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ConfigParseUtilTest` +Expected: PASS, identical results to Step 2. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +git diff --cached --name-only +git commit -m "WW-5675 perf(config): hoist the whitespace pattern in validatePackageNames + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 2: Create the `SecurityMemberAccessConfig` bean + +A standalone bean that owns all parsing. It does not touch `SecurityMemberAccess` yet, so it is independently testable. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java` + +**Interfaces:** +- Consumes: `ConfigParseUtil.toClassesSet`, `toClassObjectsSet`, `toNewClassesSet`, `toNewPatternsSet`, `toNewPackageNamesSet`, `toPackageNamesSet` (all `public static`, unchanged). +- Produces, relied on by Task 3: + - `boolean isAllowStaticFieldAccess()` + - `Set getExcludedClasses()` + - `Set getExcludedPackageNamePatterns()` + - `Set getExcludedPackageNames()` + - `Set getExcludedPackageExemptClasses()` + - `boolean isEnforceAllowlistEnabled()` + - `Set> getAllowlistClasses()` + - `Set getAllowlistPackageNames()` + - `boolean isDisallowProxyObjectAccess()` + - `boolean isDisallowProxyMemberAccess()` + - `boolean isDisallowDefaultPackageAccess()` + + The four excluded-* getters return the **effective** sets, with dev-mode already applied. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java` with the ASF license header copied verbatim from `SecurityMemberAccess.java` lines 1-18. + +The `legacy*` methods below are **frozen oracles**: verbatim copies of the accumulation logic they replace. They must never be deleted, nor rewritten to delegate to production code — that would make the differential vacuous. This mirrors the approach used in `SecurityMemberAccessPackageMatchingTest` for WW-5674. + +```java +package org.apache.struts2.ognl; + +import org.junit.Test; + +import java.util.Set; +import java.util.regex.Pattern; + +import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class SecurityMemberAccessConfigTest { + + /** + * Frozen oracle: the accumulation SecurityMemberAccess performed before WW-5675. + * Never delete this, and never make it delegate to production code. + */ + private static Set legacyExcludedClassAccumulation(boolean allowStaticFieldAccess, String configured) { + Set excludedClasses = Set.of(Object.class.getName()); + if (!allowStaticFieldAccess) { + excludedClasses = toNewClassesSet(excludedClasses, Class.class.getName()); + } + return toNewClassesSet(excludedClasses, configured); + } + + private SecurityMemberAccessConfig configWith(boolean devMode, String excludedClasses, String devModeExcludedClasses) { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode(String.valueOf(devMode)); + config.useExcludedClasses(excludedClasses); + config.useDevModeExcludedClasses(devModeExcludedClasses); + config.init(); + return config; + } + + @Test + public void excludedClassesMatchLegacyAccumulation() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder"); + config.init(); + + assertEquals(legacyExcludedClassAccumulation(true, "java.lang.Runtime,java.lang.ProcessBuilder"), + config.getExcludedClasses()); + } + + @Test + public void disallowingStaticFieldAccessAddsClassToExclusions() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useAllowStaticFieldAccess("false"); + config.useExcludedClasses("java.lang.Runtime"); + config.init(); + + assertFalse(config.isAllowStaticFieldAccess()); + assertEquals(legacyExcludedClassAccumulation(false, "java.lang.Runtime"), config.getExcludedClasses()); + } + + /** + * The container iterates getDeclaredMethods(), whose order the JDK leaves unspecified. + * The accumulation must therefore be commutative, as it was before WW-5675. + */ + @Test + public void setterOrderDoesNotAffectExcludedClasses() { + SecurityMemberAccessConfig forward = new SecurityMemberAccessConfig(); + forward.useAllowStaticFieldAccess("false"); + forward.useExcludedClasses("java.lang.Runtime"); + forward.init(); + + SecurityMemberAccessConfig reverse = new SecurityMemberAccessConfig(); + reverse.useExcludedClasses("java.lang.Runtime"); + reverse.useAllowStaticFieldAccess("false"); + reverse.init(); + + assertEquals(forward.getExcludedClasses(), reverse.getExcludedClasses()); + } + + @Test + public void devModeDisabledPublishesNormalExclusions() { + SecurityMemberAccessConfig config = configWith(false, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + assertFalse(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + } + + @Test + public void devModeEnabledPublishesDevModeExclusions() { + SecurityMemberAccessConfig config = configWith(true, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + assertFalse(config.getExcludedClasses().contains("java.lang.Runtime")); + } + + @Test + public void packageNamesAreStrippedOfDots() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNames("java.io.,.java.net"); + config.init(); + + assertTrue(config.getExcludedPackageNames().contains("java.io")); + assertTrue(config.getExcludedPackageNames().contains("java.net")); + } + + @Test + public void patternsAreCompiledOnce() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNamePatterns("^java\\.lang\\..*"); + config.init(); + + Set patterns = config.getExcludedPackageNamePatterns(); + assertEquals(1, patterns.size()); + assertTrue(patterns.iterator().next().matcher("java.lang.Runtime").matches()); + } + + /** + * A missing init() must fail closed: production exclusions, never the dev-mode ones. + */ + @Test + public void withoutInitTheNormalExclusionsApply() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode("true"); + config.useExcludedClasses("java.lang.Runtime"); + config.useDevModeExcludedClasses("java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigTest` +Expected: FAIL — compilation error, `SecurityMemberAccessConfig` does not exist. + +- [ ] **Step 3: Create the bean** + +Create `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java` with the ASF license header copied verbatim from `SecurityMemberAccess.java` lines 1-18. + +Note that `init()` overwrites the normal fields with the dev-mode ones, exactly mirroring the `useDevModeConfiguration()` method it replaces. This is deliberate: if `init()` never runs, the normal production exclusions remain in force, which fails closed. + +```java +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.Set; +import java.util.regex.Pattern; + +import static java.util.Collections.emptySet; +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. + *

+ * {@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. + *

+ * 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); + + private boolean allowStaticFieldAccess = true; + + private Set excludedClasses = Set.of(Object.class.getName()); + private Set excludedPackageNamePatterns = emptySet(); + private Set excludedPackageNames = emptySet(); + private Set excludedPackageExemptClasses = emptySet(); + + private boolean isDevMode; + private Set devModeExcludedClasses = Set.of(Object.class.getName()); + private Set devModeExcludedPackageNamePatterns = emptySet(); + private Set devModeExcludedPackageNames = emptySet(); + private Set devModeExcludedPackageExemptClasses = emptySet(); + + private boolean enforceAllowlistEnabled = false; + private Set> allowlistClasses = emptySet(); + private Set allowlistPackageNames = emptySet(); + + 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); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) + public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { + this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void useDevMode(String devMode) { + this.isDevMode = BooleanUtils.toBoolean(devMode); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) + public void useDevModeExcludedClasses(String commaDelimitedClasses) { + this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) + public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { + this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + } + + public boolean isAllowStaticFieldAccess() { + return allowStaticFieldAccess; + } + + public Set getExcludedClasses() { + return excludedClasses; + } + + public Set getExcludedPackageNamePatterns() { + return excludedPackageNamePatterns; + } + + public Set getExcludedPackageNames() { + return excludedPackageNames; + } + + public Set getExcludedPackageExemptClasses() { + return excludedPackageExemptClasses; + } + + public boolean isEnforceAllowlistEnabled() { + return enforceAllowlistEnabled; + } + + public Set> getAllowlistClasses() { + return allowlistClasses; + } + + public Set getAllowlistPackageNames() { + return allowlistPackageNames; + } + + public boolean isDisallowProxyObjectAccess() { + return disallowProxyObjectAccess; + } + + public boolean isDisallowProxyMemberAccess() { + return disallowProxyMemberAccess; + } + + public boolean isDisallowDefaultPackageAccess() { + return disallowDefaultPackageAccess; + } +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigTest` +Expected: PASS, 9 tests. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java +git diff --cached --name-only +git commit -m "WW-5675 feat(ognl): add a container-singleton OGNL security config bean + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 3: Register the bean and wire it into `SecurityMemberAccess` + +`SecurityMemberAccess` starts reading the shared configuration. Its own setters lose `@Inject` and become deprecated, but keep mutating the instance so the roughly 110 existing direct call sites behave identically. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java:416-420` +- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:473-536` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` + +**Interfaces:** +- Consumes: every getter from Task 2. +- Produces: `public void useConfig(SecurityMemberAccessConfig config)` on `SecurityMemberAccess`, annotated `@Inject`. Task 4 removes the dev-mode setters; Task 5 changes the allowlist walk. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` with the ASF license header copied verbatim from `SecurityMemberAccess.java` lines 1-18. + +`XWorkTestCase` lives in `core/src/main/java/org/apache/struts2/XWorkTestCase.java` and exposes `protected Container container`. It is a JUnit 3 style `TestCase`, so test methods must be named `testXxx` — an `@Test` annotation alone will not run them. + +```java +package org.apache.struts2.ognl; + +import org.apache.struts2.XWorkTestCase; + +import java.util.Set; + +public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { + + /** + * Reference identity proves no re-parsing occurred: any re-parse necessarily + * allocates a fresh set. + */ + public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { + SecurityMemberAccess first = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess second = container.getInstance(SecurityMemberAccess.class); + + assertNotSame("expected a prototype bean", first, second); + + Set firstExcluded = SecurityMemberAccessTest.reflectField(first, "excludedClasses"); + Set secondExcluded = SecurityMemberAccessTest.reflectField(second, "excludedClasses"); + assertSame("excluded classes were re-parsed per instance", firstExcluded, secondExcluded); + + Set firstPackages = SecurityMemberAccessTest.reflectField(first, "excludedPackageNames"); + Set secondPackages = SecurityMemberAccessTest.reflectField(second, "excludedPackageNames"); + assertSame("excluded package names were re-parsed per instance", firstPackages, secondPackages); + } + + public void testConfigBeanIsASingleton() { + assertSame(container.getInstance(SecurityMemberAccessConfig.class), + container.getInstance(SecurityMemberAccessConfig.class)); + } + + /** + * The shared sets must not be perturbed by a deprecated setter call on one instance. + */ + public void testDeprecatedSetterDoesNotLeakToSiblings() throws Exception { + SecurityMemberAccess mutated = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess untouched = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccessConfig config = container.getInstance(SecurityMemberAccessConfig.class); + + Set before = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + mutated.useExcludedClasses("java.lang.Runtime"); + Set after = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + + assertSame("a sibling instance was affected", before, after); + assertFalse("the shared config was mutated", config.getExcludedClasses().contains("java.lang.Runtime")); + + Set mutatedSet = SecurityMemberAccessTest.reflectField(mutated, "excludedClasses"); + assertTrue("the setter did not affect its own instance", mutatedSet.contains("java.lang.Runtime")); + } + + /** + * Guards the fail-open hole avoided by using setter rather than constructor injection: + * a subclass calling the two-argument super constructor must still receive the config. + */ + public void testSubclassReceivesConfigThroughInheritedSetter() throws Exception { + SubclassedSecurityMemberAccess subclassed = new SubclassedSecurityMemberAccess( + container.getInstance(ProviderAllowlist.class), + container.getInstance(ThreadAllowlist.class)); + + container.inject(subclassed); + + Set excluded = SecurityMemberAccessTest.reflectField(subclassed, "excludedClasses"); + assertSame("subclass did not receive the shared config", + container.getInstance(SecurityMemberAccessConfig.class).getExcludedClasses(), excluded); + } + + static class SubclassedSecurityMemberAccess extends SecurityMemberAccess { + SubclassedSecurityMemberAccess(ProviderAllowlist providerAllowlist, ThreadAllowlist threadAllowlist) { + super(providerAllowlist, threadAllowlist); + } + } +} +``` + +`Container.inject(Object)` is declared at `core/src/main/java/org/apache/struts2/inject/Container.java:83`, so the call above drives the real injection path — including `ContainerImpl.addInjectors`, which recurses into superclasses at `ContainerImpl.java:97`. That recursion is exactly what this test exists to protect. + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest` +Expected: FAIL — `SecurityMemberAccessConfig` is not registered in the container, and the sets are still parsed per instance so `assertSame` fails. + +- [ ] **Step 3: Register the bean** + +In `core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java`, inside `bootstrapFactories`, add the registration immediately after the `ThreadAllowlist` line: + +```java + .factory(ProviderAllowlist.class, Scope.SINGLETON) + .factory(ThreadAllowlist.class, Scope.SINGLETON) + .factory(SecurityMemberAccessConfig.class, Scope.SINGLETON) +``` + +Add the import alongside the existing OGNL imports near line 123: + +```java +import org.apache.struts2.ognl.SecurityMemberAccessConfig; +``` + +- [ ] **Step 4: Add the config setter to `SecurityMemberAccess`** + +In `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`, add this method immediately after `setProxyService` (around line 113): + +```java + /** + * Copies the shared, already-parsed configuration into this instance. This is the only injected + * member that touches the configuration fields, so the unspecified order in which the container + * iterates {@code getDeclaredMethods()} cannot affect the result. + * + * @since Struts 7.4.0 + */ + @Inject + public void useConfig(SecurityMemberAccessConfig config) { + this.allowStaticFieldAccess = config.isAllowStaticFieldAccess(); + this.excludedClasses = config.getExcludedClasses(); + this.excludedPackageNamePatterns = config.getExcludedPackageNamePatterns(); + this.excludedPackageNames = config.getExcludedPackageNames(); + this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); + this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); + this.allowlistClasses = config.getAllowlistClasses(); + this.allowlistPackageNames = config.getAllowlistPackageNames(); + this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); + this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); + this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); + } +``` + +- [ ] **Step 5: Deprecate the eleven remaining setters** + +Still in `SecurityMemberAccess.java`, for each of these methods remove the `@Inject(...)` annotation and add `@Deprecated` plus a Javadoc `@deprecated` tag. Leave every method body exactly as it is. + +Apply to: `useAllowStaticFieldAccess`, `useExcludedClasses`, `useExcludedPackageNamePatterns`, `useExcludedPackageNames`, `useExcludedPackageExemptClasses`, `useEnforceAllowlistEnabled`, `useAllowlistClasses`, `useAllowlistPackageNames`, `useDisallowProxyObjectAccess`, `useDisallowProxyMemberAccess`, `useDisallowDefaultPackageAccess`. + +The pattern for each, shown for `useExcludedClasses`: + +```java + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated + public void useExcludedClasses(String commaDelimitedClasses) { + this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); + } +``` + +Do **not** annotate `useDevMode` or the four `useDevModeExcluded*` methods — Task 4 deletes those. Do **not** touch `useAcceptProperties` or `useExcludeProperties`; they carry per-request state, not configuration, and are not deprecated. + +Deprecation is by annotation only. Do not add runtime warnings — roughly 110 test call sites would flood the build output. + +- [ ] **Step 6: Run the new test** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest` +Expected: PASS, 4 tests. + +- [ ] **Step 7: Run the existing security suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='SecurityMemberAccessTest,ExternalSecurityMemberAccessTest,OgnlUtilTest,OgnlValueStackTest'` +Expected: PASS. Surefire needs comma separation; `+` is not valid. + +If `SecurityMemberAccessTest` fails, the deprecated setters have not kept their exact semantics — fix the setter, not the test. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +git diff --cached --name-only +git commit -m "WW-5675 perf(ognl): share parsed config across SecurityMemberAccess instances + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 4: Delete the dev-mode state from `SecurityMemberAccess` + +The lazy dev-mode flip runs on the access path today. With dev-mode resolved once by the config bean, all of it goes. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:89-94, 264, 538-574` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` + +**Interfaces:** +- Consumes: `SecurityMemberAccessConfig` getters, which already publish dev-mode-resolved sets. +- Produces: removal only. `useDevMode`, `useDevModeExcludedClasses`, `useDevModeExcludedPackageNamePatterns`, `useDevModeExcludedPackageNames`, `useDevModeExcludedPackageExemptClasses` and `useDevModeConfiguration` no longer exist on `SecurityMemberAccess`. + +- [ ] **Step 1: Write the failing test** + +Append to `SecurityMemberAccessConfigSharingTest`: + +```java + /** + * Dev-mode exclusions must be in force from the first access, with no lazy flip. + */ + public void testDevModeExclusionsApplyWithoutAnAccess() throws Exception { + loadConfigurationProviders(new StubConfigurationProvider() { + @Override + public void register(ContainerBuilder builder, LocatableProperties props) { + props.setProperty(StrutsConstants.STRUTS_DEVMODE, "true"); + props.setProperty(StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, "java.lang.ProcessBuilder"); + } + }); + + SecurityMemberAccess sma = container.getInstance(SecurityMemberAccess.class); + Set excluded = SecurityMemberAccessTest.reflectField(sma, "excludedClasses"); + + assertTrue("dev-mode exclusions were not applied at startup", + excluded.contains("java.lang.ProcessBuilder")); + } + + public void testDevModeMethodsAreGone() throws Exception { + for (String name : new String[]{"useDevMode", "useDevModeExcludedClasses", + "useDevModeExcludedPackageNamePatterns", "useDevModeExcludedPackageNames", + "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"}) { + for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { + assertFalse("SecurityMemberAccess still declares " + name, method.getName().equals(name)); + } + } + } +``` + +Add these imports to the test file: + +```java +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.ContainerBuilder; +import org.apache.struts2.test.StubConfigurationProvider; +import org.apache.struts2.util.location.LocatableProperties; +``` + +Note `LocatableProperties` is in `org.apache.struts2.util.location`, not `org.apache.struts2.config.entities`. + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest#testDevModeMethodsAreGone` +Expected: FAIL — the methods still exist. + +- [ ] **Step 3: Delete the dev-mode state** + +In `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`: + +Delete these fields (lines 89-94): + +```java + private volatile boolean isDevModeInit; + private boolean isDevMode; + private Set devModeExcludedClasses = Set.of(Object.class.getName()); + private Set devModeExcludedPackageNamePatterns = emptySet(); + private Set devModeExcludedPackageNames = emptySet(); + private Set devModeExcludedPackageExemptClasses = emptySet(); +``` + +Delete the `useDevModeConfiguration()` method entirely, and its call at the top of `checkExclusionList` so that the method begins: + +```java + protected boolean checkExclusionList(Object target, Member member) { + Class memberClass = member.getDeclaringClass(); +``` + +Delete the five dev-mode setters: `useDevMode`, `useDevModeExcludedClasses`, `useDevModeExcludedPackageNamePatterns`, `useDevModeExcludedPackageNames`, `useDevModeExcludedPackageExemptClasses`. + +- [ ] **Step 4: Run the tests** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest` +Expected: PASS, 6 tests. + +- [ ] **Step 5: Run the existing security suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='SecurityMemberAccessTest,ExternalSecurityMemberAccessTest'` +Expected: PASS. If a test called a dev-mode setter directly, the earlier survey was wrong — stop and report rather than deleting the test. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +git diff --cached --name-only +git commit -m "WW-5675 refactor(ognl): drop the lazy dev-mode flip from the access path + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 5: Collapse the allowlist two-set walk into one precomputed union + +WW-5674 merged the two allowlist walks with a three-argument helper. With the configuration shared, the union can be precomputed, so the helper reverts to a single set. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:257-263, 392-441` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java` + +**Interfaces:** +- Consumes: `SecurityMemberAccessConfig.getAllowlistPackageNames()`. +- Produces: `static boolean isPackageBelongsToPackages(String packageName, Set matchingPackages)` — two arguments, replacing the three-argument form. The three-argument `isClassBelongsToPackages(Class, Set, Set)` overload is deleted. + +- [ ] **Step 1: Write the failing test** + +Append to `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`. Do not modify or delete the frozen oracles already in that file. + +```java + /** + * The union must never lose ALLOWLIST_REQUIRED_PACKAGES. Dropping them would be a silent + * fail-open: Struts' own components would stop being allowlisted with nothing failing loudly. + */ + @Test + public void allowlistUnionRetainsRequiredPackagesAfterSetterCall() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + sma.useAllowlistPackageNames("com.example.app"); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertTrue("configured package missing", union.contains("com.example.app")); + assertTrue("required Struts package dropped from the allowlist", + union.contains("org.apache.struts2.components")); + } + + @Test + public void allowlistUnionContainsRequiredPackagesByDefault() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertTrue(union.contains("org.apache.struts2.components")); + assertTrue(union.contains("org.apache.struts2.views.jsp")); + assertTrue(union.contains("org.apache.struts2.validator.validators")); + } + + @Test + public void singleSetWalkMatchesTheFrozenOracle() { + for (String packageName : PACKAGE_NAMES) { + for (Set candidates : CANDIDATE_SETS) { + assertEquals("mismatch for " + packageName + " against " + candidates, + legacyPrefixMatch(packageName, candidates), + SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates)); + } + } + } +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest` +Expected: FAIL — `allowlistPackageNamesUnion` does not exist and `isPackageBelongsToPackages` still takes three arguments. + +- [ ] **Step 3: Add the union field and its single computation site** + +In `SecurityMemberAccess.java`, replace the allowlist field declarations (around line 97-98): + +```java + private Set> allowlistClasses = emptySet(); + private Set allowlistPackageNames = emptySet(); + private Set allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; +``` + +The union defaults to `ALLOWLIST_REQUIRED_PACKAGES` so that an instance which never receives configuration — the eight direct `new SecurityMemberAccess(null, null)` test constructions — still allowlists Struts' own packages, exactly as before. + +Add the single computation site and its helper: + +```java + /** + * The only place the allowlist union is computed. Both the injected configuration and the + * deprecated setter route through here; splitting this in two would risk silently dropping + * {@code ALLOWLIST_REQUIRED_PACKAGES}, which fails open. + */ + private void applyAllowlistPackageNames(Set packageNames) { + this.allowlistPackageNames = packageNames; + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, packageNames); + } + + private static Set union(Set required, Set configured) { + if (configured.isEmpty()) { + return required; + } + Set union = new HashSet<>(required); + union.addAll(configured); + return unmodifiableSet(union); + } +``` + +Add these imports: + +```java +import java.util.HashSet; + +import static java.util.Collections.unmodifiableSet; +``` + +- [ ] **Step 4: Route both callers through it** + +In `useConfig`, replace the direct assignment: + +```java + this.allowlistPackageNames = config.getAllowlistPackageNames(); +``` + +with: + +```java + applyAllowlistPackageNames(config.getAllowlistPackageNames()); +``` + +In the deprecated `useAllowlistPackageNames`, replace the body: + +```java + @Deprecated + public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); + } +``` + +- [ ] **Step 5: Collapse the walk** + +Replace the last clause of `isClassAllowlisted`: + +```java + || isClassBelongsToPackages(clazz, allowlistPackageNamesUnion); +``` + +Delete the three-argument `isClassBelongsToPackages(Class, Set, Set)` overload entirely, and rewrite the remaining pair: + +```java + public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { + return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages); + } + + /** + * Tests whether the given package name, or any of its parent packages, is present in the set. + * Walks the name in place rather than building the full prefix list, since this runs on the OGNL + * member-access path. Shortest prefix first, so broad entries such as {@code java.io} + * short-circuit earliest. + * + *

+ * The package name must not end in {@code '.'}. Such a name is probed one prefix more than by the + * implementation this replaced, which matches more broadly — tightening exclusion but + * loosening the allowlist. {@link Class#getPackageName()} cannot produce a trailing dot, + * and {@code ConfigParseUtil.toPackageNamesSet} strips them from configured names, so every + * current caller is safe; route any other string through here only after confirming the same. + * + * @param packageName the package name to test, empty for the default package, never ending in {@code '.'} + * @param matchingPackages the package names to match against + * @return {@code true} if the package or any parent package is in the set + */ + static boolean isPackageBelongsToPackages(String packageName, Set matchingPackages) { + if (matchingPackages.isEmpty()) { + return false; + } + int idx = packageName.indexOf('.'); + while (idx != -1) { + if (matchingPackages.contains(packageName.substring(0, idx))) { + return true; + } + idx = packageName.indexOf('.', idx + 1); + } + return matchingPackages.contains(packageName); + } +``` + +Remove the now-unused `emptySet` static import only if no other usage remains — check with `grep -n "emptySet" core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`. + +- [ ] **Step 6: Run the package-matching tests** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest` +Expected: PASS, 12 tests. + +- [ ] **Step 7: Run the security suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='SecurityMemberAccessTest,ExternalSecurityMemberAccessTest,SecurityMemberAccessConfigSharingTest,SecurityMemberAccessConfigTest'` +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +git diff --cached --name-only +git commit -m "WW-5675 perf(ognl): precompute the allowlist package union + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 6: Verify the whole module and the plugins + +**Files:** none modified unless a failure is found. + +**Interfaces:** none. + +- [ ] **Step 1: Run the full core suite** + +Run: `mvn test -DskipAssembly -pl core` +Expected: PASS with zero failures and zero errors. For reference, the suite stood at 3158 tests when WW-5674 merged; this plan adds roughly 15. + +Record the actual counts. Do not describe the work as complete without this output in hand. + +- [ ] **Step 2: Run the plugin suites that touch `SecurityMemberAccess`** + +Run: `mvn test -DskipAssembly -pl plugins/spring,plugins/cdi` +Expected: PASS. Both modules construct `new SecurityMemberAccess(null, null)` directly in proxy tests, which exercises the un-injected path. + +- [ ] **Step 3: Confirm no stray `@Inject` remains on the deprecated setters** + +Run: + +```bash +grep -n -B2 "public void use" core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java | grep -A2 "@Inject" +``` + +Expected: only `useConfig` and `setProxyService` appear. Any other hit means a setter kept its annotation and will still be injected per instance, silently defeating the change. + +- [ ] **Step 4: Confirm the dev-mode state is gone** + +Run: + +```bash +grep -n "devMode\|DevMode" core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +``` + +Expected: no matches. + +- [ ] **Step 5: Commit any fixes, then push and open a draft PR** + +Only if Steps 1-4 are clean: + +```bash +git push -u origin WW-5675-share-parsed-ognl-security-config +``` + +Open a draft PR titled `WW-5675 Share parsed OGNL security configuration across SecurityMemberAccess instances`, with `Fixes [WW-5675](https://issues.apache.org/jira/browse/WW-5675)` in the description. + +The PR description must state plainly that removing the five dev-mode setters is a source-breaking change in a minor release, why it was accepted, and that a Migration Guide entry is owed for 7.4.0. + +--- + +## Follow-ups (not part of this plan) + +- File a Jira Improvement for 8.0.0 to remove the eleven deprecated setters, cross-referencing WW-5675 and WW-5678. +- Add the Version Notes and Migration Guide entry for the dev-mode setter removal. +- Update WW-5667 to record that this ticket, not WW-5674, is the one expected to move the reported 9%. +- WW-5678's first item is resolved here by Task 5; the remaining visibility narrowing stays with that ticket. From 66113f81d6fed3f89d2b583a1212772a04ae1cb0 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:05:55 +0200 Subject: [PATCH 05/18] WW-5675 docs(ognl): fix Task 5 to handle the tests the signature change breaks Co-Authored-By: Claude Opus 5 --- ...-5675-share-parsed-ognl-security-config.md | 60 +++++++++++++------ 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md b/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md index 4ce8dbc8e0..bb6361f024 100644 --- a/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md +++ b/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md @@ -852,7 +852,41 @@ WW-5674 merged the two allowlist walks with a three-argument helper. With the co - [ ] **Step 1: Write the failing test** -Append to `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`. Do not modify or delete the frozen oracles already in that file. +Work in `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`. **Do not modify or delete the frozen oracles** (`legacyPrefixMatch`, `legacyToPackageName`) or the `PACKAGE_NAMES` / `CANDIDATE_SETS` / `classShapes()` fixtures. This file uses AssertJ (`assertThat`), not JUnit assertions — match that style. + +Narrowing the signatures in this task breaks three tests already in the file. Handle them exactly as follows; do not improvise, and do not delete a test merely because it fails to compile. + +**a. Convert `indexWalkMatchesLegacyAcrossPackageNameShapes` to the two-argument walk:** + +```java + @Test + public void indexWalkMatchesLegacyAcrossPackageNameShapes() { + for (String packageName : PACKAGE_NAMES) { + for (Set candidates : CANDIDATE_SETS) { + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates)) + .as("packageName=[%s] candidates=%s", packageName, candidates) + .isEqualTo(legacyPrefixMatch(packageName, candidates)); + } + } + } +``` + +**b. Convert `bothSetsEmptyShortCircuitsToFalse`, renaming it since there is now one set:** + +```java + @Test + public void emptyCandidateSetShortCircuitsToFalse() { + for (String packageName : PACKAGE_NAMES) { + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet())) + .as("packageName=[%s] with no configured packages", packageName) + .isFalse(); + } + } +``` + +**c. Delete `twoSetOverloadEqualsDisjunctionOfSingleSetCalls` entirely.** It characterises the equivalence of the three-argument overload against two single-set calls; that overload no longer exists, so the property it asserts is gone. This is the one test in this file you may remove. + +**d. Append these two new tests:** ```java /** @@ -866,9 +900,7 @@ Append to `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackag Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); - assertTrue("configured package missing", union.contains("com.example.app")); - assertTrue("required Struts package dropped from the allowlist", - union.contains("org.apache.struts2.components")); + assertThat(union).contains("com.example.app", "org.apache.struts2.components"); } @Test @@ -877,20 +909,10 @@ Append to `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackag Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); - assertTrue(union.contains("org.apache.struts2.components")); - assertTrue(union.contains("org.apache.struts2.views.jsp")); - assertTrue(union.contains("org.apache.struts2.validator.validators")); - } - - @Test - public void singleSetWalkMatchesTheFrozenOracle() { - for (String packageName : PACKAGE_NAMES) { - for (Set candidates : CANDIDATE_SETS) { - assertEquals("mismatch for " + packageName + " against " + candidates, - legacyPrefixMatch(packageName, candidates), - SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates)); - } - } + assertThat(union).contains( + "org.apache.struts2.components", + "org.apache.struts2.views.jsp", + "org.apache.struts2.validator.validators"); } ``` @@ -1017,7 +1039,7 @@ Remove the now-unused `emptySet` static import only if no other usage remains - [ ] **Step 6: Run the package-matching tests** Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest` -Expected: PASS, 12 tests. +Expected: PASS. The file had 9 tests; one is deleted and two are added, so expect 10. - [ ] **Step 7: Run the security suites** From 75b162f9613abc55b6d8db8c250fefc80e9f5e18 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:07:39 +0200 Subject: [PATCH 06/18] WW-5675 perf(config): hoist the whitespace pattern in validatePackageNames Co-Authored-By: Claude Opus 5 --- .../apache/struts2/util/ConfigParseUtil.java | 4 ++- .../struts2/util/ConfigParseUtilTest.java | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java b/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java index 116ca637dd..06dc1a43c5 100644 --- a/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java +++ b/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java @@ -47,6 +47,8 @@ public class ConfigParseUtil { .maximumSize(MAX_CLASSLOADER_CACHE_SIZE) .build(); + private static final Pattern WHITESPACE = Pattern.compile("\\s"); + private ConfigParseUtil() { } @@ -140,7 +142,7 @@ public static Set toNewPackageNamesSet(Collection oldPackageName } public static void validatePackageNames(Collection packageNames) { - if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) { + if (packageNames.stream().anyMatch(s -> WHITESPACE.matcher(s).find())) { throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames); } } diff --git a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java index 84cf9a0a33..6e9c79727e 100644 --- a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java @@ -29,9 +29,11 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.Set; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -187,6 +189,34 @@ public String toString() { innerCache.estimatedSize() <= limit); } + @Test + public void validatePackageNamesAcceptsNamesWithoutWhitespace() { + ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache.struts2", "")); + } + + @Test + public void validatePackageNamesRejectsSpace() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache struts2"))); + } + + @Test + public void validatePackageNamesRejectsTab() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java\tlang"))); + } + + @Test + public void validatePackageNamesRejectsNewline() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java\nlang"))); + } + + @Test + public void validatePackageNamesAcceptsEmptyCollection() { + ConfigParseUtil.validatePackageNames(List.of()); + } + @SuppressWarnings("unchecked") private static Cache validatedClassCache() { try { From 32ceb19246cd905d7b685b084f0625c78abad4a8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:11:32 +0200 Subject: [PATCH 07/18] WW-5675 feat(ognl): add a container-singleton OGNL security config bean Co-Authored-By: Claude Opus 5 --- .../ognl/SecurityMemberAccessConfig.java | 227 ++++++++++++++++++ .../ognl/SecurityMemberAccessConfigTest.java | 143 +++++++++++ 2 files changed, 370 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java create mode 100644 core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java new file mode 100644 index 0000000000..6009738307 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java @@ -0,0 +1,227 @@ +/* + * 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.Set; +import java.util.regex.Pattern; + +import static java.util.Collections.emptySet; +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. + *

+ * {@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. + *

+ * 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); + + private boolean allowStaticFieldAccess = true; + + private Set excludedClasses = Set.of(Object.class.getName()); + private Set excludedPackageNamePatterns = emptySet(); + private Set excludedPackageNames = emptySet(); + private Set excludedPackageExemptClasses = emptySet(); + + private boolean isDevMode; + private Set devModeExcludedClasses = Set.of(Object.class.getName()); + private Set devModeExcludedPackageNamePatterns = emptySet(); + private Set devModeExcludedPackageNames = emptySet(); + private Set devModeExcludedPackageExemptClasses = emptySet(); + + private boolean enforceAllowlistEnabled = false; + private Set> allowlistClasses = emptySet(); + private Set allowlistPackageNames = emptySet(); + + 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); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) + public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { + this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void useDevMode(String devMode) { + this.isDevMode = BooleanUtils.toBoolean(devMode); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) + public void useDevModeExcludedClasses(String commaDelimitedClasses) { + this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) + public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { + this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + } + + public boolean isAllowStaticFieldAccess() { + return allowStaticFieldAccess; + } + + public Set getExcludedClasses() { + return excludedClasses; + } + + public Set getExcludedPackageNamePatterns() { + return excludedPackageNamePatterns; + } + + public Set getExcludedPackageNames() { + return excludedPackageNames; + } + + public Set getExcludedPackageExemptClasses() { + return excludedPackageExemptClasses; + } + + public boolean isEnforceAllowlistEnabled() { + return enforceAllowlistEnabled; + } + + public Set> getAllowlistClasses() { + return allowlistClasses; + } + + public Set getAllowlistPackageNames() { + return allowlistPackageNames; + } + + public boolean isDisallowProxyObjectAccess() { + return disallowProxyObjectAccess; + } + + public boolean isDisallowProxyMemberAccess() { + return disallowProxyMemberAccess; + } + + public boolean isDisallowDefaultPackageAccess() { + return disallowDefaultPackageAccess; + } +} diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java new file mode 100644 index 0000000000..e95e99e081 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java @@ -0,0 +1,143 @@ +/* + * 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.junit.Test; + +import java.util.Set; +import java.util.regex.Pattern; + +import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class SecurityMemberAccessConfigTest { + + /** + * Frozen oracle: the accumulation SecurityMemberAccess performed before WW-5675. + * Never delete this, and never make it delegate to production code. + */ + private static Set legacyExcludedClassAccumulation(boolean allowStaticFieldAccess, String configured) { + Set excludedClasses = Set.of(Object.class.getName()); + if (!allowStaticFieldAccess) { + excludedClasses = toNewClassesSet(excludedClasses, Class.class.getName()); + } + return toNewClassesSet(excludedClasses, configured); + } + + private SecurityMemberAccessConfig configWith(boolean devMode, String excludedClasses, String devModeExcludedClasses) { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode(String.valueOf(devMode)); + config.useExcludedClasses(excludedClasses); + config.useDevModeExcludedClasses(devModeExcludedClasses); + config.init(); + return config; + } + + @Test + public void excludedClassesMatchLegacyAccumulation() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder"); + config.init(); + + assertEquals(legacyExcludedClassAccumulation(true, "java.lang.Runtime,java.lang.ProcessBuilder"), + config.getExcludedClasses()); + } + + @Test + public void disallowingStaticFieldAccessAddsClassToExclusions() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useAllowStaticFieldAccess("false"); + config.useExcludedClasses("java.lang.Runtime"); + config.init(); + + assertFalse(config.isAllowStaticFieldAccess()); + assertEquals(legacyExcludedClassAccumulation(false, "java.lang.Runtime"), config.getExcludedClasses()); + } + + /** + * The container iterates getDeclaredMethods(), whose order the JDK leaves unspecified. + * The accumulation must therefore be commutative, as it was before WW-5675. + */ + @Test + public void setterOrderDoesNotAffectExcludedClasses() { + SecurityMemberAccessConfig forward = new SecurityMemberAccessConfig(); + forward.useAllowStaticFieldAccess("false"); + forward.useExcludedClasses("java.lang.Runtime"); + forward.init(); + + SecurityMemberAccessConfig reverse = new SecurityMemberAccessConfig(); + reverse.useExcludedClasses("java.lang.Runtime"); + reverse.useAllowStaticFieldAccess("false"); + reverse.init(); + + assertEquals(forward.getExcludedClasses(), reverse.getExcludedClasses()); + } + + @Test + public void devModeDisabledPublishesNormalExclusions() { + SecurityMemberAccessConfig config = configWith(false, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + assertFalse(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + } + + @Test + public void devModeEnabledPublishesDevModeExclusions() { + SecurityMemberAccessConfig config = configWith(true, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + assertFalse(config.getExcludedClasses().contains("java.lang.Runtime")); + } + + @Test + public void packageNamesAreStrippedOfDots() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNames("java.io.,.java.net"); + config.init(); + + assertTrue(config.getExcludedPackageNames().contains("java.io")); + assertTrue(config.getExcludedPackageNames().contains("java.net")); + } + + @Test + public void patternsAreCompiledOnce() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNamePatterns("^java\\.lang\\..*"); + config.init(); + + Set patterns = config.getExcludedPackageNamePatterns(); + assertEquals(1, patterns.size()); + assertTrue(patterns.iterator().next().matcher("java.lang.Runtime").matches()); + } + + /** + * A missing init() must fail closed: production exclusions, never the dev-mode ones. + */ + @Test + public void withoutInitTheNormalExclusionsApply() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode("true"); + config.useExcludedClasses("java.lang.Runtime"); + config.useDevModeExcludedClasses("java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + } +} From cac69c15c303a7d684d2007eaac45f800716b999 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:17:07 +0200 Subject: [PATCH 08/18] WW-5675 perf(ognl): share parsed config across SecurityMemberAccess instances Co-Authored-By: Claude Opus 5 --- .../config/impl/DefaultConfiguration.java | 2 + .../struts2/ognl/SecurityMemberAccess.java | 99 ++++++++++++++++--- ...SecurityMemberAccessConfigSharingTest.java | 91 +++++++++++++++++ 3 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java 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 dcf4f1602e..53bf20dc85 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 @@ -121,6 +121,7 @@ import org.apache.struts2.factory.StrutsResultFactory; import org.apache.struts2.ognl.OgnlGuard; import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.ognl.SecurityMemberAccessConfig; import org.apache.struts2.ognl.StrutsOgnlGuard; import org.apache.struts2.ognl.ThreadAllowlist; @@ -417,6 +418,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) .factory(ProviderAllowlist.class, Scope.SINGLETON) .factory(ThreadAllowlist.class, Scope.SINGLETON) + .factory(SecurityMemberAccessConfig.class, Scope.SINGLETON) .factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); } diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index badad3dee3..8ba19f5405 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -112,6 +112,28 @@ public void setProxyService(ProxyService proxyService) { this.proxyService = proxyService; } + /** + * Copies the shared, already-parsed configuration into this instance. This is the only injected + * member that touches the configuration fields, so the unspecified order in which the container + * iterates {@code getDeclaredMethods()} cannot affect the result. + * + * @since Struts 7.4.0 + */ + @Inject + public void useConfig(SecurityMemberAccessConfig config) { + this.allowStaticFieldAccess = config.isAllowStaticFieldAccess(); + this.excludedClasses = config.getExcludedClasses(); + this.excludedPackageNamePatterns = config.getExcludedPackageNamePatterns(); + this.excludedPackageNames = config.getExcludedPackageNames(); + this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); + this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); + this.allowlistClasses = config.getAllowlistClasses(); + this.allowlistPackageNames = config.getAllowlistPackageNames(); + this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); + this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); + this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); + } + @Override public Object setup(OgnlContext context, Object target, Member member, String propertyName) { Object result = null; @@ -470,7 +492,12 @@ public void useAcceptProperties(Set acceptedProperties) { this.acceptProperties = acceptedProperties; } - @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); if (!this.allowStaticFieldAccess) { @@ -478,27 +505,52 @@ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { } } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useExcludedClasses(String commaDelimitedClasses) { this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useExcludedPackageNames(String commaDelimitedPackageNames) { this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } - @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); if (!this.enforceAllowlistEnabled) { @@ -510,27 +562,52 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { } } - @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useAllowlistClasses(String commaDelimitedClasses) { this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); } - @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useAllowlistPackageNames(String commaDelimitedPackageNames) { this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java new file mode 100644 index 0000000000..b6af1db29f --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -0,0 +1,91 @@ +/* + * 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.struts2.XWorkTestCase; + +import java.util.Set; + +public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { + + /** + * Reference identity proves no re-parsing occurred: any re-parse necessarily + * allocates a fresh set. + */ + public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { + SecurityMemberAccess first = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess second = container.getInstance(SecurityMemberAccess.class); + + assertNotSame("expected a prototype bean", first, second); + + Set firstExcluded = SecurityMemberAccessTest.reflectField(first, "excludedClasses"); + Set secondExcluded = SecurityMemberAccessTest.reflectField(second, "excludedClasses"); + assertSame("excluded classes were re-parsed per instance", firstExcluded, secondExcluded); + + Set firstPackages = SecurityMemberAccessTest.reflectField(first, "excludedPackageNames"); + Set secondPackages = SecurityMemberAccessTest.reflectField(second, "excludedPackageNames"); + assertSame("excluded package names were re-parsed per instance", firstPackages, secondPackages); + } + + public void testConfigBeanIsASingleton() { + assertSame(container.getInstance(SecurityMemberAccessConfig.class), + container.getInstance(SecurityMemberAccessConfig.class)); + } + + /** + * The shared sets must not be perturbed by a deprecated setter call on one instance. + */ + public void testDeprecatedSetterDoesNotLeakToSiblings() throws Exception { + SecurityMemberAccess mutated = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess untouched = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccessConfig config = container.getInstance(SecurityMemberAccessConfig.class); + + Set before = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + mutated.useExcludedClasses("java.lang.Runtime"); + Set after = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + + assertSame("a sibling instance was affected", before, after); + assertFalse("the shared config was mutated", config.getExcludedClasses().contains("java.lang.Runtime")); + + Set mutatedSet = SecurityMemberAccessTest.reflectField(mutated, "excludedClasses"); + assertTrue("the setter did not affect its own instance", mutatedSet.contains("java.lang.Runtime")); + } + + /** + * Guards the fail-open hole avoided by using setter rather than constructor injection: + * a subclass calling the two-argument super constructor must still receive the config. + */ + public void testSubclassReceivesConfigThroughInheritedSetter() throws Exception { + SubclassedSecurityMemberAccess subclassed = new SubclassedSecurityMemberAccess( + container.getInstance(ProviderAllowlist.class), + container.getInstance(ThreadAllowlist.class)); + + container.inject(subclassed); + + Set excluded = SecurityMemberAccessTest.reflectField(subclassed, "excludedClasses"); + assertSame("subclass did not receive the shared config", + container.getInstance(SecurityMemberAccessConfig.class).getExcludedClasses(), excluded); + } + + static class SubclassedSecurityMemberAccess extends SecurityMemberAccess { + SubclassedSecurityMemberAccess(ProviderAllowlist providerAllowlist, ThreadAllowlist threadAllowlist) { + super(providerAllowlist, threadAllowlist); + } + } +} From 02fa6aa9e8248e6745951fbee629cf77849124c8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:29:59 +0200 Subject: [PATCH 09/18] WW-5675 test(ognl): strengthen SecurityMemberAccessConfigSharingTest assertions Two review findings: (1) the config-derived-set assertions compared instance to instance only, which is vacuous for every emptySet()-defaulted field since Collections.emptySet() is a JVM-wide singleton shared by both the config bean's own default and SecurityMemberAccess's own default; deleting a useConfig assignment for such a field would still pass. Fixed by additionally asserting each field directly against the shared SecurityMemberAccessConfig bean, with the container reloaded to set every relevant constant away from its hardcoded default so the comparison is not itself vacuous by coincidence. (2) testConfigBeanIsASingleton passed on assertSame(null, null) when the bean was not registered at all; added assertNotNull before the identity check. Co-Authored-By: Claude Opus 5 --- ...SecurityMemberAccessConfigSharingTest.java | 64 ++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java index b6af1db29f..5680e59fb9 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -18,8 +18,10 @@ */ package org.apache.struts2.ognl; +import org.apache.struts2.StrutsConstants; import org.apache.struts2.XWorkTestCase; +import java.util.Map; import java.util.Set; public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { @@ -27,10 +29,38 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { /** * Reference identity proves no re-parsing occurred: any re-parse necessarily * allocates a fresh set. + *

+ * The instance-to-instance {@code assertSame} calls below are necessary but not sufficient: + * for a field whose default is {@link java.util.Collections#emptySet()}, two independently + * unseeded instances would also compare same, since {@code emptySet()} returns a + * JVM-wide singleton. Only {@code excludedClasses}, whose default {@code Set.of(...)} allocates + * a fresh instance per object, is proven by the instance-to-instance form alone. Every field is + * therefore additionally compared directly against the shared {@link SecurityMemberAccessConfig} + * bean, which fails on omission regardless of the default's identity. + *

+ * That direct comparison is itself vacuous unless the configured value actually differs from the + * hardcoded default: {@code SecurityMemberAccess} and {@code SecurityMemberAccessConfig} share the + * same hardcoded defaults, so an unseeded field and a config parsed from an all-default container + * would also compare equal/same by coincidence. The container is therefore reloaded here with every + * relevant constant set away from its default, so a config value only matches the instance's field + * when {@code useConfig} actually ran. */ public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { + loadButSet(Map.of( + StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, "false", + StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, "^org\\.apache\\.struts2\\.ognl\\.testpkg\\..*", + StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg", + StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, "java.lang.String", + StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true", + StrutsConstants.STRUTS_ALLOWLIST_CLASSES, "java.lang.String", + StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg", + StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, "true", + StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, "true", + StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, "true")); + SecurityMemberAccess first = container.getInstance(SecurityMemberAccess.class); SecurityMemberAccess second = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccessConfig config = container.getInstance(SecurityMemberAccessConfig.class); assertNotSame("expected a prototype bean", first, second); @@ -41,11 +71,41 @@ public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { Set firstPackages = SecurityMemberAccessTest.reflectField(first, "excludedPackageNames"); Set secondPackages = SecurityMemberAccessTest.reflectField(second, "excludedPackageNames"); assertSame("excluded package names were re-parsed per instance", firstPackages, secondPackages); + + assertSame("excludedClasses not seeded from config", + config.getExcludedClasses(), SecurityMemberAccessTest.reflectField(first, "excludedClasses")); + assertSame("excludedPackageNamePatterns not seeded from config", + config.getExcludedPackageNamePatterns(), SecurityMemberAccessTest.reflectField(first, "excludedPackageNamePatterns")); + assertSame("excludedPackageNames not seeded from config", + config.getExcludedPackageNames(), SecurityMemberAccessTest.reflectField(first, "excludedPackageNames")); + assertSame("excludedPackageExemptClasses not seeded from config", + config.getExcludedPackageExemptClasses(), SecurityMemberAccessTest.reflectField(first, "excludedPackageExemptClasses")); + assertSame("allowlistClasses not seeded from config", + config.getAllowlistClasses(), SecurityMemberAccessTest.reflectField(first, "allowlistClasses")); + assertSame("allowlistPackageNames not seeded from config", + config.getAllowlistPackageNames(), SecurityMemberAccessTest.reflectField(first, "allowlistPackageNames")); + + boolean firstAllowStaticFieldAccess = SecurityMemberAccessTest.reflectField(first, "allowStaticFieldAccess"); + assertEquals("allowStaticFieldAccess not seeded from config", + config.isAllowStaticFieldAccess(), firstAllowStaticFieldAccess); + boolean firstEnforceAllowlistEnabled = SecurityMemberAccessTest.reflectField(first, "enforceAllowlistEnabled"); + assertEquals("enforceAllowlistEnabled not seeded from config", + config.isEnforceAllowlistEnabled(), firstEnforceAllowlistEnabled); + boolean firstDisallowProxyObjectAccess = SecurityMemberAccessTest.reflectField(first, "disallowProxyObjectAccess"); + assertEquals("disallowProxyObjectAccess not seeded from config", + config.isDisallowProxyObjectAccess(), firstDisallowProxyObjectAccess); + boolean firstDisallowProxyMemberAccess = SecurityMemberAccessTest.reflectField(first, "disallowProxyMemberAccess"); + assertEquals("disallowProxyMemberAccess not seeded from config", + config.isDisallowProxyMemberAccess(), firstDisallowProxyMemberAccess); + boolean firstDisallowDefaultPackageAccess = SecurityMemberAccessTest.reflectField(first, "disallowDefaultPackageAccess"); + assertEquals("disallowDefaultPackageAccess not seeded from config", + config.isDisallowDefaultPackageAccess(), firstDisallowDefaultPackageAccess); } public void testConfigBeanIsASingleton() { - assertSame(container.getInstance(SecurityMemberAccessConfig.class), - container.getInstance(SecurityMemberAccessConfig.class)); + SecurityMemberAccessConfig instance = container.getInstance(SecurityMemberAccessConfig.class); + assertNotNull("SecurityMemberAccessConfig is not registered in the container", instance); + assertSame(instance, container.getInstance(SecurityMemberAccessConfig.class)); } /** From cb3dff2ff2df3e375dbb2832de88f0d5be0ded61 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:40:01 +0200 Subject: [PATCH 10/18] WW-5675 refactor(ognl): drop the lazy dev-mode flip from the access path Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 45 ------------------- ...SecurityMemberAccessConfigSharingTest.java | 25 +++++++++++ 2 files changed, 25 insertions(+), 45 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index 8ba19f5405..b7ed69caaa 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -86,13 +86,6 @@ public class SecurityMemberAccess implements MemberAccess { private Set excludedPackageNames = emptySet(); private Set excludedPackageExemptClasses = emptySet(); - private volatile boolean isDevModeInit; - private boolean isDevMode; - private Set devModeExcludedClasses = Set.of(Object.class.getName()); - private Set devModeExcludedPackageNamePatterns = emptySet(); - private Set devModeExcludedPackageNames = emptySet(); - private Set devModeExcludedPackageExemptClasses = emptySet(); - private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); private Set allowlistPackageNames = emptySet(); @@ -283,7 +276,6 @@ protected boolean isClassAllowlisted(Class clazz) { * @return {@code true} if member access is allowed */ protected boolean checkExclusionList(Object target, Member member) { - useDevModeConfiguration(); Class memberClass = member.getDeclaringClass(); if (isClassExcluded(memberClass)) { LOG.warn("Declaring class of member type [{}] is excluded!", memberClass); @@ -612,41 +604,4 @@ public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } - @Inject(StrutsConstants.STRUTS_DEVMODE) - protected void useDevMode(String devMode) { - this.isDevMode = BooleanUtils.toBoolean(devMode); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) - public void useDevModeExcludedClasses(String commaDelimitedClasses) { - this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) - public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) - public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { - this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) - public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { - this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); - } - - private void useDevModeConfiguration() { - if (!isDevMode || isDevModeInit) { - return; - } - logWarningForFirstOccurrence("devMode", LOG, - "DevMode enabled, using DevMode excluded classes and packages for OGNL security enforcement!"); - isDevModeInit = true; - excludedClasses = devModeExcludedClasses; - excludedPackageNamePatterns = devModeExcludedPackageNamePatterns; - excludedPackageNames = devModeExcludedPackageNames; - excludedPackageExemptClasses = devModeExcludedPackageExemptClasses; - } } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java index 5680e59fb9..eb09c9fb47 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -148,4 +148,29 @@ static class SubclassedSecurityMemberAccess extends SecurityMemberAccess { super(providerAllowlist, threadAllowlist); } } + + /** + * Dev-mode exclusions must be in force from the first access, with no lazy flip. + */ + public void testDevModeExclusionsApplyWithoutAnAccess() throws Exception { + loadButSet(Map.of( + StrutsConstants.STRUTS_DEVMODE, "true", + StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, "java.lang.ProcessBuilder")); + + SecurityMemberAccess sma = container.getInstance(SecurityMemberAccess.class); + Set excluded = SecurityMemberAccessTest.reflectField(sma, "excludedClasses"); + + assertTrue("dev-mode exclusions were not applied at startup", + excluded.contains("java.lang.ProcessBuilder")); + } + + public void testDevModeMethodsAreGone() throws Exception { + for (String name : new String[]{"useDevMode", "useDevModeExcludedClasses", + "useDevModeExcludedPackageNamePatterns", "useDevModeExcludedPackageNames", + "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"}) { + for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { + assertFalse("SecurityMemberAccess still declares " + name, method.getName().equals(name)); + } + } + } } From cb162c6316c08870b4992800b8e5b35a0494e8da Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:44:31 +0200 Subject: [PATCH 11/18] WW-5675 fix(ognl): register SecurityMemberAccessConfig for the Dispatcher container Co-Authored-By: Claude Opus 5 --- core/src/main/resources/struts-beans.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 2de4ebc405..84f0919dcd 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -174,6 +174,7 @@ class="org.apache.struts2.ognl.StrutsOgnlGuard"/> + From 9b1eb89be1bfa018e48cb95ec4f5be30ff97f1e1 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:45:23 +0200 Subject: [PATCH 12/18] WW-5675 docs(ognl): correct the wiring claim the full-suite run disproved Dispatcher installs its own provider list and never adds StrutsDefaultConfigurationProvider, so bootstrapFactories is not on the production path. The bean needs registering in struts-beans.xml too, matching ProviderAllowlist and ThreadAllowlist. Co-Authored-By: Claude Opus 5 --- ...ity-member-access-config-sharing-design.md | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md index 50ecb29627..319ce54ec7 100644 --- a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -86,16 +86,33 @@ Concrete class, no interface, **not** aliased in `StrutsBeanSelectionProvider`. the shape of `ProviderAllowlist` and `ThreadAllowlist` (`DefaultConfiguration.java:418-419`), not a user extension point. -`bootstrapFactories` is on the production path, not test-only: `ConfigurationManager.addDefaultContainerProviders` -(`ConfigurationManager.java:94`) registers `StrutsDefaultConfigurationProvider`, which calls it at -`StrutsDefaultConfigurationProvider.java:116`, and `Dispatcher` drives `ConfigurationManager`. It reads as -test-oriented in a grep only because a dozen tests name the provider explicitly and `XWorkTestCaseHelper` — test -scaffolding that lives in `core/src/main` — registers it too. - -The method serves both the bootstrap container (`DefaultConfiguration.java:360`) and the main container, so each -gets its own configuration singleton. The bootstrap container carries only `BOOTSTRAP_CONSTANTS`, so most security -constants are absent there, the `required = false` setters do not fire, and the bean falls back to defaults — -exactly as a `SecurityMemberAccess` constructed in that container behaves today. +**The bean must be registered in two places.** An earlier draft of this design claimed `bootstrapFactories` was on +the production path because `ConfigurationManager.addDefaultContainerProviders` (`ConfigurationManager.java:94`) +registers `StrutsDefaultConfigurationProvider`, which calls it at +`StrutsDefaultConfigurationProvider.java:116`. **That claim is wrong**, and it was only caught when the full core +suite failed with 1579 errors during implementation. + +`ConfigurationManager.addDefaultContainerProviders()` fires only when `containerProviders.isEmpty()` +(`ConfigurationManager.java:78-80`). `Dispatcher.init()` (`Dispatcher.java:711-719`) installs its own provider +list — including `StrutsBeanSelectionProvider` via `init_AliasStandardObjects` — so the list is never empty and +`StrutsDefaultConfigurationProvider` is never added. The production container is built from +`StrutsBeanSelectionProvider` plus `struts-beans.xml`, and `bootstrapFactories` is not on that path at all. + +The registration therefore goes in both places, which is precisely what `ProviderAllowlist` and `ThreadAllowlist` +already do — `DefaultConfiguration.java:418-419` and `struts-beans.xml:175-176`: + +```xml + +``` + +The `DefaultConfiguration` registration serves the bootstrap container (`DefaultConfiguration.java:360`) and the +`XWorkTestCase` harness; the `struts-beans.xml` entry serves the real Dispatcher container. The bootstrap +container carries only `BOOTSTRAP_CONSTANTS`, so most security constants are absent there, the +`required = false` setters do not fire, and the bean falls back to defaults — exactly as a `SecurityMemberAccess` +constructed in that container behaves today. + +This failure mode is loud, not silent: `useConfig` is a mandatory `@Inject`, so a container missing the binding +throws at build time rather than running with empty exclusions. The `TODO: SpringObjectFactoryTest fails when these are SINGLETON` comment at the top of `bootstrapFactories` applies to the `*Factory` beans in the first block, not to this region, where singletons are already the norm. From ae41c5a8dbbcceac4f9af820f67c2022435a7fea Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:53:44 +0200 Subject: [PATCH 13/18] WW-5675 test(ognl): cover the production registration of the config bean Co-Authored-By: Claude Opus 5 --- ...ccessConfigProductionRegistrationTest.java | 43 +++++++++++++++++++ ...SecurityMemberAccessConfigSharingTest.java | 6 +++ 2 files changed, 49 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java new file mode 100644 index 0000000000..cd1018d50a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java @@ -0,0 +1,43 @@ +/* + * 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.struts2.StrutsInternalTestCase; + +/** + * Covers the {@code struts-beans.xml} registration of {@link SecurityMemberAccessConfig}, which + * {@link SecurityMemberAccessConfigSharingTest} cannot: that test extends {@link org.apache.struts2.XWorkTestCase} + * directly, whose container is built from {@code StrutsDefaultConfigurationProvider} alone and never loads + * {@code struts-beans.xml}. Production, via {@link org.apache.struts2.dispatcher.Dispatcher#init()}, never adds + * that provider and relies entirely on the {@code struts-beans.xml} entry. + *

+ * {@link StrutsInternalTestCase} boots a real {@link org.apache.struts2.dispatcher.Dispatcher}, so its container + * is wired the way production's is. Without this test, the singleton scope of the {@code struts-beans.xml} + * entry — the entire point of WW-5675 sharing parsed configuration across {@link SecurityMemberAccess} + * instances — could regress to {@code scope="prototype"} with the whole suite staying green. + */ +public class SecurityMemberAccessConfigProductionRegistrationTest extends StrutsInternalTestCase { + + public void testConfigBeanIsASingletonInTheProductionContainer() { + SecurityMemberAccessConfig first = container.getInstance(SecurityMemberAccessConfig.class); + assertNotNull("SecurityMemberAccessConfig is not registered in the production container", first); + assertSame("SecurityMemberAccessConfig is not a singleton in the production container", + first, container.getInstance(SecurityMemberAccessConfig.class)); + } +} diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java index eb09c9fb47..ffce445f84 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -162,6 +162,12 @@ public void testDevModeExclusionsApplyWithoutAnAccess() throws Exception { assertTrue("dev-mode exclusions were not applied at startup", excluded.contains("java.lang.ProcessBuilder")); + // The `contains` check above is non-vacuous only because the test container does not load + // struts-excluded-classes.xml, where java.lang.ProcessBuilder happens to sit in both the + // production and dev-mode excluded-classes sets. Asserting identity with the config bean's + // set keeps this test meaningful even if the harness starts loading that file. + assertSame("excludedClasses was not seeded from the dev-mode-resolved config", + container.getInstance(SecurityMemberAccessConfig.class).getExcludedClasses(), excluded); } public void testDevModeMethodsAreGone() throws Exception { From 6028e9dc86b43c8467daadb2a015950a893bc0e9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 14:59:23 +0200 Subject: [PATCH 14/18] WW-5675 perf(ognl): precompute the allowlist package union Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 76 ++++++++++--------- ...curityMemberAccessPackageMatchingTest.java | 46 ++++++----- 2 files changed, 70 insertions(+), 52 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index b7ed69caaa..80c36f05ca 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -32,12 +32,14 @@ import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; +import java.util.HashSet; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import static java.text.MessageFormat.format; 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; @@ -89,6 +91,7 @@ public class SecurityMemberAccess implements MemberAccess { private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); private Set allowlistPackageNames = emptySet(); + private Set allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -121,12 +124,31 @@ public void useConfig(SecurityMemberAccessConfig config) { this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); this.allowlistClasses = config.getAllowlistClasses(); - this.allowlistPackageNames = config.getAllowlistPackageNames(); + applyAllowlistPackageNames(config.getAllowlistPackageNames()); this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); } + /** + * The only place the allowlist union is computed. Both the injected configuration and the + * deprecated setter route through here; splitting this in two would risk silently dropping + * {@code ALLOWLIST_REQUIRED_PACKAGES}, which fails open. + */ + private void applyAllowlistPackageNames(Set packageNames) { + this.allowlistPackageNames = packageNames; + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, packageNames); + } + + private static Set union(Set required, Set configured) { + if (configured.isEmpty()) { + return required; + } + Set union = new HashSet<>(required); + union.addAll(configured); + return unmodifiableSet(union); + } + @Override public Object setup(OgnlContext context, Object target, Member member, String propertyName) { Object result = null; @@ -269,7 +291,7 @@ protected boolean isClassAllowlisted(Class clazz) { || ALLOWLIST_REQUIRED_CLASSES.contains(clazz) || (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz)) || (threadAllowlist != null && threadAllowlist.getAllowlist().contains(clazz)) - || isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames); + || isClassBelongsToPackages(clazz, allowlistPackageNamesUnion); } /** @@ -404,54 +426,38 @@ protected boolean isExcludedPackageNames(Class clazz) { } public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { - return isClassBelongsToPackages(clazz, matchingPackages, emptySet()); - } - - /** - * Tests the class's package against two sets in a single walk. Equivalent to calling - * {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing the results, but - * walks the package name only once. - * - * @param clazz the class whose package is tested - * @param first the first set of package names to match against - * @param second the second set of package names to match against - * @return {@code true} if the class's package or any parent package is in either set - */ - static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { - return isPackageBelongsToPackages(toPackageName(clazz), first, second); + return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages); } /** - * Tests whether the given package name, or any of its parent packages, is present in either - * set. Walks the name in place rather than building the full prefix list, since this runs on - * the OGNL member-access path. Shortest prefix first, so broad entries such as {@code java.io} + * Tests whether the given package name, or any of its parent packages, is present in the set. + * Walks the name in place rather than building the full prefix list, since this runs on the OGNL + * member-access path. Shortest prefix first, so broad entries such as {@code java.io} * short-circuit earliest. * *

- * The package name must not end in {@code '.'}. Such a name is probed one prefix more than by - * the implementation this replaced, which matches more broadly — tightening exclusion but - * loosening the allowlist. {@link Class#getPackageName()} cannot produce a trailing - * dot, so every current caller is safe; route any other string through here only after - * confirming the same. + * The package name must not end in {@code '.'}. Such a name is probed one prefix more than by the + * implementation this replaced, which matches more broadly — tightening exclusion but + * loosening the allowlist. {@link Class#getPackageName()} cannot produce a trailing dot, + * and {@code ConfigParseUtil.toPackageNamesSet} strips them from configured names, so every + * current caller is safe; route any other string through here only after confirming the same. * - * @param packageName the package name to test, empty for the default package, never ending in {@code '.'} - * @param first the first set of package names to match against - * @param second the second set of package names to match against - * @return {@code true} if the package or any parent package is in either set + * @param packageName the package name to test, empty for the default package, never ending in {@code '.'} + * @param matchingPackages the package names to match against + * @return {@code true} if the package or any parent package is in the set */ - static boolean isPackageBelongsToPackages(String packageName, Set first, Set second) { - if (first.isEmpty() && second.isEmpty()) { + static boolean isPackageBelongsToPackages(String packageName, Set matchingPackages) { + if (matchingPackages.isEmpty()) { return false; } int idx = packageName.indexOf('.'); while (idx != -1) { - String prefix = packageName.substring(0, idx); - if (first.contains(prefix) || second.contains(prefix)) { + if (matchingPackages.contains(packageName.substring(0, idx))) { return true; } idx = packageName.indexOf('.', idx + 1); } - return first.contains(packageName) || second.contains(packageName); + return matchingPackages.contains(packageName); } protected boolean isClassExcluded(Class clazz) { @@ -571,7 +577,7 @@ public void useAllowlistClasses(String commaDelimitedClasses) { */ @Deprecated public void useAllowlistPackageNames(String commaDelimitedPackageNames) { - this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); } /** diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java index 413b0c6695..3dfa540dd4 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -117,21 +117,21 @@ private static List> classShapes() throws Exception { public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() { Set excluded = Set.of("org.apache.struts2"); - assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2x", excluded, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2x", excluded)) .as("a sibling package sharing a character prefix must not match (production)") .isFalse(); assertThat(legacyPrefixMatch("org.apache.struts2x", excluded)) .as("a sibling package sharing a character prefix must not match (legacy oracle)") .isFalse(); - assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2", excluded, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2", excluded)) .as("an exact match must match (production)") .isTrue(); assertThat(legacyPrefixMatch("org.apache.struts2", excluded)) .as("an exact match must match (legacy oracle)") .isTrue(); - assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2.ognl", excluded, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2.ognl", excluded)) .as("a sub-package must match (production)") .isTrue(); assertThat(legacyPrefixMatch("org.apache.struts2.ognl", excluded)) @@ -192,7 +192,7 @@ public void classEntryPointMatchesLegacyAcrossCandidateSets() throws Exception { public void indexWalkMatchesLegacyAcrossPackageNameShapes() { for (String packageName : PACKAGE_NAMES) { for (Set candidates : CANDIDATE_SETS) { - assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates)) .as("packageName=[%s] candidates=%s", packageName, candidates) .isEqualTo(legacyPrefixMatch(packageName, candidates)); } @@ -200,25 +200,37 @@ public void indexWalkMatchesLegacyAcrossPackageNameShapes() { } @Test - public void bothSetsEmptyShortCircuitsToFalse() { + public void emptyCandidateSetShortCircuitsToFalse() { for (String packageName : PACKAGE_NAMES) { - assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet(), emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet())) .as("packageName=[%s] with no configured packages", packageName) .isFalse(); } } + /** + * The union must never lose ALLOWLIST_REQUIRED_PACKAGES. Dropping them would be a silent + * fail-open: Struts' own components would stop being allowlisted with nothing failing loudly. + */ @Test - public void twoSetOverloadEqualsDisjunctionOfSingleSetCalls() throws Exception { - for (Class clazz : classShapes()) { - for (Set first : CANDIDATE_SETS) { - for (Set second : CANDIDATE_SETS) { - assertThat(isClassBelongsToPackages(clazz, first, second)) - .as("clazz=[%s] first=%s second=%s", clazz.getName(), first, second) - .isEqualTo(isClassBelongsToPackages(clazz, first) - || isClassBelongsToPackages(clazz, second)); - } - } - } + public void allowlistUnionRetainsRequiredPackagesAfterSetterCall() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + sma.useAllowlistPackageNames("com.example.app"); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains("com.example.app", "org.apache.struts2.components"); + } + + @Test + public void allowlistUnionContainsRequiredPackagesByDefault() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains( + "org.apache.struts2.components", + "org.apache.struts2.views.jsp", + "org.apache.struts2.validator.validators"); } } From 571014c98ccc6d8f6141a35c60fc9c5abef364b9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 15:21:40 +0200 Subject: [PATCH 15/18] WW-5675 fix(ognl): move the allowlist union onto the config bean Final-review cleanup: SecurityMemberAccess.applyAllowlistPackageNames was still allocating a fresh HashSet per instantiation, landing precisely on deployments that configure struts.allowlist.packageNames. Move ALLOWLIST_REQUIRED_PACKAGES and union(...) onto SecurityMemberAccessConfig, which now precomputes allowlistPackageNamesUnion once per container; useConfig copies the reference, and the deprecated setter path reuses the same static union() method, so there remains exactly one computation site. Also: mark the eleven deprecated SecurityMemberAccess setters with since/forRemoval per repo convention, document union()'s Set.of(...) aliasing contract, pin allowlistPackageNamesUnion into the immutability and dev-mode-field-removal tests, restore alphabetical import order in ConfigParseUtilTest, switch the sharing test off the Map.of ten-pair ceiling, and correct the design doc's bootstrap-container wiring claim and drop its unimplemented counting-probe promise. Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 55 +++++++----------- .../ognl/SecurityMemberAccessConfig.java | 41 +++++++++++++ ...SecurityMemberAccessConfigSharingTest.java | 44 +++++++++----- .../ognl/SecurityMemberAccessConfigTest.java | 21 +++++++ .../ognl/SecurityMemberAccessTest.java | 1 + .../struts2/util/ConfigParseUtilTest.java | 2 +- ...ity-member-access-config-sharing-design.md | 58 +++++++++++++++---- 7 files changed, 159 insertions(+), 63 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index 80c36f05ca..fcc337a630 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -32,14 +32,12 @@ import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; -import java.util.HashSet; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import static java.text.MessageFormat.format; 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; @@ -58,12 +56,6 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); - private static final Set ALLOWLIST_REQUIRED_PACKAGES = Set.of( - "org.apache.struts2.validator.validators", - "org.apache.struts2.components", - "org.apache.struts2.views.jsp" - ); - private static final Set> ALLOWLIST_REQUIRED_CLASSES = Set.of( java.lang.Enum.class, java.lang.String.class, @@ -91,7 +83,7 @@ public class SecurityMemberAccess implements MemberAccess { private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); private Set allowlistPackageNames = emptySet(); - private Set allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; + private Set allowlistPackageNamesUnion = SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -124,29 +116,24 @@ public void useConfig(SecurityMemberAccessConfig config) { this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); this.allowlistClasses = config.getAllowlistClasses(); - applyAllowlistPackageNames(config.getAllowlistPackageNames()); + this.allowlistPackageNames = config.getAllowlistPackageNames(); + this.allowlistPackageNamesUnion = config.getAllowlistPackageNamesUnion(); this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); } /** - * The only place the allowlist union is computed. Both the injected configuration and the - * deprecated setter route through here; splitting this in two would risk silently dropping - * {@code ALLOWLIST_REQUIRED_PACKAGES}, which fails open. + * Used only by the deprecated {@link #useAllowlistPackageNames(String)} setter path. The injected + * configuration path seeds both fields directly from {@link SecurityMemberAccessConfig}, which + * precomputes the union exactly once per container; both routes call + * {@link SecurityMemberAccessConfig#union(Set, Set)}, so there remains exactly one place in the + * codebase that computes the union, and {@code ALLOWLIST_REQUIRED_PACKAGES} cannot be silently + * dropped from either. */ private void applyAllowlistPackageNames(Set packageNames) { this.allowlistPackageNames = packageNames; - this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, packageNames); - } - - private static Set union(Set required, Set configured) { - if (configured.isEmpty()) { - return required; - } - Set union = new HashSet<>(required); - union.addAll(configured); - return unmodifiableSet(union); + this.allowlistPackageNamesUnion = SecurityMemberAccessConfig.union(SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES, packageNames); } @Override @@ -495,7 +482,7 @@ public void useAcceptProperties(Set acceptedProperties) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); if (!this.allowStaticFieldAccess) { @@ -508,7 +495,7 @@ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedClasses(String commaDelimitedClasses) { this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } @@ -518,7 +505,7 @@ public void useExcludedClasses(String commaDelimitedClasses) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } @@ -528,7 +515,7 @@ public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNames(String commaDelimitedPackageNames) { this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } @@ -538,7 +525,7 @@ public void useExcludedPackageNames(String commaDelimitedPackageNames) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } @@ -548,7 +535,7 @@ public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); if (!this.enforceAllowlistEnabled) { @@ -565,7 +552,7 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistClasses(String commaDelimitedClasses) { this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); } @@ -575,7 +562,7 @@ public void useAllowlistClasses(String commaDelimitedClasses) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistPackageNames(String commaDelimitedPackageNames) { applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); } @@ -585,7 +572,7 @@ public void useAllowlistPackageNames(String commaDelimitedPackageNames) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); } @@ -595,7 +582,7 @@ public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } @@ -605,7 +592,7 @@ public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. */ - @Deprecated + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java index 6009738307..59aef3c03e 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java @@ -25,10 +25,12 @@ 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; @@ -57,6 +59,20 @@ 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 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 excludedClasses = Set.of(Object.class.getName()); @@ -73,6 +89,7 @@ public class SecurityMemberAccessConfig implements Initializable { private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); private Set allowlistPackageNames = emptySet(); + private Set allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -139,6 +156,26 @@ public void useAllowlistClasses(String 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. + *

+ * 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 union(Set required, Set configured) { + if (configured.isEmpty()) { + return required; + } + Set union = new HashSet<>(required); + union.addAll(configured); + return unmodifiableSet(union); } @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) @@ -213,6 +250,10 @@ public Set getAllowlistPackageNames() { return allowlistPackageNames; } + public Set getAllowlistPackageNamesUnion() { + return allowlistPackageNamesUnion; + } + public boolean isDisallowProxyObjectAccess() { return disallowProxyObjectAccess; } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java index ffce445f84..9d74bc6375 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -24,6 +24,8 @@ import java.util.Map; import java.util.Set; +import static org.assertj.core.api.Assertions.assertThat; + public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { /** @@ -46,17 +48,17 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { * when {@code useConfig} actually ran. */ public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { - loadButSet(Map.of( - StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, "false", - StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, "^org\\.apache\\.struts2\\.ognl\\.testpkg\\..*", - StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg", - StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, "java.lang.String", - StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true", - StrutsConstants.STRUTS_ALLOWLIST_CLASSES, "java.lang.String", - StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg", - StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, "true", - StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, "true", - StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, "true")); + loadButSet(Map.ofEntries( + Map.entry(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, "false"), + Map.entry(StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, "^org\\.apache\\.struts2\\.ognl\\.testpkg\\..*"), + Map.entry(StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg"), + Map.entry(StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, "java.lang.String"), + Map.entry(StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true"), + Map.entry(StrutsConstants.STRUTS_ALLOWLIST_CLASSES, "java.lang.String"), + Map.entry(StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg"), + Map.entry(StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, "true"), + Map.entry(StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, "true"), + Map.entry(StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, "true"))); SecurityMemberAccess first = container.getInstance(SecurityMemberAccess.class); SecurityMemberAccess second = container.getInstance(SecurityMemberAccess.class); @@ -84,6 +86,10 @@ public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { config.getAllowlistClasses(), SecurityMemberAccessTest.reflectField(first, "allowlistClasses")); assertSame("allowlistPackageNames not seeded from config", config.getAllowlistPackageNames(), SecurityMemberAccessTest.reflectField(first, "allowlistPackageNames")); + Set firstAllowlistPackageNamesUnion = SecurityMemberAccessTest.reflectField(first, "allowlistPackageNamesUnion"); + assertSame("allowlistPackageNamesUnion not seeded from config", + config.getAllowlistPackageNamesUnion(), firstAllowlistPackageNamesUnion); + assertThat(firstAllowlistPackageNamesUnion).contains("org.apache.struts2.ognl.testpkg", "org.apache.struts2.components"); boolean firstAllowStaticFieldAccess = SecurityMemberAccessTest.reflectField(first, "allowStaticFieldAccess"); assertEquals("allowStaticFieldAccess not seeded from config", @@ -171,12 +177,18 @@ public void testDevModeExclusionsApplyWithoutAnAccess() throws Exception { } public void testDevModeMethodsAreGone() throws Exception { - for (String name : new String[]{"useDevMode", "useDevModeExcludedClasses", + Set removedMethods = Set.of("useDevMode", "useDevModeExcludedClasses", "useDevModeExcludedPackageNamePatterns", "useDevModeExcludedPackageNames", - "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"}) { - for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { - assertFalse("SecurityMemberAccess still declares " + name, method.getName().equals(name)); - } + "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"); + for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { + assertFalse("SecurityMemberAccess still declares " + method.getName(), removedMethods.contains(method.getName())); + } + + Set removedFields = Set.of("isDevModeInit", "isDevMode", "devModeExcludedClasses", + "devModeExcludedPackageNamePatterns", "devModeExcludedPackageNames", + "devModeExcludedPackageExemptClasses"); + for (java.lang.reflect.Field field : SecurityMemberAccess.class.getDeclaredFields()) { + assertFalse("SecurityMemberAccess still declares field " + field.getName(), removedFields.contains(field.getName())); } } } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java index e95e99e081..652d26d8c0 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java @@ -140,4 +140,25 @@ public void withoutInitTheNormalExclusionsApply() { assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); } + + @Test + public void allowlistPackageNamesUnionDefaultsToRequiredPackagesOnly() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + + assertEquals(Set.of("org.apache.struts2.validator.validators", + "org.apache.struts2.components", + "org.apache.struts2.views.jsp"), + config.getAllowlistPackageNamesUnion()); + } + + @Test + public void allowlistPackageNamesUnionRetainsRequiredPackagesWhenConfigured() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useAllowlistPackageNames("com.example.app"); + + assertTrue(config.getAllowlistPackageNamesUnion().contains("com.example.app")); + assertTrue(config.getAllowlistPackageNamesUnion().contains("org.apache.struts2.components")); + assertTrue(config.getAllowlistPackageNamesUnion().contains("org.apache.struts2.validator.validators")); + assertTrue(config.getAllowlistPackageNamesUnion().contains("org.apache.struts2.views.jsp")); + } } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java index a9b7b8c12d..84c11a6ddf 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -112,6 +112,7 @@ public void configurationCollectionsImmutable() throws Exception { "excludedPackageExemptClasses", "allowlistClasses", "allowlistPackageNames", + "allowlistPackageNamesUnion", "excludeProperties", "acceptProperties"); for (String field : fields) { diff --git a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java index 6e9c79727e..18567bd894 100644 --- a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java @@ -33,9 +33,9 @@ import java.util.Map; import java.util.Set; -import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md index 319ce54ec7..8dbdb7122b 100644 --- a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -96,7 +96,16 @@ suite failed with 1579 errors during implementation. (`ConfigurationManager.java:78-80`). `Dispatcher.init()` (`Dispatcher.java:711-719`) installs its own provider list — including `StrutsBeanSelectionProvider` via `init_AliasStandardObjects` — so the list is never empty and `StrutsDefaultConfigurationProvider` is never added. The production container is built from -`StrutsBeanSelectionProvider` plus `struts-beans.xml`, and `bootstrapFactories` is not on that path at all. +`StrutsBeanSelectionProvider` plus `struts-beans.xml`, and `bootstrapFactories` is not on the path of that *main +Dispatcher* container. + +It is, however, on a different, load-bearing path: `DefaultConfiguration.reloadContainer` builds a **bootstrap** +container from `bootstrapFactories` (`DefaultConfiguration.java:283`, via `createBootstrapContainer` at +`DefaultConfiguration.java:348-373`), then calls `setContext(bootstrap)` (`DefaultConfiguration.java:307`), which +calls `bootstrap.getInstance(ValueStackFactory.class).createValueStack()` — and building a value stack instantiates +`SecurityMemberAccess` through `CompoundRootAccessor`/`RootAccessor`. So the bootstrap container's registration +of `SecurityMemberAccessConfig` is not a fallback for some other, unused path: it is exercised on every +`reloadContainer()` call, before the main Dispatcher container even exists. The registration therefore goes in both places, which is precisely what `ProviderAllowlist` and `ThreadAllowlist` already do — `DefaultConfiguration.java:418-419` and `struts-beans.xml:175-176`: @@ -106,10 +115,11 @@ already do — `DefaultConfiguration.java:418-419` and `struts-beans.xml:175-176 ``` The `DefaultConfiguration` registration serves the bootstrap container (`DefaultConfiguration.java:360`) and the -`XWorkTestCase` harness; the `struts-beans.xml` entry serves the real Dispatcher container. The bootstrap -container carries only `BOOTSTRAP_CONSTANTS`, so most security constants are absent there, the -`required = false` setters do not fire, and the bean falls back to defaults — exactly as a `SecurityMemberAccess` -constructed in that container behaves today. +`XWorkTestCase` harness; the `struts-beans.xml` entry serves the real Dispatcher container. **Both registrations +are load-bearing** — production would throw at startup without either, since `useConfig` is a mandatory `@Inject` +on `SecurityMemberAccess`. The bootstrap container carries only `BOOTSTRAP_CONSTANTS`, so most security constants +are absent there, the `required = false` setters do not fire, and the bean falls back to defaults — exactly as a +`SecurityMemberAccess` constructed in that container behaves today. This failure mode is loud, not silent: `useConfig` is a mandatory `@Inject`, so a container missing the binding throws at build time rather than running with empty exclusions. @@ -230,16 +240,40 @@ This deletes the three-argument `isClassBelongsToPackages` overload and the two- The ticket flagged this as a fail-open hazard: if the union were computed in two places — once seeded from configuration, once when the deprecated `useAllowlistPackageNames` setter fires — the two could drift, silently -dropping `ALLOWLIST_REQUIRED_PACKAGES` from the allowlist with nothing failing loudly. Both routes therefore -funnel through one private method, so exactly one line in the codebase computes the union: +dropping `ALLOWLIST_REQUIRED_PACKAGES` from the allowlist with nothing failing loudly. It is also a fail-open +hazard if the union is *re-computed* per instance: that reintroduces exactly the per-instantiation `HashSet` +allocation this ticket exists to remove, and lands on the deployments that configure the allowlist properly, +inverting the ticket's intent. + +Both hazards are avoided by moving `ALLOWLIST_REQUIRED_PACKAGES` and the `union(...)` helper onto +`SecurityMemberAccessConfig`, which precomputes `allowlistPackageNamesUnion` once, inside its own +`useAllowlistPackageNames` setter, when the constant fires during container construction: ```java -private void applyAllowlistPackageNames(Set names) { - this.allowlistPackageNames = names; - this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, names); +// SecurityMemberAccessConfig +static final Set ALLOWLIST_REQUIRED_PACKAGES = Set.of( + "org.apache.struts2.validator.validators", + "org.apache.struts2.components", + "org.apache.struts2.views.jsp" +); + +public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames); } + +static Set union(Set required, Set configured) { … } ``` +`SecurityMemberAccess.useConfig` copies the precomputed reference (`config.getAllowlistPackageNamesUnion()`) — +no allocation on the hot instantiation path. Its deprecated `useAllowlistPackageNames` setter, which still mutates +a single instance directly and has no `SecurityMemberAccessConfig` to read from, calls the same +`SecurityMemberAccessConfig.union(...)` static method. Both routes therefore funnel through the one method, so +exactly one line in the codebase computes the union, and `ALLOWLIST_REQUIRED_PACKAGES` cannot drift out of it +through a second implementation. The constant and helper live on the config bean — the class that owns computing +and exposing configuration-derived state — rather than being duplicated onto `SecurityMemberAccess`, whose +deprecated setter merely calls back into it. + `isPackageBelongsToPackages` currently early-returns on `first.isEmpty() && second.isEmpty()`. Since `ALLOWLIST_REQUIRED_PACKAGES` is never empty, that guard simply stops firing on the allowlist path; the exclusion path, where both sets genuinely can be empty, keeps it. The guard was only ever an optimization, so this is not a @@ -277,8 +311,8 @@ Core tests are JUnit 4 or extend `XWorkTestCase`. A JUnit 5 `@Test` added to the 1. **Sharing proof.** Request several `SecurityMemberAccess` instances from one container and assert their configuration-derived sets are reference-identical (`assertSame`, not `assertEquals`). Reference identity is a - dependency-free proof that no re-parsing occurred, since any re-parse necessarily produces a fresh set. Backed - by a counting probe asserting exactly one parse per container. + dependency-free proof that no re-parsing occurred, since any re-parse necessarily produces a fresh set; this is + the sound substitute for a counting probe and is what the implementation actually asserts. 2. **Instance isolation.** Calling a deprecated setter on one instance must not perturb a sibling instance or the singleton. The sets are `unmodifiableSet`, so an in-place mutation bug would throw rather than corrupt silently, but this invariant deserves an explicit assertion. From 85a9aeb5d19064733efd91fda6840d081e3e8aec Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 17:01:57 +0200 Subject: [PATCH 16/18] WW-5675 fix(ognl): address SonarCloud and review findings on config sharing Hoists the throwing Set.of(...) calls out of three assertThrows lambdas in ConfigParseUtilTest (java:S5778); removes the now-dead allowlistPackageNames field from SecurityMemberAccess, which was written but never read after the union moved onto the config bean (java:S1068), updating the two tests that reflected on it so the meaningful allowlistPackageNamesUnion assertions remain; documents on all eleven deprecated setters that the container no longer invokes them, so a subclass override silently stops taking effect; corrects two factual claims in the design doc about when the missing-binding failure and the dev-mode warning actually fire, given the main container is built lazily via builder.create(false); and narrows SecurityMemberAccessConfig's sixteen use* setters from public to package-private, since ContainerImpl injects via setAccessible and narrower is a smaller blast radius for a container-wide singleton. Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 55 +++++++++++++------ .../ognl/SecurityMemberAccessConfig.java | 32 +++++------ ...SecurityMemberAccessConfigSharingTest.java | 2 - .../ognl/SecurityMemberAccessTest.java | 1 - .../struts2/util/ConfigParseUtilTest.java | 9 ++- ...ity-member-access-config-sharing-design.md | 12 +++- 6 files changed, 69 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index fcc337a630..95ee748b0c 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -82,7 +82,6 @@ public class SecurityMemberAccess implements MemberAccess { private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); - private Set allowlistPackageNames = emptySet(); private Set allowlistPackageNamesUnion = SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; @@ -116,7 +115,6 @@ public void useConfig(SecurityMemberAccessConfig config) { this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); this.allowlistClasses = config.getAllowlistClasses(); - this.allowlistPackageNames = config.getAllowlistPackageNames(); this.allowlistPackageNamesUnion = config.getAllowlistPackageNamesUnion(); this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); @@ -125,14 +123,13 @@ public void useConfig(SecurityMemberAccessConfig config) { /** * Used only by the deprecated {@link #useAllowlistPackageNames(String)} setter path. The injected - * configuration path seeds both fields directly from {@link SecurityMemberAccessConfig}, which - * precomputes the union exactly once per container; both routes call - * {@link SecurityMemberAccessConfig#union(Set, Set)}, so there remains exactly one place in the - * codebase that computes the union, and {@code ALLOWLIST_REQUIRED_PACKAGES} cannot be silently + * configuration path seeds {@code allowlistPackageNamesUnion} directly from + * {@link SecurityMemberAccessConfig}, which precomputes the union exactly once per container; both + * routes call {@link SecurityMemberAccessConfig#union(Set, Set)}, so there remains exactly one place + * in the codebase that computes the union, and {@code ALLOWLIST_REQUIRED_PACKAGES} cannot be silently * dropped from either. */ private void applyAllowlistPackageNames(Set packageNames) { - this.allowlistPackageNames = packageNames; this.allowlistPackageNamesUnion = SecurityMemberAccessConfig.union(SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES, packageNames); } @@ -480,7 +477,9 @@ public void useAcceptProperties(Set acceptedProperties) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { @@ -493,7 +492,9 @@ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedClasses(String commaDelimitedClasses) { @@ -503,7 +504,9 @@ public void useExcludedClasses(String commaDelimitedClasses) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { @@ -513,7 +516,9 @@ public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNames(String commaDelimitedPackageNames) { @@ -523,7 +528,9 @@ public void useExcludedPackageNames(String commaDelimitedPackageNames) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { @@ -533,7 +540,9 @@ public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { @@ -550,7 +559,9 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistClasses(String commaDelimitedClasses) { @@ -560,7 +571,9 @@ public void useAllowlistClasses(String commaDelimitedClasses) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistPackageNames(String commaDelimitedPackageNames) { @@ -570,7 +583,9 @@ public void useAllowlistPackageNames(String commaDelimitedPackageNames) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { @@ -580,7 +595,9 @@ public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { @@ -590,7 +607,9 @@ public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { /** * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for - * tests and existing callers; it will be removed in Struts 8.0.0. + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add + * their own exclusions must now do so another way. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java index 59aef3c03e..9a98a299bc 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java @@ -109,7 +109,7 @@ public void init() { } @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) - public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { + void useAllowStaticFieldAccess(String allowStaticFieldAccess) { this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); if (!this.allowStaticFieldAccess) { useExcludedClasses(Class.class.getName()); @@ -117,27 +117,27 @@ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) - public void useExcludedClasses(String commaDelimitedClasses) { + void useExcludedClasses(String commaDelimitedClasses) { this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) - public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) - public void useExcludedPackageNames(String commaDelimitedPackageNames) { + void useExcludedPackageNames(String commaDelimitedPackageNames) { this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) - public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { + void useExcludedPackageExemptClasses(String commaDelimitedClasses) { this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) - public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { + void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); if (!this.enforceAllowlistEnabled) { String msg = "OGNL allowlist is disabled!" + @@ -149,12 +149,12 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { } @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false) - public void useAllowlistClasses(String commaDelimitedClasses) { + void useAllowlistClasses(String commaDelimitedClasses) { this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); } @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) - public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + void useAllowlistPackageNames(String commaDelimitedPackageNames) { this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames); } @@ -179,42 +179,42 @@ static Set union(Set required, Set configured) { } @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) - public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { + void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); } @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) - public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) - public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } @Inject(StrutsConstants.STRUTS_DEVMODE) - public void useDevMode(String devMode) { + void useDevMode(String devMode) { this.isDevMode = BooleanUtils.toBoolean(devMode); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) - public void useDevModeExcludedClasses(String commaDelimitedClasses) { + void useDevModeExcludedClasses(String commaDelimitedClasses) { this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) - public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) - public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { + void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) - public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { + void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java index 9d74bc6375..afcffc19a3 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -84,8 +84,6 @@ public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { config.getExcludedPackageExemptClasses(), SecurityMemberAccessTest.reflectField(first, "excludedPackageExemptClasses")); assertSame("allowlistClasses not seeded from config", config.getAllowlistClasses(), SecurityMemberAccessTest.reflectField(first, "allowlistClasses")); - assertSame("allowlistPackageNames not seeded from config", - config.getAllowlistPackageNames(), SecurityMemberAccessTest.reflectField(first, "allowlistPackageNames")); Set firstAllowlistPackageNamesUnion = SecurityMemberAccessTest.reflectField(first, "allowlistPackageNamesUnion"); assertSame("allowlistPackageNamesUnion not seeded from config", config.getAllowlistPackageNamesUnion(), firstAllowlistPackageNamesUnion); diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java index 84c11a6ddf..0338636316 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -111,7 +111,6 @@ public void configurationCollectionsImmutable() throws Exception { "excludedPackageNamePatterns", "excludedPackageExemptClasses", "allowlistClasses", - "allowlistPackageNames", "allowlistPackageNamesUnion", "excludeProperties", "acceptProperties"); diff --git a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java index 18567bd894..e495e6902f 100644 --- a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java @@ -196,20 +196,23 @@ public void validatePackageNamesAcceptsNamesWithoutWhitespace() { @Test public void validatePackageNamesRejectsSpace() { + Set packageNames = Set.of("java.lang", "org.apache struts2"); assertThrows(ConfigurationException.class, - () -> ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache struts2"))); + () -> ConfigParseUtil.validatePackageNames(packageNames)); } @Test public void validatePackageNamesRejectsTab() { + Set packageNames = Set.of("java\tlang"); assertThrows(ConfigurationException.class, - () -> ConfigParseUtil.validatePackageNames(Set.of("java\tlang"))); + () -> ConfigParseUtil.validatePackageNames(packageNames)); } @Test public void validatePackageNamesRejectsNewline() { + Set packageNames = Set.of("java\nlang"); assertThrows(ConfigurationException.class, - () -> ConfigParseUtil.validatePackageNames(Set.of("java\nlang"))); + () -> ConfigParseUtil.validatePackageNames(packageNames)); } @Test diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md index 8dbdb7122b..8d5339adfd 100644 --- a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -122,7 +122,10 @@ are absent there, the `required = false` setters do not fire, and the bean falls `SecurityMemberAccess` constructed in that container behaves today. This failure mode is loud, not silent: `useConfig` is a mandatory `@Inject`, so a container missing the binding -throws at build time rather than running with empty exclusions. +fails closed with a `DependencyException`, not by running with empty exclusions. Because `SecurityMemberAccess` is +`Scope.PROTOTYPE` and `ContainerImpl`'s injector cache is built lazily, that exception fires at the first +`getInstance(SecurityMemberAccess.class)` rather than at `builder.create(...)` — still loud and still fail-closed, +just not at container-build time. The `TODO: SpringObjectFactoryTest fails when these are SINGLETON` comment at the top of `bootstrapFactories` applies to the `*Factory` beans in the first block, not to this region, where singletons are already the norm. @@ -301,7 +304,12 @@ configuration bean makes it a genuine once-per-container event. One, accepted during design review: the `"DevMode enabled, using DevMode excluded classes and packages for OGNL security enforcement!"` warning currently fires on the first OGNL access and will now fire when the configuration -singleton is built. This makes it a deterministic startup signal rather than one contingent on traffic. +singleton's `init()` runs. The main Dispatcher container is built with `builder.create(false)` (lazy singletons), so +for it that is still triggered by first use — the config bean is constructed the first time something asks for a +`SecurityMemberAccess`, not at container-build/startup time. The bootstrap container does use `create(true)` and so +does log eagerly there. The change is still worth making — it moves the warning from being contingent on OGNL +traffic to being contingent on the config bean's first use, which happens earlier and more predictably — but it is +not a guaranteed startup-time log line for the main container. No other externally visible behaviour changes. OGNL allow/deny semantics are identical. From b8cd7d1283b432a26b1359d22c935fd061fc6328 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 17:04:10 +0200 Subject: [PATCH 17/18] WW-5675 fix(ognl): enforce the union immutability contract instead of documenting it Set.copyOf short-circuits to the same instance for an already-immutable set, so the usual path still allocates nothing while a mutable argument would be copied rather than aliased into a container-wide shared set. Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccessConfig.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java index 9a98a299bc..6278717d24 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java @@ -165,13 +165,15 @@ void useAllowlistPackageNames(String commaDelimitedPackageNames) { * setter path call this method, so {@code ALLOWLIST_REQUIRED_PACKAGES} can never silently drop out * of the union through a second, drifted implementation. *

- * 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}. + * The result is always immutable, whatever the caller passes. When nothing is configured the + * required set is returned through {@link Set#copyOf}, which the JDK short-circuits to the same + * instance for an already-immutable set — so the usual case allocates nothing, while a mutable + * {@code required} would still be defensively copied rather than aliased into a set shared by + * every {@link SecurityMemberAccess} in the container. */ static Set union(Set required, Set configured) { if (configured.isEmpty()) { - return required; + return Set.copyOf(required); } Set union = new HashSet<>(required); union.addAll(configured); From 44a078fd10bf99371d9c853e5df7a6040090e913 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 14 Aug 2026 17:24:43 +0200 Subject: [PATCH 18/18] WW-5675 docs(ognl): trim the deprecation note on the retained setters Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 33 +++++++------------ 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index 95ee748b0c..3e626e664d 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -478,8 +478,7 @@ public void useAcceptProperties(Set acceptedProperties) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { @@ -493,8 +492,7 @@ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedClasses(String commaDelimitedClasses) { @@ -505,8 +503,7 @@ public void useExcludedClasses(String commaDelimitedClasses) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { @@ -517,8 +514,7 @@ public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNames(String commaDelimitedPackageNames) { @@ -529,8 +525,7 @@ public void useExcludedPackageNames(String commaDelimitedPackageNames) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { @@ -541,8 +536,7 @@ public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { @@ -560,8 +554,7 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistClasses(String commaDelimitedClasses) { @@ -572,8 +565,7 @@ public void useAllowlistClasses(String commaDelimitedClasses) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistPackageNames(String commaDelimitedPackageNames) { @@ -584,8 +576,7 @@ public void useAllowlistPackageNames(String commaDelimitedPackageNames) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { @@ -596,8 +587,7 @@ public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { @@ -608,8 +598,7 @@ public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { * @deprecated since 7.4.0, configuration is parsed once per container by * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes - * this setter, so overriding it in a subclass no longer affects configuration; subclasses that add - * their own exclusions must now do so another way. + * this setter. */ @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) {