fix(security): address second-pass review (session DoS, NPE, log injection)

- SanitizingOAuth2AuthenticationFailureHandler: use getSession(false) so a
  failed OAuth2 callback does not allocate a session for sessionless callers
  (prevents unauthenticated scanners from forcing session creation). A real
  OAuth2 flow already has a session, so the user-facing message is preserved.
- UserVerificationService.generateNewVerificationToken: guard against a null
  resolved token (unknown/consumed/malformed) and throw IllegalArgumentException
  instead of NPEing on updateToken.
- TokenHasher.fingerprint: strip control chars (CR/LF) from the logged prefix
  to close CodeQL log-injection on token-derived log lines.
- MfaFilterMergingConfiguration: convert tab indentation to 4 spaces per
  CLAUDE.md style.
- Tests: model the real (session-present) OAuth2 callback and add a test
  asserting no session is created for a sessionless request.

Author: devondragon

src/main/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfiguration.java

@@ -39,36 +39,36 @@
 @ConditionalOnProperty(name = "user.mfa.enabled", havingValue = "true", matchIfMissing = false)
 public class MfaFilterMergingConfiguration {
 
-	/**
-	 * Replicates the behaviour of {@code @EnableMultiFactorAuthentication}'s internal {@code EnableMfaFiltersPostProcessor}
-	 * using only public API, by invoking the public {@link AbstractAuthenticationProcessingFilter#setMfaEnabled(boolean)} on
-	 * every authentication processing filter. Without this, completing a second factor would REPLACE the first factor's
-	 * authentication (dropping its authority) and the user could never satisfy all required factors (the H4 lockout).
-	 *
-	 * <p>
-	 * Declared {@code static} so the post-processor can be registered without eagerly instantiating this configuration
-	 * class. The bean exists only when {@code user.mfa.enabled=true}, so the default (no-MFA) login path is unaffected.
-	 * </p>
-	 *
-	 * @return a {@link BeanPostProcessor} that calls {@code setMfaEnabled(true)} on authentication processing filters
-	 */
-	@Bean
-	public static BeanPostProcessor mfaFilterMergingPostProcessor() {
-		return new BeanPostProcessor() {
-			@Override
-			public Object postProcessAfterInitialization(Object bean, String beanName) {
-				// Intentionally scoped to AbstractAuthenticationProcessingFilter, which covers every authentication
-				// mechanism this framework configures: formLogin, webAuthn, and oauth2Login all extend it. SS's internal
-				// EnableMfaFiltersPostProcessor additionally flips the flag on AuthenticationFilter,
-				// BasicAuthenticationFilter, and pre-authentication filters; this framework does not configure those
-				// mechanisms, so they are deliberately not targeted here. See the class-level WARNING regarding
-				// consumer-defined filters that extend this base class.
-				if (bean instanceof AbstractAuthenticationProcessingFilter filter) {
-					filter.setMfaEnabled(true);
-					log.debug("MFA factor merging enabled on filter: {}", bean.getClass().getName());
-				}
-				return bean;
-			}
-		};
-	}
+    /**
+     * Replicates the behaviour of {@code @EnableMultiFactorAuthentication}'s internal {@code EnableMfaFiltersPostProcessor}
+     * using only public API, by invoking the public {@link AbstractAuthenticationProcessingFilter#setMfaEnabled(boolean)} on
+     * every authentication processing filter. Without this, completing a second factor would REPLACE the first factor's
+     * authentication (dropping its authority) and the user could never satisfy all required factors (the H4 lockout).
+     *
+     * <p>
+     * Declared {@code static} so the post-processor can be registered without eagerly instantiating this configuration
+     * class. The bean exists only when {@code user.mfa.enabled=true}, so the default (no-MFA) login path is unaffected.
+     * </p>
+     *
+     * @return a {@link BeanPostProcessor} that calls {@code setMfaEnabled(true)} on authentication processing filters
+     */
+    @Bean
+    public static BeanPostProcessor mfaFilterMergingPostProcessor() {
+        return new BeanPostProcessor() {
+            @Override
+            public Object postProcessAfterInitialization(Object bean, String beanName) {
+                // Intentionally scoped to AbstractAuthenticationProcessingFilter, which covers every authentication
+                // mechanism this framework configures: formLogin, webAuthn, and oauth2Login all extend it. SS's internal
+                // EnableMfaFiltersPostProcessor additionally flips the flag on AuthenticationFilter,
+                // BasicAuthenticationFilter, and pre-authentication filters; this framework does not configure those
+                // mechanisms, so they are deliberately not targeted here. See the class-level WARNING regarding
+                // consumer-defined filters that extend this base class.
+                if (bean instanceof AbstractAuthenticationProcessingFilter filter) {
+                    filter.setMfaEnabled(true);
+                    log.debug("MFA factor merging enabled on filter: {}", bean.getClass().getName());
+                }
+                return bean;
+            }
+        };
+    }
 }

src/main/java/com/digitalsanctuary/spring/user/security/SanitizingOAuth2AuthenticationFailureHandler.java

@@ -8,6 +8,7 @@
 import jakarta.servlet.ServletException;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
