This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5675-share-parsed-ognl-security-config in repository https://gitbox.apache.org/repos/asf/struts.git
commit b10154e1702f88cccc4b9107bb6fc9e52dcb1e6b Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Aug 14 13:45:30 2026 +0200 WW-5675 docs(ognl): add design for sharing parsed OGNL security config Co-Authored-By: Claude Opus 5 <[email protected]> --- ...security-member-access-config-sharing-design.md | 286 +++++++++++++++++++++ 1 file changed, 286 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 new file mode 100644 index 000000000..b5435fe33 --- /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<String> 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%.
