Avoid reflective final field mutation in BasicLookupStrategy#19331
Open
seonwooj0810 wants to merge 1 commit into
Open
Avoid reflective final field mutation in BasicLookupStrategy#19331seonwooj0810 wants to merge 1 commit into
seonwooj0810 wants to merge 1 commit into
Conversation
`BasicLookupStrategy.convert(...)` previously used reflection to re-write two final fields when re-parenting ACEs onto the freshly created `AclImpl`: - `AclImpl.aces` was overwritten via `Field.set(...)` - `AccessControlEntryImpl.acl` was overwritten via `Field.set(...)` On Java 26 the JVM emits a `WARNING: Final field ... has been mutated reflectively` for each ACL load, and a future release intends to block this entirely. Mutating the field reference was never required: - The new `AclImpl` is constructed with an empty, mutable `aces` list, so the ACEs can be added to that existing list in place. Only the field is `final`; the list it points to is not. - `AccessControlEntryImpl` is documented as immutable and exposes a constructor that accepts every field. Re-creating each ACE with the converted `Acl` back-reference removes the StubAclParent reference exactly as before, without touching its final `acl` field. `fieldAcl` and `setAclOnAce(...)` are removed, and `setAces(...)` is replaced with `addAces(...)` that calls `addAll` on the result's existing aces list. The reflective read of `AclImpl.aces` is kept, since reads of final fields are unaffected. Closes spring-projectsgh-19127
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes gh-19127
Background
On Java 26 the JVM warns whenever ACLs are loaded through
BasicLookupStrategy:BasicLookupStrategy.convert(...)is responsible for re-parenting a loadedAclImplonto a freshly constructed one: stub parent references are resolved and the back-reference on everyAccessControlEntryImplis rewritten to point at the new ACL (per SEC-951). The previous implementation did this by reflectively rewriting twofinalfields:AclImpl.aceswas replaced throughField.set(...)(setAces).AccessControlEntryImpl.aclwas replaced throughField.set(...)(setAclOnAce).Both writes trigger the Java 26 warning today and will be rejected outright on the JVMs that flip the default. Reading the
acesfield reflectively is not affected — only mutation is.Change
Neither final field actually needs to be replaced:
AclImplis created with an empty, mutableaceslist (initialised inline asnew ArrayList<>()). Adding to that existing list in place achieves the same end state without touching the field reference.addAces(replacingsetAces) reuses the existing read helper and callsaddAllon the list.AccessControlEntryImplis documented as immutable and exposes a public constructor that takes every field. Re-creating each ACE with the convertedAcllets us substitute theStubAclParent-bearing ACE for an equivalent one without mutating any final field.setAclOnAceand the cachedFieldforAccessControlEntryImpl.aclgo away entirely.The reflective read of
AclImpl.acesis preserved — it is necessary to populate the result list and is unaffected by the upcoming JVM restriction. The internalProcessResultSet.mapRowpath is unchanged: it already uses the same read-then-addidiom to populate the input ACLs.Tests
Added
readAclsByIdWhenChildLoadedThenAcesPointToConvertedAcltoAbstractBasicLookupStrategyTests, which asserts the SEC-951 invariant directly: afterreadAclsById(...), every ACE returned byacl.getEntries()reports the convertedAcl(not aStubAclParent) viagetAcl(). BothBasicLookupStrategyTestsandBasicLookupStrategyWithAclClassTypeTestsextend the abstract class, so the new test runs against both class-id strategies../gradlew :spring-security-acl:checkpasses (test, checkFormat, checkstyle, license, jacoco).Verification done
gh pr list --repo spring-projects/spring-security --search "AclImpl OR \"final field\" OR \"Java 26\""returns nothing related, andgh pr list --search "19127"is empty.acl/src/main/java/.../BasicLookupStrategy.javaand the existing abstract test class.fieldAces.set(...)andfieldAcl.set(...)still live inBasicLookupStrategyat HEAD (5594fc018f).BasicLookupStrategymutatingAclImpl.aces, which is the call this change removes.