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

[devondragon]

Summary

Addresses four findings from the security assessment of main. Each fix is test-first (red→green); the full suite is green (962 tests, 0 failures) and ./gradlew check passes.

Sanitized per public-repo disclosure hygiene — defect + fix, no exploit recipes (see the linked issues).

Fixes

• SUF-01 — Host-header link poisoning (Security: password-reset/verification links can derive authority from an untrusted Host header (SUF-01) #327). AppUrlResolver validated X-Forwarded-Host against user.security.trustedHosts but trusted the ordinary request.getServerName() (Host header) unconditionally. Now, when trustedHosts is configured, the ordinary host must also be allow-listed; a non-allow-listed host falls back to the first (canonical) trusted host with the port reset to the scheme default. Adds an opt-in user.security.requireCanonicalAppUrl (default false, planned to become the default next major) that fails startup unless appUrl or trustedHosts is set; otherwise a startup warning is logged. Non-breaking.

• SUF-03 — Sessions not revoked on account delete/disable (Security: account deletion/disable does not revoke the user's other active sessions (SUF-03) #329). UserService.deleteOrDisableUser() now calls SessionInvalidationService.invalidateUserSessions(user) at the service layer (covering every caller, both soft-disable and hard-delete), mirroring the GDPR deletion path.

• SUF-04 — Password change bypassed lockout (Security: authenticated password change is not wired into account lockout / rate limiting (SUF-04) #330). /user/updatePassword now participates in the same brute-force lockout as login/WebAuthn: locked accounts are rejected with HTTP 423, wrong current-password guesses call LoginAttemptService.loginFailed(), and success clears the counter.

• SUF-06 (docs only) — Audit rotation default (Security/docs: audit log rotation disabled by default; CONFIG.md contradicts the actual default (SUF-06) #332). Corrected CONFIG.md, which claimed rotation defaults to 10 MB / enabled; the real default is 0 (disabled), deliberately opt-in because rotated archives are excluded from GDPR/audit queries.

Not included (tracked on their issues)

• SUF-02 (Security: initial password creation on passwordless accounts lacks step-up and session handling (SUF-02) #328) — passwordless setPassword step-up needs a WebAuthn step-up primitive (feature work; being planned separately).
• SUF-05 (Security: password-reset token retained in the change-password page URL (SUF-05) #331) — reset-token-in-URL needs a token→flash/session exchange + 303.
• SUF-06 code hardening (Security/docs: audit log rotation disabled by default; CONFIG.md contradicts the actual default (SUF-06) #332) — anonymous audit-before-validity publish, per-request session creation, and rate limiting remain open.

Validation

• ./gradlew check — green (962 tests, 0 failures/errors/skips).
• Recommend a demo-app E2E pass (two-session delete, password-change lockout, reset flow) before the release that ships these.

Closes #327
Closes #329
Closes #330
Refs #332

[claude]

Review

Solid, well-scoped security PR — three targeted fixes (host-header link poisoning, session revocation on delete/disable, lockout on password-change) plus a docs correction, each with dedicated tests. The updatePassword lockout logic deliberately mirrors the existing WebAuthnManagementAPI.requireCurrentPasswordIfSet pattern, which keeps the codebase consistent. Findings below are minor; nothing blocking.

SUF-01 — AppUrlResolver host validation

• The fallback-to-trustedHosts.get(0) + reset-port-to-scheme-default logic is correct and well-tested (allow-listed passthrough, non-allow-listed fallback, port-leak prevention all covered in AppUrlResolverTest).
• Filtering blank entries out of trustedHosts in the constructor (AppUrlResolver.java:50) closes a real gap — an empty user.security.trustedHosts= property could previously bind to [""], which would have made the new allow-list guard effectively a no-op (empty string never equals a real host, but "list is non-empty" would still be true, triggering fallback-to-"" for every request). Good catch, and it's covered by strictMode_failsStartupWhenTrustedHostsBlankOnly.
• Note for consuming apps that rely on trustedHosts: the fix now also validates the ordinary Host header (not just X-Forwarded-Host), which is the actual security fix, but it does mean any legitimate vhost/domain that a deployment serves must be explicitly listed or it will silently get rewritten to the canonical host. That's documented in CONFIG.md and the Javadoc, so this is just worth calling out explicitly in the PR/release notes for anyone upgrading with trustedHosts already configured — "Non-breaking" in the description is true for new configs but existing trustedHosts users could see link behavior change.

SUF-03 — Session revocation in UserService.deleteOrDisableUser

• Calling sessionInvalidationService.invalidateUserSessions(user) at the top of the method (before delete/disable) is the right call — it covers both the hard-delete and soft-disable paths and all current/future callers, per the class comment.
• One subtlety: invalidateUserSessions acts on Spring's in-memory SessionRegistry, which is not transactional. If a later step in this @Transactional method throws and the DB transaction rolls back (e.g., the user row deletion fails), the sessions will already be permanently revoked even though the account still exists/is still enabled. This fails toward the safe side (over-invalidation, not under-invalidation) so it's a reasonable trade-off, but it might be worth a one-line comment noting the non-transactional nature of this side effect for future maintainers, since it's a slightly unusual pattern relative to the rest of the transactional method.
• UserAPI.deleteAccount (line ~409) now indirectly invalidates the caller's own session via deleteOrDisableUser, then calls logoutUser(request) right after — redundant but harmless.

SUF-04 — Lockout on /user/updatePassword

• Correct and consistent with the WebAuthn management pattern. Good test coverage for the three new branches (locked → 423, wrong password → recorded + rejected, success → counter reset).
• Minor: loginAttemptService.isLocked() and loginFailed()/loginSucceeded() each independently re-fetch the User row from the repository (see LoginAttemptService.java), separate from the user entity already loaded in UserAPI.updatePassword. This is pre-existing behavior mirrored from WebAuthnManagementAPI, so not a new issue, but it does mean this endpoint now does up to 3 extra DB round-trips per call. Not a concern at current scale, just flagging for awareness given this is an auth-adjacent hot path.
• The new message key message.update-password.account-locked is only added to the single (default) properties file, consistent with the repo only maintaining one locale currently — no gap there.

SUF-06 — Docs-only fix

• The CONFIG.md correction is accurate and cross-references the Query Scope section, which exists.

Test coverage

• Good red→green coverage across all three functional fixes: AppUrlResolverTest (host validation + port-leak + regression guard for allow-listed server names), UserServiceTest (both delete and disable paths invalidate sessions), UserAPIUnitTest (three new lockout scenarios), and a new UserSecurityBeansAutoConfigurationTest for the requireCanonicalAppUrl startup-fail-fast behavior including the blank-only-trustedHosts edge case.
• Nothing obviously missing; the PR description's suggestion to do a demo-app E2E pass (two-session delete, password-change lockout, reset flow) before release is a reasonable belt-and-suspenders step given these all touch session/security state that unit tests can't fully exercise.

Nit

• UserAPI.java:352 — the lockout check happens after fetching the user but before the try block that wraps the rest of the method; that's fine functionally (it returns early), but for consistency with the rest of the method's error handling it might read slightly cleaner inside the try. Not worth blocking on.

Overall: clean, well-documented, test-first security fixes that follow established patterns in the codebase. Approve.

[Copilot]

Pull request overview

This PR addresses several security-assessment findings by hardening (1) how security email links derive their base URL (mitigating Host/X-Forwarded-Host poisoning), (2) session revocation behavior when accounts are deleted/disabled, and (3) brute-force lockout integration for authenticated password changes. It also corrects audit-rotation documentation and adds configuration/docs for an opt-in “fail fast” mode around canonical email-link authority.

Changes:

• Harden AppUrlResolver so that when user.security.trustedHosts is configured, both X-Forwarded-Host and the ordinary request host must be allow-listed; otherwise fall back to the first trusted host and reset the port to scheme default.
• Invalidate all sessions when a user is deleted or disabled via UserService.deleteOrDisableUser().
• Wire /user/updatePassword into account lockout (423 Locked when locked; failed guesses recorded; successful re-auth clears the counter) and add corresponding messages/docs/tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java	Enforces trusted-host allowlist on non-forwarded host and falls back to canonical trusted host to prevent Host-header link poisoning.
src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java	Adds regression tests covering canonical fallback and port non-leak behavior when hosts aren’t allow-listed.
src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java	Adds opt-in strict startup mode (requireCanonicalAppUrl) and warning when neither appUrl nor trustedHosts is configured.
src/test/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfigurationTest.java	Unit tests for strict vs non-strict startup behavior around canonical URL configuration.
src/main/java/com/digitalsanctuary/spring/user/service/UserService.java	Revokes all user sessions at the service layer during delete/disable to prevent stale authenticated sessions.
src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java	Verifies session invalidation is invoked for both hard-delete and soft-disable paths.
src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java	Integrates password update into lockout semantics (lock check, failed attempt recording, success reset).
src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java	Adds unit tests asserting lockout behavior for update-password (failed, success, locked).
src/main/resources/messages/dsspringusermessages.properties	Adds a localized message for locked-account password update attempts.
src/main/resources/config/dsspringuserconfig.properties	Documents trustedHosts behavior change and introduces requireCanonicalAppUrl defaulted to false.
CONFIG.md	Corrects audit rotation defaults and documents canonical email-link authority configuration options.

---

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

[claude]

Review

Went through the diff in detail (AppUrlResolver, UserSecurityBeansAutoConfiguration, UserService.deleteOrDisableUser, UserAPI.updatePassword, plus the associated tests and CONFIG.md) and cross-checked against the existing WebAuthnManagementAPI.requireCurrentPasswordIfSet pattern this PR mirrors. Overall this is a strong, well-scoped security PR — each fix is narrowly targeted at its issue, the reasoning is documented at the point of risk (not just in the PR description), and the tests exercise the actual attacker-relevant scenarios (case-insensitive host matching, port-leak prevention, blank-entry filtering, real end-to-end lockout wiring in UserApiTest, not just mock verification). A few notes, nothing blocking:

SUF-01 (AppUrlResolver)

• The "ordinary host must also be allow-listed when trustedHosts is set" fix is correct and closes a real gap — previously only X-Forwarded-Host was validated, so a direct request with a spoofed Host header sailed through unchecked whenever trustedHosts (but not appUrl) was configured.
• Falling back to the first trusted host + resetting the port to the scheme default (rather than emitting the untrusted host/port) is the right call, and doesNotLeakUntrustedRequestPortWhenFallingBackToTrustedHost specifically pins that behavior down.
• Filtering blank trustedHosts entries (filter(s -> !s.isBlank())) fixes a real edge case: an unset trustedHosts= property can bind as [""], which previously would have made the fallback host "" → https:// with no authority. Good catch, and it's covered by filtersBlankTrustedHostEntriesSoTheFallbackUsesTheRealHost.
• Minor: requireCanonicalAppUrl is opt-in for now with just a startup warning. Given appUrl/trustedHosts being unset is the default out-of-the-box state for consuming apps, most deployments will silently keep the pre-fix exposure until the next major version flips the default. That's a reasonable transition strategy for a library, just flagging it's a real gap in the interim (already called out in the PR description, so this is more a "make sure this is prioritized" note than a new finding).

SUF-03 (UserService.deleteOrDisableUser)

• Doing session revocation as an afterCommit synchronization is the right choice — revoking before commit would leave the race window the code comment describes (a login landing between the session scan and the commit would authenticate against the still-enabled/present row and register a session that survives the delete/disable). Doing it after commit closes that.
• runAfterCommit generalizing the former publishEventAfterCommit is a clean, minimal refactor — no behavior change for the event-publish path, confirmed by reading the diff (same synchronization-registration logic, just parameterized over a Runnable).
• The sessionInvalidationService.invalidateUserSessions(user) call in the hard-delete branch runs against a JPA entity that's already been passed to userRepository.delete(...) by the time the synchronization fires post-commit. That's fine here because SessionInvalidationService.invalidateUserSessions only reads user.getId()/user.getEmail() off the in-memory object (no re-fetch), but it's a bit of an implicit contract between the two classes — worth a one-line comment noting invalidateUserSessions must not re-fetch from the DB, so a future change to that method doesn't silently break the hard-delete path.

SUF-04 (UserAPI.updatePassword)

• Guard ordering (passwordless check → lockout check → verify-and-record) is correct: a passwordless account is rejected without ever touching the lockout counter, which is exactly the bypass this PR is closing (an authenticated/session-hijacking caller could otherwise lock any passkey/OAuth-only account out of every auth method by hammering this endpoint with hasPassword() == false guesses).
• Error code reuse (3 = locked, 4 = no-password) doesn't collide with this endpoint's existing codes (1, 2, 5), verified against the other buildErrorResponse calls in the same method.
• Good addition of the full-context UserApiTest (repeatedWrongOldPasswordLocksAccountEndToEnd) alongside the mocked UserAPIUnitTest — that one actually proves the atomic increment + threshold + isLocked() wiring works end-to-end rather than just asserting the controller calls a mock.

SUF-06 (docs)

• Verified the corrected CONFIG.md claim (maxFileSizeMb defaults to 0/disabled) against dsspringuserconfig.properties, where the default is indeed user.audit.maxFileSizeMb=0. Doc now matches code.

Test coverage: comprehensive — IPv6 host handling, case-insensitive matching, blank-entry filtering, port-leak prevention, and both the unit-mock and full-Spring-context angles for the lockout change. No gaps I could find for the stated scope.

Not addressed (per PR description, tracked separately): SUF-02, SUF-05, and SUF-06's code-hardening items remain open on their own issues — reasonable to ship the four fixes here independently rather than block on the rest.

Nice work — the "test-first, red→green" framing shows in how tightly each test maps to the specific attacker scenario being closed.

[claude]

Review

Solid, well-scoped security PR — each fix is test-first, mirrors existing conventions in the codebase (e.g. WebAuthnManagementAPI.requireCurrentPasswordIfSet), and the reasoning is well-documented in code comments and CHANGELOG/MIGRATION docs. A few observations, nothing blocking:

SUF-01 — AppUrlResolver host allow-listing

• The ordinary-host guard, canonical fallback, port reset to scheme default, blank-entry filtering, and case-insensitive matching are all correct and well covered by tests (including IPv6 literals and mixed-case hosts).
• Nice touch resetting the port to the scheme default on fallback so an internal/attacker port never leaks into the email link.
• Minor: the canonical fallback host (trustedHosts.get(0)) is stored lower-cased (constructor normalizes for matching), so the emitted link host will always be lowercase even if the operator configured trustedHosts=App.Example.Com. Cosmetic only — DNS is case-insensitive — but could surprise someone diffing generated links against their configured casing.
• The new requireCanonicalAppUrl opt-in with fail-fast is a good incremental hardening step, and defaulting to false with a startup warning (rather than breaking existing deployments) is the right call for a minor/patch release.

SUF-03 — Session revocation on delete/disable

• Moving SessionInvalidationService.invalidateUserSessions() to run after commit (via the new shared runAfterCommit helper) correctly closes the pre-commit race described in the comment (a login landing between the session scan and commit). Good catch, and reusing the existing publishEventAfterCommit machinery instead of duplicating it is a clean refactor.
• For the hard-delete path, the deferred lambda captures the User entity after userRepository.delete(user) — worth double-checking in CI/integration testing that user.getId()/getEmail() are still populated at that point in your JPA provider (Hibernate doesn't null these out on delete(), so this should be fine, but it's a subtle enough dependency that it's worth calling out explicitly, e.g. in the comment or a targeted test using a fresh EntityManager/detached scenario).
• Test coverage (UserServiceTest) directly verifies the sync count and post-commit invocation for both paths — good.

SUF-04 — Password-change lockout participation

• Logic order (not-found → passwordless guard → locked check → verify → record) is correct and avoids feeding the lockout counter for passwordless accounts, which is the key risk this fix addresses (a session holder locking a passkey-only account out of every auth method).
• Consistent with the pattern already established in WebAuthnManagementAPI.
• Both unit-level (mocked LoginAttemptService) and full-context end-to-end tests are included — the end-to-end test in UserApiTest that drives real lockout through maxFailedLoginAttempts attempts and then asserts 423 on a correct password is a great regression guard.
• Minor/pre-existing nit (not introduced by this PR): error code values in UserAPI.updatePassword aren't unique per-method (code 1 is reused for both "user not found" and "invalid old password"). Not a functional issue since HTTP status differs, but flagging in case it's ever relied on by a client for exact error branching.

SUF-06 — Docs correction

• Straightforward and clearly correct per the code (maxFileSizeMb default is indeed 0/disabled).

General

• CHANGELOG.md/CONFIG.md/MIGRATION.md updates are thorough and clearly flag the one behavior change (423 from updatePassword).
• Test coverage across all four fixes is strong — unit, auto-configuration, and full-context integration tests are all present, including negative/edge cases (blank trustedHosts entries, IPv6, passwordless accounts).

No blocking issues found. Nice work.