From 5cbf01864d228f3ff4ae55ed9fcb624fed31836b Mon Sep 17 00:00:00 2001
From: "detail-app[bot]" <180357370+detail-app[bot]@users.noreply.github.com>
Date: Sun, 6 Sep 2026 14:00:47 +0000
Subject: [PATCH] fix(security): route non-CSRF access denials to a generic 403
 handler

SecurityConfig wired CsrfAccessDeniedHandler as the catch-all
AccessDeniedHandler, so every authenticated non-CSRF AccessDeniedException
(including controller-thrown denials) was serialized as a false "CSRF token
missing or invalid" 403. Install a DelegatingAccessDeniedHandler that routes
MissingCsrfTokenException/InvalidCsrfTokenException to the CSRF handler and
all other denials to a new GenericAccessDeniedHandler emitting a generic
"Access denied." JSON 403, so non-CSRF failures no longer claim a CSRF
failure while CSRF messaging is preserved.
---
 config/spotbugs/spotbugs-exclude.xml          |  10 ++
 .../security/GenericAccessDeniedHandler.java  |  66 +++++++++++
 .../javachat/config/SecurityConfig.java       |  16 ++-
 .../GenericAccessDeniedHandlerTest.java       |  64 ++++++++++
 .../web/RevokeApiKeyAuthIntegrationTest.java  | 112 ++++++++++++++++++
 5 files changed, 267 insertions(+), 1 deletion(-)
 create mode 100644 src/main/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandler.java
 create mode 100644 src/test/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandlerTest.java
 create mode 100644 src/test/java/com/williamcallahan/javachat/web/RevokeApiKeyAuthIntegrationTest.java

diff --git a/config/spotbugs/spotbugs-exclude.xml b/config/spotbugs/spotbugs-exclude.xml
index fd34e59a..76673913 100644
--- a/config/spotbugs/spotbugs-exclude.xml
+++ b/config/spotbugs/spotbugs-exclude.xml
@@ -380,6 +380,16 @@
         <Class name="com.williamcallahan.javachat.config.SecurityConfigTest"/>
     </Match>
 
+    <!--
+        RevokeApiKeyAuthIntegrationTest reads the XSRF-TOKEN cookie issued by /api/security/csrf
+        to attach a valid CSRF token while exercising the revoke-endpoint authorization paths; it
+        does not persist application secrets in cookies.
+    -->
+    <Match>
+        <Bug pattern="COOKIE_USAGE"/>
+        <Class name="com.williamcallahan.javachat.web.RevokeApiKeyAuthIntegrationTest"/>
+    </Match>
+
     <!--
         Manual extraction runners walk repository-owned documentation roots projected from the
         canonical source manifests. They do not accept filesystem paths from runtime users.
