mirror of
https://github.com/apache/struts.git
synced 2026-09-12 09:05:04 +00:00
Lukasz Lenart
d637e4541d
* WW-5675 docs(ognl): add design for sharing parsed OGNL security config Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 docs(ognl): record that bootstrapFactories is on the production path Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 docs(ognl): settle the dev-mode setter removal as decided Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 docs(ognl): add implementation plan for sharing parsed security config Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 docs(ognl): fix Task 5 to handle the tests the signature change breaks Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 perf(config): hoist the whitespace pattern in validatePackageNames Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 feat(ognl): add a container-singleton OGNL security config bean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 perf(ognl): share parsed config across SecurityMemberAccess instances Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * WW-5675 refactor(ognl): drop the lazy dev-mode flip from the access path Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 fix(ognl): register SecurityMemberAccessConfig for the Dispatcher container Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * WW-5675 test(ognl): cover the production registration of the config bean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-5675 perf(ognl): precompute the allowlist package union Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * WW-5675 docs(ognl): trim the deprecation note on the retained setters Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>