From 13f4fdabf7120de75f8c618c59462ee35c779098 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 21 Aug 2026 14:54:32 -0600 Subject: [PATCH 1/3] fix: escape passkey label in notification email and defer event to after-commit Follow-up hardening from a post-release review of the 5.3.3 WebAuthn work. - Escape the client-supplied credential label before it reaches the passkey-registered notification template. The template renders it with th:utext (so the message's own markup survives), so an unescaped label could inject HTML into the very email that warns the owner of an unrecognized passkey. UserEmailService now HTML-escapes it. - Move WebAuthnCredentialRegistrationListener to @TransactionalEventListener(AFTER_COMMIT, fallbackExecution=true). The event is published inside the @Transactional save() before commit, so a commit failure (e.g. a label longer than the 64-char column) previously sent a notification and recorded an audit entry for a registration that never persisted. - Clarify CONFIG.md on custom StepUpService precedence: setPassword consults the bean whenever present, while the passkey delete/rename/remove-password endpoints require user.security.stepUp.enabled=true. This matches the deliberate per-endpoint behavior in the code (WebAuthnManagementAPI keys on the property to avoid newly enforcing step-up on 5.3.1 SPI adopters). Adds direct unit tests for the label escaping and null fallback, a no-recipient no-op test, and a guard test pinning the AFTER_COMMIT phase. Claude-Session: https://claude.ai/code/session_01KL5qvKVGQLLQDjHToy34vj --- CONFIG.md | 2 +- ...ebAuthnCredentialRegistrationListener.java | 17 ++++++-- .../spring/user/service/UserEmailService.java | 6 ++- ...thnCredentialRegistrationListenerTest.java | 25 +++++++++-- .../user/service/UserEmailServiceTest.java | 43 +++++++++++++++++++ 5 files changed, 83 insertions(+), 10 deletions(-) diff --git a/CONFIG.md b/CONFIG.md index f9065ba..156a1ce 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -160,7 +160,7 @@ user: **Reserved authority names.** Do not name a role or privilege `FACTOR_*` in `user.roles-and-privileges`. Spring Security uses that prefix for factor authorities, and a plain authority with such a name is indistinguishable from a real factor by name: it satisfies MFA enforcement without the factor ever being completed, and it shadows the genuine factor in a step-up freshness check. Startup fails when such a name is configured while MFA or step-up is enabled, and logs an error otherwise. -**Custom implementations.** A `StepUpService` bean supplied by your application takes precedence over the built-in one, whatever `enabled` is set to. Implement the SPI to require TOTP, a hardware token, or any other proof. +**Custom implementations.** A `StepUpService` bean supplied by your application always replaces the built-in one (the built-in bean backs off when yours is present). Implement the SPI to require TOTP, a hardware token, or any other proof. Where that bean is consulted depends on the endpoint: `POST /user/setPassword` uses it whenever the bean is present, whatever `enabled` is set to (this is the original SPI surface from 5.3.1). The passkey delete, rename, and remove-password endpoints consult it only when `user.security.stepUp.enabled=true`, so an application that adopted the SPI for `setPassword` alone does not have step-up newly enforced on those endpoints on upgrade. To gate every credential-altering operation through your bean, set `enabled=true`. - **Allow Without Step-Up (`user.security.allowInitialPasswordSetWithoutStepUp`)**: When no `StepUpService` bean is present at all, `setPassword` is **disabled** (`HTTP 403`) unless this is `true`, which restores the previous session-only behavior. Default: `false`. diff --git a/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java b/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java index c7c7862..a4190d1 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java +++ b/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java @@ -1,8 +1,9 @@ package com.digitalsanctuary.spring.user.listener; import org.springframework.context.ApplicationEventPublisher; -import org.springframework.context.ApplicationListener; import org.springframework.stereotype.Component; +import org.springframework.transaction.event.TransactionPhase; +import org.springframework.transaction.event.TransactionalEventListener; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.event.WebAuthnCredentialRegisteredEvent; import com.digitalsanctuary.spring.user.security.WebAuthnConfigProperties; @@ -24,7 +25,7 @@ @Slf4j @Component @RequiredArgsConstructor -public class WebAuthnCredentialRegistrationListener implements ApplicationListener { +public class WebAuthnCredentialRegistrationListener { private final UserEmailService userEmailService; private final ApplicationEventPublisher eventPublisher; @@ -33,10 +34,18 @@ public class WebAuthnCredentialRegistrationListener implements ApplicationListen /** * Audits the enrollment and, unless disabled, emails the account owner. * + *

+ * Runs after the enrollment transaction commits. The credential is written through a {@code @Transactional} + * {@code save()} that publishes this event before the commit, so a commit failure (for example a label longer + * than the column) would otherwise send a notification and record an audit entry for a registration that never + * persisted. {@code fallbackExecution = true} keeps the listener firing if the event is ever published outside a + * transaction. + *

+ * * @param event the registration event */ - @Override - public void onApplicationEvent(WebAuthnCredentialRegisteredEvent event) { + @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT, fallbackExecution = true) + public void onCredentialRegistered(WebAuthnCredentialRegisteredEvent event) { // Audit first and unconditionally: the notification is a courtesy the operator can switch off, and a mail // outage must not cost us the security-relevant record of the enrollment. eventPublisher.publishEvent(AuditEvent.builder().source(this).user(event.getUser()) diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java index 6d59506..ad83ed2 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java @@ -17,6 +17,7 @@ import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; +import org.springframework.web.util.HtmlUtils; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.mail.MailService; import com.digitalsanctuary.spring.user.persistence.model.PasswordResetToken; @@ -193,7 +194,10 @@ public void sendPasskeyRegisteredNotification(final User user, final String labe } Map variables = new HashMap<>(); variables.put("user", user); - variables.put("label", label != null ? label : "Passkey"); + // The label is client-supplied at enrollment and the template renders it unescaped (th:utext, so the + // message's own markup survives). Escape it here so a crafted label cannot inject HTML into the + // very email that warns the owner of an unrecognized passkey. + variables.put("label", HtmlUtils.htmlEscape(label != null ? label : "Passkey")); mailService.sendTemplateMessage(user.getEmail(), "New passkey added to your account", variables, "mail/webauthn-credential-registered.html"); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListenerTest.java b/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListenerTest.java index a8bcaa2..23e43c5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListenerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListenerTest.java @@ -10,6 +10,8 @@ import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.springframework.context.ApplicationEventPublisher; +import org.springframework.transaction.event.TransactionPhase; +import org.springframework.transaction.event.TransactionalEventListener; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.event.WebAuthnCredentialRegisteredEvent; import com.digitalsanctuary.spring.user.persistence.model.User; @@ -49,7 +51,7 @@ private WebAuthnCredentialRegistrationListener listener() { @Test @DisplayName("should record an audit event when a passkey is registered") void shouldRecordAuditEventWhenPasskeyRegistered() { - listener().onApplicationEvent(event()); + listener().onCredentialRegistered(event()); ArgumentCaptor captor = ArgumentCaptor.forClass(AuditEvent.class); verify(eventPublisher).publishEvent(captor.capture()); @@ -61,7 +63,7 @@ void shouldRecordAuditEventWhenPasskeyRegistered() { @Test @DisplayName("should notify the account owner by email when a passkey is registered") void shouldNotifyOwnerWhenPasskeyRegistered() { - listener().onApplicationEvent(event()); + listener().onCredentialRegistered(event()); verify(userEmailService).sendPasskeyRegisteredNotification(user, "Work Laptop"); } @@ -72,7 +74,7 @@ void shouldStillAuditWhenNotificationDisabled() { // The email is a courtesy the operator may not want; the audit trail is not optional. config.setNotifyOnRegistration(false); - listener().onApplicationEvent(event()); + listener().onCredentialRegistered(event()); verify(userEmailService, never()).sendPasskeyRegisteredNotification(any(), any()); verify(eventPublisher).publishEvent(any(AuditEvent.class)); @@ -85,8 +87,23 @@ void shouldAuditEvenWhenNotificationFails() { org.mockito.Mockito.doThrow(new RuntimeException("smtp down")).when(userEmailService) .sendPasskeyRegisteredNotification(any(), any()); - listener().onApplicationEvent(event()); + listener().onCredentialRegistered(event()); verify(eventPublisher).publishEvent(any(AuditEvent.class)); } + + @Test + @DisplayName("should react only after the enrollment transaction commits") + void shouldReactAfterCommit() throws NoSuchMethodException { + // The credential is written through a @Transactional save() that publishes the event before commit. Reacting + // after commit stops a rolled-back registration (e.g. a label too long for the column) from emailing the + // owner and recording an audit entry for a passkey that never persisted. + TransactionalEventListener annotation = WebAuthnCredentialRegistrationListener.class + .getMethod("onCredentialRegistered", WebAuthnCredentialRegisteredEvent.class) + .getAnnotation(TransactionalEventListener.class); + + assertThat(annotation).isNotNull(); + assertThat(annotation.phase()).isEqualTo(TransactionPhase.AFTER_COMMIT); + assertThat(annotation.fallbackExecution()).isTrue(); + } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java index 8c6d16b..2f01101 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java @@ -750,6 +750,49 @@ void initiateAdminPasswordReset_deprecated_hasPreAuthorizeAnnotation() throws No } } + @Nested + @DisplayName("Passkey Registration Notification Tests") + class PasskeyRegistrationNotificationTests { + + @Test + @DisplayName("sendPasskeyRegisteredNotification - HTML-escapes a crafted label before it reaches the template") + void sendPasskeyRegisteredNotification_escapesLabel() { + // The label is client-supplied at enrollment and the template renders it with th:utext. An unescaped + // label would inject live HTML into the very email that warns the owner of an unrecognized passkey. + userEmailService.sendPasskeyRegisteredNotification(testUser, "remove"); + + ArgumentCaptor> variablesCaptor = ArgumentCaptor.forClass(Map.class); + verify(mailService).sendTemplateMessage( + eq(testUser.getEmail()), + eq("New passkey added to your account"), + variablesCaptor.capture(), + eq("mail/webauthn-credential-registered.html")); + + String label = (String) variablesCaptor.getValue().get("label"); + assertThat(label).doesNotContain("> variablesCaptor = ArgumentCaptor.forClass(Map.class); + verify(mailService).sendTemplateMessage(any(), any(), variablesCaptor.capture(), any()); + assertThat(variablesCaptor.getValue().get("label")).isEqualTo("Passkey"); + } + + @Test + @DisplayName("sendPasskeyRegisteredNotification - is a no-op when the user has no email") + void sendPasskeyRegisteredNotification_noRecipientNoOp() { + User noEmail = UserTestDataBuilder.aUser().withEmail(null).build(); + + userEmailService.sendPasskeyRegisteredNotification(noEmail, "Work Laptop"); + + verify(mailService, never()).sendTemplateMessage(anyString(), anyString(), any(Map.class), anyString()); + } + } + @Nested @DisplayName("Deprecated Method Tests") class DeprecatedMethodTests { From 0acfda77d98dc53fa14ff06c9f2347d54c5e0e7d Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 21 Aug 2026 15:33:30 -0600 Subject: [PATCH 2/3] fix: return 401 step-up-required JSON on passkey enrollment denial The enrollment gate is a filter-chain authorizeHttpRequests rule, so its denial is raised before any controller and never reaches WebAuthnManagementAPIAdvice. It therefore returned a bare 403, the only one of the four step-up-gated operations not returning 401 + JSON {"error":"step-up-required"}, leaving a client unable to tell "re-run your login ceremony and retry" from an ordinary authorization failure. Add StepUpEnrollmentAccessDeniedHandler, scoped to POST /webauthn/register via defaultAccessDeniedHandlerFor, that renders an AuthorizationDeniedException as 401 step-up-required and delegates everything else (e.g. CSRF) to the default handler. Scoping leaves the existing MFA missing-authority handler in place for every other path. The enforcement mechanism (the authorization manager) is unchanged; only the denial response is. Updates WebAuthnEnrollmentGateIntegrationTest to assert the 401 + step-up-required contract and to prove the fresh-factor case is not step-up-denied. Claude-Session: https://claude.ai/code/session_01KL5qvKVGQLLQDjHToy34vj --- .../StepUpEnrollmentAccessDeniedHandler.java | 50 +++++++++++++++++++ .../user/security/WebSecurityConfig.java | 13 +++++ ...WebAuthnEnrollmentGateIntegrationTest.java | 15 ++++-- 3 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java new file mode 100644 index 0000000..eb021f1 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java @@ -0,0 +1,50 @@ +package com.digitalsanctuary.spring.user.security; + +import java.io.IOException; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.security.authorization.AuthorizationDeniedException; +import org.springframework.security.web.access.AccessDeniedHandler; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnStepUpRequiredException; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import lombok.RequiredArgsConstructor; + +/** + * Renders a step-up denial on the passkey enrollment endpoint the way the credential-management endpoints do: HTTP + * 401 with a JSON body carrying the {@code step-up-required} error code, so a client can launch its login ceremony + * and retry rather than receiving a bare 403 it cannot interpret. + * + *

+ * The enrollment gate is a filter-chain {@code authorizeHttpRequests} rule, so its denial is raised before any + * controller runs and never reaches {@code WebAuthnManagementAPIAdvice}. Only a freshness denial (an + * {@link AuthorizationDeniedException}) is treated as step-up; anything else that can deny the endpoint, such as a + * CSRF failure, is passed to the delegate so it keeps its normal 403. + *

+ */ +@RequiredArgsConstructor +public class StepUpEnrollmentAccessDeniedHandler implements AccessDeniedHandler { + + /** Handles denials on the endpoint that are not the freshness gate (for example, CSRF). */ + private final AccessDeniedHandler delegate; + + @Override + public void handle(HttpServletRequest request, HttpServletResponse response, AccessDeniedException accessDeniedException) + throws IOException, ServletException { + if (!(accessDeniedException instanceof AuthorizationDeniedException)) { + delegate.handle(request, response, accessDeniedException); + return; + } + response.setStatus(HttpStatus.UNAUTHORIZED.value()); + response.setContentType(MediaType.APPLICATION_JSON_VALUE); + response.setCharacterEncoding("UTF-8"); + // The message and error code are compile-time constants with no characters that need JSON escaping, so the + // body is assembled directly. This keeps the handler independent of whichever Jackson version the consuming + // application ships, matching the {message, error} shape GenericResponse serializes for the sibling endpoints. + response.getWriter().write("{\"message\":\"Recent authentication is required to add a passkey. " + + "Please verify with your passkey or password and retry.\",\"error\":\"" + WebAuthnStepUpRequiredException.ERROR_CODE + + "\"}"); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index aa71d66..f54d4bd 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -22,10 +22,14 @@ import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.web.AuthenticationEntryPoint; import org.springframework.security.web.SecurityFilterChain; +import org.springframework.security.web.access.AccessDeniedHandler; +import org.springframework.security.web.access.AccessDeniedHandlerImpl; import org.springframework.security.web.access.DelegatingMissingAuthorityAccessDeniedHandler; import org.springframework.security.web.authentication.LoginUrlAuthenticationEntryPoint; import org.springframework.security.web.authentication.rememberme.PersistentTokenRepository; import org.springframework.security.web.savedrequest.RequestCache; +import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; import org.springframework.security.web.webauthn.authentication.WebAuthnAuthenticationFilter; import com.digitalsanctuary.spring.user.service.DSOAuth2UserService; import com.digitalsanctuary.spring.user.service.DSOidcUserService; @@ -195,6 +199,15 @@ public SecurityFilterChain buildSecurityFilterChain(HttpSecurity http, SessionRe Duration.ofSeconds(stepUpConfigProperties.getEnrollmentTtlSeconds()), Clock.systemUTC()); http.authorizeHttpRequests((authorize) -> authorize .requestMatchers(HttpMethod.POST, "/webauthn/register").access(enrollmentGate)); + + // The gate denies in the filter chain, before any controller, so its denial never reaches + // WebAuthnManagementAPIAdvice. Render it the way the credential-management endpoints render step-up: + // HTTP 401 with the step-up-required error code, instead of a bare 403 the client cannot interpret. + // Scoped to this endpoint via defaultAccessDeniedHandlerFor, so Spring keeps the existing default + // handler (the MFA missing-authority handler when configured) for every other path. + RequestMatcher enrollmentPost = PathPatternRequestMatcher.withDefaults().matcher(HttpMethod.POST, "/webauthn/register"); + AccessDeniedHandler enrollmentDeniedHandler = new StepUpEnrollmentAccessDeniedHandler(new AccessDeniedHandlerImpl()); + http.exceptionHandling(handling -> handling.defaultAccessDeniedHandlerFor(enrollmentDeniedHandler, enrollmentPost)); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java index 8389959..9f1e84b 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java @@ -4,6 +4,7 @@ import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import java.time.Instant; import java.util.List; @@ -48,26 +49,30 @@ private static Authentication withFactor(Instant issuedAt) { @DisplayName("should reject passkey enrollment when the session carries no authentication factor") void shouldRejectEnrollmentWithoutFactor() throws Exception { // The attack this closes: a stolen session cookie enrolls an attacker-controlled passkey, asserts with it to - // mint a fresh FACTOR_WEBAUTHN, and thereby satisfies every step-up gate. + // mint a fresh FACTOR_WEBAUTHN, and thereby satisfies every step-up gate. The denial returns the same + // 401 + step-up-required contract as the credential-management endpoints, not a bare 403. mockMvc.perform(post("/webauthn/register").with(user("user@test.com").roles("USER")).with(csrf()) - .contentType(MediaType.APPLICATION_JSON).content("{}")).andExpect(status().isForbidden()); + .contentType(MediaType.APPLICATION_JSON).content("{}")).andExpect(status().isUnauthorized()) + .andExpect(jsonPath("$.error").value("step-up-required")); } @Test @DisplayName("should reject passkey enrollment when the only factor is older than the window") void shouldRejectEnrollmentWithStaleFactor() throws Exception { mockMvc.perform(post("/webauthn/register").with(authentication(withFactor(Instant.now().minusSeconds(601)))) - .with(csrf()).contentType(MediaType.APPLICATION_JSON).content("{}")).andExpect(status().isForbidden()); + .with(csrf()).contentType(MediaType.APPLICATION_JSON).content("{}")).andExpect(status().isUnauthorized()) + .andExpect(jsonPath("$.error").value("step-up-required")); } @Test @DisplayName("should not reject passkey enrollment when a factor was issued inside the window") void shouldAllowEnrollmentWithFreshFactor() throws Exception { // The request body is not a real attestation, so the endpoint itself fails it. What matters here is that the - // gate let it through: anything other than 403 proves authorization passed and the filter ran. + // gate let it through: neither the 401 the gate now returns on denial nor a 403 proves authorization passed + // and the filter ran. mockMvc.perform(post("/webauthn/register").with(authentication(withFactor(Instant.now().minusSeconds(5)))) .with(csrf()).contentType(MediaType.APPLICATION_JSON).content("{}")) - .andExpect(status().is(not(403))); + .andExpect(status().is(not(401))).andExpect(status().is(not(403))); } private static org.hamcrest.Matcher not(int status) { From 3d96b5fcd58a93f11cc005ad6fdf2952a440f1fb Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 21 Aug 2026 16:06:41 -0600 Subject: [PATCH 3/3] fix: scope enrollment step-up access-denied handler and harden listener Follow-up from a five-agent review of #371. The step-up enrollment denial handler was not actually scoped to POST /webauthn/register, plus smaller hardening the review surfaced. - Scope the step-up denial handler correctly. A lone defaultAccessDeniedHandlerFor(handler, matcher) does not scope in Spring Security 7.1: with a single mapping and no explicit accessDeniedHandler, ExceptionHandlingConfigurer.createDefaultAccessDeniedHandler returns the handler directly and discards the matcher, making the step-up handler the app-wide default (every AuthorizationDeniedException, including the DELETE /webauthn/register/** denyAll and deny-mode anyRequest, returned 401 step-up-required); and when MFA installs its own handler the mapping is ignored outright, making the feature a no-op. setupMfa now returns its DelegatingMissingAuthorityAccessDeniedHandler instead of installing it, and the access-denied handler is built once as a RequestMatcherDelegatingAccessDeniedHandler: enrollment POST -> step-up handler, everything else -> the MFA handler when configured, else 403. - Guard the audit publish in WebAuthnCredentialRegistrationListener. It is the path the method comment calls "unconditional," but after AFTER_COMMIT an exception there is swallowed by Spring's adapter with no trace. Wrap it in the same try/catch + log the mail path already has. - StepUpEnrollmentAccessDeniedHandler: enforce the non-null delegate with @NonNull (was a deferred NPE), skip writing when the response is already committed, and document that the AuthorizationDeniedException-is-step-up classification holds only while the freshness gate is the sole authz rule on the path. Tests: - New StepUpEnrollmentAccessDeniedHandlerTest covers both branches (401 step-up JSON for AuthorizationDeniedException, delegate for other denials), the committed-response guard, and null-delegate rejection. - WebAuthnEnrollmentGateIntegrationTest adds two regression guards through the real filter chain: a denied DELETE /webauthn/register/** stays a bare 403 (not step-up), and a CSRF failure on the enrollment path stays 403. These fail against the old unscoped wiring. - Full suite: 1264 tests, 0 failures. Claude-Session: https://claude.ai/code/session_018iUyT6zqNZZpAdmzkXNwLW --- ...ebAuthnCredentialRegistrationListener.java | 14 ++- .../StepUpEnrollmentAccessDeniedHandler.java | 17 +++- .../user/security/WebSecurityConfig.java | 56 ++++++++---- ...epUpEnrollmentAccessDeniedHandlerTest.java | 87 +++++++++++++++++++ ...WebAuthnEnrollmentGateIntegrationTest.java | 28 ++++++ 5 files changed, 178 insertions(+), 24 deletions(-) create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandlerTest.java diff --git a/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java b/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java index a4190d1..c4c5ecf 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java +++ b/src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java @@ -47,10 +47,16 @@ public class WebAuthnCredentialRegistrationListener { @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT, fallbackExecution = true) public void onCredentialRegistered(WebAuthnCredentialRegisteredEvent event) { // Audit first and unconditionally: the notification is a courtesy the operator can switch off, and a mail - // outage must not cost us the security-relevant record of the enrollment. - eventPublisher.publishEvent(AuditEvent.builder().source(this).user(event.getUser()) - .action("PasskeyRegistration").actionStatus("Success") - .message("Passkey registered: " + event.getLabel()).build()); + // outage must not cost us the security-relevant record of the enrollment. Because this runs after commit, an + // exception here would be swallowed by Spring's transactional-listener adapter with no trace, so log it in this + // class rather than lose the record silently. + try { + eventPublisher.publishEvent(AuditEvent.builder().source(this).user(event.getUser()) + .action("PasskeyRegistration").actionStatus("Success") + .message("Passkey registered: " + event.getLabel()).build()); + } catch (RuntimeException e) { + log.error("Failed to record passkey registration audit event for user {}", event.getUser().getId(), e); + } if (!webAuthnConfigProperties.isNotifyOnRegistration()) { return; diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java index eb021f1..0a4277f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandler.java @@ -10,6 +10,7 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import lombok.NonNull; import lombok.RequiredArgsConstructor; /** @@ -23,11 +24,20 @@ * {@link AuthorizationDeniedException}) is treated as step-up; anything else that can deny the endpoint, such as a * CSRF failure, is passed to the delegate so it keeps its normal 403. *

+ * + *

+ * The step-up classification is by exception type, and it is sound only because the freshness gate is the sole + * authorization rule on {@code POST /webauthn/register}. If another {@code authorizeHttpRequests} rule (a role or + * scope check) is ever added to that path, its denial is also an {@link AuthorizationDeniedException} and would be + * mislabeled as step-up, telling the client to re-authenticate when re-authentication cannot help. Keep this handler + * scoped to a path whose only authorization rule is the freshness gate. + *

*/ @RequiredArgsConstructor public class StepUpEnrollmentAccessDeniedHandler implements AccessDeniedHandler { - /** Handles denials on the endpoint that are not the freshness gate (for example, CSRF). */ + /** Handles denials on the endpoint that are not the freshness gate (for example, CSRF). Required (non-null). */ + @NonNull private final AccessDeniedHandler delegate; @Override @@ -37,6 +47,11 @@ public void handle(HttpServletRequest request, HttpServletResponse response, Acc delegate.handle(request, response, accessDeniedException); return; } + // A prior filter having already committed the response would make setStatus/setContentType silently no-ops and + // append the JSON to whatever was flushed, so bail rather than emit a malformed body. + if (response.isCommitted()) { + return; + } response.setStatus(HttpStatus.UNAUTHORIZED.value()); response.setContentType(MediaType.APPLICATION_JSON_VALUE); response.setCharacterEncoding("UTF-8"); diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index f54d4bd..5346fd9 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -4,6 +4,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -25,6 +26,7 @@ import org.springframework.security.web.access.AccessDeniedHandler; import org.springframework.security.web.access.AccessDeniedHandlerImpl; import org.springframework.security.web.access.DelegatingMissingAuthorityAccessDeniedHandler; +import org.springframework.security.web.access.RequestMatcherDelegatingAccessDeniedHandler; import org.springframework.security.web.authentication.LoginUrlAuthenticationEntryPoint; import org.springframework.security.web.authentication.rememberme.PersistentTokenRepository; import org.springframework.security.web.savedrequest.RequestCache; @@ -172,10 +174,11 @@ public SecurityFilterChain buildSecurityFilterChain(HttpSecurity http, SessionRe setupWebAuthn(http); } - // Configure MFA if enabled - if (mfaConfigProperties.isEnabled()) { - setupMfa(http); - } + // Configure MFA if enabled. setupMfa returns its missing-authority handler instead of installing it, so the + // WebAuthn enrollment step-up handler below can compose over it. The access-denied handler is installed once, + // after both features have been configured. + DelegatingMissingAuthorityAccessDeniedHandler mfaAccessDeniedHandler = + mfaConfigProperties.isEnabled() ? setupMfa() : null; // Close Spring Security's built-in WebAuthn credential-delete endpoint (DELETE /webauthn/register/{id}), // registered by http.webAuthn(...). It deletes a passkey after checking only credential ownership, bypassing @@ -199,18 +202,32 @@ public SecurityFilterChain buildSecurityFilterChain(HttpSecurity http, SessionRe Duration.ofSeconds(stepUpConfigProperties.getEnrollmentTtlSeconds()), Clock.systemUTC()); http.authorizeHttpRequests((authorize) -> authorize .requestMatchers(HttpMethod.POST, "/webauthn/register").access(enrollmentGate)); - - // The gate denies in the filter chain, before any controller, so its denial never reaches - // WebAuthnManagementAPIAdvice. Render it the way the credential-management endpoints render step-up: - // HTTP 401 with the step-up-required error code, instead of a bare 403 the client cannot interpret. - // Scoped to this endpoint via defaultAccessDeniedHandlerFor, so Spring keeps the existing default - // handler (the MFA missing-authority handler when configured) for every other path. - RequestMatcher enrollmentPost = PathPatternRequestMatcher.withDefaults().matcher(HttpMethod.POST, "/webauthn/register"); - AccessDeniedHandler enrollmentDeniedHandler = new StepUpEnrollmentAccessDeniedHandler(new AccessDeniedHandlerImpl()); - http.exceptionHandling(handling -> handling.defaultAccessDeniedHandlerFor(enrollmentDeniedHandler, enrollmentPost)); } } + // Install the access-denied handler once, composing the feature handlers so each path keeps its correct denial + // response. The enrollment gate denies in the filter chain, before any controller, so its denial never reaches + // WebAuthnManagementAPIAdvice; render it the way the credential-management endpoints render step-up (HTTP 401 + // with the step-up-required error code) instead of a bare 403 the client cannot interpret. This must be built + // explicitly rather than with a single defaultAccessDeniedHandlerFor mapping: Spring collapses a lone mapping to + // the handler alone and drops the matcher (ExceptionHandlingConfigurer.createDefaultAccessDeniedHandler), which + // would make the step-up handler the app-wide default; and when MFA sets its own handler, the mapping is ignored + // outright. So compose a RequestMatcherDelegatingAccessDeniedHandler: enrollment POST -> step-up handler, + // everything else -> the MFA missing-authority handler when configured, else a plain 403. + AccessDeniedHandler baseAccessDeniedHandler = + mfaAccessDeniedHandler != null ? mfaAccessDeniedHandler : new AccessDeniedHandlerImpl(); + if (webAuthnConfigProperties.isEnabled() && stepUpConfigProperties.isEnabled()) { + RequestMatcher enrollmentPost = + PathPatternRequestMatcher.withDefaults().matcher(HttpMethod.POST, "/webauthn/register"); + LinkedHashMap deniedHandlers = new LinkedHashMap<>(); + deniedHandlers.put(enrollmentPost, new StepUpEnrollmentAccessDeniedHandler(baseAccessDeniedHandler)); + AccessDeniedHandler enrollmentAwareHandler = + new RequestMatcherDelegatingAccessDeniedHandler(deniedHandlers, baseAccessDeniedHandler); + http.exceptionHandling(handling -> handling.accessDeniedHandler(enrollmentAwareHandler)); + } else if (mfaAccessDeniedHandler != null) { + http.exceptionHandling(handling -> handling.accessDeniedHandler(mfaAccessDeniedHandler)); + } + // Configure authorization rules based on the default action String defaultAction = userSecurityConfig.getDefaultAction(); if (DEFAULT_ACTION_DENY.equals(defaultAction)) { @@ -276,14 +293,15 @@ private void setupWebAuthn(HttpSecurity http) throws Exception { /** * Setup MFA specific configuration. *

- * Configures a {@link DelegatingMissingAuthorityAccessDeniedHandler} that redirects partially-authenticated users to - * the appropriate factor login page when they are missing a required factor authority. + * Builds a {@link DelegatingMissingAuthorityAccessDeniedHandler} that redirects partially-authenticated users to the + * appropriate factor login page when they are missing a required factor authority. The caller installs the returned + * handler (composing it with the WebAuthn enrollment step-up handler when both features are enabled), so this method + * does not touch {@code http} itself. *

* - * @param http the http security object to configure - * @throws Exception the exception + * @return the missing-authority access-denied handler for the configured factors */ - private void setupMfa(HttpSecurity http) throws Exception { + private DelegatingMissingAuthorityAccessDeniedHandler setupMfa() { DelegatingMissingAuthorityAccessDeniedHandler.Builder handlerBuilder = DelegatingMissingAuthorityAccessDeniedHandler.builder(); @@ -300,8 +318,8 @@ private void setupMfa(HttpSecurity http) throws Exception { } DelegatingMissingAuthorityAccessDeniedHandler handler = handlerBuilder.build(); - http.exceptionHandling(handling -> handling.accessDeniedHandler(handler)); log.info("MFA configured with access denied handler for factors: {}", mfaConfigProperties.getFactors()); + return handler; } /** diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandlerTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandlerTest.java new file mode 100644 index 0000000..2cc93c9 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpEnrollmentAccessDeniedHandlerTest.java @@ -0,0 +1,87 @@ +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 static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.security.authorization.AuthorizationDeniedException; +import org.springframework.security.web.access.AccessDeniedHandler; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnStepUpRequiredException; + +/** + * Tests for {@link StepUpEnrollmentAccessDeniedHandler}, which must render only a freshness + * ({@link AuthorizationDeniedException}) denial as the 401 step-up-required contract and delegate everything else so it + * keeps its normal 403. + */ +@DisplayName("StepUpEnrollmentAccessDeniedHandler Tests") +class StepUpEnrollmentAccessDeniedHandlerTest { + + private AccessDeniedHandler delegate; + private StepUpEnrollmentAccessDeniedHandler handler; + + @BeforeEach + void setUp() { + delegate = mock(AccessDeniedHandler.class); + handler = new StepUpEnrollmentAccessDeniedHandler(delegate); + } + + @Test + @DisplayName("should render 401 step-up-required JSON when the denial is a freshness AuthorizationDeniedException") + void shouldRenderStepUpContractWhenAuthorizationDenied() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + handler.handle(request, response, new AuthorizationDeniedException("Access Denied")); + + assertThat(response.getStatus()).isEqualTo(401); + assertThat(response.getContentType()).contains("application/json"); + assertThat(response.getCharacterEncoding()).isEqualTo("UTF-8"); + String body = response.getContentAsString(); + assertThat(body).contains("\"error\":\"" + WebAuthnStepUpRequiredException.ERROR_CODE + "\""); + assertThat(body).contains("Recent authentication is required to add a passkey"); + verifyNoInteractions(delegate); + } + + @Test + @DisplayName("should delegate and leave the response untouched when the denial is not an AuthorizationDeniedException") + void shouldDelegateWhenNotAuthorizationDenied() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + // A CSRF failure is an AccessDeniedException but not an AuthorizationDeniedException; it must keep its 403. + AccessDeniedException csrfDenial = new AccessDeniedException("Invalid CSRF token"); + + handler.handle(request, response, csrfDenial); + + verify(delegate).handle(request, response, csrfDenial); + assertThat(response.getStatus()).isEqualTo(200); + assertThat(response.getContentAsString()).isEmpty(); + } + + @Test + @DisplayName("should not write the step-up body when the response is already committed") + void shouldReturnEarlyWhenResponseCommitted() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + response.setCommitted(true); + + handler.handle(request, response, new AuthorizationDeniedException("Access Denied")); + + assertThat(response.getStatus()).isEqualTo(200); + assertThat(response.getContentAsString()).isEmpty(); + verifyNoInteractions(delegate); + } + + @Test + @DisplayName("should reject a null delegate at construction rather than deferring the failure") + void shouldRejectNullDelegate() { + assertThatThrownBy(() -> new StepUpEnrollmentAccessDeniedHandler(null)) + .isInstanceOf(NullPointerException.class); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java index 9f1e84b..8cb1979 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java @@ -3,7 +3,9 @@ import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import java.time.Instant; @@ -75,6 +77,32 @@ void shouldAllowEnrollmentWithFreshFactor() throws Exception { .andExpect(status().is(not(401))).andExpect(status().is(not(403))); } + @Test + @DisplayName("should deny a non-enrollment path with a bare 403, not the step-up contract, while the gate is active") + void shouldNotApplyStepUpContractToNonEnrollmentDenials() throws Exception { + // Regression guard for the access-denied handler scoping (the reason StepUpEnrollmentAccessDeniedHandler is + // composed into a RequestMatcherDelegatingAccessDeniedHandler rather than installed via a lone + // defaultAccessDeniedHandlerFor mapping, which Spring silently unscopes). DELETE /webauthn/register/** is + // denyAll, so an authenticated user is denied there; that denial is an AuthorizationDeniedException too, and it + // must stay a bare 403 without the step-up-required body. If the step-up handler leaked to the app-wide default + // this would return 401 step-up-required instead. + mockMvc.perform(delete("/webauthn/register/some-credential-id").with(user("user@test.com").roles("USER")).with(csrf())) + .andExpect(status().isForbidden()) + .andExpect(content().string(org.hamcrest.Matchers.not(org.hamcrest.Matchers.containsString("step-up-required")))); + } + + @Test + @DisplayName("should return a bare 403 for a CSRF failure on the enrollment path, not the step-up contract") + void shouldDelegateCsrfFailureOnEnrollmentPath() throws Exception { + // A CSRF failure is an AccessDeniedException but not an AuthorizationDeniedException, so the enrollment handler + // must delegate it and keep the normal 403 rather than telling the client to re-run a login ceremony that + // cannot fix a missing CSRF token. Posting without csrf() triggers the CSRF filter before the authorization gate. + mockMvc.perform(post("/webauthn/register").with(user("user@test.com").roles("USER")) + .contentType(MediaType.APPLICATION_JSON).content("{}")) + .andExpect(status().isForbidden()) + .andExpect(content().string(org.hamcrest.Matchers.not(org.hamcrest.Matchers.containsString("step-up-required")))); + } + private static org.hamcrest.Matcher not(int status) { return org.hamcrest.Matchers.not(org.hamcrest.Matchers.is(status)); }