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 961d5820..5e32cbfa 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 @@ -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 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. 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..893c7908 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,36 @@ 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. + 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/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/service/UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java index a05451c4..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,6 +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 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 @@ -563,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/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/main/resources/messages/dsspringusermessages.properties b/src/main/resources/messages/dsspringusermessages.properties index d0d6d1b1..54aa408f 100644 --- a/src/main/resources/messages/dsspringusermessages.properties +++ b/src/main/resources/messages/dsspringusermessages.properties @@ -14,6 +14,8 @@ 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.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 420a6411..4552e6c0 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; @@ -425,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); @@ -469,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); @@ -484,6 +490,137 @@ 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(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); + + 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(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); + + 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(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"); + + mockMvc.perform(post("/user/updatePassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(passwordDto)) + .with(csrf())) + .andExpect(status().isLocked()) + .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()); + } + @Test @DisplayName("POST /user/updatePassword - not authenticated") void updatePassword_notAuthenticated() throws Exception { 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 { 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 new file mode 100644 index 00000000..7fca576c --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java @@ -0,0 +1,67 @@ +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.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) + * 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, and the resolver uses it") + void strictMode_allowsStartupWhenAppUrlConfigured() { + 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, and the resolver uses it") + void strictMode_allowsStartupWhenTrustedHostsConfigured() { + 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 + @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/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java index a8f93770..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()); @@ -437,6 +447,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 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..ab58cd9a 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,64 @@ 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://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("http://internal:8080"); - assertThat(resolved).doesNotContain("evil.com"); + 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]"); } }