+import jakarta.servlet.http.HttpSession;
 import lombok.extern.slf4j.Slf4j;
 
 /**
@@ -71,7 +72,14 @@ public void onAuthenticationFailure(HttpServletRequest request, HttpServletRespo
         log.debug("OAuth2 login failure detail", exception);
 
         // Store ONLY a generic, non-sensitive message for the UI. Never the raw exception message.
-        request.getSession().setAttribute(ERROR_MESSAGE_SESSION_ATTRIBUTE, resolveUserFacingMessage(exception));
+        // Use getSession(false) so we do not allocate a session for callers that do not already have one:
+        // a legitimate OAuth2 attempt establishes a session during the authorization-request phase, while an
+        // unauthenticated scanner hitting the callback cold has none and should not be able to force session
+        // creation. When there is no session the login page simply shows its default message.
+        HttpSession session = request.getSession(false);
+        if (session != null) {
+            session.setAttribute(ERROR_MESSAGE_SESSION_ATTRIBUTE, resolveUserFacingMessage(exception));
+        }
         response.sendRedirect(loginPageURI);
     }
 

src/main/java/com/digitalsanctuary/spring/user/service/TokenHasher.java

@@ -99,9 +99,14 @@ public String hash(final String rawToken) {
      * (followed by an ellipsis) for longer tokens. Intended purely for correlating log lines, not for
      * any security decision.
      *
+     * <p>
+     * The returned prefix is stripped of CR/LF and other control characters so that an attacker-supplied
+     * token (these values arrive as request parameters) cannot forge or split log lines (log injection).
+     * </p>
+     *
      * @param token the raw token (may be {@code null})
      * @return {@code "null"} if the token is {@code null}, {@code "****"} if it is 8 characters or fewer,
-     *         otherwise the first 6 characters followed by an ellipsis
+     *         otherwise the first 6 (control-character-stripped) characters followed by an ellipsis
      */
     public static String fingerprint(final String token) {
         if (token == null) {
@@ -110,7 +115,9 @@ public static String fingerprint(final String token) {
         if (token.length() <= 8) {
             return "****";
         }
-        return token.substring(0, 6) + "…";
+        // Strip control characters (incl. CR/LF) from the logged prefix to prevent log injection/forging.
+        final String prefix = token.substring(0, 6).replaceAll("\\p{Cntrl}", "");
+        return prefix + "…";
     }
 
     /**

src/main/java/com/digitalsanctuary/spring/user/service/UserVerificationService.java

@@ -115,6 +115,13 @@ public VerificationToken getVerificationToken(final String verificationToken) {
     @Transactional
     public VerificationToken generateNewVerificationToken(final String existingVerificationToken) {
         VerificationToken vToken = resolveByRawToken(existingVerificationToken);
+        if (vToken == null) {
+            // The supplied token does not resolve to a stored row (unknown, already consumed, or malformed).
+            // Fail explicitly rather than NPE on the update below.
+            log.warn("UserVerificationService.generateNewVerificationToken: no token found for {}",
+                    TokenHasher.fingerprint(existingVerificationToken));
+            throw new IllegalArgumentException("No verification token found for the supplied value");
+        }
         final String rawToken = UUID.randomUUID().toString();
         // Store the hash of the new raw token; the raw value is what gets emailed to the user.
         vToken.updateToken(tokenHasher.hash(rawToken), verificationTokenValidityMinutes);

src/test/java/com/digitalsanctuary/spring/user/security/SanitizingOAuth2AuthenticationFailureHandlerTest.java

@@ -33,6 +33,9 @@ void setUp() {
     void shouldNotLeakRawMessageForLockedException() throws Exception {
         // Given - a LockedException whose message contains the account email (per Task 1.4)
         MockHttpServletRequest request = new MockHttpServletRequest();
+        // A real OAuth2 callback already has a session (the authorization request is stored there during the
+        // redirect phase), so the handler stores the user-facing message on the existing session.
+        request.getSession(true);
         MockHttpServletResponse response = new MockHttpServletResponse();
         LockedException raw = new LockedException("Account is locked for user secret.email@example.com");
 
@@ -55,6 +58,7 @@ void shouldNotLeakRawMessageForLockedException() throws Exception {
     void shouldNotLeakRawMessageForOAuth2Exception() throws Exception {
         // Given
         MockHttpServletRequest request = new MockHttpServletRequest();
+        request.getSession(true);
         MockHttpServletResponse response = new MockHttpServletResponse();
         OAuth2AuthenticationException raw = new OAuth2AuthenticationException(
                 new OAuth2Error("User Registered With Alternate Provider"),
@@ -75,6 +79,7 @@ void shouldNotLeakRawMessageForOAuth2Exception() throws Exception {
     void shouldMapEmailNotVerifiedToSpecificGenericMessage() throws Exception {
         // Given
         MockHttpServletRequest request = new MockHttpServletRequest();
+        request.getSession(true);
         MockHttpServletResponse response = new MockHttpServletResponse();
         OAuth2AuthenticationException raw = new OAuth2AuthenticationException(
                 new OAuth2Error("email_not_verified"), "Email verified=false for victim@example.com");
@@ -88,4 +93,20 @@ void shouldMapEmailNotVerifiedToSpecificGenericMessage() throws Exception {
         assertThat(stored).isEqualTo(SanitizingOAuth2AuthenticationFailureHandler.EMAIL_NOT_VERIFIED_MESSAGE);
         assertThat(response.getRedirectedUrl()).isEqualTo(LOGIN_PAGE_URI);
     }
+
+    @Test
+    @DisplayName("Should NOT allocate a session for a sessionless request (no session forced by scanners)")
+    void shouldNotCreateSessionWhenNonePresent() throws Exception {
+        // Given - a cold request with no existing session (e.g. an unauthenticated scanner hitting the callback)
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        MockHttpServletResponse response = new MockHttpServletResponse();
+        LockedException raw = new LockedException("Account is locked for user secret.email@example.com");
+
+        // When
+        handler.onAuthenticationFailure(request, response, raw);
+
+        // Then - no session is created (the handler uses getSession(false)) and the redirect still happens.
+        assertThat(request.getSession(false)).isNull();
+        assertThat(response.getRedirectedUrl()).isEqualTo(LOGIN_PAGE_URI);
+    }
 }