Security & reliability hardening for 4.4.0 (Tier 1 remediation, MFA #313, session regen)

[devondragon]

Summary

Security and reliability hardening for the 4.4.0 release (Spring Boot 4.0 / Spring Security 7). This branch executes the Tier 1 remediation plan, fixes issues found by a follow-up code review, resolves the MFA factor-merging issue (#313), and was validated end-to-end against the demo app (Playwright 103/103).

46 commits. Library suite: 854 tests green, Javadoc builds, runtime-tested on Java 21 + 25.

Highlights

Security fixes

• RegistrationGuard bypass closed — internal persist* methods are now package-private so consumers can't skip the centralized guard.
• Verification tokens are single-use — validateVerificationToken now enables the user and deletes the token atomically (was a non-atomic two-step that allowed replay).
• Facebook OAuth2 now rejects an explicitly-unverified email (parity with the Google path).
• OAuth2 failure handler no longer logs raw exception messages (which can embed account emails) at ERROR; expected failures log a non-sensitive summary at WARN.
• MFA factor merging (MFA: second-factor login replaces session authentication instead of merging factor authorities (flow can never complete) #313) — the framework enables SS7 factor merging on the auth filters itself (no consumer @EnableMultiFactorAuthentication workaround needed), and auto-unprotects the configured factor entry-point URIs to prevent the partial-auth redirect loop.

Reliability / correctness

• SecurityFilterChain coexistence (was a regression) — the library chain backed off via a type-based @ConditionalOnMissingBean(SecurityFilterChain.class), which silently suppressed the entire library chain (form login, CSRF, sessions, WebAuthn, OAuth2) the moment a consumer defined any chain — e.g. a narrow actuator/test-API chain. Now keyed on the bean name securityFilterChain, so additional consumer chains coexist (standard multi-chain @Order layering) and a full replacement is an explicit opt-in. This is what broke ~70 demo Playwright tests until fixed.
• Session regeneration on password change — a self-service password change now preserves and regenerates the current session (OWASP) and invalidates only the user's other sessions, instead of logging the user out of the device they just used. Toggle: user.session.invalidation.keep-current-session-on-password-change (default true).
• Security beans (methodSecurityExpressionHandler, httpSessionEventPublisher, authenticationEventPublisher) moved to an auto-config with @ConditionalOnMissingBean so consumer overrides no longer conflict.
• MFA filter-merging post-processor isolated into its own dependency-free @Configuration to avoid early-instantiation of the event-listening MfaConfiguration.

Infra

• CI workflow added (Phase 0): build/test on Java 21 + 25 plus CodeQL (security-extended). Java 17 dropped (SB 4.0 requires 21+).
• Javadoc jar now builds (release blocker fixed: registered @apiNote/@implSpec/@implNote tags; fixed a Lombok-getter @link).

Breaking changes (documented in MIGRATION.md)

• UserEmailService constructor gained a TokenHasher parameter (affects subclasses).
• Passay upgraded 1.x → 2.0.0 (package relocations; only affects apps using Passay directly).
• Full-replacement SecurityFilterChain must now be named securityFilterChain.
• Session-on-password-change behavior change (above), with an opt-out flag.

Validation

• Library: 854 tests, 0 failures; Javadoc builds; testJdk25 green.
• Demo app (consuming the local snapshot): boots clean, Playwright 103/103, MFA integration tests green with the consumer workaround removed.
• The SecurityFilterChain regression and the session/MFA fixes were each caught or confirmed by running the demo app end-to-end.

Closes #313.

[claude]

Code Review — PR #314: Security & Reliability Hardening for 4.4.0

This is a well-scoped, security-first release with thorough documentation. The architectural refactors (bean lifecycle, auto-configuration, multi-chain layering) are correct and the security fixes are substantive. Below is a focused review organized by topic.

---

Security ✅ (Fixes are solid — a few notes)

Token hashing (TokenHasher, dual-read fallback) — well implemented. The dual-read strategy (hash first, raw fallback) is backward-compatible and the fallback doesn't need an expiry cutoff because existing tokens expire naturally. No concerns.

Single-use tokens — validateVerificationToken now atomically enables the user AND deletes the token in one @Transactional method. validateAndConsumePasswordResetToken does the same. This closes the replay window. Good.

Email leak in exception messages — LoginHelperService.assertAccountUsable embeds the user's email in the LockedException/DisabledException message:

throw new LockedException("Account is locked: " + user.getEmail());

The SanitizingOAuth2AuthenticationFailureHandler correctly sanitizes this for the OAuth2 path (logs only the exception type). But if these exceptions propagate through any other handler (e.g., a consumer's global @ExceptionHandler, or Spring's default error attributes) they could appear in logs at ERROR level with PII in the message. Consider embedding the email in a structured field and keeping the message generic, or at minimum document the sanitization dependency.

CompositeRegistrationGuard.evaluate — null guard return — the null check if (decision != null && !decision.allowed()) silently treats a null-returning guard as "allow." If a custom consumer guard accidentally returns null (e.g. an uninitialized code path), registration proceeds without any guard applied. It would be safer to throw IllegalStateException on null so the bug is caught at startup/test time rather than being silently permissive.

isExplicitlyUnverified for unknown types — the comment says "trust (do not reject) rather than risk locking out legitimate users" for unknown email_verified types. This is a reasonable default, but worth noting in docs/changelog: a provider that returns the claim as an Integer (1/0) would be trusted without verification. Consider a debug-level log for the unknown-type case so operators can detect unexpected claim formats.

---

Architecture / Spring Boot Lifecycle ✅

WebSecurityFilterChainAutoConfiguration + name-based conditional — the switch from @ConditionalOnMissingBean(SecurityFilterChain.class) (type) to @ConditionalOnMissingBean(name = "securityFilterChain") is architecturally correct and the documentation in MIGRATION.md is clear.

UserSecurityBeansAutoConfiguration — moving @ConditionalOnMissingBean-guarded beans from the component-scanned WebSecurityConfig to an @AutoConfiguration class is the right fix. These conditionals are only reliable in auto-configuration (which loads after user-defined beans).

MfaFilterMergingConfiguration as a separate @Configuration — correct. Placing a static BeanPostProcessor @Bean in its own dependency-free configuration avoids the "not eligible for getting processed by all BeanPostProcessors" warning on MfaConfiguration.

---

Correctness Concerns

Propagation.NOT_SUPPORTED + self proxy pattern — the design is sound: bcrypt outside the transaction, short DB-write inside. A few notes:

1. registerNewUserAccount now evaluates the guard before checking for a duplicate email. Previously the duplicate check short-circuited before the guard. Now a duplicate-email submission hits the guard, triggers the slow bcrypt encode, then fails with UserAlreadyExistException inside persistNewUserAccount. Under the default bcrypt strength (12, ~300ms), an attacker can force deliberate CPU work by submitting registrations for existing emails. This is likely acceptable given expected rate limiting at the controller/gateway level, but worth documenting.

2. The self reference (@Lazy @Autowired private UserService self) is a known pattern for transaction proxy call-through, but if a consumer subclasses UserService and overrides registerNewUserAccount, the self reference will call through the proxy to the subclass, which could double-apply the guard if the subclass calls super.registerNewUserAccount(...). This is an edge case but worth a brief note in the JavaDoc.

cleanUpPasswordHistory — comment vs. reality — the comment says "This method runs within the caller's class-level transaction (it is invoked via self-invocation, so any method-level @Transactional would be bypassed by the proxy and never apply)." With the new NOT_SUPPORTED outer methods, the actual call chain is changeUserPassword (NOT_SUPPORTED) → self.persistChangedPassword (@transactional via proxy) → savePasswordHistory → cleanUpPasswordHistory. The comment's claim about "bypassed by proxy" is now misleading since the path goes through the proxy at persistChangedPassword. The behavior is correct but the comment needs updating.

FileAuditLogFlushScheduler conditional default mismatch — @ConditionalOnExpression("${user.audit.logEvents:true} && !${user.audit.flushOnWrite:true}") defaults flushOnWrite to true, meaning the scheduler is not created when the property is absent. But AuditConfig.flushOnWrite defaults to false (Java field default), meaning if the properties file isn't loaded the config class says "buffer writes" but no scheduler exists to flush the buffer. In practice dsspringuserconfig.properties sets flushOnWrite=false explicitly, so it works. But the SpEL default true is misleading and will surprise anyone who inspects the @ConditionalOnExpression without reading the properties file.

---

Code Quality / Cleanup

tokenFingerprint duplication — the method is copied identically into UserVerificationService and UserActionController. It should live in TokenHasher (or a shared utility) as a public static method and be called from both.

disableCSRFURIs default changed — changed from /no-csrf-test to empty. Good: no test path should ship as a production default.

user.mail.fromAddress default — changed from test@test.com to empty. Correct; the startup warning for blank fromAddress (added in MailService.init) is a good companion.

LoginSuccessService debug logging — the old log line "LoginSuccessService.onAuthenticatonSuccess: called with authentiation: {}" (two typos) is fixed. Good catch.

---

Test Coverage

Given the number of security-critical changes, a few specific coverage questions:

• Is there a test exercising the HMAC-SHA-256 path in TokenHasher (i.e., with user.security.tokenHashSecret set)? The dual-read fallback path (hash lookup → raw fallback) should have a dedicated test for the backward-compat case.
• Is the CompositeRegistrationGuard null-decision behavior tested?
• Is invalidateSessionsAfterPasswordChange tested when no current request/session is bound to the thread (the non-web thread path)?

---

Minor Nits

• UserService.persistNewUserAccount and persistChangedPassword are package-private to prevent bypass — but the JavaDoc references "CGLIB self-invocation … generated in this same package." The CGLIB proxy subclass is generated at runtime in the same package as the target class, so this is correct. Just worth confirming this holds if the package is ever renamed/split.
• The serialVersionUID = 1L added to User, Role, Privilege is correct for distributed session stores. Consider also auditing any @Embedded types or collections the session-stored DSUserDetails reaches at runtime (e.g., the role/privilege collections).

---

Summary

This is a high-quality, well-motivated PR. The security fixes are correct, the architectural moves to auto-configuration are sound, and the documentation (MIGRATION.md, CONFIG.md, REGISTRATION-GUARD.md) is thorough. The main actionable items are:

1. PII in exception messages (LockedException/DisabledException in assertAccountUsable) — either make the message generic or document the sanitization dependency more prominently.
2. Null guard return in CompositeRegistrationGuard.evaluate — throw instead of silently allowing.
3. FlushScheduler SpEL default mismatch — fix the :true default to :false to match AuditConfig.
4. tokenFingerprint duplication — extract to a shared utility.
5. Update the cleanUpPasswordHistory comment to reflect the new call chain.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Hardens the framework for the 4.4.0 line by tightening security boundaries (token single-use + at-rest hashing, registration-guard enforcement, OAuth/OIDC safety), fixing a Spring Security 7 MFA factor-merging integration gap, and improving operational reliability (session regeneration policy, audit logging scalability, mail executor isolation, CI coverage).

Changes:

• Hash verification/password-reset tokens at rest (optional HMAC secret), enforce single-use/atomic consumption, and update related controller/service logic + tests.
• Fix MFA factor merging by enabling SS7 merging on authentication processing filters when user.mfa.enabled=true, and auto-unprotect MFA factor entry-point URIs to prevent partial-auth redirect loops.
• Improve reliability/ops: name-keyed SecurityFilterChain backoff, bounded/rotating audit log sink + bounded queries, dedicated mail executor, session invalidation behavior changes, and CI workflow (JDK 21 + 25 + CodeQL).

Reviewed changes

Copilot reviewed 115 out of 116 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/test/java/com/digitalsanctuary/spring/user/util/JpaAuditingConfigTest.java	New tests for user.jpa.auditing.enabled gating behavior.
src/test/java/com/digitalsanctuary/spring/user/service/UserVerificationServiceTest.java	Updates verification-token validation tests for hashing + single-use consumption.
src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java	Updates email/token tests to assert raw-vs-hash behavior with TokenHasher.
src/test/java/com/digitalsanctuary/spring/user/service/TokenHasherTest.java	Adds unit coverage for token hashing behavior (SHA-256 vs keyed HMAC).
src/test/java/com/digitalsanctuary/spring/user/service/SessionInvalidationServiceTest.java	Adds tests for preserving/regenerating current session on password change.
src/test/java/com/digitalsanctuary/spring/user/service/PostgreSQLConcurrentRegistrationTest.java	Adds Postgres Testcontainers coverage for concurrent registration race.
src/test/java/com/digitalsanctuary/spring/user/service/MariaDBConcurrentRegistrationTest.java	Adds MariaDB Testcontainers coverage for concurrent registration race.
src/test/java/com/digitalsanctuary/spring/user/service/LogoutSuccessServiceTest.java	Adjusts header stubbing to avoid strict-mock failures.
src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsTest.java	Adds tests for account locked status mapping.
src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsSerializationTest.java	Adds serialization round-trip test for session principal graph.
src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java	Expands OIDC tests for registration event publishing and email_verified handling.
src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java	Updates guard enforcement tests to service-layer guard + exception translation.
src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java	Updates OAuth2 guard enforcement tests to service-layer guard + exception translation.
src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityCsrfTest.java	Adds CSRF enforcement tests against the real filter chain.
src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationFailClosedTest.java	Adds fail-closed tests for invalid user.security.defaultAction.
src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java	Adds authorization-model tests for defaultAction=deny.
src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationAllowTest.java	Adds authorization-model tests for defaultAction=allow.
src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java	Updates tests for optional event publishing on WebAuthn success.
src/test/java/com/digitalsanctuary/spring/user/security/SessionInvalidationIntegrationTest.java	Adds integration test to assert SessionRegistry population on login.
src/test/java/com/digitalsanctuary/spring/user/security/SanitizingOAuth2AuthenticationFailureHandlerTest.java	Adds tests ensuring OAuth2/OIDC failure messages are sanitized.
src/test/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfigurationTest.java	Adds tests for MFA filter-merging post-processor gating and behavior.
src/test/java/com/digitalsanctuary/spring/user/security/LogoutAuditIntegrationTest.java	Adds integration test proving logout audit handler wiring and event publication.
src/test/java/com/digitalsanctuary/spring/user/security/HtmxAwareAuthenticationEntryPointConfigurationTest.java	Adjusts test config to avoid component-scan leakage into integration contexts.
src/test/java/com/digitalsanctuary/spring/user/registration/RegistrationGuardConfigurationTest.java	Adds wiring tests for default + composite guard behavior.
src/test/java/com/digitalsanctuary/spring/user/registration/CompositeRegistrationGuardTest.java	Adds unit tests for first-deny-wins ordering semantics.
src/test/java/com/digitalsanctuary/spring/user/profile/session/SessionScopedProfileTest.java	Adds tests proving session-scoping requirement and meta-annotation behavior.
src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryTest.java	Adds repository slice tests for atomic failed-attempt increments.
src/test/java/com/digitalsanctuary/spring/user/persistence/repository/PasswordHistoryRepositoryTest.java	Adds repository slice tests for bounded pruning queries.
src/test/java/com/digitalsanctuary/spring/user/persistence/model/UserToStringTest.java	Adds test to ensure User#toString() does not leak password hashes.
src/test/java/com/digitalsanctuary/spring/user/mail/MailServiceTest.java	Adds log-based tests for from-address warnings when mail is enabled.
src/test/java/com/digitalsanctuary/spring/user/mail/MailServiceOverrideTest.java	Adds override-contract tests for consumer replaceability of MailService.
src/test/java/com/digitalsanctuary/spring/user/mail/MailExecutorConfigurationTest.java	Adds tests for dedicated bounded mail executor + @Async qualification.
src/test/java/com/digitalsanctuary/spring/user/listener/RegistrationListenerTest.java	Updates tests for skipping verification email for already-enabled social users.
src/test/java/com/digitalsanctuary/spring/user/jdbc/Jdbc.java	Removes legacy JDBC test helper (and embedded credentials).
src/test/java/com/digitalsanctuary/spring/user/jdbc/ConnectionManager.java	Removes legacy JDBC connection helper (and embedded credentials).
src/test/java/com/digitalsanctuary/spring/user/event/UserDisabledEventTest.java	Adds unit tests for new UserDisabledEvent.
src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java	Updates controller test expectations for atomic token consumption behavior.
src/test/java/com/digitalsanctuary/spring/user/audit/FileAuditLogWriterTest.java	Adds rotation behavior tests for file-based audit writer.
src/test/java/com/digitalsanctuary/spring/user/audit/FileAuditLogQueryServiceTest.java	Adds tests for bounded max-results query behavior.
src/test/java/com/digitalsanctuary/spring/user/audit/AuditEventListenerTest.java	Updates tests for lazy AuditLogWriter resolution via ObjectProvider.
src/test/java/com/digitalsanctuary/spring/user/architecture/ArchitectureTest.java	Adds ArchUnit rules for layering + logging restrictions.
src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java	Removes unused RegistrationGuard mocking after service-layer enforcement change.
src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java	Updates guard-denial tests to service-layer RegistrationDeniedException.
src/test/java/com/digitalsanctuary/spring/user/api/MfaFeatureEnabledIntegrationTest.java	Adds integration assertion for auto-unprotected MFA entry-point URI.
src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports	Registers new auto-configurations (audit/mail + security beans/filter chain).
src/main/resources/META-INF/additional-spring-configuration-metadata.json	Adds metadata for new/updated configuration properties (auditing, audit rotation, token hashing, validity minutes).
src/main/resources/config/dsspringuserconfig.properties	Updates defaults/docs for audit rotation, CSRF exemptions, token properties, mail fromAddress default.
src/main/java/com/digitalsanctuary/spring/user/util/JpaAuditingConfig.java	Gates JPA auditing behind user.jpa.auditing.enabled and reduces sensitive logging.
src/main/java/com/digitalsanctuary/spring/user/UserConfiguration.java	Adjusts component-scan exclude filters to respect TypeExclude/AutoConfiguration filters.
src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java	Adds TokenHasher, token validity minutes, and hashed single-token issuance logic.
src/main/java/com/digitalsanctuary/spring/user/service/TokenHasher.java	Introduces deterministic token hashing (SHA-256 / HMAC-SHA-256).
src/main/java/com/digitalsanctuary/spring/user/service/LogoutSuccessService.java	Reduces sensitive logout logging (logs user name instead of full auth).
src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java	Reduces sensitive login logging (logs user name instead of full auth/principal).
src/main/java/com/digitalsanctuary/spring/user/service/LoginHelperService.java	Enforces locked/disabled status across auth paths (form/OAuth2/OIDC/WebAuthn).
src/main/java/com/digitalsanctuary/spring/user/service/LoginAttemptService.java	Uses atomic DB increment to avoid lockout-evasion lost-update race.
src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java	Enforces centralized registration guard; publishes registration event; rejects explicit email_verified=false.
src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityFilterChainAutoConfiguration.java	Provides library SecurityFilterChain via name-keyed conditional backoff.
src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandler.java	Publishes InteractiveAuthenticationSuccessEvent for converted DSUserDetails-bearing auth.
src/main/java/com/digitalsanctuary/spring/user/security/SanitizingOAuth2AuthenticationFailureHandler.java	Adds sanitized OAuth2/OIDC failure handler to avoid leaking exception messages to session/logs.
src/main/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfiguration.java	Enables SS7 factor merging on auth-processing filters when MFA enabled.
src/main/java/com/digitalsanctuary/spring/user/security/MfaConfiguration.java	Documents/enforces split between MFA enforcement vs filter-side merging.
src/main/java/com/digitalsanctuary/spring/user/registration/RegistrationGuardConfiguration.java	Adds composite registration guard and ensures safe default fallback behavior.
src/main/java/com/digitalsanctuary/spring/user/registration/RegistrationDeniedException.java	Adds service-layer exception for guard denials.
src/main/java/com/digitalsanctuary/spring/user/registration/CompositeRegistrationGuard.java	Implements first-deny-wins guard composition.
src/main/java/com/digitalsanctuary/spring/user/profile/session/SessionScopedProfile.java	Adds meta-annotation to avoid session-scope inheritance trap.
src/main/java/com/digitalsanctuary/spring/user/profile/session/BaseSessionProfile.java	Documents non-inherited scope risk and points to meta-annotation.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/VerificationTokenRepository.java	Adds deleteByUser for single-active-token enforcement.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java	Adds atomic increment query for failed login attempts.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/PasswordResetTokenRepository.java	Adds deleteByUser for single-active-token enforcement.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/PasswordHistoryRepository.java	Adds bounded prune queries (cutoff id + set-based delete).
src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java	Adds configurable lifetime + transient plain-token carrier field.
src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java	Implements Serializable and excludes password from toString().
src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java	Implements Serializable for session principal graph safety.
src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java	Implements Serializable for session principal graph safety.
src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java	Adds configurable token lifetime constructor.
src/main/java/com/digitalsanctuary/spring/user/mail/MailService.java	Moves to auto-configured bean, warns on blank fromAddress, and qualifies @Async to dedicated executor.
src/main/java/com/digitalsanctuary/spring/user/mail/MailExecutorConfiguration.java	Adds bounded dedicated executor dsMailExecutor with backpressure.
src/main/java/com/digitalsanctuary/spring/user/listener/RegistrationListener.java	Skips verification email for already-enabled users (social registration).
src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionService.java	Defers post-delete event publication until after transaction commit.
src/main/java/com/digitalsanctuary/spring/user/event/UserDisabledEvent.java	Adds new post-commit observable event for soft-deletes.
src/main/java/com/digitalsanctuary/spring/user/event/UserDeletedEvent.java	Clarifies event is delivered after commit and how it’s deferred.
src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java	Stops logging raw tokens; fingerprints tokens; resolves user before consuming token.
src/main/java/com/digitalsanctuary/spring/user/audit/FileAuditLogQueryService.java	Bounds query memory/CPU with a capped window of most recent matches.
src/main/java/com/digitalsanctuary/spring/user/audit/FileAuditLogFlushScheduler.java	Moves scheduler to auto-config bean to back off when file writer is replaced/absent.
src/main/java/com/digitalsanctuary/spring/user/audit/AuditMailAutoConfiguration.java	Adds auto-config for default audit writer + flush scheduler + consumer-overridable MailService.
src/main/java/com/digitalsanctuary/spring/user/audit/AuditEventListener.java	Lazily resolves AuditLogWriter to allow audit to be fully disabled without missing-bean failures.
src/main/java/com/digitalsanctuary/spring/user/audit/AuditConfig.java	Adds audit rotation properties + clarifies flush durability behavior.
src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java	Moves registration guard enforcement to service layer; atomically consumes password-reset token.
REGISTRATION-GUARD.md	Updates SPI docs for composite guard behavior + service-layer enforcement.
PROFILE.md	Documents session-scope inheritance trap + @SessionScopedProfile usage.
CONFIG.md	Updates config docs for audit rotation/durability, JPA auditing gate, token hashing, and mail executor.
CLAUDE.md	Updates test dependency overview documentation.
build.gradle	Updates test deps, adds Javadoc tags, drops JDK17 test task, adds JDK25 testing.
.github/workflows/build.yml	Adds CI workflow for build/test on JDK 21 + 25 and CodeQL security-extended.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[devondragon]

Good catch — confirmed the @Transactional was a no-op via self-invocation. Fixed in the latest commit: added a @Lazy @Autowired UserEmailService self proxy reference (mirroring the existing pattern in UserService) and routed both callers (sendForgotPasswordVerificationEmail and initiateAdminPasswordReset) through self.createPasswordResetTokenForUser(...) so the deleteByUser + save now commit atomically. Updated UserEmailServiceTest to wire self to the instance (no proxy in unit tests).

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[claude]

Code Review — PR #314: Security & reliability hardening for 4.4.0

Great PR overall. The security fixes are well-reasoned, the documentation is thorough, and the test suite additions are comprehensive. Here are my findings, organized by severity.

---

Security

✅ Token hashing at rest (TokenHasher)
Clean implementation. The dual-read fallback (hash → plaintext) for pre-upgrade tokens is safe because every token has a bounded expiry, so no lingering plaintext token is permanently trusted. The HMAC-vs-SHA-256 tradeoff is well-documented.

One note: TokenHasher.hash() creates a new Mac/MessageDigest instance on every call. These are lightweight but not free. For high-traffic deployments a ThreadLocal or object pool might be worth considering in a follow-up. Not a blocker.

✅ Verification token atomic consumption
Enabling the user AND deleting the token in the same @Transactional is the right fix for the replay vulnerability. The controller correctly resolves the User before calling validateVerificationToken — the code comment makes this ordering clear.

⚠️ validateAndConsumePasswordResetToken — potential TOCTOU under extreme concurrency
The method reads and deletes the token in a single @Transactional boundary, which is good. However, with the default READ_COMMITTED isolation, two concurrent requests can both read the same token row before either commits a delete. Both would receive the user and both would proceed to changeUserPassword. The second delete is a no-op (the row is already gone), but both password changes could execute. Adding a SELECT FOR UPDATE (via @Lock(LockModeType.PESSIMISTIC_WRITE) on the repository method) or promoting the method to SERIALIZABLE isolation would close this window. Low-probability in practice, but worth hardening given the security context.

⚠️ assertAccountUsable exception messages embed the user email

In LoginHelperService:

throw new LockedException("Account is locked: " + user.getEmail());
throw new DisabledException("Account is disabled: " + user.getEmail());

The SanitizingOAuth2AuthenticationFailureHandler correctly prevents these messages from reaching the browser, and they only surface at DEBUG level in the sanitizing path. That is acceptable. However, if a non-sanitizing failure handler is ever used (e.g. a consumer's custom handler), the email would leak. Consider using a generic message ("Account is locked") to be safe by default, then log the email separately at DEBUG in the handler itself.

✅ All other security fixes are solid — RegistrationGuard bypass (package-private persist* methods), Facebook OAuth2 email_verified parity, OAuth2 failure handler sanitization, atomic failed-login increment, session fixation protection in authWithoutPassword, account status enforcement on OAuth2/OIDC/WebAuthn paths, and the SecurityFilterChain name-based conditional fix.

---

Architecture & Design

⚠️ UserService.self — self-injected proxy reference
The pattern is explained clearly in the Javadoc and it works correctly with Spring CGLIB proxying. A few risks to document for future maintainers:

1. If proxyTargetClass=false is ever configured, CGLIB proxying is disabled and self-calls would silently bypass the transaction.
2. registerPasswordlessAccount is still @Transactional(isolation = Isolation.SERIALIZABLE) directly on the public method (not the NOT_SUPPORTED + delegated-persist pattern used for registerNewUserAccount). Since passwordless registration doesn't do bcrypt, this is fine functionally, but it's inconsistent and may confuse future contributors.

⚠️ tokenFingerprint() is duplicated
The method is identical in both UserActionController and UserVerificationService. Extract it to a shared utility (e.g., TokenUtils) to avoid drift.

⚠️ MailExecutorConfiguration is @Configuration, not @AutoConfiguration
AuditMailAutoConfiguration and UserSecurityBeansAutoConfiguration correctly explain why @ConditionalOnMissingBean must live on @AutoConfiguration classes (evaluated after user-defined beans). MailExecutorConfiguration uses @Configuration (component-scanned) with @ConditionalOnMissingBean(name = "dsMailExecutor"). The name-based condition is more forgiving than type-based, so this likely works in practice. But it is inconsistent with the documented pattern and could be surprising. Consider moving it to an @AutoConfiguration class in a follow-up.

✅ RegistrationGuardConfiguration circular-dependency handling
The @ConditionalOnMissingBean(value = RegistrationGuard.class, ignored = CompositeRegistrationGuard.class) logic is subtle but correct: it guarantees the composite always has at least one delegate without triggering itself. The List<RegistrationGuard> injected into compositeRegistrationGuard() correctly excludes the composite (still under construction) from the delegate list.

✅ SecurityFilterChain name-based conditional
Switching from @ConditionalOnMissingBean(SecurityFilterChain.class) (suppresses on any consumer chain) to @ConditionalOnMissingBean(name = "securityFilterChain") is the correct fix and is well-documented in both code and MIGRATION.md.

---

Performance

✅ bcrypt outside the transaction
Propagation.NOT_SUPPORTED on registerNewUserAccount, changeUserPassword, and setInitialPassword is a meaningful fix for connection pool exhaustion under load. The pattern is well-documented.

✅ Dedicated mail executor
The bounded dsMailExecutor (core 2, max 4, queue 50, CallerRunsPolicy) is a sensible default and correctly backs off via @ConditionalOnMissingBean(name = "dsMailExecutor").

✅ Audit log ring-buffer query and log rotation
Both additions are well-implemented and significantly improve production operability.

---

Tests

The new test suite additions are comprehensive and cover the right things:

• WebSecurityConfigCompositionTest explicitly guards the name-based conditional regression.
• AccountLockoutIntegrationTest tests lockout through the real authentication path.
• AbstractConcurrentRegistrationTest with real Postgres/MariaDB testcontainers is excellent.
• MfaLoginIntegrationTest closes the gap on the H4 fix.
• ArchitectureTest with ArchUnit is a nice addition.

One gap: validateAndConsumePasswordResetToken has no concurrent-consumption test (two threads simultaneously submitting the same reset token). Given the TOCTOU concern above, a test similar to AbstractConcurrentRegistrationTest would be valuable.

---

Minor / Nits

• FileAuditLogWriter.setMaxFileSizeBytesForTesting() — annotating with @VisibleForTesting (Guava was removed, so use a comment or a custom annotation) would clarify intent for future readers.
• @ToString.Exclude on User.password is an important silent security fix; worth calling out in release notes for anyone who relied on User.toString() for debugging.
• The MIGRATION.md coverage of all breaking changes is thorough and well-written. The code examples for both Option A (coexisting chains) and Option B (full replacement) are exactly what consumers need.

---

Summary

Two items most worth addressing before merge:

1. TOCTOU in validateAndConsumePasswordResetToken — pessimistic lock or SERIALIZABLE isolation.
2. PII in assertAccountUsable exception messages — use generic message text; log email at DEBUG separately.

Everything else ranges from minor style suggestions to optional follow-ups. This is a high-quality, well-tested PR with exemplary documentation.

Reviewed by Claude Sonnet 4.6

[claude]

Code Review — PR #314: Security & Reliability Hardening for 4.4.0

Overall this is an excellent, well-scoped security hardening PR. The changes are coherent, the MIGRATION.md is present, and 854 tests + Playwright 103/103 is solid validation. Below is a detailed review organized by concern level.

---

🔴 Issues Worth Addressing

1. Account email leaked in exception messages (LoginHelperService)

assertAccountUsable() throws:

throw new LockedException("Account is locked: " + user.getEmail());
throw new DisabledException("Account is disabled: " + user.getEmail());

SanitizingOAuth2AuthenticationFailureHandler correctly intercepts this for the OAuth2 path, but these exceptions can propagate to other handlers (e.g., AuthenticationFailureHandler implementations, Spring's default form-login failure URL, or custom error pages). The email is PII and, more critically, it confirms account existence via a timing/error oracle.

Suggestion: Remove the email from the exception message. The DEBUG log already records it server-side. Use a generic message like "Account is locked".

---

2. currentFileBytes tracked in UTF-16 code units, not bytes (FileAuditLogWriter)

The byte counter is incremented with String.length(), which returns UTF-16 code units. Any Unicode characters in audit data (usernames, email addresses, error messages) will cause the counter to undercount actual bytes written, so files can silently exceed maxFileSizeMb before rotation triggers.

For a compliance deployment where exact log size matters, this is a correctness issue. Consider tracking actual written bytes (e.g., via writer.write(line); currentFileBytes += line.getBytes(StandardCharsets.UTF_8).length), or at least document this approximation in the property Javadoc/CLAUDE.md.

---

3. validateVerificationToken() behavioral contract change is easy to miss for consumers

The method now atomically consumes the token inside a transaction. Any consumer (not just UserActionController) who previously called getUserByVerificationToken() after validateVerificationToken() will now get null. This is documented in MIGRATION.md, but it is a silent behavioral break — the code still compiles and fails at runtime with a NullPointerException.

Suggestion: Consider renaming the method to validateAndConsumeVerificationToken() to make the destructive semantics obvious, similar to the naming convention used for validateAndConsumePasswordResetToken() in UserService.

---

🟡 Things to Consider

4. Two InteractiveAuthenticationSuccessEvents per passkey login

WebAuthnAuthenticationSuccessHandler now explicitly emits a second InteractiveAuthenticationSuccessEvent. This is documented. However, a consumer listener that checks principal instanceof DSUserDetails on the first event and WebAuthnAuthenticationToken on the second will observe two events per login. If that listener does something non-idempotent (audit write, session counter increment), it will double-fire.

Suggestion: Add a note in MIGRATION.md specifically calling this out with an example of the safe guard (if (!(authentication instanceof WebAuthnAuthenticationToken))), since consumers who have BaseAuthenticationListener subclasses are most affected.

---

5. registerPasswordlessAccount() guard runs inside the serializable transaction

The @Transactional(isolation = Isolation.SERIALIZABLE) method now calls enforceRegistrationGuard() before the email-exists check. If the guard throws anything other than RegistrationDeniedException, it propagates inside a SERIALIZABLE transaction that acquired a lock on the user table range. A poorly-implemented or throwing guard could hold this lock until the exception unwinds, potentially stalling concurrent registrations.

Suggestion: Evaluate the guard before entering the SERIALIZABLE transaction, or document that guard implementations must not throw checked or unexpected unchecked exceptions.

---

6. CompositeRegistrationGuard injection — self-inclusion edge case

RegistrationGuardConfiguration.compositeRegistrationGuard(List<RegistrationGuard> guards) relies on Spring resolving the List<RegistrationGuard> before the CompositeRegistrationGuard bean is created (so it cannot include itself). This is correct for the standard @Bean method lifecycle, but if a consumer manually registers a CompositeRegistrationGuard bean in their application context, Spring could include both the library's composite and the consumer's, creating a chain that re-evaluates the library's delegates.

Suggestion: A @Primary @ConditionalOnMissingBean(CompositeRegistrationGuard.class) would make the library's composite only register if no other composite exists, which is already the intent. The current exclusion pattern achieves this but could benefit from a code comment explaining why.

---

7. Propagation.NOT_SUPPORTED + outer @Transactional callers

registerNewUserAccount(), changeUserPassword(), and setInitialPassword() are now Propagation.NOT_SUPPORTED. This means if a consumer wraps any of these in their own @Transactional method, those calls will suspend the outer transaction, commit independently, and an outer rollback will not undo them. This is the correct behavior for avoiding long transactions during bcrypt, but it's a significant semantic change.

The MIGRATION.md covers this. Consider also adding a Javadoc warning on each method noting the non-transactional outer behavior:

/**
 * ...
 * <p>Runs with {@link Propagation#NOT_SUPPORTED}; bcrypt hashing occurs outside any transaction.
 * The actual DB write is committed in a separate inner transaction and is NOT rolled back if an
 * outer transaction rolls back.
 */

---

8. CI workflow — codeql job missing needs: build

The codeql job runs independently and in parallel with build. This means CodeQL may analyze code that has failing tests. Not a blocking issue, but usually CodeQL analysis should gate on a successful build:

codeql:
  needs: build
  ...

---

9. CI workflow — actions/setup-java@v4 version

github/codeql-action/init@v3, analyze@v3, upload-artifact@v4 are all pinned to major version tags. For a security-critical library it is worth pinning to commit SHAs (e.g. actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683) to prevent tag-mutable action injection. This is a best-practice recommendation, not a blocker.

---

🟢 Strengths

• Token hashing design is solid. SHA-256 storage + dual-read backward-compatibility fallback is the right approach. HMAC-SHA-256 opt-in via tokenHashSecret is a sensible upgrade path.
• Single-active-token invariant (delete existing before issuing new) closes a real token accumulation/replay risk.
• @Transactional atomic token consumption for both verification and password-reset tokens eliminates the double-use race window.
• incrementFailedAttempts() as a bulk UPDATE is the correct fix for the lost-update race under concurrent brute-force — read-modify-write at the application layer is a classic lockout-evasion vector.
• SecurityFilterChain conditional keyed on bean name rather than type is exactly the right fix for the regression. The comment explaining this is clear.
• SanitizingOAuth2AuthenticationFailureHandler is a clean, testable extraction — much better than an inline lambda that was hard to test and logged sensitive data.
• User.password @ToString.Exclude and log sanitization throughout (authentication.getName() vs .toString()) are small but important security hygiene items.
• SessionScopedProfile meta-annotation is a great DX improvement that prevents the subtle singleton-leaks-profile bug.
• UserDisabledEvent after-commit publication is the right fix for the listener-observes-uncommitted-data issue.
• Bounded ArrayDeque in FileAuditLogQueryService replaces a full-file sort that could OOM on large log files.
• MfaFilterMergingConfiguration isolation (separate from MfaConfiguration to avoid BeanPostProcessor early-init) shows careful understanding of Spring's initialization order.

---

📋 Minor / Style

• TokenHasher.hash() null input: consider a null check with a clear message rather than letting MessageDigest.update() throw a NullPointerException with an unhelpful stack trace.
• addIfHasText() helper in WebSecurityConfig: well-named utility. Worth a one-line Javadoc.
• UserEmailService constructor change in MIGRATION.md: the guide mentions the TokenHasher parameter addition — it would help to include a code snippet showing the new constructor signature so consumers extending the class know exactly what to update.
• Test cleanup in UserApiTest via TransactionTemplate: correctly avoids @Transactional on the test itself (which would conflict with NOT_SUPPORTED). Good pattern to call out in TESTING.md for future test authors writing against service-layer methods that use NOT_SUPPORTED.

---

Summary: This PR is in excellent shape. The two primary items worth fixing before merge are the email-in-exception-message issue (#1) and the validateVerificationToken naming ambiguity (#3). Items #4–#9 are improvements worth considering but not blocking. Great work on the test coverage and demo app validation.

[devondragon]

Review feedback addressed

Thanks for the thorough review. Pushed fixes for the actionable items:

#	Item	Fix
1	PII in LockedException/DisabledException	Messages are now generic ("Account is locked" / "Account is disabled"); email retained only in DEBUG logs. Removes the dependency on downstream handlers sanitizing the message.
2	Null guard return in CompositeRegistrationGuard.evaluate	Now throws IllegalStateException on a null delegate decision instead of failing open. Added two unit tests covering the fail-fast behavior.
3	FileAuditLogFlushScheduler SpEL default mismatch	Changed !${user.audit.flushOnWrite:true} → :false to match AuditConfig's Java field default, so the scheduler is correctly created when the property is absent.
4	tokenFingerprint duplication	Extracted to public static TokenHasher.fingerprint(...); removed the identical private copies in UserVerificationService and UserActionController.
5	cleanUpPasswordHistory comment	Rewritten to reflect the real call chain (changeUserPassword → self.persistChangedPassword (proxied) → savePasswordHistory → cleanUpPasswordHistory); removed the misleading "bypassed by proxy" claim.

Plus the must-fix Copilot caught: the @Transactional on createPasswordResetTokenForUser was a no-op via self-invocation — now routed through a @Lazy self proxy so the delete+save commit atomically.

Deferred / declined:

• Guard-before-duplicate-check bcrypt amplification — valid, but moving the fast-path duplicate check ahead of the guard is a design change out of scope for this PR. Tracking separately; expected rate limiting at the controller/gateway mitigates in the interim.
• email_verified unknown-type debug log and UserService subclass double-guard edge case — declined as low-signal / theoretical API-misuse; behavior is intentional and documented inline.

All affected test suites pass.

🤖 Generated with Claude Code

[claude]

Code Review — Security & Reliability Hardening for 4.4.0

Reviewed by Claude Code. 117 files, 8254 additions analyzed.

This is a well-scoped, well-structured PR. The overall direction is correct and the security motivations are sound. The MFA factor-merging fix, the token-hashing approach, the @ConditionalOnMissingBean name-keying, and the OAuth2 failure-handler fix are all good decisions. The notes below are ranked by severity so the highest-impact items are easy to find.

---

🔴 P0 — Critical

No test for the plaintext-to-hashed token backward-compatibility path

The dual-read strategy in UserVerificationService.resolveByRawToken() (hash-lookup first, then plaintext fallback) is the correct approach for zero-downtime upgrade, but the plaintext fallback path has zero test coverage. This is the single most critical behavioral guarantee in the PR — a regression here causes every user with a pending verification or password-reset token at deploy time to get an invalid-token error. A @DatabaseTest that stores a raw (unhashed) token row and verifies it's still resolved post-upgrade is required before merge.

---

🟠 P1 — High Priority

evaluateRegistrationGuard logs consumer-supplied reason at INFO — PII risk

log.info("Registration denied for source: {} provider: {} reason: {}", source, providerName, decision.reason());

The reason string is consumer-controlled. A guard implementation could easily include the user's email (e.g., "Email domain not allowed for user@company.com"), making this a PII leak to INFO-level logs. Recommend downgrading to log.debug(...) or logging only decision.code() at INFO and leaving the full reason to DEBUG.

removeAll(Collections.emptyList()) is a no-op — potential NPE in security config

In WebSecurityConfig.getUnprotectedURIsList():

unprotectedURIs.removeAll(Collections.emptyList());

This does nothing. removeAll with an empty collection removes no elements. If the intent is to purge null entries that can sneak in from @Value-injected YAML lists, the correct call is unprotectedURIs.removeIf(Objects::isNull). A null entry in that list will cause a NullPointerException inside Spring Security's AntPathRequestMatcher at runtime. This pre-exists the PR but the method is being touched — please fix it here.

No integration test for session-ID regeneration after password change

SessionInvalidationIntegrationTest verifies that the SessionRegistry is populated after login, but does NOT verify the core guarantee of the new session-regeneration feature: that after changeUserPassword, the HTTP session ID changes, the old ID is invalidated in the SessionRegistry, and the new ID is registered. This is the most important new behavioral contract and should have an end-to-end integration test.

---

🟡 P2 — Medium Priority

VerificationToken.updateToken(String) single-arg overload silently ignores configured expiry

The new configurable-expiry overload (updateToken(String token, int expiryTimeInMinutes)) is used in internal paths, but the old public single-arg updateToken(String token) still uses the hardcoded 24-hour EXPIRATION constant. Any consumer or existing code path calling the single-arg form will silently ignore verificationTokenValidityMinutes. Deprecate the single-arg overload and delegate it to the new one using the configured value.

Missing items from MIGRATION.md

The following breaking changes are not documented:

• VerificationToken constructor gained a new parameter — consumers constructing it directly get a compile error
• User now implements Serializable with serialVersionUID = 1L — consumers with distributed sessions or caches containing serialized User objects should flush their session stores before deploying; any field addition/removal triggers InvalidClassException on deserialization
• UserAPI no longer injects RegistrationGuard — consumers who subclassed UserAPI and used the guard field will fail to compile
• WebSecurityConfig.buildSecurityFilterChain(HttpSecurity) signature changed to accept SessionRegistry as second parameter

MFA BeanPostProcessor blast radius underdocumented

MfaFilterMergingConfiguration calls setMfaEnabled(true) on ALL AbstractAuthenticationProcessingFilter instances. If a consumer's custom filter extending that class produces Authentication objects that don't support the factor-merging contract, the failure occurs at runtime (not startup) and only when MFA is actually invoked. Consider adding a startup WARN log listing which filters had mfaEnabled set, so operators can verify the list looks correct.

SecurityFilterChain conditional migration documentation could be clearer

The MIGRATION.md documents the name-keying change, but the risk for consumers with a differently-named replacement chain is implicit. Add a sentence like: "If your custom SecurityFilterChain bean was NOT named securityFilterChain, you now have two active chains (yours + the library's). Rename your bean to securityFilterChain to suppress the library chain as before."

---

🔵 P3 — Low Priority / Polish

OnRegistrationCompleteEvent published with null appUrl for OAuth2/OIDC registrations

The early-return for already-enabled users in RegistrationListener prevents sending verification emails for OAuth2 users (good), but consumer-written listeners implementing ApplicationListener<OnRegistrationCompleteEvent> that reference event.getAppUrl() may NPE. Document in the event Javadoc that appUrl may be null for OAuth2/OIDC registration paths.

SanitizingOAuth2AuthenticationFailureHandler name is misleading

The handler's method signature accepts AuthenticationException (not just OAuth2AuthenticationException), and the test verifies it handles LockedException. The name implies OAuth2-only scope. Consider SanitizingAuthFailureHandler or add a Javadoc note clarifying the broader scope.

@Lazy @Autowired self pattern is underdocumented in class bodies

Both UserService and UserEmailService mix @RequiredArgsConstructor (constructor injection) with a @Lazy @Autowired self field injection. This is a valid Spring workaround for transactional self-invocation, but contributors unfamiliar with it may remove it or replace self.method() with this.method() (bypassing the proxy). A single-line comment on the field explaining the pattern would prevent future breakage.

Double-qualification on VerificationToken.plainToken

@Transient private transient String plainToken;

Both @Transient (JPA) and transient (Java serialization) are present. They serve different purposes (JPA vs Java serialization exclusion), so having both is technically correct, but a comment would prevent future contributors from removing one thinking it's redundant.

---

✅ What's Done Well

• Token hashing design: HMAC-SHA-256 with secret, SHA-256 fallback, 64-char hex that fits the existing column — no schema migration needed. Clean.
• OAuth2 failure handler: SanitizingOAuth2AuthenticationFailureHandlerTest covers all three PII-leak scenarios. One of the better-tested changes in the PR.
• OAuth2 email verification: isExplicitlyUnverified() correctly handles Boolean.FALSE, String "false", and absent claim for both OAuth2 and OIDC paths.
• WebSecurityConfigCompositionTest: Clear regression guards for both the coexistence case and the full-replacement case using ApplicationContextRunner. This is exactly the right test to have for a @ConditionalOnMissingBean change.
• Bcrypt outside the transaction: Propagation.NOT_SUPPORTED on registerNewUserAccount / changeUserPassword correctly avoids holding a DB connection during the 100ms+ hash operation.
• afterCommit() for UserDisabledEvent / UserDeletedEvent: Events that trigger side effects (session invalidation, cleanup) correctly fire after the transaction commits, not during.
• MfaLoginIntegrationTest: Separates the merging-flag assertion (via reflection) from the enforcement assertion (via pre-merged token) cleanly. The Javadoc explaining the test approach is exemplary.
• CI workflow: Two-job design (build/test + CodeQL with security-extended) is appropriate for a library. Running CodeQL only on compileJava (not tests) is standard practice.

---

Summary Table

Priority	Issue
🔴 P0	No test for plaintext-token backward-compatibility path
🟠 P1	Guard denial reason logged at INFO — consumer PII leak risk
🟠 P1	removeAll(Collections.emptyList()) no-op — potential NPE from null URI entries
🟠 P1	No integration test for session-ID regeneration after password change
🟡 P2	updateToken(String) single-arg overload ignores configured expiry
🟡 P2	Missing from MIGRATION.md: VerificationToken constructor, User.Serializable, UserAPI field, buildSecurityFilterChain signature
🟡 P2	MFA BeanPostProcessor blast-radius underdocumented (runtime failure risk for consumer filters)
🟡 P2	SecurityFilterChain migration docs need clearer statement for non-standard bean names
🔵 P3	appUrl can be null for OAuth2 registrations — consumer listener NPE risk (needs Javadoc)
🔵 P3	SanitizingOAuth2AuthenticationFailureHandler name scope misleading
🔵 P3	@Lazy @Autowired self pattern underdocumented — future contributors may break proxy chain

The P0 item (missing backward-compat token test) and the P1 PII log issue are the ones I'd want resolved before merge. Everything else is actionable but would be reasonable to file as follow-up issues if needed to avoid blocking the release.

[claude]

Code Review — PR #314: Security & reliability hardening for 4.4.0

This is a large, well-motivated security hardening release. The changes are architecturally sound and the documentation (JavaDoc, migration guide) is thorough. Below are my findings organized by severity.

---

🔴 Issues to fix before merge

1. SanitizingOAuth2AuthenticationFailureHandler creates a new session on failure

request.getSession().setAttribute(ERROR_MESSAGE_SESSION_ATTRIBUTE, resolveUserFacingMessage(exception));

HttpServletRequest.getSession() (no argument) creates a new session if one does not exist. An unauthenticated bot or scanner hitting any OAuth2 callback path repeatedly will cause the server to allocate a session object for every request. Use request.getSession(true) explicitly (to document intent) or more defensively request.getSession(false) with a null check before setAttribute. The error redirect still works without a session — the login page can simply show a default message if the attribute is absent.

2. generateNewVerificationToken silently NPEs if the token is not found

public VerificationToken generateNewVerificationToken(final String existingVerificationToken) {
    VerificationToken vToken = resolveByRawToken(existingVerificationToken);
    vToken.updateToken(tokenHasher.hash(rawToken), verificationTokenValidityMinutes); // NPE if vToken == null

resolveByRawToken returns null when the token is not found, and there is no null-check before calling vToken.updateToken(...). The old code had the same bug, but since this method is now @Transactional and the contract has changed, this is a good opportunity to add an explicit guard (e.g., throw IllegalArgumentException with a descriptive message).

---

🟡 Worth addressing

3. CI actions are not pinned to commit hashes

- uses: actions/checkout@v4
- uses: gradle/actions/setup-gradle@v4
- uses: github/codeql-action/init@v3

For a security library, best practice is to pin to a specific commit SHA (e.g., actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683). A compromised tag at the action provider can silently run arbitrary code in the build. GitHub's Security Hardening guide recommends SHA pins for all third-party actions.

4. MFA filter-merging warning scope is underspecified in practice

The class-level warning in MfaFilterMergingConfiguration correctly documents that setMfaEnabled(true) is applied to every AbstractAuthenticationProcessingFilter. However, the warning only appears in JavaDoc — consider surfacing it as a log.warn(...) at startup when MFA is enabled, so operators who do not read library JavaDoc are still alerted. Something like:

log.warn("MFA factor merging is enabled. All AbstractAuthenticationProcessingFilter beans, including any custom ones, will have setMfaEnabled(true) applied. See MfaFilterMergingConfiguration for implications.");

5. Indentation inconsistency in MfaFilterMergingConfiguration

The new class uses tabs for indentation (visible in the @Bean method body), while the rest of the codebase uses 4 spaces (per CLAUDE.md). Minor but worth fixing for consistency.

6. Propagation.NOT_SUPPORTED behavior change deserves more prominent callout

registerNewUserAccount and changeUserPassword are now annotated @Transactional(propagation = Propagation.NOT_SUPPORTED), meaning they suspend any caller-supplied transaction. The @implNote in the JavaDoc documents this, but consuming apps that wrap these calls in their own @Transactional expecting the user creation/password change to roll back with the outer transaction will silently lose that guarantee. Consider also adding this note to MIGRATION.md alongside the other breaking changes.

---

🟢 Strengths worth calling out

Token hashing design — The TokenHasher class is well-designed. The plain SHA-256 vs HMAC-SHA-256 choice is clearly documented with the security rationale. The dual-read backward-compat fallback is permanently safe (bounded by token lifetime) and requires no operator action. The fingerprint() static helper for log-safe logging is a nice touch.

Atomic token consumption — validateVerificationToken now enables the user and deletes the token in a single @Transactional call. The behavior of expired token cleanup (delete-on-reject) is also correct. The test updates properly verify the delete() call via Mockito.

SecurityFilterChain name-based conditional — The shift from @ConditionalOnMissingBean(SecurityFilterChain.class) to @ConditionalOnMissingBean(name = "securityFilterChain") is the correct fix for multi-chain coexistence. The JavaDoc explaining why (type-based suppresses on any consumer chain; name-based preserves multi-chain @Order layering) is excellent and will save future maintainers time.

Connection pool optimization — Moving bcrypt hashing outside the transaction via Propagation.NOT_SUPPORTED + proxied self.persist*() is the right solution. The self-injection pattern (@Lazy @Autowired) is explained clearly and is the standard approach when field injection is the only option.

cleanUpPasswordHistory query improvement — The shift from load-all-then-deleteAll to a set-based cutoff DELETE (by ID ordering) eliminates the read-then-delete race and is more efficient. Good improvement.

publishEventAfterCommit — Deferring UserDeletedEvent and UserDisabledEvent until after commit prevents @Async listeners from acting on uncommitted data. The fallback to immediate publish when no transaction is active is correct.

isExplicitlyUnverified — The trust-on-null policy (absent email_verified → trusted, only explicit false → rejected) correctly handles providers that do not expose the claim, avoiding false lockouts.

---

ℹ️ Observations / nits

• The TokenHasher.fingerprint() exposes the first 6 chars of the raw token. For a UUID this is ~24 bits, low risk for log correlation purposes, and the approach is clearly documented. No action needed, just flagging it is a considered decision.
• deletePasswordResetToken does a dual-delete (hash first, then raw fallback). This is consistent with the dual-read strategy and will naturally converge as pre-upgrade tokens expire.
• The UserService.self field uses @Autowired (field injection) which diverges from the project's @RequiredArgsConstructor + final field convention. This is unavoidable given the circular proxy dependency and is well-documented — just noting it.

---

Summary: The core security fixes (single-use tokens, atomic consume, Facebook email verification, sanitized failure handler, filter chain coexistence) are correct and well-executed. Fix the session-creation issue in the failure handler (#1) and the NPE in generateNewVerificationToken (#2) before merging, and consider the CI action pinning (#3) given this is a security library.

[devondragon]

Second review pass addressed (commit 5a7578b)

Thanks @claude — caught real issues on the re-review. Triaged and fixed the valid ones:

🔴 Fixed before merge

Item	Fix
#1 Session created on every OAuth2 failure	SanitizingOAuth2AuthenticationFailureHandler now uses request.getSession(false) + null-guard, so a sessionless caller (unauthenticated scanner hitting the callback) no longer forces session allocation. A genuine OAuth2 flow already has a session from the authorization-request phase, so the user-facing message is preserved. Tests updated to model the real path; added a test asserting no session is created for a cold request.
#2 NPE in generateNewVerificationToken	Added a null guard on the resolved token; now throws IllegalArgumentException with a clear message instead of NPEing on updateToken(...).
CodeQL log-injection ×3 (advanced-security bot)	TokenHasher.fingerprint() now strips control chars (CR/LF) from the logged prefix, closing the log-forging vector centrally for all callers.

🟡 Quick wins

• Redundant configmethod in WebSecurityConfig #5 Converted tab indentation → 4 spaces in MfaFilterMergingConfiguration (CLAUDE.md style).

Already covered / declining

• Issue 5 redundant configmethod in web security config #6 (NOT_SUPPORTED migration note) — already documented in MIGRATION.md § Password hashing moved outside the transaction, including the bold "an outer rollback will not roll back the registration or password change" caveat. No change needed.
• https://github.com/devondragon/SpringUserFramework/issues/2 #3 SHA-pin CI actions — valid hardening, but deferring to a follow-up: the actions were just bumped this PR, and pinning every third-party action to a SHA (with a maintenance story) is its own task.
• Docker compose #4 startup log.warn for MFA merging — low value vs. the existing class-level JavaDoc + per-filter DEBUG log; skipping.

Full test suite green locally.

🤖 Generated with Claude Code

[claude]

Claude finished @devondragon's task in 4m 26s —— View job

---

Third-Pass Review — PR #314 Security & Reliability Hardening

Tasks

• Load review tools
• Read changed files from commit 5a7578b — verify the four fixes
• Check new CodeQL alerts (log injection ×3 in UserActionController + UserVerificationService)
• Check previously flagged P0/P1 items (backward-compat token test, session regeneration test)
• Post final review

---

Fix Verification

All four stated fixes from commit 5a7578b are confirmed correct:

✅ Fix #1 — Session not allocated for cold OAuth2 callbacks

SanitizingOAuth2AuthenticationFailureHandler.java:79-82:

HttpSession session = request.getSession(false);
if (session != null) {
    session.setAttribute(ERROR_MESSAGE_SESSION_ATTRIBUTE, resolveUserFacingMessage(exception));
}

getSession(false) + null-guard is correctly implemented. A genuine OAuth2 flow has a session from the authorization-request phase; an unauthenticated scanner hitting the callback cold does not, and no session is created. ✅

✅ Fix #2 — NPE guard in generateNewVerificationToken

UserVerificationService.java:118-124 now has an explicit null check with a clear IllegalArgumentException message before calling vToken.updateToken(...). ✅

✅ Fix #3 — TokenHasher.fingerprint() strips control chars

TokenHasher.java:119:

final String prefix = token.substring(0, 6).replaceAll("\\p{Cntrl}", "");

\p{Cntrl} covers the full Unicode control character category (including CR/LF). This resolves the second-scan CodeQL alerts 16, 17, and 18 in UserActionController.java:82, UserActionController.java:115, and UserVerificationService.java:77 — all three of which already used TokenHasher.fingerprint(token) and now get sanitized output. ✅

✅ Fix #4 — MfaFilterMergingConfiguration indentation

File consistently uses 4-space indentation throughout. ✅

---

Previously Flagged Test Coverage Concerns — Resolved

Two earlier reviews flagged these as P0/P1 gaps. Both tests exist:

✅ Plaintext backward-compat token test — TokenHashingSecurityTest.java:155-169 covers the DUAL-READ path: hash lookup misses (simulating a pre-upgrade plaintext token), raw lookup succeeds, and the token is resolved correctly. Both validatePasswordResetToken and getUserByPasswordResetToken are exercised on the pre-upgrade path.

✅ Session regeneration after password change — SessionInvalidationServiceTest.java:298-323 verifies: (a) the current session is preserved but receives a new ID (fixation protection), (b) only OTHER sessions are expired, (c) the session registry is updated with the new ID. The no-current-request path is also tested.

---

Remaining Open Items

These are all carry-overs from prior reviews — noting their status for completeness.

removeAll(Collections.emptyList()) at WebSecurityConfig.java:329 — Still dead code. The addIfHasText helper already prevents null entries from entering the list, so this is harmless in practice, but it should either be removed or replaced with the originally-intended removeIf(Objects::isNull). Low-risk but confusing to future readers. Fix this →

First-scan CodeQL alerts IDs 3 & 6 in UserAPI.java:124,429 — These log ex.getReason() (a consumer-controlled string from RegistrationDeniedException) at INFO level. The fingerprint() fix addressed token values, not reason strings. A consumer guard implementation that returns a reason containing CR/LF would create a log injection vector at this call site. Additionally, evaluateRegistrationGuard in UserService.java:924 already logs the same reason at INFO before throwing the exception, making the catch-block log in UserAPI redundant. Both of these are fixable independently:

• Remove the duplicate UserAPI log lines (they add the email but the reason is already logged), or
• Sanitize ex.getReason() before logging: ex.getReason().replaceAll("\\p{Cntrl}", "").

The first-scan CodeQL alerts will continue showing in the Code Scanning UI until these lines are addressed or the alerts are dismissed.

First-scan CodeQL alerts IDs 8-15 in UserService.java — Email addresses in DEBUG-level logs. CodeQL flags any user-provided value reaching a log statement regardless of level or operational context. These are acceptable for server-side operational logging (operators who enable DEBUG accept PII in logs), but they won't disappear from the Code Scanning UI without sanitization or alert dismissal.

request.getSession() at UserActionController.java:85 — The showChangePasswordPage handler (password reset via email link) calls request.getSession().getId() for audit logging before the token validation result is checked. A scanner sending requests with arbitrary ?token=x values will create a session per request. This is lower-exposure than the OAuth2 callback (the endpoint is not the OAuth redirect URI) but the same getSession(false) + null-guard pattern would close it cleanly.

---

Summary

The four stated fixes in commit 5a7578b are all correct and complete. The previously flagged P0/P1 test coverage concerns (backward-compat token path, session regeneration) are confirmed present. The PR is in good shape for merge. The remaining open items are minor cleanup and CodeQL alert resolution that can happen post-merge if preferred; none are blocking security issues.