security: remaining SUF fixes (SUF-02 step-up, SUF-05 reset token, SUF-06 hardening)

[devondragon]

Summary

Addresses the remaining findings from the security assessment (SUF-02, SUF-05, SUF-06 code hardening) that were deferred out of #333. Each was scoped to the pragmatic, non-breaking approach; the one deliberate behavior change (SUF-02 disabling setPassword by default) is called out below and documented in MIGRATION.md. Full suite green (982 tests, 0 failures), ./gradlew check passes, and the demo app builds/tests cleanly against the SNAPSHOT (301 tests, 0 failures).

Fixes

• SUF-02 — Passwordless setPassword step-up (Security: initial password creation on passwordless accounts lacks step-up and session handling (SUF-02) #328). Adding an initial password to a passwordless (passkey-only) account had no proof of presence beyond an authenticated session. New SPI StepUpService: if a consumer provides the bean it is required (failure → HTTP 401); if none is present the endpoint is disabled by default (HTTP 403). user.security.allowInitialPasswordSetWithoutStepUp=true restores the prior session-only behavior. This is the interim guard — the full WebAuthn step-up primitive is deliberately deferred as separate feature work.

• SUF-05 — Reset token leaked via Referer/cache (Security: password-reset token retained in the change-password page URL (SUF-05) #331). A scoped interceptor now sets Referrer-Policy: no-referrer and Cache-Control: no-store on the configured reset URIs, so the token (which appears in the reset page URL) is not sent in the Referer header or written to caches. Non-breaking: the token-in-body savePassword contract is unchanged. Residual (accepted): the token still briefly appears in the address bar/history — removing that requires the breaking token→session exchange, which was intentionally not taken.

• SUF-06 code hardening (Security/docs: audit log rotation disabled by default; CONFIG.md contradicts the actual default (SUF-06) #332). showChangePasswordPage no longer mints an HttpSession for the anonymous token-validation request, and audits an invalid token as Failure (was always Success). Intentionally not changed: audit-log rotation stays disabled by default (rotated archives are excluded from GDPR/audit queries — a deliberate tradeoff); rate-limiting anonymous endpoints remains delegated to Bucket4J.

⚠️ Behavior change

POST /user/setPassword is now disabled by default (SUF-02). It returns HTTP 403 unless a StepUpService bean is provided (then step-up is required; failures → 401), or user.security.allowInitialPasswordSetWithoutStepUp=true is set. Consumers whose passwordless users set an initial password must choose one of those options — see MIGRATION.md.

Testing

• Library: 982 tests, 0 failures (./gradlew check green). New coverage: setPassword step-up guard (disabled-by-default / opt-in flag / StepUpService grant+deny), reset-page security headers, and the anonymous-session + audit-status hygiene on showChangePasswordPage (all test-first, red→green).
• Demo app: ./gradlew test --refresh-dependencies resolved 5.0.2-SNAPSHOT → 301 tests, 0 failures (API-breakage guard).
• Browser E2E note: the setPassword flow now 403s by default, so a full Playwright pass of that flow requires the demo to set allowInitialPasswordSetWithoutStepUp=true or provide a StepUpService (the documented migration action). Reset-flow E2E (SUF-05/06) is non-breaking.

Scope / deferred (tracked)

• Full WebAuthn step-up primitive (beyond the SUF-02 SPI) — separate feature work.
• SUF-05 residual address-bar/history exposure — would need the breaking token→session exchange.
• SUF-06 rate-limiting + a bounded rotation default — intentionally deferred (external / GDPR tradeoff).

Refs #328
Refs #331
Refs #332

[Copilot]

Pull request overview

This PR completes the remaining SUF security hardening items (SUF-02/05/06) by introducing a step-up SPI for initial password creation on passwordless accounts, adding reset-flow privacy/no-cache headers, and hardening anonymous reset-token validation to avoid session minting and to audit invalid tokens as failures.

Changes:

• SUF-02: Add StepUpService SPI and gate POST /user/setPassword (disabled-by-default unless a step-up bean is present or an explicit opt-in property is set).
• SUF-05: Add a scoped interceptor that sets Referrer-Policy: no-referrer and Cache-Control: no-store on reset-related URIs.
• SUF-06: Avoid creating an HttpSession during anonymous reset-token validation and mark invalid-token audits as Failure; add/adjust tests and documentation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java	Adds tests for anonymous-session avoidance, audit-status labeling, and reset privacy headers.
src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java	Adds unit coverage for the new /user/setPassword step-up/disabled-by-default behavior.
src/main/resources/messages/dsspringusermessages.properties	Adds i18n messages for step-up-required and disabled-by-default setPassword responses.
src/main/java/com/digitalsanctuary/spring/user/web/WebInterceptorConfig.java	Registers the reset-flow security-headers interceptor on the configured reset URIs.
src/main/java/com/digitalsanctuary/spring/user/web/PasswordResetSecurityHeadersInterceptor.java	Implements the Referrer-Policy and Cache-Control headers for reset pages.
src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java	Introduces the step-up SPI used to protect initial password creation on passwordless accounts.
src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java	Stops minting sessions for anonymous token validation and audits invalid tokens as failures.
src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java	Enforces step-up/disabled-by-default gating for POST /user/setPassword (SUF-02).
MIGRATION.md	Documents the setPassword behavior change, step-up SPI, and the opt-in compatibility flag.
CONFIG.md	Documents StepUpService and user.security.allowInitialPasswordSetWithoutStepUp.
CHANGELOG.md	Summarizes SUF-02/05/06 changes and explicitly calls out the setPassword default behavior change.

---

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

[claude]

Review: security/suf-remaining (SUF-02, SUF-05, SUF-06)

Overall this is a well-scoped, well-documented set of fixes — good tests-first coverage, clear CHANGELOG/MIGRATION/CONFIG updates, and each change stays within its stated "pragmatic, non-breaking" (or clearly-flagged breaking) scope. A few things worth considering before merge:

Security

• No startup-time warning for the setPassword default-disable (SUF-02). This PR's SUF-01 fix logs a log.warn(...) at startup in UserSecurityBeansAutoConfiguration when requireCanonicalAppUrl isn't set, giving operators proactive visibility into a safety-relevant default. SUF-02 introduces an analogous, arguably more impactful default-behavior change (POST /user/setPassword returns 403 unless a StepUpService bean or the opt-in flag is present), but there's no equivalent startup log. Today, a consuming app upgrading the library only discovers this when a real passwordless user hits 403 in production. Consider logging a WARN at startup (mirroring the SUF-01 pattern) when no StepUpService bean is registered and allowInitialPasswordSetWithoutStepUp=false, so operators get the same fail-fast visibility this PR already establishes elsewhere.
• Reused error code=6 for two distinct failure reasons in UserAPI.setPassword — the step-up-denied branch (401) and the step-up-disabled branch (403) both return code: 6, whereas every other branch in this method (and the sibling endpoints) uses a distinct numeric code (1, 2, 3, 5). Not a functional bug since HTTP status still differentiates them, but it weakens the code field for API consumers that branch on it rather than status. Minor — consider splitting into two codes.
• StepUpService.isStepUpSatisfied(user, "set-password", request) hardcodes the action identifier as a string literal at the one call site with no corresponding constant on the StepUpService interface itself. Fine for a single action today, but worth adding e.g. StepUpService.ACTION_SET_PASSWORD before a second step-up-gated action is added, to avoid typo drift between the SPI and callers.

Test coverage

• The only assertion that the SUF-05 headers actually reach a response (resetPage_carriesPrivacyHeaders in UserActionControllerTest) builds its own standalone MockMvc with PasswordResetSecurityHeadersInterceptor manually mapped in the test itself, rather than exercising the real WebInterceptorConfig.addInterceptors wiring (its @Value-injected changePasswordURI/forgotPasswordChangeURI properties) or the actual Spring Security filter chain. As written, a bug in WebInterceptorConfig itself (wrong property key, mismatched default, interceptor never registered) would not be caught by this test, and there's no full-context (@IntegrationTest/@SecurityTest) check that the headers survive alongside Spring Security's own HeaderWriterFilter (which sets Cache-Control/Referrer-Policy defaults, generally deferred/guarded by containsHeader, but that interaction is currently unverified). Worth an integration test that boots the real WebInterceptorConfig and hits the configured URIs.

Code quality

• new PasswordResetSecurityHeadersInterceptor() is instantiated directly in WebInterceptorConfig rather than being a Spring-managed bean, unlike globalUserModelInterceptor which is constructor-injected. Harmless since the interceptor is stateless, but slightly inconsistent with the rest of the class.
• The new SUF-06 tests in UserActionControllerTest use raw Java assert statements — consistent with the pre-existing tests in that same file, so not a regression, but worth noting these rely on JVM assertions being enabled (true for the Gradle test task's default here, but easy to silently no-op under a differently-configured runner). The project's stated convention (per CLAUDE.md) is AssertJ fluent assertions; might be worth migrating this file at some point.

Bugs / correctness

Nothing that looks broken — traced through the setPassword step-up gate, the showChangePasswordPage session/audit change, and the interceptor path registration (changePasswordURI/forgotPasswordChangeURI both correctly default-match the properties used elsewhere in the codebase), and didn't find a functional defect. The ordering of checks in setPassword (already-has-password check before the step-up gate) is fine since the caller is always the account owner.

Performance

No concerns — the changes are a couple of conditional branches and a stateless header-writing interceptor on low-traffic auth endpoints.

Nice work overall — the docs (CHANGELOG/CONFIG/MIGRATION) are thorough and the behavior-change callout is prominent and actionable.

[claude]

Review: SUF-02 / SUF-05 / SUF-06 hardening

Solid, well-scoped PR — the docs (CHANGELOG/CONFIG/MIGRATION) are thorough and the trade-offs are clearly called out. A few things worth a look before merge.

Potential bugs / design issues

• StepUpService Javadoc promises body access that isn't actually available. StepUpService.java says implementations can read the step-up proof from "headers, body, or a prior challenge stored in the session." But by the time isStepUpSatisfied(user, "set-password", request) runs in UserAPI.setPassword (UserAPI.java:567-573), Spring MVC has already fully consumed the request InputStream to bind @Valid @RequestBody SetPasswordDto. There's no ContentCachingRequestWrapper (or similar) anywhere in the framework's filter chain, so a consumer implementation that tries request.getInputStream()/getReader() to pull a WebAuthn assertion or TOTP code out of the body will get an already-drained/empty stream. Worth either fixing the Javadoc to say headers/session only, or (if body-based proofs are meant to be supported) documenting that the consumer needs their own body-caching filter in front of this endpoint.

• Error code 6 is reused for two distinct setPassword failures. Both the step-up-denied (401) and disabled-by-default (403) branches call buildErrorResponse(..., 6, ...) (UserAPI.java:571-577), whereas the other failure branches in the same method each use a distinct code (1/2/3/5). Clients that key off the JSON code field (rather than HTTP status) can't tell the two cases apart. Minor, but easy to fix while it's fresh.

Test coverage

• All the new setPassword step-up coverage in UserAPIUnitTest is Mockito-level (manually swapping stepUpServiceProvider via ReflectionTestUtils). There's no @IntegrationTest/@SecurityTest exercising POST /user/setPassword through the real Spring context + security filter chain with zero vs. one StepUpService bean actually in the ApplicationContext. Given this is a default-behavior change on a security-sensitive endpoint, a real-context test would catch wiring mistakes (e.g. ObjectProvider misconfiguration, ordering with other filters) that a mocked unit test can't.
• Similarly, resetPage_carriesPrivacyHeaders in UserActionControllerTest builds a standalone MockMvc and manually registers PasswordResetSecurityHeadersInterceptor itself — it never goes through WebInterceptorConfig. So a typo in the @Value property keys/defaults in WebInterceptorConfig (e.g. if user.security.changePasswordURI and the interceptor's fallback default ever drift apart) wouldn't be caught by any test today.

Minor / nits

• UserAPIUnitTest.java's new imports (StepUpService, java.util.List, ObjectProvider) are inserted out of alphabetical order, ahead of OnRegistrationCompleteEvent — breaks the "alphabetical imports" convention from CLAUDE.md (harmless, just a nit).
• The new user.security.allowInitialPasswordSetWithoutStepUp property isn't added to additional-spring-configuration-metadata.json for IDE autocomplete (though a couple of other recent user.security.* properties, like requireCanonicalAppUrl, are also missing there, so this isn't a new gap this PR introduces).
• Small ordering nit in setPassword: the step-up check runs before the password-confirmation-match and password-policy checks, so a request that's going to fail validation anyway (e.g. typo'd confirm password) still burns a step-up attempt/challenge. Not a security issue, just a slightly odd UX order.

Security

• The core design is sound: fail-closed by default (403 when no StepUpService), fail-closed on StepUpService exceptions (caught by the outer catch (Exception ex) → 500, not silently allowed), and the flag only relaxes behavior when explicitly opted in. No IDOR — setPassword always operates on the caller's own account via userDetails, so the differentiated 400/401/403 responses don't create a cross-account enumeration oracle.
• SUF-05/06 changes (no session minted for anonymous token-validation GETs, invalid tokens audited as Failure, Referrer-Policy/Cache-Control on the reset pages) look correct and match the documented residual risk (address-bar/history exposure remains, as noted).
• One scope observation (not a blocker): the same Referer/cache token-leak pattern (CWE-598) plausibly also applies to registrationConfirm's email-verification token URL, which isn't covered by the new interceptor. That token is deleted atomically on first use so the exposure window is smaller, but might be worth a follow-up if you want full parity.

Performance

Nothing notable — the interceptor only sets two headers on two specific configured URIs, and ObjectProvider#getIfAvailable() is a cheap lookup on the hot path.

Nice work overall — the fail-closed default for SUF-02 and the careful non-breaking scoping on SUF-05/06 are the right calls.