diff --git a/src/main/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandler.java b/src/main/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandler.java
new file mode 100644
index 00000000..05e0d4c9
--- /dev/null
+++ b/src/main/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandler.java
@@ -0,0 +1,66 @@
+package com.williamcallahan.javachat.adapters.in.web.security;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.williamcallahan.javachat.domain.errors.ApiErrorResponse;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.Objects;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.MediaType;
+import org.springframework.security.access.AccessDeniedException;
+import org.springframework.security.web.access.AccessDeniedHandler;
+
+/**
+ * Returns a generic JSON 403 response for non-CSRF access denials.
+ *
+ * <p>Installed as the default branch of the composed {@link AccessDeniedHandler}
+ * wired in {@code SecurityConfig}, so authenticated callers denied for non-CSRF
+ * reasons (for example a controller-thrown {@link AccessDeniedException}) no
+ * longer receive the CSRF-specific message emitted by
+ * {@link CsrfAccessDeniedHandler}. CSRF failures keep their tailored messaging
+ * because {@code SecurityConfig} routes {@code MissingCsrfTokenException} and
+ * {@code InvalidCsrfTokenException} to {@link CsrfAccessDeniedHandler} before
+ * this default handler is reached.
+ */
+public final class GenericAccessDeniedHandler implements AccessDeniedHandler {
+    private static final String ACCESS_DENIED_MESSAGE = "Access denied.";
+
+    private final ObjectMapper objectMapper;
+
+    /**
+     * Creates the handler using the shared ObjectMapper for JSON serialization.
+     *
+     * @param objectMapper Spring-managed JSON mapper
+     */
+    public GenericAccessDeniedHandler(ObjectMapper objectMapper) {
+        this.objectMapper = Objects.requireNonNull(objectMapper, "objectMapper");
+    }
+
+    /**
+     * Writes a generic JSON 403 response that does not claim a CSRF failure.
+     *
+     * @param httpRequest incoming HTTP request
+     * @param httpResponse outgoing HTTP response
+     * @param accessDeniedException access denied exception from Spring Security
+     * @throws IOException when the response cannot be written
+     * @throws ServletException when the servlet container rejects the write
+     */
+    @Override
+    public void handle(
+            HttpServletRequest httpRequest,
+            HttpServletResponse httpResponse,
+            AccessDeniedException accessDeniedException)
+            throws IOException, ServletException {
+        if (httpResponse.isCommitted()) {
+            return;
+        }
+
+        ApiErrorResponse accessDeniedError = ApiErrorResponse.error(ACCESS_DENIED_MESSAGE);
+
+        httpResponse.setStatus(HttpStatus.FORBIDDEN.value());
+        httpResponse.setContentType(MediaType.APPLICATION_JSON_VALUE);
+        objectMapper.writeValue(httpResponse.getOutputStream(), accessDeniedError);
+    }
+}
diff --git a/src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java b/src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java
index f0d9bcb3..e18cd725 100644
--- a/src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java
+++ b/src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java
@@ -4,7 +4,9 @@
 import com.williamcallahan.javachat.adapters.in.web.security.ClerkApiKeyAuthenticationFilter;
 import com.williamcallahan.javachat.adapters.in.web.security.ClerkAuthorizedPartyValidator;
 import com.williamcallahan.javachat.adapters.in.web.security.CsrfAccessDeniedHandler;
+import com.williamcallahan.javachat.adapters.in.web.security.GenericAccessDeniedHandler;
 import com.williamcallahan.javachat.adapters.out.clerk.ClerkApiKeyVerifier;
+import java.util.LinkedHashMap;
 import java.util.List;
 import org.springframework.beans.factory.ObjectProvider;
 import org.springframework.boot.actuate.autoconfigure.security.servlet.EndpointRequest;
@@ -14,6 +16,7 @@
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.core.annotation.Order;
+import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
 import org.springframework.security.oauth2.core.DelegatingOAuth2TokenValidator;
 import org.springframework.security.oauth2.core.OAuth2TokenValidator;
@@ -24,8 +27,12 @@
 import org.springframework.security.oauth2.server.resource.web.DefaultBearerTokenResolver;
 import org.springframework.security.oauth2.server.resource.web.authentication.BearerTokenAuthenticationFilter;
 import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.security.web.access.AccessDeniedHandler;
+import org.springframework.security.web.access.DelegatingAccessDeniedHandler;
 import org.springframework.security.web.csrf.CookieCsrfTokenRepository;
 import org.springframework.security.web.csrf.CsrfTokenRequestAttributeHandler;
+import org.springframework.security.web.csrf.InvalidCsrfTokenException;
+import org.springframework.security.web.csrf.MissingCsrfTokenException;
 import org.springframework.web.cors.CorsConfiguration;
 import org.springframework.web.cors.CorsConfigurationSource;
 import org.springframework.web.cors.UrlBasedCorsConfigurationSource;
