From 34bd36c8b1df8ff206664a0d64428588ed842b70 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 08:39:06 -0600 Subject: [PATCH 1/8] fix(security): revoke all user sessions on account delete/disable (SUF-03) The ordinary self-service account deletion/disable flow only logged out the caller's current request. Other active sessions kept carrying the cached DSUserDetails (authorities and the enabled flag are captured at login and not re-checked per request), so a concurrent session stayed authorized until natural expiry. Both the default soft-delete and hard-delete were affected. deleteOrDisableUser() now calls SessionInvalidationService.invalidateUserSessions(user) at the service layer, so every caller is covered (not just UserAPI.deleteAccount), mirroring the existing GDPR deletion path (GdprAPI.logoutUser). Adds two service-level tests proving all sessions are revoked for both deletion modes. Refs #329 --- .../spring/user/service/UserService.java | 6 ++++ .../spring/user/service/UserServiceTest.java | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java index a05451c4..2c65fe60 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java @@ -500,6 +500,12 @@ private void cleanUpPasswordHistory(User user) { @Transactional public void deleteOrDisableUser(final User user) { log.debug("UserService.deleteOrDisableUser: called for user: {}", user != null ? user.getEmail() : null); + // Revoke every active session for this user before the account is removed or disabled. Otherwise a + // concurrent session keeps carrying the cached DSUserDetails (authorities and the enabled flag are + // captured at login and not re-checked per request), so it stays authorized until natural expiry — the + // API-level logout only terminates the caller's current request. Done at the service layer so all callers + // of deleteOrDisableUser() are covered, mirroring the GDPR deletion path (GdprAPI.logoutUser). + sessionInvalidationService.invalidateUserSessions(user); if (actuallyDeleteAccount) { log.debug("UserService.deleteOrDisableUser: actuallyDeleteAccount is true, deleting user: {}", user.getEmail()); // Capture user details before deletion for the post-delete event diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java index a8f93770..84ec8909 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java @@ -437,6 +437,34 @@ void deleteOrDisableUser_publishesUserPreDeleteEvent() { assertThat(publishedEvent.getUserId()).isEqualTo(testUser.getId()); assertThat(publishedEvent.getUserEmail()).isEqualTo(testUser.getEmail()); } + + @Test + @DisplayName("deleteOrDisableUser - hard delete revokes all of the user's sessions") + void deleteOrDisableUser_whenActuallyDeleteTrue_invalidatesAllUserSessions() { + // Given + ReflectionTestUtils.setField(userService, "actuallyDeleteAccount", true); + + // When + userService.deleteOrDisableUser(testUser); + + // Then: every session for the user must be revoked, not just the caller's current request, + // otherwise a concurrent session keeps carrying the now-deleted principal until it expires. + verify(sessionInvalidationService).invalidateUserSessions(testUser); + } + + @Test + @DisplayName("deleteOrDisableUser - soft disable revokes all of the user's sessions") + void deleteOrDisableUser_whenActuallyDeleteFalse_invalidatesAllUserSessions() { + // Given + ReflectionTestUtils.setField(userService, "actuallyDeleteAccount", false); + when(userRepository.save(any(User.class))).thenReturn(testUser); + + // When + userService.deleteOrDisableUser(testUser); + + // Then: a disabled account's other sessions must not remain authenticated on cached authorities. + verify(sessionInvalidationService).invalidateUserSessions(testUser); + } } @Test From 5dbedc64fac3a9d5ebdecfd06a2cc7b6c5b967ba Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 08:45:12 -0600 Subject: [PATCH 2/8] fix(security): enforce account lockout on authenticated password change (SUF-04) /user/updatePassword verified the current password with a bare passwordEncoder.matches() and was not wired into the brute-force lockout used elsewhere: it did not check lock state, did not record failed guesses, and did not reset the counter on success. A session-holding actor could therefore make unlimited current-password guesses without ever tripping the configured lockout. updatePassword now mirrors WebAuthnManagementAPI.requireCurrentPasswordIfSet: - reject a locked account up front with HTTP 423 (LOCKED) before hashing - record each wrong old password via LoginAttemptService.loginFailed() - clear the counter via loginAttemptService.loginSucceeded() on success Adds the message.update-password.account-locked bundle key and three unit tests (failed-attempt recorded, counter reset on success, locked account rejected with 423 without touching the password). Refs #330 --- .../spring/user/api/UserAPI.java | 15 +++ .../messages/dsspringusermessages.properties | 1 + .../spring/user/api/UserAPIUnitTest.java | 100 ++++++++++++++++++ 3 files changed, 116 insertions(+) diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java index 3ac8bf81..2992bc44 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -34,6 +34,7 @@ import com.digitalsanctuary.spring.user.registration.RegistrationDeniedException; import com.digitalsanctuary.spring.user.registration.RegistrationGuard; import com.digitalsanctuary.spring.user.service.DSUserDetails; +import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.PasswordPolicyService; import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; @@ -90,6 +91,7 @@ public class UserAPI { private final PasswordPolicyService passwordPolicyService; private final ObjectProvider webAuthnCredentialManagementServiceProvider; private final AppUrlResolver appUrlResolver; + private final LoginAttemptService loginAttemptService; @Value("${user.security.registrationPendingURI}") private String registrationPendingURI; @@ -344,11 +346,24 @@ public ResponseEntity updatePassword(@AuthenticationPrincipal DSUs return buildErrorResponse(messages.getMessage("message.user.not-found", null, "User not found", locale), 1, HttpStatus.BAD_REQUEST); } + // Verifying the current password is an authentication surface, so it participates in the same brute-force + // lockout as login: reject a locked account up front (HTTP 423) so a session-holding actor cannot make + // unlimited old-password guesses here. Mirrors WebAuthnManagementAPI.requireCurrentPasswordIfSet. + if (loginAttemptService.isLocked(user.getEmail())) { + logAuditEvent("PasswordUpdate", "Failure", "Account locked", user, request); + return buildErrorResponse(messages.getMessage("message.update-password.account-locked", null, + "Account is locked due to too many failed attempts. Please try again later.", locale), 3, HttpStatus.LOCKED); + } + try { // Verify old password is correct if (!userService.checkIfValidOldPassword(user, passwordDto.getOldPassword())) { + // A wrong guess counts toward lockout, locking the account once the configured threshold is reached. + loginAttemptService.loginFailed(user.getEmail()); throw new InvalidOldPasswordException("Invalid old password"); } + // Successful reauthentication clears the failed-attempt counter, matching login semantics. + loginAttemptService.loginSucceeded(user.getEmail()); // Validate new password against policy List errors = passwordPolicyService.validate(user, passwordDto.getNewPassword(), user.getEmail(), diff --git a/src/main/resources/messages/dsspringusermessages.properties b/src/main/resources/messages/dsspringusermessages.properties index d0d6d1b1..5bcc7291 100644 --- a/src/main/resources/messages/dsspringusermessages.properties +++ b/src/main/resources/messages/dsspringusermessages.properties @@ -14,6 +14,7 @@ email.signature=Best regards,
The DigitalSanctuary Team message.update-user.success=Your profile has been successfully updated. message.update-password.success=Your password has been successfully updated. message.update-password.invalid-old=The old password is incorrect. +message.update-password.account-locked=Your account is locked due to too many failed attempts. Please try again later. message.password.mismatch=Passwords do not match. message.reset-password.success=Your password has been successfully reset. You can now log in with your new password. diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 420a6411..0c2c914d 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -27,6 +27,7 @@ import com.digitalsanctuary.spring.user.exceptions.UserAlreadyExistException; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.service.DSUserDetails; +import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.PasswordPolicyService; import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; @@ -97,6 +98,9 @@ public JSONResponse handleSecurityException(SecurityException e) { @Mock private AppUrlResolver appUrlResolver; + @Mock + private LoginAttemptService loginAttemptService; + @InjectMocks private UserAPI userAPI; @@ -484,6 +488,102 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter .andExpect(jsonPath("$.messages[0]").value("Invalid old password")); } + /** + * Builds a standalone MockMvc that resolves the {@code @AuthenticationPrincipal} argument to + * {@link #testUserDetails}, matching the setup the other updatePassword tests use inline. + */ + private MockMvc updatePasswordMockMvc() { + return MockMvcBuilders.standaloneSetup(userAPI) + .setCustomArgumentResolvers(new HandlerMethodArgumentResolver() { + @Override + public boolean supportsParameter(org.springframework.core.MethodParameter parameter) { + return parameter.getParameterType().equals(DSUserDetails.class); + } + + @Override + public Object resolveArgument(org.springframework.core.MethodParameter parameter, + org.springframework.web.method.support.ModelAndViewContainer mavContainer, + org.springframework.web.context.request.NativeWebRequest webRequest, + org.springframework.web.bind.support.WebDataBinderFactory binderFactory) { + return testUserDetails; + } + }) + .setControllerAdvice(new TestExceptionHandler()) + .build(); + } + + @Test + @DisplayName("POST /user/updatePassword - wrong old password is recorded for lockout") + void updatePassword_wrongOldPassword_recordsFailedAttempt() throws Exception { + PasswordDto passwordDto = new PasswordDto(); + passwordDto.setOldPassword("wrongPassword"); + passwordDto.setNewPassword("newPassword123"); + + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(messageSource.getMessage(eq("message.update-password.invalid-old"), any(), any(), any(Locale.class))) + .thenReturn("Invalid old password"); + when(userService.checkIfValidOldPassword(any(User.class), eq("wrongPassword"))).thenReturn(false); + + mockMvc.perform(post("/user/updatePassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(passwordDto)) + .with(csrf())) + .andExpect(status().isBadRequest()); + + // A wrong current-password guess must count toward brute-force lockout, like the login path. + verify(loginAttemptService).loginFailed(testUser.getEmail()); + verify(userService, never()).changeUserPassword(any(), any()); + } + + @Test + @DisplayName("POST /user/updatePassword - successful change resets the lockout counter") + void updatePassword_success_resetsLockoutCounter() throws Exception { + PasswordDto passwordDto = new PasswordDto(); + passwordDto.setOldPassword("oldPassword"); + passwordDto.setNewPassword("newPassword123"); + + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(messageSource.getMessage(eq("message.update-password.success"), any(), any(), any(Locale.class))) + .thenReturn("Password updated successfully"); + when(userService.checkIfValidOldPassword(any(User.class), eq("oldPassword"))).thenReturn(true); + + mockMvc.perform(post("/user/updatePassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(passwordDto)) + .with(csrf())) + .andExpect(status().isOk()); + + // Correct reauthentication clears the failed-attempt counter, matching login semantics. + verify(loginAttemptService).loginSucceeded(testUser.getEmail()); + verify(loginAttemptService, never()).loginFailed(any()); + } + + @Test + @DisplayName("POST /user/updatePassword - locked account is rejected with 423 without touching the password") + void updatePassword_lockedAccount_returnsLocked() throws Exception { + PasswordDto passwordDto = new PasswordDto(); + passwordDto.setOldPassword("oldPassword"); + passwordDto.setNewPassword("newPassword123"); + + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(loginAttemptService.isLocked(testUser.getEmail())).thenReturn(true); + when(messageSource.getMessage(eq("message.update-password.account-locked"), any(), any(), any(Locale.class))) + .thenReturn("Account is locked"); + + mockMvc.perform(post("/user/updatePassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(passwordDto)) + .with(csrf())) + .andExpect(status().isLocked()) + .andExpect(jsonPath("$.success").value(false)); + + verify(userService, never()).checkIfValidOldPassword(any(), any()); + verify(userService, never()).changeUserPassword(any(), any()); + } + @Test @DisplayName("POST /user/updatePassword - not authenticated") void updatePassword_notAuthenticated() throws Exception { From c086a9b35f36da7a87636f4ee32cab207a6c54b7 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 08:46:29 -0600 Subject: [PATCH 3/8] docs(security): correct audit rotation default in CONFIG.md (SUF-06) CONFIG.md claimed user.audit.maxFileSizeMb "Defaults to 10" and that "Rotation is enabled by default", contradicting the actual shipped default of 0 (rotation disabled) in dsspringuserconfig.properties and AuditConfig. This misled operators into believing a disk-growth safety net was active when it was not. Correct the entry to state the real default (0/disabled, active file grows unbounded) and explain why rotation is deliberately opt-in: audit queries (GDPR export/investigations) read only the active file, so rotated archives are excluded from those results. Points operators concerned about unbounded growth to enable rotation alongside external retention or a database-backed sink. Note: the remaining SUF-06 code hardening (the anonymous changePassword GET publishing an audit event before the token-validity check and minting a session per request, plus rate limiting) is left open on #332. Refs #332 --- CONFIG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONFIG.md b/CONFIG.md index 961d5820..fddfc8d1 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -40,7 +40,7 @@ Welcome to the User Framework SpringBoot Configuration Guide! This document outl - **Flush on Write (`user.audit.flushOnWrite`)**: Set to `true` for immediate log flushing on every write. Defaults to `false` for performance. See **Durability** below. - **Flush Rate (`user.audit.flushRate`)**: The interval, in milliseconds, at which the buffered audit log is flushed to disk when `flushOnWrite=false`. Defaults to `30000` (30 seconds). - **Max Query Results (`user.audit.maxQueryResults`)**: Maximum number of audit events returned from queries. The query service streams the active log file and retains only the most-recent `maxQueryResults` matching events in a bounded ring buffer, so query memory stays bounded regardless of file size. Defaults to `10000`. -- **Max File Size (`user.audit.maxFileSizeMb`)**: Maximum size, in megabytes, of the active audit log file before it is rotated. When exceeded, the active file is renamed to `.1` (shifting existing archives up to `maxFiles`) and a fresh active file is opened. Set to `0` or a negative value to **disable rotation** (logs grow unbounded). Defaults to `10`. **Rotation is enabled by default** to prevent unbounded disk growth. +- **Max File Size (`user.audit.maxFileSizeMb`)**: Maximum size, in megabytes, of the active audit log file before it is rotated. When exceeded, the active file is renamed to `.1` (shifting existing archives up to `maxFiles`) and a fresh active file is opened. **Defaults to `0`, which disables rotation — the active audit file grows unbounded.** Rotation is opt-in (rather than on by default) because audit queries used by GDPR export and investigations read only the *active* file, so once events rotate into `.1`, `.2`, ... they are excluded from those results (see **Query Scope** below). Enable rotation (a positive value) only alongside external log retention or a database-backed `AuditLogWriter`/`AuditLogQueryService`; when enabled, `maxFiles` bounds how many archives are retained. If unbounded growth of the active file is a concern for your deployment, enable rotation with one of those retention strategies in place. - **Max Files (`user.audit.maxFiles`)**: Maximum number of rotated archive files to retain (e.g. `user-audit.log.1` .. `user-audit.log.5`). The oldest archive beyond this count is deleted on rotation. Defaults to `5`. ### Durability From 7287295eb613c8eeab0bfc1a2f1b518c5b8158db Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 09:00:48 -0600 Subject: [PATCH 4/8] fix(security): validate ordinary Host against allow-list for email links (SUF-01) AppUrlResolver validated X-Forwarded-Host against user.security.trustedHosts but trusted request.getServerName() (derived from the Host header on common servlet containers) unconditionally. With appUrl and trustedHosts both empty by default, a spoofed Host header could poison password-reset and verification links, leaking the bearer token (CWE-640). Non-breaking hardening: - When trustedHosts is configured, the ordinary server name must also be in the allow-list; a non-allow-listed host now falls back to the first (canonical) trusted host instead of being emitted, with the request port reset to the scheme default so an internal port cannot leak. - Blank entries in trustedHosts are filtered so an empty property does not bind as a bogus single-entry allow-list. - When neither appUrl nor trustedHosts is set, behavior is unchanged but a startup warning is logged. Opt-in strict mode: - New user.security.requireCanonicalAppUrl (default false) fails startup unless appUrl or a non-empty trustedHosts is configured. Documented as becoming the default in the next major version. Updates the AppUrlResolver test that encoded the pre-fix behavior (falling back to the raw server name), and adds resolver + config unit tests. Documents the settings in CONFIG.md and dsspringuserconfig.properties. Refs #327 --- CONFIG.md | 10 ++++ .../UserSecurityBeansAutoConfiguration.java | 25 ++++++++- .../spring/user/util/AppUrlResolver.java | 24 +++++++-- .../config/dsspringuserconfig.properties | 8 ++- ...serSecurityBeansAutoConfigurationTest.java | 54 +++++++++++++++++++ .../spring/user/util/AppUrlResolverTest.java | 49 +++++++++++++++-- 6 files changed, 159 insertions(+), 11 deletions(-) create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java diff --git a/CONFIG.md b/CONFIG.md index fddfc8d1..1309ec12 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -82,6 +82,16 @@ user: - **Account Lockout Duration (`spring.security.accountLockoutDuration`)**: Duration (in minutes) for account lockout. - **BCrypt Strength (`spring.security.bcryptStrength`)**: Adjust the bcrypt strength for password hashing. Default is `12`. +### Email Link Authority (Host-header poisoning defense, CWE-640) + +Password-reset and verification emails contain a link back to your application. The host in that link determines where the bearer token is sent, so it must not be derived from an attacker-controllable `Host` header. Configure at least one of the following in production. + +- **App URL (`user.security.appUrl`)**: Canonical base URL for security email links (e.g. `https://app.example.com`). **Strongly recommended in production.** When set, request-derived hosts and `X-Forwarded-Host` are ignored entirely. Default: unset. +- **Trusted Hosts (`user.security.trustedHosts`)**: Comma-separated allow-list used when `appUrl` is unset. It gates **both** `X-Forwarded-Host` and the ordinary request server name (the `Host` header). A request host not in the list falls back to the first entry (treated as the canonical host) rather than being emitted into the link. Default: empty. +- **Require Canonical App URL (`user.security.requireCanonicalAppUrl`)**: When `true`, application startup fails unless `appUrl` or a non-empty `trustedHosts` is configured — a hard guarantee that email links can never derive their authority from a spoofable `Host` header. Default `false` (a startup warning is logged instead). Planned to become the default in the next major version. + +When neither `appUrl` nor `trustedHosts` is set, links are built from the request host (backward-compatible behavior) and a startup warning is logged. + ### Token Security Verification and password-reset tokens are **hashed at rest**. The raw token is only ever sent to the user in the emailed link; the database stores its hash. Lookups hash the incoming token and match by hash, with a transparent fallback to plaintext lookup so that any links issued before upgrading keep working until they expire. This requires no schema migration and no action from consuming applications. diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java index e703a689..f41d8731 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java @@ -175,14 +175,35 @@ public AuthenticationEventPublisher authenticationEventPublisher(ApplicationEven * ({@code user.security.trustedHosts}), defending against Host-header / X-Forwarded-Host poisoning (CWE-640). Backs off entirely if the consuming * application defines its own {@link AppUrlResolver}. * + *

+ * When neither {@code user.security.appUrl} nor {@code user.security.trustedHosts} is configured, email links derive their authority from the + * request {@code Host} header, which can be spoofed (CWE-640). By default the library logs a startup warning in that case; setting + * {@code user.security.requireCanonicalAppUrl=true} makes it fail startup instead, so an operator who wants a hard guarantee can opt into fail-fast. + *

+ * * @param appUrl the configured canonical base URL, or {@code null} when unset - * @param trustedHosts the allow-listed forwarded hosts (empty when unset) + * @param trustedHosts the allow-listed hosts (empty when unset) + * @param requireCanonicalAppUrl when {@code true}, fail startup unless {@code appUrl} or a non-empty {@code trustedHosts} is configured * @return the default {@link AppUrlResolver} */ @Bean @ConditionalOnMissingBean(AppUrlResolver.class) public AppUrlResolver appUrlResolver(@Value("${user.security.appUrl:#{null}}") String appUrl, - @Value("${user.security.trustedHosts:}") List trustedHosts) { + @Value("${user.security.trustedHosts:}") List trustedHosts, + @Value("${user.security.requireCanonicalAppUrl:false}") boolean requireCanonicalAppUrl) { + boolean appUrlConfigured = appUrl != null && !appUrl.isBlank(); + boolean trustedHostsConfigured = trustedHosts != null && trustedHosts.stream().anyMatch(h -> h != null && !h.isBlank()); + if (!appUrlConfigured && !trustedHostsConfigured) { + if (requireCanonicalAppUrl) { + throw new IllegalStateException("user.security.requireCanonicalAppUrl is enabled but neither user.security.appUrl nor " + + "user.security.trustedHosts is configured. Set a canonical user.security.appUrl (recommended) or a non-empty " + + "user.security.trustedHosts so password-reset and verification links cannot be poisoned via the Host header (CWE-640)."); + } + log.warn("AppUrlResolver: neither user.security.appUrl nor user.security.trustedHosts is configured; password-reset and verification " + + "email links will derive their authority from the request Host header, which can be spoofed (CWE-640). Set " + + "user.security.appUrl to a canonical URL (recommended) or user.security.trustedHosts to close this exposure, or set " + + "user.security.requireCanonicalAppUrl=true to fail startup instead of warning."); + } return new AppUrlResolver(appUrl, trustedHosts); } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java index d6c8fe2c..12fe1597 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java +++ b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java @@ -13,8 +13,10 @@ * Resolution order: *
    *
  1. If {@code user.security.appUrl} is configured, always use it (forwarded headers ignored).
    2. - *
  2. Otherwise build from the request, honoring {@code X-Forwarded-*} only when the resolved host is in {@code user.security.trustedHosts}; - * otherwise use the container's own server name (the untrusted forwarded host is ignored and a warning is logged).
    3. + *
  3. Otherwise build from the request. {@code X-Forwarded-*} is honored only when the forwarded host is in {@code user.security.trustedHosts}. When + * {@code trustedHosts} is configured, the ordinary request server name must also be in it — a non-allow-listed host (e.g. a spoofed {@code Host} + * header) falls back to the first configured trusted host rather than being emitted into the link. When {@code trustedHosts} is empty, the request + * server name is used as-is; configure {@code user.security.appUrl} or {@code trustedHosts} to prevent Host-header link poisoning.
    4. *
* *

@@ -45,7 +47,7 @@ public AppUrlResolver(String configuredAppUrl, List trustedHosts) { // Hostnames are case-insensitive (RFC 4343); normalise the allow-list to lower case so a mixed-case // configured or forwarded host (e.g. "App.Example.Com") still matches "app.example.com". this.trustedHosts = trustedHosts == null ? List.of() - : trustedHosts.stream().map(s -> s.trim().toLowerCase(Locale.ROOT)).toList(); + : trustedHosts.stream().map(s -> s.trim().toLowerCase(Locale.ROOT)).filter(s -> !s.isBlank()).toList(); } /** @@ -73,6 +75,22 @@ public String resolveAppUrl(HttpServletRequest request) { String host = useForwarded ? stripPort(fwdHost) : request.getServerName(); int port = useForwarded ? forwardedPort(request, scheme) : request.getServerPort(); + // SUF-01 (CWE-640): when a trusted-host allow-list is configured, the finally-chosen host must be in it — + // including the ordinary request server name, which on common servlet containers is derived from the Host + // header and is therefore attacker-influenced. A trusted X-Forwarded-Host already satisfies this (it is only + // used when allow-listed), so this guard effectively validates the non-forwarded server name. If the host is + // not allow-listed, fall back to the first configured trusted host (a known-good canonical authority) rather + // than emitting the untrusted value, and reset the port to the scheme default so the untrusted request's port + // cannot leak. When trustedHosts is empty this block is skipped and the server name is used as-is (see the + // startup warning in UserSecurityBeansAutoConfiguration). + if (!trustedHosts.isEmpty() && (host == null || !trustedHosts.contains(host.toLowerCase(Locale.ROOT)))) { + String canonical = trustedHosts.get(0); + log.warn("AppUrlResolver: request host '{}' is not in user.security.trustedHosts; using canonical trusted host '{}' for the email link", + sanitizeForLog(host), canonical); + host = canonical; + port = "https".equalsIgnoreCase(scheme) ? DEFAULT_HTTPS_PORT : DEFAULT_HTTP_PORT; + } + StringBuilder url = new StringBuilder(); url.append(scheme).append("://").append(host); if (!isDefaultPort(scheme, port)) { diff --git a/src/main/resources/config/dsspringuserconfig.properties b/src/main/resources/config/dsspringuserconfig.properties index 2e5cb603..e05091f2 100644 --- a/src/main/resources/config/dsspringuserconfig.properties +++ b/src/main/resources/config/dsspringuserconfig.properties @@ -81,8 +81,14 @@ user.security.passwordResetTokenValidityMinutes=1440 # Canonical base URL for security email links (password reset, verification). STRONGLY recommended in production # to prevent Host-header poisoning (CWE-640). When set, X-Forwarded-Host is ignored. user.security.appUrl= -# When appUrl is not set, X-Forwarded-Host is honored only for hosts in this comma-separated allowlist. +# When appUrl is not set, this comma-separated allowlist gates BOTH X-Forwarded-Host and the ordinary request +# server name (Host header). A request host not in the list falls back to the first entry (canonical) rather than +# being emitted into the link, closing Host-header poisoning (CWE-640). user.security.trustedHosts= +# When true, fail application startup unless appUrl or a non-empty trustedHosts is configured, guaranteeing that +# security email links can never derive their authority from a spoofable Host header. Default false (a startup +# warning is logged instead). This becomes the default in the next major version. +user.security.requireCanonicalAppUrl=false # If true, the test hash time will be logged to the console on startup. This is useful for determining the optimal bcryptStrength value. user.security.testHashTime=true # The default action for all requests. This can be either deny or allow. diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java new file mode 100644 index 00000000..0e47d9ff --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java @@ -0,0 +1,54 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import java.util.List; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.security.core.userdetails.UserDetailsService; +import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; + +/** + * Unit tests for {@link UserSecurityBeansAutoConfiguration#appUrlResolver}, focused on the SUF-01 (CWE-640) + * opt-in strict mode ({@code user.security.requireCanonicalAppUrl}). + */ +class UserSecurityBeansAutoConfigurationTest { + + private final UserSecurityBeansAutoConfiguration config = + new UserSecurityBeansAutoConfiguration(mock(UserDetailsService.class), mock(RolesAndPrivilegesConfig.class)); + + @Test + @DisplayName("strict mode fails startup when neither appUrl nor trustedHosts is configured") + void strictMode_failsStartupWhenNothingConfigured() { + assertThatThrownBy(() -> config.appUrlResolver(null, List.of(), true)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("requireCanonicalAppUrl"); + } + + @Test + @DisplayName("strict mode allows startup when a canonical appUrl is configured") + void strictMode_allowsStartupWhenAppUrlConfigured() { + assertThat(config.appUrlResolver("https://app.example.com", List.of(), true)).isNotNull(); + } + + @Test + @DisplayName("strict mode allows startup when a trusted-host allow-list is configured") + void strictMode_allowsStartupWhenTrustedHostsConfigured() { + assertThat(config.appUrlResolver(null, List.of("app.example.com"), true)).isNotNull(); + } + + @Test + @DisplayName("strict mode treats a blank-only trustedHosts value as unconfigured and fails startup") + void strictMode_failsStartupWhenTrustedHostsBlankOnly() { + // An empty user.security.trustedHosts= property can bind as [""]; that is not a real allow-list. + assertThatThrownBy(() -> config.appUrlResolver(null, List.of(""), true)) + .isInstanceOf(IllegalStateException.class); + } + + @Test + @DisplayName("non-strict mode returns a resolver even when nothing is configured (warns, does not fail)") + void nonStrictMode_returnsResolverWhenNothingConfigured() { + assertThat(config.appUrlResolver(null, List.of(), false)).isNotNull(); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java index c40d305e..a31ce816 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java @@ -40,6 +40,44 @@ void rejectsForwardedHostNotInAllowlistWhenNoConfiguredUrl() { assertThat(resolver.resolveAppUrl(req)).contains("trusted.example.com").doesNotContain("evil.com"); } + @Test + void usesFirstTrustedHostWhenNonForwardedServerNameNotAllowListed() { + // SUF-01 (CWE-640): with trustedHosts configured, the ordinary request server name (derived from the + // Host header on common servlet containers) must ALSO be validated against the allow-list, not just + // X-Forwarded-Host. An attacker-supplied Host must never flow into a reset/verification link; fall back + // to the canonical first trusted host instead. + AppUrlResolver resolver = new AppUrlResolver(null, List.of("app.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("attacker.example"); + req.setServerPort(443); + String resolved = resolver.resolveAppUrl(req); + assertThat(resolved).isEqualTo("https://app.example.com"); + assertThat(resolved).doesNotContain("attacker.example"); + } + + @Test + void usesNonForwardedServerNameWhenItIsAllowListed() { + // Regression guard: a server name that IS in the allow-list is used as-is (no fallback). + AppUrlResolver resolver = new AppUrlResolver(null, List.of("app.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("app.example.com"); + req.setServerPort(443); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://app.example.com"); + } + + @Test + void doesNotLeakUntrustedRequestPortWhenFallingBackToTrustedHost() { + // The untrusted request's port (e.g. an internal 8443) must not leak into the canonical link. + AppUrlResolver resolver = new AppUrlResolver(null, List.of("app.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("attacker.example"); + req.setServerPort(8443); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://app.example.com"); + } + @Test void honorsForwardedHostOnlyWhenAllowListed() { AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); @@ -154,7 +192,7 @@ void ignoresInvalidForwardedProtoAndFallsBackToContainerScheme() { } @Test - void ignoresUntrustedForwardedHostAndUsesContainerServerName() { + void fallsBackToTrustedHostWhenNeitherForwardedNorServerNameIsAllowListed() { AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); MockHttpServletRequest req = new MockHttpServletRequest(); req.setScheme("http"); @@ -163,10 +201,11 @@ void ignoresUntrustedForwardedHostAndUsesContainerServerName() { req.addHeader("X-Forwarded-Proto", "https"); req.addHeader("X-Forwarded-Host", "evil.com"); req.addHeader("X-Forwarded-Port", "443"); - // Untrusted forwarded host -> forwarded headers are NOT honored; the container's own - // scheme/host/port are used instead. + // SUF-01 (CWE-640): the untrusted forwarded host is ignored AND the ordinary server name ("internal") is + // not allow-listed either, so neither may be emitted. Fall back to the canonical first trusted host with + // the port reset to the scheme default. The internal host/port must not leak into the link. String resolved = resolver.resolveAppUrl(req); - assertThat(resolved).isEqualTo("http://internal:8080"); - assertThat(resolved).doesNotContain("evil.com"); + assertThat(resolved).isEqualTo("http://trusted.example.com"); + assertThat(resolved).doesNotContain("evil.com").doesNotContain("internal").doesNotContain("8080"); } } From 51e68ccaafc35d07f2d167e81b45a011da430c54 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 13:01:50 -0600 Subject: [PATCH 5/8] fix(security): skip lockout for passwordless accounts in updatePassword (SUF-04) checkIfValidOldPassword() always returns false when no password is set, so the SUF-04 lockout integration let any authenticated (or session-hijacking) caller lock a passwordless (passkey-only / OAuth-only) account out of every auth method by repeatedly POSTing /user/updatePassword. Guard passwordless accounts up front - before the lockout check and without touching the failed-attempt counter - returning 400 and directing the user to the set-password flow, mirroring WebAuthnManagementAPI.requireCurrentPasswordIfSet and setPassword(). Adds a unit test proving the counter is never fed for a passwordless account and an end-to-end UserApiTest that drives real lockout wiring (atomic increment, threshold, isLocked enforcement) through the full Spring context. --- .../spring/user/api/UserAPI.java | 12 +++++ .../messages/dsspringusermessages.properties | 1 + .../spring/user/api/UserAPIUnitTest.java | 39 ++++++++++++++- .../spring/user/api/UserApiTest.java | 47 +++++++++++++++++++ 4 files changed, 98 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java index 2992bc44..893c7908 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -346,6 +346,18 @@ public ResponseEntity updatePassword(@AuthenticationPrincipal DSUs return buildErrorResponse(messages.getMessage("message.user.not-found", null, "User not found", locale), 1, HttpStatus.BAD_REQUEST); } + // A passwordless (passkey-only / OAuth-only) account has no current password to verify or change here. + // checkIfValidOldPassword() always returns false for such an account, so without this guard every call would + // report a "failed attempt" to the lockout counter below — letting any authenticated (or session-hijacking) + // caller lock the account out of EVERY authentication method by hitting this endpoint repeatedly. Reject up + // front, before the lockout logic and without touching the counter, and point the user at the set-password + // flow. Mirrors WebAuthnManagementAPI.requireCurrentPasswordIfSet and the symmetric guard in setPassword(). + if (!userService.hasPassword(user)) { + logAuditEvent("PasswordUpdate", "Failure", "No password set", user, request); + return buildErrorResponse(messages.getMessage("message.update-password.no-password", null, + "No password is set on this account. Use the set password feature instead.", locale), 4, HttpStatus.BAD_REQUEST); + } + // Verifying the current password is an authentication surface, so it participates in the same brute-force // lockout as login: reject a locked account up front (HTTP 423) so a session-holding actor cannot make // unlimited old-password guesses here. Mirrors WebAuthnManagementAPI.requireCurrentPasswordIfSet. diff --git a/src/main/resources/messages/dsspringusermessages.properties b/src/main/resources/messages/dsspringusermessages.properties index 5bcc7291..54aa408f 100644 --- a/src/main/resources/messages/dsspringusermessages.properties +++ b/src/main/resources/messages/dsspringusermessages.properties @@ -15,6 +15,7 @@ message.update-user.success=Your profile has been successfully updated. message.update-password.success=Your password has been successfully updated. message.update-password.invalid-old=The old password is incorrect. message.update-password.account-locked=Your account is locked due to too many failed attempts. Please try again later. +message.update-password.no-password=No password is set on this account. Please use the set password feature instead. message.password.mismatch=Passwords do not match. message.reset-password.success=Your password has been successfully reset. You can now log in with your new password. diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 0c2c914d..4552e6c0 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -429,6 +429,7 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter .build(); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(true); when(messageSource.getMessage(eq("message.update-password.success"), any(), any(), any(Locale.class))) .thenReturn("Password updated successfully"); when(userService.checkIfValidOldPassword(any(User.class), eq("oldPassword"))).thenReturn(true); @@ -473,6 +474,7 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter .build(); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(true); when(messageSource.getMessage(eq("message.update-password.invalid-old"), any(), any(), any(Locale.class))) .thenReturn("Invalid old password"); when(userService.checkIfValidOldPassword(any(User.class), eq("wrongPassword"))).thenReturn(false); @@ -521,6 +523,7 @@ void updatePassword_wrongOldPassword_recordsFailedAttempt() throws Exception { mockMvc = updatePasswordMockMvc(); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(true); when(messageSource.getMessage(eq("message.update-password.invalid-old"), any(), any(), any(Locale.class))) .thenReturn("Invalid old password"); when(userService.checkIfValidOldPassword(any(User.class), eq("wrongPassword"))).thenReturn(false); @@ -545,6 +548,7 @@ void updatePassword_success_resetsLockoutCounter() throws Exception { mockMvc = updatePasswordMockMvc(); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(true); when(messageSource.getMessage(eq("message.update-password.success"), any(), any(), any(Locale.class))) .thenReturn("Password updated successfully"); when(userService.checkIfValidOldPassword(any(User.class), eq("oldPassword"))).thenReturn(true); @@ -569,6 +573,7 @@ void updatePassword_lockedAccount_returnsLocked() throws Exception { mockMvc = updatePasswordMockMvc(); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(true); when(loginAttemptService.isLocked(testUser.getEmail())).thenReturn(true); when(messageSource.getMessage(eq("message.update-password.account-locked"), any(), any(), any(Locale.class))) .thenReturn("Account is locked"); @@ -578,8 +583,40 @@ void updatePassword_lockedAccount_returnsLocked() throws Exception { .content(objectMapper.writeValueAsString(passwordDto)) .with(csrf())) .andExpect(status().isLocked()) - .andExpect(jsonPath("$.success").value(false)); + .andExpect(jsonPath("$.success").value(false)) + .andExpect(jsonPath("$.code").value(3)); + + verify(userService, never()).checkIfValidOldPassword(any(), any()); + verify(userService, never()).changeUserPassword(any(), any()); + } + + @Test + @DisplayName("POST /user/updatePassword - passwordless account is rejected without feeding the lockout counter") + void updatePassword_passwordlessAccount_rejectedWithoutLockout() throws Exception { + PasswordDto passwordDto = new PasswordDto(); + passwordDto.setOldPassword("anything"); + passwordDto.setNewPassword("newPassword123"); + + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + // Passwordless (passkey-only / OAuth-only) account: no password is set. + when(userService.hasPassword(testUser)).thenReturn(false); + when(messageSource.getMessage(eq("message.update-password.no-password"), any(), any(), any(Locale.class))) + .thenReturn("No password is set on this account. Use the set password feature instead."); + + mockMvc.perform(post("/user/updatePassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(passwordDto)) + .with(csrf())) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.success").value(false)) + .andExpect(jsonPath("$.code").value(4)); + // A passwordless account has no current password to guess, so this endpoint must never report a failed + // attempt — otherwise any authenticated (or session-hijacking) caller could lock the account out of every + // auth method by hammering this endpoint. The guard also short-circuits before the lockout check itself. + verify(loginAttemptService, never()).loginFailed(any()); + verify(loginAttemptService, never()).isLocked(any()); verify(userService, never()).checkIfValidOldPassword(any(), any()); verify(userService, never()).changeUserPassword(any(), any()); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java index 44385ee5..1340e010 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java @@ -34,6 +34,7 @@ import com.digitalsanctuary.spring.user.persistence.repository.UserRepository; import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository; import com.digitalsanctuary.spring.user.service.DSUserDetails; +import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.TokenHasher; import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; @@ -308,6 +309,52 @@ void shouldAcceptResetPasswordRequest() throws Exception { @DisplayName("Update Password (authenticated)") class UpdatePassword { + @Autowired + private LoginAttemptService loginAttemptService; + + @Test + @DisplayName("Repeated wrong old passwords lock the account through the real lockout wiring, and the lock is then enforced") + void repeatedWrongOldPasswordLocksAccountEndToEnd() throws Exception { + // Unlike UserAPIUnitTest (which mocks LoginAttemptService), this drives updatePassword through the full + // Spring context so it proves the real wiring — bean injection, the atomic failed-attempt increment, the + // threshold, and the isLocked() guard — actually enforces lockout, not just that the controller calls a mock. + User user = userService.registerNewUserAccount(baseTestUser); + DSUserDetails principal = new DSUserDetails(user); + int maxAttempts = loginAttemptService.getMaxFailedLoginAttempts(); + assertThat(maxAttempts).as("test profile must have account lockout enabled").isGreaterThan(0); + + Map wrong = Map.of("oldPassword", "WrongOldPass9!", "newPassword", NEW_VALID_PASSWORD); + // Each wrong guess is a 400 (invalid old password) that feeds the real LoginAttemptService. + for (int i = 0; i < maxAttempts; i++) { + mockMvc.perform(post(URL + "/updatePassword") + .with(user(principal)) + .with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content(json(wrong))) + .andExpect(status().isBadRequest()); + } + + // The real wiring must now record the account as locked (the atomic increment reached the threshold). + User locked = userRepository.findByEmail(testEmail); + assertThat(locked.isLocked()).as("account must be locked after %d failed attempts", maxAttempts).isTrue(); + assertThat(locked.getFailedLoginAttempts()).isGreaterThanOrEqualTo(maxAttempts); + + // The endpoint must ENFORCE the lock: even the CORRECT current password is rejected with 423 while locked. + Map correct = Map.of("oldPassword", VALID_PASSWORD, "newPassword", NEW_VALID_PASSWORD); + mockMvc.perform(post(URL + "/updatePassword") + .with(user(principal)) + .with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content(json(correct))) + .andExpect(status().isLocked()) + .andExpect(jsonPath("$.success").value(false)) + .andExpect(jsonPath("$.code").value(3)); + + // And the password was never changed while the account was locked. + User after = userRepository.findByEmail(testEmail); + assertThat(userService.checkIfValidOldPassword(after, VALID_PASSWORD)).isTrue(); + } + @Test @DisplayName("Should update the password with a valid old password") void shouldUpdatePasswordWhenOldPasswordValid() throws Exception { From 7282a600bfe43e48810a607006cd8a5933e6a130 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 13:02:00 -0600 Subject: [PATCH 6/8] fix(security): defer session revocation until after account delete/disable commits (SUF-03) deleteOrDisableUser() revoked sessions as its first statement, before the delete/disable committed. A login landing between the session scan and the commit authenticated against the still-present/enabled row and registered a new session the scan had already passed; because DSUserDetails caches the enabled flag at login, that session survived the delete/disable - reopening the bypass SUF-03 closed. Move revocation into an after-commit hook (new shared runAfterCommit helper, also used by publishEventAfterCommit) so it runs only once the change is durable, and not at all if the transaction rolls back. The only residual gap is SessionRegistry's own documented getAllPrincipals()/expireNow() window. Updates the after-commit tests to assert sessions are not revoked before commit and are revoked after. --- .../spring/user/service/UserService.java | 43 ++++++++++++++----- .../spring/user/service/UserServiceTest.java | 26 +++++++---- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java index 2c65fe60..20fa906f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java @@ -500,12 +500,17 @@ private void cleanUpPasswordHistory(User user) { @Transactional public void deleteOrDisableUser(final User user) { log.debug("UserService.deleteOrDisableUser: called for user: {}", user != null ? user.getEmail() : null); - // Revoke every active session for this user before the account is removed or disabled. Otherwise a - // concurrent session keeps carrying the cached DSUserDetails (authorities and the enabled flag are - // captured at login and not re-checked per request), so it stays authorized until natural expiry — the - // API-level logout only terminates the caller's current request. Done at the service layer so all callers - // of deleteOrDisableUser() are covered, mirroring the GDPR deletion path (GdprAPI.logoutUser). - sessionInvalidationService.invalidateUserSessions(user); + // Revoke every active session for this user AFTER the delete/disable commits. Otherwise a concurrent session + // keeps carrying the cached DSUserDetails (authorities and the enabled flag are captured at login and not + // re-checked per request), so it stays authorized until natural expiry — the API-level logout only terminates + // the caller's current request. Doing it post-commit (rather than as the first statement) closes a race the + // pre-commit ordering left open: a login landing between the session scan and the commit would authenticate + // against the still-present/enabled row and register a NEW session the scan already passed, and that session + // would then survive the delete/disable. After commit the row is gone/disabled, so no new login can succeed and + // this scan catches every session created before commit. Done at the service layer so all callers of + // deleteOrDisableUser() are covered, mirroring the GDPR deletion path (GdprAPI.logoutUser). The only residual + // gap is SessionRegistry's own documented getAllPrincipals()/expireNow() window (see SessionInvalidationService). + runAfterCommit(() -> sessionInvalidationService.invalidateUserSessions(user)); if (actuallyDeleteAccount) { log.debug("UserService.deleteOrDisableUser: actuallyDeleteAccount is true, deleting user: {}", user.getEmail()); // Capture user details before deletion for the post-delete event @@ -569,17 +574,35 @@ public void deleteOrDisableUser(final User user) { * @param event the event to publish after commit */ private void publishEventAfterCommit(final ApplicationEvent event) { + runAfterCommit(() -> { + log.debug("Publishing {} after commit/synchronously", event.getClass().getSimpleName()); + eventPublisher.publishEvent(event); + }); + } + + /** + * Runs the given action after the current transaction commits. + * + *

+ * If a transaction is active, the action runs from {@link TransactionSynchronization#afterCommit()} so it never + * observes (or acts on) a change that has not yet been committed, and never runs at all if the transaction rolls + * back. If no transaction is active, the action runs immediately so behavior is still correct in non-transactional + * callers. Used both to defer event publication ({@link UserDeletedEvent}/{@link UserDisabledEvent}) and to revoke + * the user's sessions only once a delete/disable is durable. + *

+ * + * @param action the action to run after commit (or immediately when no transaction is active) + */ + private void runAfterCommit(final Runnable action) { if (TransactionSynchronizationManager.isSynchronizationActive()) { TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { @Override public void afterCommit() { - log.debug("Publishing {} after commit", event.getClass().getSimpleName()); - eventPublisher.publishEvent(event); + action.run(); } }); } else { - log.debug("Publishing {} (no active transaction)", event.getClass().getSimpleName()); - eventPublisher.publishEvent(event); + action.run(); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java index 84ec8909..dd199f6f 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java @@ -357,17 +357,22 @@ void deleteOrDisableUser_publishesUserDisabledEventAfterCommit() { // When userService.deleteOrDisableUser(testUser); - // Then: the disable event must NOT yet be published + // Then: neither the disable event nor session revocation may happen before commit. Revoking sessions + // pre-commit leaves a race window (SUF-03): a login landing between the session scan and the commit + // registers a new session against the still-enabled row and, because DSUserDetails caches the enabled + // flag at login, that session would survive the disable. verify(eventPublisher, never()).publishEvent(any(UserDisabledEvent.class)); + verify(sessionInvalidationService, never()).invalidateUserSessions(any()); - // A synchronization was registered for after-commit delivery + // Two after-commit synchronizations were registered: session revocation and the disable event. List syncs = TransactionSynchronizationManager.getSynchronizations(); - assertThat(syncs).hasSize(1); + assertThat(syncs).hasSize(2); - // When the transaction commits, the disable event is delivered + // When the transaction commits, sessions are revoked and the disable event is delivered syncs.forEach(TransactionSynchronization::afterCommit); // Then + verify(sessionInvalidationService).invalidateUserSessions(testUser); ArgumentCaptor captor = ArgumentCaptor.forClass(UserDisabledEvent.class); verify(eventPublisher).publishEvent(captor.capture()); assertThat(captor.getValue().getUserId()).isEqualTo(testUser.getId()); @@ -387,18 +392,23 @@ void deleteOrDisableUser_publishesUserDeletedEventAfterCommit() { // When userService.deleteOrDisableUser(testUser); - // Then: the pre-delete event fires immediately, but the deleted event must NOT yet + // Then: the pre-delete event fires immediately, but the deleted event must NOT yet, and sessions must + // NOT be revoked before commit. Pre-commit revocation leaves a race window (SUF-03): a login landing + // between the session scan and the commit registers a new session against the still-present row that + // would survive the delete. verify(eventPublisher).publishEvent(any(UserPreDeleteEvent.class)); verify(eventPublisher, never()).publishEvent(any(UserDeletedEvent.class)); + verify(sessionInvalidationService, never()).invalidateUserSessions(any()); - // A synchronization was registered for after-commit delivery + // Two after-commit synchronizations were registered: session revocation and the deleted event. List syncs = TransactionSynchronizationManager.getSynchronizations(); - assertThat(syncs).hasSize(1); + assertThat(syncs).hasSize(2); - // When the transaction commits, the deleted event is delivered + // When the transaction commits, sessions are revoked and the deleted event is delivered syncs.forEach(TransactionSynchronization::afterCommit); // Then + verify(sessionInvalidationService).invalidateUserSessions(testUser); ArgumentCaptor captor = ArgumentCaptor.forClass(UserDeletedEvent.class); verify(eventPublisher).publishEvent(captor.capture()); assertThat(captor.getValue().getUserId()).isEqualTo(testUser.getId()); From c144b1f545c566f12b4f20707492f203f4bcd7ff Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 13:02:09 -0600 Subject: [PATCH 7/8] test(security): cover AppUrlResolver host guard and requireCanonicalAppUrl binding (SUF-01) Fills coverage gaps in the existing SUF-01 defenses: - AppUrlResolver: blank/whitespace trustedHosts filtering at the resolver level, and the ordinary server-name guard for case-insensitive and IPv6-literal hosts (previously only the X-Forwarded-Host branch was covered). - CoreBeanOverrideTest: exercises appUrlResolver() through real @Value binding via ApplicationContextRunner - requireCanonicalAppUrl fail-fast, appUrl/trustedHosts start paths, @ConditionalOnMissingBean override. - UserSecurityBeansAutoConfigurationTest: the strict-mode start tests now assert the resolver actually uses the configured appUrl/trustedHosts, not just non-null. Test-only; no production changes. --- .../user/security/CoreBeanOverrideTest.java | 76 +++++++++++++++++++ ...serSecurityBeansAutoConfigurationTest.java | 21 ++++- .../spring/user/util/AppUrlResolverTest.java | 53 +++++++++++++ 3 files changed, 146 insertions(+), 4 deletions(-) diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/CoreBeanOverrideTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/CoreBeanOverrideTest.java index aab5add7..4087e5fd 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/CoreBeanOverrideTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/CoreBeanOverrideTest.java @@ -11,6 +11,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.test.context.runner.ApplicationContextRunner; import org.springframework.context.annotation.Bean; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.security.access.hierarchicalroles.RoleHierarchy; import org.springframework.security.access.hierarchicalroles.RoleHierarchyImpl; import org.springframework.security.authentication.dao.DaoAuthenticationProvider; @@ -22,6 +23,7 @@ import org.springframework.security.crypto.password.NoOpPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import com.digitalsanctuary.spring.user.util.AppUrlResolver; /** * Proves that the four core, overridable security beans — {@link PasswordEncoder}, {@link SessionRegistry}, {@link RoleHierarchy}, and @@ -96,6 +98,55 @@ void libraryRoleHierarchyPresentByDefault() { assertThat(context.getBean(RoleHierarchy.class)).isInstanceOf(RoleHierarchyImpl.class); }); } + + @Test + @DisplayName("Library provides an AppUrlResolver by default") + void libraryAppUrlResolverPresentByDefault() { + contextRunner.run(context -> assertThat(context).hasSingleBean(AppUrlResolver.class)); + } + } + + @Nested + @DisplayName("SUF-01 requireCanonicalAppUrl strict mode (through real @Value binding)") + class AppUrlResolverStrictMode { + + @Test + @DisplayName("requireCanonicalAppUrl=true fails context startup when neither appUrl nor trustedHosts is bound") + void strictModeFailsStartupWhenNothingConfigured() { + // Drives the real @Value binding of user.security.requireCanonicalAppUrl/appUrl/trustedHosts, not a + // direct method call, proving the property binding wires the fail-fast guard end-to-end. + contextRunner.withPropertyValues("user.security.requireCanonicalAppUrl=true").run(context -> { + assertThat(context).hasFailed(); + assertThat(context.getStartupFailure()).rootCause() + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("requireCanonicalAppUrl"); + }); + } + + @Test + @DisplayName("requireCanonicalAppUrl=true starts when user.security.appUrl is bound, and the resolver uses it") + void strictModeStartsWhenAppUrlBound() { + contextRunner.withPropertyValues("user.security.requireCanonicalAppUrl=true", + "user.security.appUrl=https://app.example.com").run(context -> { + assertThat(context).hasNotFailed().hasSingleBean(AppUrlResolver.class); + assertThat(context.getBean(AppUrlResolver.class).resolveAppUrl(new MockHttpServletRequest())) + .isEqualTo("https://app.example.com"); + }); + } + + @Test + @DisplayName("requireCanonicalAppUrl=true starts when user.security.trustedHosts is bound") + void strictModeStartsWhenTrustedHostsBound() { + contextRunner.withPropertyValues("user.security.requireCanonicalAppUrl=true", + "user.security.trustedHosts=app.example.com") + .run(context -> assertThat(context).hasNotFailed().hasSingleBean(AppUrlResolver.class)); + } + + @Test + @DisplayName("default (non-strict) mode starts even when nothing is configured") + void nonStrictModeStartsWhenNothingConfigured() { + contextRunner.run(context -> assertThat(context).hasNotFailed().hasSingleBean(AppUrlResolver.class)); + } } @Nested @@ -143,6 +194,15 @@ void consumerAuthProviderWins() { }); } + @Test + @DisplayName("Consumer AppUrlResolver replaces the library's default") + void consumerAppUrlResolverWins() { + contextRunner.withUserConfiguration(ConsumerAppUrlResolverConfig.class).run(context -> { + assertThat(context).hasSingleBean(AppUrlResolver.class); + assertThat(context.getBean(AppUrlResolver.class)).isSameAs(ConsumerAppUrlResolverConfig.CONSUMER_RESOLVER); + }); + } + @Test @DisplayName("authProvider() honors a consumer-supplied PasswordEncoder (no intra-class self-call to encoder())") void authProviderUsesConsumerEncoder() { @@ -192,6 +252,13 @@ void authProviderIsConditionalAndParameterized() throws Exception { // authProvider must RECEIVE the PasswordEncoder (so a consumer override is honored) rather than self-call encoder(). assertThat(method.getParameterTypes()).as("authProvider must receive PasswordEncoder via injection").contains(PasswordEncoder.class); } + + @Test + @DisplayName("appUrlResolver() is @ConditionalOnMissingBean") + void appUrlResolverIsConditional() throws Exception { + Method method = UserSecurityBeansAutoConfiguration.class.getMethod("appUrlResolver", String.class, List.class, boolean.class); + assertThat(method.getAnnotation(ConditionalOnMissingBean.class)).isNotNull(); + } } // ---- Consumer-supplied stand-in configurations. Not @Configuration so the integration tests' component scan does not pick them up. ---- @@ -233,6 +300,15 @@ DaoAuthenticationProvider consumerAuthProvider() { } } + static class ConsumerAppUrlResolverConfig { + static final AppUrlResolver CONSUMER_RESOLVER = new AppUrlResolver("https://consumer.example.com", List.of()); + + @Bean + AppUrlResolver consumerAppUrlResolver() { + return CONSUMER_RESOLVER; + } + } + /** * A trivial custom {@link SessionRegistry} that is NOT a {@link SessionRegistryImpl}, so the test can assert the consumer's instance wins. */ diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java index 0e47d9ff..7fca576c 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java @@ -6,8 +6,10 @@ import java.util.List; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.security.core.userdetails.UserDetailsService; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import com.digitalsanctuary.spring.user.util.AppUrlResolver; /** * Unit tests for {@link UserSecurityBeansAutoConfiguration#appUrlResolver}, focused on the SUF-01 (CWE-640) @@ -27,15 +29,26 @@ void strictMode_failsStartupWhenNothingConfigured() { } @Test - @DisplayName("strict mode allows startup when a canonical appUrl is configured") + @DisplayName("strict mode allows startup when a canonical appUrl is configured, and the resolver uses it") void strictMode_allowsStartupWhenAppUrlConfigured() { - assertThat(config.appUrlResolver("https://app.example.com", List.of(), true)).isNotNull(); + AppUrlResolver resolver = config.appUrlResolver("https://app.example.com", List.of(), true); + assertThat(resolver).isNotNull(); + // Prove the configured appUrl actually flows into the resolver, not just that a bean was returned. + assertThat(resolver.resolveAppUrl(new MockHttpServletRequest())).isEqualTo("https://app.example.com"); } @Test - @DisplayName("strict mode allows startup when a trusted-host allow-list is configured") + @DisplayName("strict mode allows startup when a trusted-host allow-list is configured, and the resolver uses it") void strictMode_allowsStartupWhenTrustedHostsConfigured() { - assertThat(config.appUrlResolver(null, List.of("app.example.com"), true)).isNotNull(); + AppUrlResolver resolver = config.appUrlResolver(null, List.of("app.example.com"), true); + assertThat(resolver).isNotNull(); + // Prove the configured allow-list actually flows into the resolver: a non-allow-listed request host must + // fall back to the canonical trusted host rather than being emitted. + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("attacker.example"); + req.setServerPort(443); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://app.example.com"); } @Test diff --git a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java index a31ce816..ab58cd9a 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java @@ -208,4 +208,57 @@ void fallsBackToTrustedHostWhenNeitherForwardedNorServerNameIsAllowListed() { assertThat(resolved).isEqualTo("http://trusted.example.com"); assertThat(resolved).doesNotContain("evil.com").doesNotContain("internal").doesNotContain("8080"); } + + @Test + void filtersBlankTrustedHostEntriesSoTheFallbackUsesTheRealHost() { + // An empty/whitespace user.security.trustedHosts= property can bind as ["", " ", "app.example.com"]. + // The blank entries must be filtered so the canonical fallback is the real host, not an empty authority + // (which would produce "https://" with no host). + AppUrlResolver resolver = new AppUrlResolver(null, List.of("", " ", "app.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("attacker.example"); + req.setServerPort(443); + String resolved = resolver.resolveAppUrl(req); + assertThat(resolved).isEqualTo("https://app.example.com"); + assertThat(resolved).doesNotContain("attacker.example"); + } + + @Test + void treatsAnAllBlankTrustedHostsListAsUnconfiguredAndUsesTheServerName() { + // If every trustedHosts entry is blank the allow-list is effectively empty, so the ordinary-host guard is + // skipped and the request server name is used as-is (matching the empty-list behavior, not a fallback to ""). + AppUrlResolver resolver = new AppUrlResolver(null, List.of("", " ")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("api.example.com"); + req.setServerPort(443); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://api.example.com"); + } + + @Test + void matchesOrdinaryServerNameAgainstAllowListCaseInsensitively() { + // Hostnames are case-insensitive (RFC 4343): a mixed-case allow-list entry must match a mixed-case ordinary + // server name (Host-derived on common containers), not just X-Forwarded-Host, otherwise the SUF-01 guard + // needlessly falls back to the canonical host for a legitimate request. + AppUrlResolver resolver = new AppUrlResolver(null, List.of("App.Example.Com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("APP.example.COM"); + req.setServerPort(443); + // Allow-listed case-insensitively, so the server name is used as-is (no fallback) with its original casing. + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://APP.example.COM"); + } + + @Test + void honorsAllowListedIpv6OrdinaryServerName() { + // Documents the ordinary-host branch for IPv6 deployments: request.getServerName() already excludes the port + // (unlike the forwarded-host path), so an allow-listed IPv6 literal is emitted correctly with no stripPort. + AppUrlResolver resolver = new AppUrlResolver(null, List.of("[::1]")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("[::1]"); + req.setServerPort(443); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://[::1]"); + } } From b07b36ffcbb1703437476fc339dd0b6cc547a078 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 10 Jul 2026 13:35:54 -0600 Subject: [PATCH 8/8] docs(security): document SUF-01/03/04 changes and requireCanonicalAppUrl default Add a CHANGELOG [Unreleased] section for the SUF security-hardening series and correct the surrounding docs: - CHANGELOG: new [Unreleased] entry for the ordinary-host allow-list guard plus the requireCanonicalAppUrl opt-in (SUF-01), session revocation deferred to after commit (SUF-03), and updatePassword lockout + passwordless rejection (SUF-04). Also removed a stranded duplicate [Unreleased] block whose content is already under [4.3.2] (a release-time changelog-duplication artifact). - MIGRATION: /user/updatePassword is no longer described as unchanged (it now enforces lockout and rejects passwordless accounts with 400); documented the requireCanonicalAppUrl opt-in and corrected the trusted-host fallback text. - CONFIG: corrected the account-lockout property prefixes (spring.security.* -> user.security.*) and noted that /user/updatePassword participates in lockout. requireCanonicalAppUrl default stays false in this minor release; documented as planned to become the default in a future major version. --- CHANGELOG.md | 34 +++++++++++++++++++++++----------- CONFIG.md | 6 +++--- MIGRATION.md | 14 +++++++++++--- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ed73dc1..848ac9a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ All notable changes to this project are documented here. This project follows [Semantic Versioning](https://semver.org/) for its own public API; the supported Spring Boot versions are tracked separately (see the README compatibility matrix) and are **not** tied to this library's major version. +## [Unreleased] + +Security hardening from the SUF review series (SUF-01, SUF-03, SUF-04) plus a documentation correction (SUF-06). All changes are backward compatible; the one new property is opt-in and defaults to the pre-existing behavior. + +### Security +- **SUF-01 — Host-header link poisoning (CWE-640): the ordinary request host is now allow-listed, not just `X-Forwarded-Host`.** When `user.security.trustedHosts` is configured, the finally-chosen host for a password-reset / verification link — including the ordinary request server name, which on common servlet containers is derived from the attacker-influenceable `Host` header — must be in the allow-list. A non-allow-listed host now falls back to the first configured trusted host (a known-good canonical authority) instead of being emitted into the link. Matching on this ordinary-host path is case-insensitive (RFC 4343; previously only the forwarded-host path was), and blank/whitespace `trustedHosts` entries are ignored. + - New opt-in property **`user.security.requireCanonicalAppUrl`** (default `false`): when `true`, application startup fails unless `user.security.appUrl` or a non-empty `user.security.trustedHosts` is configured — a hard guarantee that email links can never derive their authority from a spoofable `Host` header. The default stays `false` in this release (a startup warning is logged instead), and is **planned to become the default in a future major version**. Setting `user.security.appUrl` (recommended) or `trustedHosts` in production is advised regardless of this flag. +- **SUF-03 — Revoke every user session on account delete/disable, after the change commits.** Deleting or disabling an account now revokes *all* of that user's active sessions (not just the caller's current request; `DSUserDetails` caches the enabled flag at login and is not re-checked per request). Revocation is deferred until *after* the delete/disable transaction commits, closing a race where a login landing between the session scan and the commit could register a new, surviving session. If the transaction rolls back, sessions are no longer revoked. +- **SUF-04 — Authenticated password change participates in brute-force lockout.** `POST /user/updatePassword` now rejects a locked account up front with `HTTP 423 Locked`, reports a wrong current password to `LoginAttemptService` (counting toward lockout, matching the login path), and resets the counter on success. **Passwordless (passkey-only / OAuth-only) accounts** — which have no current password to verify — are rejected up front with `HTTP 400` (the message directs the user to `POST /user/setPassword`) and **never feed the lockout counter**, preventing a session-holding caller from locking such an account out of every authentication method. + +### Fixes +- **SUF-06** — Corrected the documented audit-log rotation default in `CONFIG.md`. + +### Behavior changes (client impact) +- `POST /user/updatePassword` can now return `HTTP 423 Locked` (account locked) in addition to its existing `200`/`400`. Clients that treat any non-`200` as a generic failure need no change; clients that branch on status can surface the locked state. A passwordless account calling this endpoint receives `400` with a "no password set" message and is directed to `POST /user/setPassword`. + +### Documentation +- `CONFIG.md`: documented `user.security.requireCanonicalAppUrl` and the ordinary-host allow-list behavior; corrected the account-lockout property prefixes to `user.security.*`; noted that `/user/updatePassword` participates in lockout. +- `MIGRATION.md`: the credential-changes section no longer states `/user/updatePassword` is "unchanged" (it now enforces lockout and rejects passwordless accounts); documented the `requireCanonicalAppUrl` opt-in and corrected the trusted-host fallback description. + +### Testing +- Added unit and full-context integration coverage: passwordless `updatePassword` rejection without touching the lockout counter; end-to-end lockout through a real Spring context; session revocation deferred to after commit (delete and disable paths); `AppUrlResolver` ordinary-host allow-list (blank-entry filtering, case-insensitive matching, IPv6 literal); and `user.security.requireCanonicalAppUrl` through real `@Value` binding (`CoreBeanOverrideTest`) plus strict-mode tests that assert the resolver actually uses the configured values. + ## [5.0.1] - 2026-06-15 ### Features - WebAuthn credential-management re-authentication now returns distinct HTTP status codes @@ -366,17 +389,6 @@ All notable changes to this project are documented here. This project follows [S - Version bump for development - gradle.properties set to 4.3.2-SNAPSHOT. -## [Unreleased] -### Features -- HTMX-aware AuthenticationEntryPoint for session expiry handling (#294) - - When HTMX requests (identified by `HX-Request: true` header) hit an expired session, the framework now returns a 401 JSON response with an `HX-Redirect` header instead of the default 302 redirect that causes HTMX to swap login page HTML into fragment targets. - - New classes: - - `HtmxAwareAuthenticationEntryPoint` — detects HTMX requests and returns 401 + JSON + `HX-Redirect`; delegates to wrapped entry point for standard browser requests - - `HtmxAwareAuthenticationEntryPointConfiguration` — registers the entry point via `@ConditionalOnMissingBean(AuthenticationEntryPoint.class)` - - `WebSecurityConfig` now always configures `exceptionHandling()` with the injected entry point (previously only configured when OAuth2 was enabled) - - Consumer override: define any `AuthenticationEntryPoint` bean to replace the default - - 100% backward-compatible: non-HTMX browser requests get the same 302 redirect as before - ## [4.3.1] - 2026-03-22 ### Features - No new user-facing features in this release. diff --git a/CONFIG.md b/CONFIG.md index 1309ec12..5e32cbfa 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -78,9 +78,9 @@ user: ## Security Settings -- **Failed Login Attempts (`spring.security.failedLoginAttempts`)**: Number of failed login attempts before account lockout. Set to `0` to disable lockout. -- **Account Lockout Duration (`spring.security.accountLockoutDuration`)**: Duration (in minutes) for account lockout. -- **BCrypt Strength (`spring.security.bcryptStrength`)**: Adjust the bcrypt strength for password hashing. Default is `12`. +- **Failed Login Attempts (`user.security.failedLoginAttempts`)**: Number of failed login attempts before account lockout. Set to `0` to disable lockout. Applies to the login path and to the authenticated password-change endpoint `POST /user/updatePassword` (a locked account is rejected with `HTTP 423`, a wrong current password counts toward lockout, and a correct one resets the counter). +- **Account Lockout Duration (`user.security.accountLockoutDuration`)**: Duration (in minutes) for account lockout. `0` disables lockout; a negative value (e.g. `-1`) locks the account until an administrator unlocks it. +- **BCrypt Strength (`user.security.bcryptStrength`)**: Adjust the bcrypt strength for password hashing. Default is `12`. ### Email Link Authority (Host-header poisoning defense, CWE-640) diff --git a/MIGRATION.md b/MIGRATION.md index 06f2890e..330a51d6 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -59,11 +59,17 @@ Password-reset and email-verification links are now built from a configured cano user.security.appUrl=https://app.example.com ``` When set, `X-Forwarded-Host` is ignored entirely and all email links use this URL. -- **Alternative:** allow-list the trusted forwarded host(s): +- **Alternative:** allow-list the trusted host(s): ```properties user.security.trustedHosts=app.example.com,www.example.com ``` - `X-Forwarded-Host` is then honored only for hosts in this list; all others fall back to the container's own server name. + When set, the allow-list gates **both** `X-Forwarded-Host` **and** the ordinary request server name (the `Host` header): a request whose host is not in the list falls back to the **first** entry (treated as the canonical host) rather than being emitted into the link. Matching is case-insensitive (RFC 4343), and blank entries are ignored. +- **Optional hard guarantee (opt-in):** make a missing configuration fail startup instead of logging a warning: + ```properties + # Default: false (a startup warning is logged). When true, startup fails unless appUrl or a + # non-empty trustedHosts is configured. Planned to become the default in a future major version. + user.security.requireCanonicalAppUrl=true + ``` Local development with no proxy needs no change. `UserUtils.getAppUrl(HttpServletRequest)` is deprecated in favor of `AppUrlResolver`. @@ -136,7 +142,9 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) > **5.0.0 → 5.0.1 note:** In 5.0.0 all three failure cases returned `HTTP 400`. As of 5.0.1 they return distinct statuses (400 missing / 401 incorrect / 423 locked) so clients can tell them apart. If you wrote a client against 5.0.0 that treats any `4xx` as "re-auth failed", no change is needed; only update it if you branched specifically on `400`. -`/user/updatePassword` is unchanged: it already required and verified `oldPassword`. +`/user/updatePassword` still requires and verifies `oldPassword`, but the 5.0.x security-hardening series (SUF-04) added two behaviors: +- **Brute-force lockout.** A locked account is rejected with `HTTP 423 Locked` before the current password is verified; a wrong `oldPassword` is reported to `LoginAttemptService` (and locks the account at the configured `user.security.failedLoginAttempts` threshold); a correct one resets the counter — matching the login path. +- **Passwordless accounts.** A passwordless (passkey-only / OAuth-only) account has no `oldPassword` to verify, so it is rejected with `HTTP 400` (the message directs the user to `POST /user/setPassword`) and does **not** feed the lockout counter — this prevents a session-holding caller from locking such an account out of every authentication method. **Action required:** Update any client that calls the three endpoints above so that it collects the user's current password and sends it in the request body. `DELETE /user/webauthn/credentials/{id}` and `DELETE /user/webauthn/password`, which previously had no request body, now accept (and for password-holding accounts require) a JSON body carrying `currentPassword`. Existing IDOR/ownership checks and last-credential lockout protection are unchanged.