security: fixes for SUF-01, SUF-03, SUF-04 and SUF-06 (docs)

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.

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 `<name>.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 `<name>.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 `<name>.1`, `<name>.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
@@ -78,9 +78,19 @@ 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)
+
+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
 

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.
 

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<WebAuthnCredentialManagementService> webAuthnCredentialManagementServiceProvider;
 	private final AppUrlResolver appUrlResolver;
+	private final LoginAttemptService loginAttemptService;
 
 	@Value("${user.security.registrationPendingURI}")
 	private String registrationPendingURI;
@@ -344,11 +346,36 @@ public ResponseEntity<JSONResponse> 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.
+		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<String> errors = passwordPolicyService.validate(user, passwordDto.getNewPassword(), user.getEmail(),