@@ -118,7 +125,14 @@ public SecurityFilterChain appSecurityFilterChain(
         CookieCsrfTokenRepository csrfTokenRepository = CookieCsrfTokenRepository.withHttpOnlyFalse();
         csrfTokenRepository.setCookieCustomizer(csrfCookie -> csrfCookie.sameSite("Lax"));
         CsrfTokenRequestAttributeHandler requestHandler = new CsrfTokenRequestAttributeHandler();
-        CsrfAccessDeniedHandler accessDeniedHandler = new CsrfAccessDeniedHandler(objectMapper);
+        AccessDeniedHandler csrfHandler = new CsrfAccessDeniedHandler(objectMapper);
+        AccessDeniedHandler genericHandler = new GenericAccessDeniedHandler(objectMapper);
+        LinkedHashMap<Class<? extends AccessDeniedException>, AccessDeniedHandler> accessDeniedHandlers =
+                new LinkedHashMap<>();
+        accessDeniedHandlers.put(MissingCsrfTokenException.class, csrfHandler);
+        accessDeniedHandlers.put(InvalidCsrfTokenException.class, csrfHandler);
+        AccessDeniedHandler accessDeniedHandler =
+                new DelegatingAccessDeniedHandler(accessDeniedHandlers, genericHandler);
 
         http.cors(c -> c.configurationSource(corsConfigurationSource))
                 .csrf(csrf -> csrf.csrfTokenRepository(csrfTokenRepository)
diff --git a/src/test/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandlerTest.java b/src/test/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandlerTest.java
new file mode 100644
index 00000000..d48c9969
--- /dev/null
+++ b/src/test/java/com/williamcallahan/javachat/adapters/in/web/security/GenericAccessDeniedHandlerTest.java
@@ -0,0 +1,64 @@
+package com.williamcallahan.javachat.adapters.in.web.security;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.williamcallahan.javachat.domain.errors.ApiErrorResponse;
+import jakarta.servlet.ServletException;
+import java.io.IOException;
+import org.junit.jupiter.api.Test;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.MediaType;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.security.access.AccessDeniedException;
+
+/**
+ * Unit coverage for the generic non-CSRF access-denied response handler.
+ */
+class GenericAccessDeniedHandlerTest {
+
+    private final ObjectMapper objectMapper = new ObjectMapper();
+
+    @Test
+    void writesGenericJson403ThatDoesNotClaimCsrfFailure() throws IOException, ServletException {
+        GenericAccessDeniedHandler handler = new GenericAccessDeniedHandler(objectMapper);
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        MockHttpServletResponse response = new MockHttpServletResponse();
+        AccessDeniedException exception = new AccessDeniedException("API key identity is required for revocation");
+
+        handler.handle(request, response, exception);
+
+        assertEquals(HttpStatus.FORBIDDEN.value(), response.getStatus());
+        assertEquals(MediaType.APPLICATION_JSON_VALUE, response.getContentType());
+        ApiErrorResponse body = objectMapper.readValue(response.getContentAsString(), ApiErrorResponse.class);
+        assertEquals("error", body.status());
+        assertEquals("Access denied.", body.message());
+        assertNull(body.details());
+        String serializedBody = response.getContentAsString();
+        assertFalse(serializedBody.contains("CSRF"));
+        assertFalse(serializedBody.contains("API key identity is required for revocation"));
+    }
+
+    @Test
+    void leavesCommittedResponseUnchanged() throws IOException, ServletException {
+        GenericAccessDeniedHandler handler = new GenericAccessDeniedHandler(objectMapper);
+        MockHttpServletRequest request = new MockHttpServletRequest();
+        MockHttpServletResponse response = new MockHttpServletResponse();
+        response.setCommitted(true);
+        response.setStatus(HttpStatus.OK.value());
+
+        handler.handle(request, response, new AccessDeniedException("ignored"));
+
+        assertEquals(HttpStatus.OK.value(), response.getStatus());
+        assertEquals(0, response.getContentAsByteArray().length);
+    }
+
+    @Test
+    void rejectsNullObjectMapper() {
+        assertThrows(NullPointerException.class, () -> new GenericAccessDeniedHandler(null));
+    }
+}
diff --git a/src/test/java/com/williamcallahan/javachat/web/RevokeApiKeyAuthIntegrationTest.java b/src/test/java/com/williamcallahan/javachat/web/RevokeApiKeyAuthIntegrationTest.java
new file mode 100644
index 00000000..3921c4b3
--- /dev/null
+++ b/src/test/java/com/williamcallahan/javachat/web/RevokeApiKeyAuthIntegrationTest.java
@@ -0,0 +1,112 @@
+package com.williamcallahan.javachat.web;
+
+import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.jwt;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
+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 com.williamcallahan.javachat.adapters.out.clerk.ClerkApiKeyVerifier;
+import com.williamcallahan.javachat.application.knowledge.KnowledgeBaseInventoryUseCase;
+import com.williamcallahan.javachat.service.EmbeddingClient;
+import io.qdrant.client.QdrantClient;
+import jakarta.servlet.http.Cookie;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Test;
+import org.mockito.Answers;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.test.context.bean.override.mockito.MockitoBean;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.MvcResult;
+
+/**
+ * Verifies the access-denied routing of {@code DELETE /api/me/api-key} under the
+ * Clerk-enabled security chain: CSRF failures keep their CSRF-specific message
+ * while authenticated, non-CSRF denials receive a generic 403 instead of the
+ * false "CSRF token missing or invalid" message.
+ *
+ * <p>Runs with the dev-shaped Clerk properties so the conditional
+ * {@code clerkJwtDecoder} bean and the resource-server wiring are active,
+ * mirroring the production security chain. The decoder itself is never invoked
+ * because {@code jwt()} injects the authentication directly; no network access
+ * occurs.
+ */
+@SpringBootTest(
+        properties = {
+            "spring.ai.vectorstore.qdrant.port=1",
+            "spring.security.oauth2.resourceserver.jwt.issuer-uri=https://romantic-cow-6.clerk.accounts.dev",
+            "spring.security.oauth2.resourceserver.jwt.jwk-set-uri=https://romantic-cow-6.clerk.accounts.dev/.well-known/jwks.json",
+            "app.clerk.authorized-parties=http://localhost:5173"
+        })
+@AutoConfigureMockMvc
+class RevokeApiKeyAuthIntegrationTest {
+
+    private static final String CLERK_USER_ID = "user_2abcDEFGHijkLMNopq";
+    private static final String CSRF_COOKIE_NAME = "XSRF-TOKEN";
+    private static final String CSRF_HEADER_NAME = "X-XSRF-TOKEN";
+    private static final String CSRF_INVALID_MESSAGE =
+            "CSRF token missing or invalid. Refresh the page and retry the request.";
+    private static final String ACCESS_DENIED_MESSAGE = "Access denied.";
+
+    @Autowired
+    MockMvc mockMvc;
+
+    @MockitoBean(answers = Answers.RETURNS_MOCKS)
+    EmbeddingClient embeddingClient;
+
+    @MockitoBean
+    QdrantClient qdrantClient;
+
+    @MockitoBean
+    ClerkApiKeyVerifier clerkApiKeyVerifier;
+
+    @MockitoBean
+    KnowledgeBaseInventoryUseCase knowledgeBaseInventoryUseCase;
+
+    @Test
+    void anonymousWithoutCsrfIsBlockedWithTheCsrfMessage() throws Exception {
+        mockMvc.perform(delete("/api/me/api-key"))
+                .andExpect(status().isForbidden())
+                .andExpect(jsonPath("$.status").value("error"))
+                .andExpect(jsonPath("$.message").value(CSRF_INVALID_MESSAGE));
+    }
+
+    @Test
+    void anonymousWithValidCsrfIsUnauthorizedByTheEntryPoint() throws Exception {
+        String csrfToken = fetchCsrfToken();
+
+        mockMvc.perform(delete("/api/me/api-key")
+                        .cookie(new Cookie(CSRF_COOKIE_NAME, csrfToken))
+                        .header(CSRF_HEADER_NAME, csrfToken))
+                .andExpect(status().isUnauthorized());
+    }
+
+    @Test
+    void clerkJwtWithValidCsrfReceivesGenericForbiddenNotCsrfMessage() throws Exception {
+        String csrfToken = fetchCsrfToken();
+
+        mockMvc.perform(delete("/api/me/api-key")
+                        .with(jwt().jwt(token -> token.subject(CLERK_USER_ID)))
+                        .cookie(new Cookie(CSRF_COOKIE_NAME, csrfToken))
+                        .header(CSRF_HEADER_NAME, csrfToken))
+                .andExpect(status().isForbidden())
+                .andExpect(jsonPath("$.status").value("error"))
+                .andExpect(jsonPath("$.message").value(ACCESS_DENIED_MESSAGE))
+                .andExpect(content().string(Matchers.not(Matchers.containsString("CSRF"))));
+    }
+
+    private String fetchCsrfToken() throws Exception {
+        MvcResult csrfResult = mockMvc.perform(get("/api/security/csrf"))
+                .andExpect(status().isOk())
+                .andReturn();
+        for (Cookie cookie : csrfResult.getResponse().getCookies()) {
+            if (CSRF_COOKIE_NAME.equals(cookie.getName())) {
+                return cookie.getValue();
+            }
+        }
+        throw new AssertionError("CSRF cookie was not issued by /api/security/csrf");
+    }
+}