Skip to content

Commit 2c939bb

Browse files
committed
fix(security): make self-proxied transactional persist methods protected (not package-private)
registerNewUserAccount, changeUserPassword, and setInitialPassword route their DB write back through the @lazy self proxy (self.persistNewUserAccount / persistChangedPassword / persistInitialPassword) so the short @transactional applies after the bcrypt hash runs outside any transaction. Those persist* methods were package-private. In Spring Framework 7 the CGLIB proxy subclass is generated in a different package, so it cannot override package-private methods: self.persistX(...) then executes the inherited body on the proxy instance — whose @Autowired fields are never injected — throwing NullPointerException ("this.userRepository is null") and silently dropping the transaction. This broke registration, password change, password reset, and set-initial-password for consumers. Caught by the demo-app Playwright E2E (Spring Boot 4.0.4); the library's own CI runs 4.0.6, where the behavior is masked — so a @SpringBootTest on the pinned CI version cannot be relied on to catch it. Fix by making the three methods protected (CGLIB overrides public/protected across packages; matches the already-correct protected GdprDeletionService.executeUserDeletion and public UserEmailService.createPasswordResetTokenForUser). Add SelfProxiedMethodVisibilityTest: a version-independent, bytecode-level guard that fails fast if any self-proxied method is ever made package-private/private again.
1 parent fb9694a commit 2c939bb

2 files changed

Lines changed: 98 additions & 11 deletions

File tree

src/main/java/com/digitalsanctuary/spring/user/service/UserService.java

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -372,18 +372,21 @@ public User registerNewUserAccount(final UserDto newUserDto) {
372372
* <p>
373373
* Internal seam: this method exists only to split the DB write away from the bcrypt hash. It MUST
374374
* be invoked through the Spring proxy (via {@link #self}) so the transaction applies. It is
375-
* deliberately <b>package-private</b> so consumers cannot call it directly and bypass the
376-
* centralized RegistrationGuard enforced by {@link #registerNewUserAccount(UserDto)}; CGLIB
377-
* self-invocation still applies the transaction because Spring's proxy subclass is generated in
378-
* this same package.
375+
* deliberately <b>protected</b> so consumers cannot call it directly and bypass the centralized
376+
* RegistrationGuard enforced by {@link #registerNewUserAccount(UserDto)}. It must be
377+
* {@code protected} rather than package-private: the CGLIB proxy subclass is generated in a
378+
* different package, so it can only override (and therefore advise/route) {@code public} or
379+
* {@code protected} methods. A package-private method is not overridden, so the {@code self}
380+
* invocation would execute on the proxy instance — whose {@code @Autowired} fields are never
381+
* populated — and both the transaction and the dependencies would be missing.
379382
* </p>
380383
*
381384
* @param user the fully built user entity (password already encoded)
382385
* @return the saved user entity
383386
* @throws UserAlreadyExistException if an account with the same email already exists
384387
*/
385388
@Transactional(isolation = Isolation.SERIALIZABLE)
386-
User persistNewUserAccount(final User user) {
389+
protected User persistNewUserAccount(final User user) {
387390
if (emailExists(user.getEmail())) {
388391
log.debug("UserService.persistNewUserAccount: email already exists: {}", user.getEmail());
389392
throw new UserAlreadyExistException(
@@ -755,15 +758,18 @@ public void changeUserPassword(final User user, final String password) {
755758
* <p>
756759
* Internal seam: this method exists only to split the DB write away from the bcrypt hash. It MUST
757760
* be invoked through the Spring proxy (via {@link #self}) so the transaction applies. It is
758-
* deliberately <b>package-private</b> so it is not part of the public API; CGLIB self-invocation
759-
* still applies the transaction because Spring's proxy subclass is generated in this same package.
761+
* <b>protected</b> so it stays out of the public API yet remains overridable by the CGLIB proxy
762+
* subclass — which is generated in a different package and therefore cannot override a
763+
* package-private method. A package-private method would not be advised and the {@code self}
764+
* invocation would run on the proxy instance (whose {@code @Autowired} fields are null), so it
765+
* must be {@code protected} (or public).
760766
* </p>
761767
*
762768
* @param user the user whose password changed (password field already set/encoded)
763769
* @param encodedPassword the already-encoded password to record in history
764770
*/
765771
@Transactional
766-
void persistChangedPassword(final User user, final String encodedPassword) {
772+
protected void persistChangedPassword(final User user, final String encodedPassword) {
767773
userRepository.save(user);
768774
savePasswordHistory(user, encodedPassword);
769775
// Force re-auth on a password change (OWASP). By default the current session is preserved and
@@ -856,15 +862,18 @@ public void setInitialPassword(User user, String rawPassword) {
856862
* <p>
857863
* Internal seam: this method exists only to split the DB write away from the bcrypt hash. It MUST
858864
* be invoked through the Spring proxy (via {@link #self}) so the transaction applies. It is
859-
* deliberately <b>package-private</b> so it is not part of the public API; CGLIB self-invocation
860-
* still applies the transaction because Spring's proxy subclass is generated in this same package.
865+
* <b>protected</b> so it stays out of the public API yet remains overridable by the CGLIB proxy
866+
* subclass — which is generated in a different package and therefore cannot override a
867+
* package-private method. A package-private method would not be advised and the {@code self}
868+
* invocation would run on the proxy instance (whose {@code @Autowired} fields are null), so it
869+
* must be {@code protected} (or public).
861870
* </p>
862871
*
863872
* @param user the user whose initial password is being set (password field already set)
864873
* @param encodedPassword the already-encoded password to record in history
865874
*/
866875
@Transactional
867-
void persistInitialPassword(final User user, final String encodedPassword) {
876+
protected void persistInitialPassword(final User user, final String encodedPassword) {
868877
userRepository.save(user);
869878
savePasswordHistory(user, encodedPassword);
870879
}
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
package com.digitalsanctuary.spring.user.architecture;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import java.lang.reflect.Method;
5+
import java.lang.reflect.Modifier;
6+
import java.util.Arrays;
7+
import java.util.List;
8+
import org.junit.jupiter.api.DisplayName;
9+
import org.junit.jupiter.params.ParameterizedTest;
10+
import org.junit.jupiter.params.provider.Arguments;
11+
import org.junit.jupiter.params.provider.MethodSource;
12+
import com.digitalsanctuary.spring.user.gdpr.GdprDeletionService;
13+
import com.digitalsanctuary.spring.user.service.UserEmailService;
14+
import com.digitalsanctuary.spring.user.service.UserService;
15+
16+
/**
17+
* Structural guard for the Spring self-invocation proxy pattern used across the service layer.
18+
*
19+
* <p>
20+
* Several services split a slow, non-transactional step (e.g. bcrypt hashing) from the short DB write by routing the
21+
* write back through their own Spring proxy: {@code self.persistX(...)} where {@code self} is an
22+
* {@code @Lazy @Autowired} reference to the same bean. For that to work the target method <strong>must be
23+
* {@code public} or {@code protected}</strong>: Spring generates the CGLIB proxy subclass in a <em>different</em>
24+
* package, so it can only override (and therefore advise + route) {@code public}/{@code protected} methods. A
25+
* <em>package-private</em> target is not overridden, so {@code self.persistX(...)} executes the inherited body on the
26+
* proxy instance — whose {@code @Autowired} fields were never populated — yielding a {@link NullPointerException} (e.g.
27+
* {@code "this.userRepository" is null}) and silently dropping the transaction.
28+
* </p>
29+
*
30+
* <p>
31+
* This bug is <strong>version-dependent at runtime</strong>: it reproduces on some Spring Framework patch releases and
32+
* is masked on others, so a {@code @SpringBootTest} on the CI's pinned Spring version cannot be relied on to catch a
33+
* regression. This bytecode-level visibility check is version-independent and fails fast if any self-proxied method is
34+
* ever made package-private (or private) again.
35+
* </p>
36+
*
37+
* <p>
38+
* Note: this rule is intentionally scoped to the specific methods invoked via {@code self}. Other package-private
39+
* {@code @Transactional} helpers (e.g. {@code RolePrivilegeSetupService.getOrCreateRole}) are called via {@code this}
40+
* from within an already-transactional method and run in the caller's transaction, so they do not need to be proxied.
41+
* </p>
42+
*/
43+
@DisplayName("Self-proxied (@Lazy self) transactional methods must be proxyable (public/protected)")
44+
class SelfProxiedMethodVisibilityTest {
45+
46+
/**
47+
* Every method that is invoked through a {@code self} proxy reference in the production code. Keep this list in
48+
* sync with the {@code self.<method>(...)} call sites in the service/gdpr packages.
49+
*/
50+
static List<Arguments> selfProxiedMethods() {
51+
return List.of(Arguments.of(UserService.class, "persistNewUserAccount"),
52+
Arguments.of(UserService.class, "persistChangedPassword"),
53+
Arguments.of(UserService.class, "persistInitialPassword"),
54+
Arguments.of(GdprDeletionService.class, "executeUserDeletion"),
55+
Arguments.of(UserEmailService.class, "createPasswordResetTokenForUser"));
56+
}
57+
58+
@ParameterizedTest(name = "{0}#{1} is public or protected")
59+
@MethodSource("selfProxiedMethods")
60+
void selfInvokedMethodMustBeProxyable(final Class<?> declaringClass, final String methodName) {
61+
final List<Method> matches = Arrays.stream(declaringClass.getDeclaredMethods())
62+
.filter(m -> m.getName().equals(methodName)).toList();
63+
64+
assertThat(matches).as("expected to find method %s#%s — has it been renamed or removed? Update this guard.",
65+
declaringClass.getSimpleName(), methodName).isNotEmpty();
66+
67+
for (final Method method : matches) {
68+
final int modifiers = method.getModifiers();
69+
assertThat(Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers))
70+
.as("%s#%s is invoked through the Spring self-proxy and MUST be public or protected; a "
71+
+ "package-private/private method is not overridden by the CGLIB proxy subclass (generated "
72+
+ "in a different package), so the self-invocation runs on the un-injected proxy instance "
73+
+ "and throws NPE while silently losing the transaction.",
74+
declaringClass.getSimpleName(), methodName)
75+
.isTrue();
76+
}
77+
}
78+
}

0 commit comments

Comments
 (0)