From b9bf7249a2da1e537614631b7124b1c019ed981f Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Thu, 20 Aug 2026 12:17:33 -0400 Subject: [PATCH 1/5] test(security): prove @PreAuthorize and the actuator boundary are enforced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The security configuration had no test that exercised it. What existed was McpToolRegistrationTest#everyMcpEndpointIsPreAuthorized, which reflects over the service classes and asserts the annotation is *present*. That is a useful guard against forgetting it on a new tool, but it cannot tell whether the annotation has any runtime effect. Demonstrated by mutation on this branch: commenting out @EnableMethodSecurity in MethodSecurityConfiguration neuters all 24 @PreAuthorize annotations, making every MCP tool callable without authentication — and McpToolRegistrationTest still reports BUILD SUCCESSFUL. The same mutation fails the new test. Adds two tests: MethodSecurityEnforcementTest calls a secured tool through the Spring proxy with an empty SecurityContext and asserts AuthenticationCredentialsNotFoundException. Note the type: with no Authentication at all Spring raises that rather than AccessDeniedException, which is for an authenticated principal lacking authority. HttpSecurityFilterChainTest pins the anonymous-access boundary — /actuator/health open for probes, /actuator/sbom/application and /actuator/metrics closed. That split is a single requestMatchers rule whose justification lives only in a code comment; widening it to permitAll() would expose the dependency tree and the metrics that map the tool surface, and would have broken no test. Verified by mutation: flipping the rule fails both assertions. Denial there is asserted as 401-or-403 rather than a fixed code. With no issuer configured there is no authentication entry point, so Spring rejects with 403; wiring an issuer turns the same request into a 401 with WWW-Authenticate. Both are correct denials — the property worth pinning is that neither is a 200. Also worth recording why the gap went unnoticed: OtlpExportIntegrationTest is the only test that activates the http profile without disabling security, and it is @Disabled over an unrelated Jetty/LGTM container issue. Every other http-profile test sets http.security.enabled=false. 376 tests, 0 failures (baseline 372). Signed-off-by: Aditya Parikh --- .../security/HttpSecurityFilterChainTest.java | 99 +++++++++++++++++++ .../MethodSecurityEnforcementTest.java | 77 +++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java create mode 100644 src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java diff --git a/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java b/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java new file mode 100644 index 00000000..42f40d30 --- /dev/null +++ b/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.mcp.server.security; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import org.apache.solr.mcp.server.TestcontainersConfiguration; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledInNativeImage; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.server.LocalServerPort; +import org.springframework.context.annotation.Import; +import org.springframework.test.context.ActiveProfiles; +import org.testcontainers.junit.jupiter.Testcontainers; + +/** + * Pins the anonymous-access boundary of the {@code http} filter chain. + * + *

+ * {@link HttpSecurityConfiguration} deliberately splits the actuator: probes + * stay open so load balancers and orchestrators can reach them, while every + * other endpoint requires authentication — otherwise an unauthenticated caller + * could read the dependency tree from {@code /actuator/sbom/application} or + * scrape metrics that map the tool surface. + * + *

+ * That decision is a one-line {@code requestMatchers} rule. Widening it to + * {@code permitAll()} would expose all of the above and break no other test, so + * this asserts both halves: health open, everything else closed. + * + *

+ * No issuer is configured here, which is the point — with OAuth2 unwired the + * chain must still deny anonymous access rather than fall open. + */ +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@Import(TestcontainersConfiguration.class) +@ActiveProfiles("http") +@Tag("integration") +@Testcontainers(disabledWithoutDocker = true) +@DisabledInNativeImage +class HttpSecurityFilterChainTest { + + @LocalServerPort + private int port; + + private int statusOf(String path) throws Exception { + HttpRequest request = HttpRequest.newBuilder().uri(URI.create("http://localhost:" + port + path)).GET().build(); + return HttpClient.newHttpClient().send(request, HttpResponse.BodyHandlers.ofString()).statusCode(); + } + + @Test + void healthProbeIsAnonymouslyReachable() throws Exception { + assertEquals(200, statusOf("/actuator/health"), + "/actuator/health must stay open for liveness and readiness probes"); + } + + /** + * Denial here is 403, not 401: with no issuer configured there is no + * authentication entry point to challenge with, so Spring Security rejects + * rather than prompting. Wiring an issuer turns the same request into a 401 + * carrying {@code WWW-Authenticate: Bearer}. Both are correct denials, so these + * accept either — what must never happen is a 200. + */ + private void assertDenied(String path, String why) throws Exception { + int status = statusOf(path); + assertTrue(status == 401 || status == 403, why + " — expected 401 or 403, got " + status); + } + + @Test + void sbomEndpointRequiresAuthentication() throws Exception { + assertDenied("/actuator/sbom/application", + "/actuator/sbom/application exposes the full dependency tree and must not be anonymous"); + } + + @Test + void metricsEndpointRequiresAuthentication() throws Exception { + assertDenied("/actuator/metrics", "/actuator/metrics maps the tool surface and must not be anonymous"); + } +} diff --git a/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java b/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java new file mode 100644 index 00000000..33b8c0e0 --- /dev/null +++ b/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.mcp.server.security; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.apache.solr.mcp.server.TestcontainersConfiguration; +import org.apache.solr.mcp.server.collection.CollectionService; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledInNativeImage; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.annotation.Import; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException; +import org.springframework.test.context.ActiveProfiles; +import org.testcontainers.junit.jupiter.Testcontainers; + +/** + * Probe: is {@code @PreAuthorize} actually enforced, or merely present? + * + *

+ * {@code McpToolRegistrationTest#everyMcpEndpointIsPreAuthorized} asserts the + * annotation is declared on every MCP entry point. That is a static check — it + * cannot tell whether {@link MethodSecurityConfiguration} is wired such that + * the annotation has any runtime effect. If the profile gate or the + * {@code http.security.enabled} property condition stopped matching, every + * annotation would silently become a no-op and the static test would still + * pass. + * + *

+ * This test runs in the {@code http} profile with security left at its default + * (enabled) and invokes a secured method through the Spring proxy with an empty + * SecurityContext. Enforcement means an {@link AccessDeniedException}. + */ +@SpringBootTest +@Import(TestcontainersConfiguration.class) +@ActiveProfiles("http") +@Tag("integration") +@Testcontainers(disabledWithoutDocker = true) +@DisabledInNativeImage +class MethodSecurityEnforcementTest { + + @Autowired + private CollectionService collectionService; + + /** + * With an entirely empty SecurityContext, Spring Security raises + * {@link AuthenticationCredentialsNotFoundException} (an + * {@code AuthenticationException}) rather than {@code AccessDeniedException} — + * the latter is for an authenticated principal lacking authority. Asserting the + * broad {@code SecurityException}-free supertype would pass for the wrong + * reason, so this pins the specific type. + */ + @Test + void unauthenticatedCallToSecuredToolIsRejected() { + assertThrows(AuthenticationCredentialsNotFoundException.class, () -> collectionService.listCollections(), + "list-collections carries @PreAuthorize(\"isAuthenticated()\") and was called with no " + + "authentication, so method security must reject it. Succeeding means the annotation " + + "is decorative: @EnableMethodSecurity is not in effect for this context."); + } +} From 233a7553f97a392656584e30deb4796dec77920b Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Thu, 20 Aug 2026 13:41:21 -0400 Subject: [PATCH 2/5] test(security): pin the CORS contract the MCP Inspector depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Inspector's origin (http://localhost:6274) is the default value of mcp.cors.allowed-origins — a plain property with nothing asserting it. Narrowing it, or setting MCP_CORS_ALLOWED_ORIGINS=*, silently stops the Inspector connecting and no test notices. The wildcard is the trap worth guarding. setAllowedOrigins is the strict API, so * alongside allowCredentials(true) does not open the server up — it rejects every origin including the Inspector's, with nothing logged. An operator reaching for * to "allow everything" gets the opposite. Replays the preflight a browser sends on the Inspector's behalf: origin echoed back specifically (not a wildcard, which is invalid with credentials), credentials allowed, and GET/POST/DELETE all permitted since Streamable HTTP uses each for a different part of the transport. Plus the negative case, so the allowlist is not decorative. Verified by mutation: flipping the default to * fails two of the three. 379 tests, 0 failures. Signed-off-by: Aditya Parikh --- .../server/security/McpInspectorCorsTest.java | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java diff --git a/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java b/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java new file mode 100644 index 00000000..ffceb746 --- /dev/null +++ b/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.mcp.server.security; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import org.apache.solr.mcp.server.TestcontainersConfiguration; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledInNativeImage; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.server.LocalServerPort; +import org.springframework.context.annotation.Import; +import org.springframework.test.context.ActiveProfiles; +import org.testcontainers.junit.jupiter.Testcontainers; + +/** + * Pins the CORS contract the MCP Inspector depends on. + * + *

+ * The Inspector's UI runs at {@code http://localhost:6274} and is the default + * value of {@code mcp.cors.allowed-origins}. That default is a plain property: + * narrowing it, reordering it, or setting {@code MCP_CORS_ALLOWED_ORIGINS=*} + * silently stops the Inspector connecting, and no other test notices. + * + *

+ * The wildcard case is the trap. {@code setAllowedOrigins} is the strict API, + * so {@code *} combined with {@code allowCredentials(true)} does not open the + * server up — it rejects every origin, including the Inspector's, with + * no warning logged. An operator reaching for {@code *} to "allow everything" + * gets the opposite. + * + *

+ * This replays the exact preflight a browser sends on the Inspector's behalf + * and asserts the response permits the request. + */ +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@Import(TestcontainersConfiguration.class) +@ActiveProfiles("http") +@Tag("integration") +@Testcontainers(disabledWithoutDocker = true) +@DisabledInNativeImage +class McpInspectorCorsTest { + + /** The MCP Inspector UI origin, and the shipped default allowlist entry. */ + private static final String INSPECTOR_ORIGIN = "http://localhost:6274"; + + @LocalServerPort + private int port; + + private HttpResponse preflight(String origin, String method, String requestHeaders) throws Exception { + HttpRequest.Builder builder = HttpRequest.newBuilder(URI.create("http://localhost:" + port + "/mcp")) + .method("OPTIONS", HttpRequest.BodyPublishers.noBody()).header("Origin", origin) + .header("Access-Control-Request-Method", method); + if (requestHeaders != null) { + builder.header("Access-Control-Request-Headers", requestHeaders); + } + return HttpClient.newHttpClient().send(builder.build(), HttpResponse.BodyHandlers.ofString()); + } + + @Test + void inspectorPreflightIsAllowed() throws Exception { + HttpResponse response = preflight(INSPECTOR_ORIGIN, "POST", "content-type,authorization"); + + assertEquals(200, response.statusCode(), + "The MCP Inspector cannot connect unless its origin passes preflight. Check that " + + "mcp.cors.allowed-origins still contains " + INSPECTOR_ORIGIN); + assertEquals(INSPECTOR_ORIGIN, response.headers().firstValue("Access-Control-Allow-Origin").orElse(null), + "The specific origin must be echoed back; a wildcard is invalid alongside credentials"); + assertEquals("true", response.headers().firstValue("Access-Control-Allow-Credentials").orElse(null), + "The Inspector sends the bearer token as a credentialed request"); + } + + @Test + void inspectorTransportMethodsAreAllowed() throws Exception { + String allowed = preflight(INSPECTOR_ORIGIN, "POST", null).headers().firstValue("Access-Control-Allow-Methods") + .orElse(""); + + // Streamable HTTP: POST sends messages, GET opens the stream, DELETE ends + // the session. Dropping any one breaks a different part of the transport. + for (String method : new String[]{"GET", "POST", "DELETE"}) { + assertTrue(allowed.contains(method), + () -> "MCP Streamable HTTP needs " + method + "; Allow-Methods was: " + allowed); + } + } + + @Test + void unknownOriginIsRejected() throws Exception { + assertEquals(403, preflight("http://not-the-inspector.example", "POST", null).statusCode(), + "Origins outside the allowlist must be refused, otherwise the allowlist is decorative"); + } +} From b37e282788e25a5aa983f97339c9fc50dbeceaec Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Mon, 24 Aug 2026 15:43:00 -0400 Subject: [PATCH 3/5] test(security): run the new security tests in the native image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three tests on this branch were marked @DisabledInNativeImage on the rationale that they are "Testcontainers-backed and proxy-dependent". Neither half of that is a reason in this repo, and the annotation cost real coverage. Every other @DisabledInNativeImage on main is a Mockito unit test. The dividing line is when the proxy is synthesised: ByteBuddy builds subclasses at runtime, which GraalVM's closed world forbids, whereas Spring's @Configuration and AOP proxies are emitted by AOT at build time. processTestAot duly generates CollectionService$$SpringCGLIB$$0/1.class alongside CGLIB classes for MethodSecurityConfiguration, HttpSecurityConfiguration and Spring Security's AuthorizationProxyWebConfiguration, so @PreAuthorize is fully AOT-visible. Testcontainers-backed integration tests are what nativeTest exists to exercise; the three existing @ActiveProfiles("http") tests already run there. Measured with ./gradlew nativeTest -Pnative on GraalVM CE 25.0.2: 234 successful / 0 failed / 142 skipped against 227 / 0 / 142 at the branch point (a84033b). That is +7 passing with the skip count unchanged, which is the figure that matters: had the tests traded the annotation for a silent skip, skipped would have risen to 149 instead. This is not tidying. Before this branch no executing test ever built a security-enabled Spring context — the other http-profile tests set http.security.enabled=false, and OtlpExportIntegrationTest is @Disabled over an unrelated container issue. Keeping the annotation would have left that true for the native image, so nothing would verify that the native-http artifact enforces authorization at all. Signed-off-by: Aditya Parikh --- .../solr/mcp/server/security/HttpSecurityFilterChainTest.java | 2 -- .../apache/solr/mcp/server/security/McpInspectorCorsTest.java | 2 -- .../solr/mcp/server/security/MethodSecurityEnforcementTest.java | 2 -- 3 files changed, 6 deletions(-) diff --git a/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java b/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java index 42f40d30..504a10e4 100644 --- a/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java +++ b/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java @@ -26,7 +26,6 @@ import org.apache.solr.mcp.server.TestcontainersConfiguration; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.condition.DisabledInNativeImage; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.server.LocalServerPort; import org.springframework.context.annotation.Import; @@ -57,7 +56,6 @@ @ActiveProfiles("http") @Tag("integration") @Testcontainers(disabledWithoutDocker = true) -@DisabledInNativeImage class HttpSecurityFilterChainTest { @LocalServerPort diff --git a/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java b/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java index ffceb746..ac9b5e03 100644 --- a/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java +++ b/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java @@ -26,7 +26,6 @@ import org.apache.solr.mcp.server.TestcontainersConfiguration; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.condition.DisabledInNativeImage; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.server.LocalServerPort; import org.springframework.context.annotation.Import; @@ -58,7 +57,6 @@ @ActiveProfiles("http") @Tag("integration") @Testcontainers(disabledWithoutDocker = true) -@DisabledInNativeImage class McpInspectorCorsTest { /** The MCP Inspector UI origin, and the shipped default allowlist entry. */ diff --git a/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java b/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java index 33b8c0e0..d9c38cf9 100644 --- a/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java +++ b/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java @@ -22,7 +22,6 @@ import org.apache.solr.mcp.server.collection.CollectionService; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.condition.DisabledInNativeImage; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.context.annotation.Import; @@ -53,7 +52,6 @@ @ActiveProfiles("http") @Tag("integration") @Testcontainers(disabledWithoutDocker = true) -@DisabledInNativeImage class MethodSecurityEnforcementTest { @Autowired From 7fcb21062666dcdd2349841ffd0391ed887c0c1a Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Sun, 30 Aug 2026 23:12:05 -0400 Subject: [PATCH 4/5] test(security): prove a secured tool call succeeds when authenticated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MethodSecurityEnforcementTest asserted only that an anonymous call to a @PreAuthorize-gated tool is rejected. That is half a contract: a rejection test cannot distinguish "correctly denies anonymous callers" from "denies every caller". Both are green, so a gate wedged permanently shut looks identical to a working one. That gap was not hypothetical. A secured tool call was for a time believed broken — reported as returning "Access Denied" even for a valid token — and no test existed that could contradict it. The report turned out to be false (the token variable was empty), but establishing that required standing up Keycloak and a live server, because the suite had nothing to say either way. Adds authenticatedCallToSecuredToolSucceeds: @WithMockUser installs an authenticated principal, list-collections is invoked through the Spring proxy, and must return. Mutation-checked to confirm it has teeth — with list-collections changed to @PreAuthorize("hasRole('NONEXISTENT')"), which denies authenticated callers while leaving the anonymous path unchanged: unauthenticatedCallToSecuredToolIsRejected PASSED authenticatedCallToSecuredToolSucceeds FAILED Only the new test catches it, which is exactly the scenario that went undetected. Adds spring-security-test to the test bundle for @WithMockUser, declared versionless so Spring Boot's BOM manages it (resolves to 6.5.10). It is testImplementation only, so it does not reach productionRuntimeClasspath and does not affect the generated binary LICENSE appendix. The annotation also clears the SecurityContext after the method, so the ThreadLocal cannot leak into the rejection test and make it order-dependent. Full suite: 380 tests, 0 failures, 0 errors, 7 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011VuxVJU4FuPBPkb8ye7oTF Signed-off-by: Aditya Parikh --- gradle/libs.versions.toml | 2 ++ .../MethodSecurityEnforcementTest.java | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 473ff9ff..c3799ce0 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -50,6 +50,7 @@ spring-boot-starter-security = { module = "org.springframework.boot:spring-boot- spring-boot-starter-oauth2-resource-server = { module = "org.springframework.boot:spring-boot-starter-oauth2-resource-server" } spring-boot-docker-compose = { module = "org.springframework.boot:spring-boot-docker-compose" } spring-boot-starter-test = { module = "org.springframework.boot:spring-boot-starter-test" } +spring-security-test = { module = "org.springframework.security:spring-security-test" } spring-boot-testcontainers = { module = "org.springframework.boot:spring-boot-testcontainers" } # Spring AI spring-ai-starter-mcp-server-webmvc = { module = "org.springframework.ai:spring-ai-starter-mcp-server-webmvc" } @@ -102,6 +103,7 @@ spring-boot-dev = [ test = [ "spring-boot-starter-test", + "spring-security-test", "spring-boot-testcontainers", "spring-ai-spring-boot-testcontainers", "testcontainers-junit-jupiter", diff --git a/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java b/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java index d9c38cf9..c556646c 100644 --- a/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java +++ b/src/test/java/org/apache/solr/mcp/server/security/MethodSecurityEnforcementTest.java @@ -16,8 +16,11 @@ */ package org.apache.solr.mcp.server.security; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.util.List; import org.apache.solr.mcp.server.TestcontainersConfiguration; import org.apache.solr.mcp.server.collection.CollectionService; import org.junit.jupiter.api.Tag; @@ -27,6 +30,7 @@ import org.springframework.context.annotation.Import; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException; +import org.springframework.security.test.context.support.WithMockUser; import org.springframework.test.context.ActiveProfiles; import org.testcontainers.junit.jupiter.Testcontainers; @@ -72,4 +76,35 @@ void unauthenticatedCallToSecuredToolIsRejected() { + "authentication, so method security must reject it. Succeeding means the annotation " + "is decorative: @EnableMethodSecurity is not in effect for this context."); } + + /** + * The necessary counterpart to + * {@link #unauthenticatedCallToSecuredToolIsRejected()}. + * + *

+ * A rejection test on its own cannot distinguish "correctly denies anonymous + * callers" from "denies every caller". Both produce the same green result, so a + * gate that were wedged permanently shut would look identical to a working one. + * That gap is not hypothetical: a secured tool call was for a time believed + * broken — reported as denying even valid tokens — and no test existed that + * could have contradicted it. The claim turned out to be false, but only a live + * server proved so. + * + *

+ * {@code @WithMockUser} installs an authenticated principal before the method + * runs and clears it afterwards, so the {@code ThreadLocal} context cannot leak + * into {@link #unauthenticatedCallToSecuredToolIsRejected()} and make that test + * order-dependent. + */ + @Test + @WithMockUser + void authenticatedCallToSecuredToolSucceeds() { + List collections = assertDoesNotThrow(() -> collectionService.listCollections(), + "list-collections is gated by @PreAuthorize(\"isAuthenticated()\") and was called with an " + + "authenticated principal, so method security must permit it. An AccessDeniedException " + + "here means the gate rejects everyone, not just anonymous callers — the annotation " + + "would be denying valid callers rather than enforcing a boundary."); + + assertNotNull(collections, "an authorized list-collections call must return a result, not null"); + } } From ab97ecb653b0801a96257fc51437c90fed221906 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Sun, 30 Aug 2026 23:46:37 -0400 Subject: [PATCH 5/5] test(security): use library constants and pin the exact denial status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanups to the security tests. Replace magic literals with the constants Spring already provides: - raw 200/401/403 -> HttpStatus.OK/UNAUTHORIZED/FORBIDDEN.value() - "GET"/"POST"/"DELETE"/"OPTIONS" -> HttpMethod..name() - "Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers", "Access-Control-Allow-Origin", "Access-Control-Allow-Credentials", "Access-Control-Allow-Methods" -> the matching HttpHeaders constants - "content-type,authorization" -> HttpHeaders.CONTENT_TYPE / AUTHORIZATION - "true" -> Boolean.TRUE.toString() preflight() now takes an HttpMethod rather than a String, so a typo is a compile error instead of a silently failing preflight. Repeated endpoint paths are named constants (HEALTH_PROBE, SBOM_ENDPOINT, METRICS_ENDPOINT, MCP_ENDPOINT), and the transport method list becomes TRANSPORT_METHODS. Assert the denial status definitively. assertDenied accepted "401 or 403", which would pass for a chain that silently lost its bearer-token entry point or gained one it should not have. Measured against the running context: both denied actuator paths return 403, never 401 — this class configures no issuer, so HttpSecurityConfiguration skips the OAuth2 wiring, no BearerTokenAuthenticationEntryPoint is installed, and Spring Security falls back to Http403ForbiddenEntryPoint. The assertion now pins FORBIDDEN exactly, and the javadoc records why 401 belongs to a different configuration that this class does not exercise. Full suite: 380 tests, 0 failures, 0 errors, 7 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011VuxVJU4FuPBPkb8ye7oTF Signed-off-by: Aditya Parikh --- .../security/HttpSecurityFilterChainTest.java | 44 ++++++++++++----- .../server/security/McpInspectorCorsTest.java | 49 +++++++++++++------ 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java b/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java index 504a10e4..c8067ee1 100644 --- a/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java +++ b/src/test/java/org/apache/solr/mcp/server/security/HttpSecurityFilterChainTest.java @@ -17,7 +17,6 @@ package org.apache.solr.mcp.server.security; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; import java.net.URI; import java.net.http.HttpClient; @@ -29,6 +28,7 @@ import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.server.LocalServerPort; import org.springframework.context.annotation.Import; +import org.springframework.http.HttpStatus; import org.springframework.test.context.ActiveProfiles; import org.testcontainers.junit.jupiter.Testcontainers; @@ -58,6 +58,15 @@ @Testcontainers(disabledWithoutDocker = true) class HttpSecurityFilterChainTest { + /** Open for liveness/readiness probes — the one anonymous actuator path. */ + private static final String HEALTH_PROBE = "/actuator/health"; + + /** Exposes the full dependency tree; must never be anonymous. */ + private static final String SBOM_ENDPOINT = "/actuator/sbom/application"; + + /** Maps the tool surface; must never be anonymous. */ + private static final String METRICS_ENDPOINT = "/actuator/metrics"; + @LocalServerPort private int port; @@ -68,30 +77,39 @@ private int statusOf(String path) throws Exception { @Test void healthProbeIsAnonymouslyReachable() throws Exception { - assertEquals(200, statusOf("/actuator/health"), - "/actuator/health must stay open for liveness and readiness probes"); + assertEquals(HttpStatus.OK.value(), statusOf(HEALTH_PROBE), + HEALTH_PROBE + " must stay open for liveness and readiness probes"); } /** - * Denial here is 403, not 401: with no issuer configured there is no - * authentication entry point to challenge with, so Spring Security rejects - * rather than prompting. Wiring an issuer turns the same request into a 401 - * carrying {@code WWW-Authenticate: Bearer}. Both are correct denials, so these - * accept either — what must never happen is a 200. + * Denial in this context is exactly {@code 403}, and the assertion pins that + * rather than accepting "401 or 403". + * + *

+ * This class configures no issuer, so {@link HttpSecurityConfiguration} skips + * the OAuth2 wiring and no {@code BearerTokenAuthenticationEntryPoint} is + * installed. Spring Security falls back to {@code Http403ForbiddenEntryPoint}, + * which rejects outright instead of issuing a challenge — measured as + * {@code 403} on every denied path here. + * + *

+ * Accepting either code would weaken the test in a way that matters: a chain + * that silently lost its bearer-token entry point, or gained one it should not + * have, would still pass. Wiring an issuer turns the same request into a + * {@code 401} carrying {@code WWW-Authenticate: Bearer} — a different + * configuration, and one this class deliberately does not exercise. */ private void assertDenied(String path, String why) throws Exception { - int status = statusOf(path); - assertTrue(status == 401 || status == 403, why + " — expected 401 or 403, got " + status); + assertEquals(HttpStatus.FORBIDDEN.value(), statusOf(path), why); } @Test void sbomEndpointRequiresAuthentication() throws Exception { - assertDenied("/actuator/sbom/application", - "/actuator/sbom/application exposes the full dependency tree and must not be anonymous"); + assertDenied(SBOM_ENDPOINT, SBOM_ENDPOINT + " exposes the full dependency tree and must not be anonymous"); } @Test void metricsEndpointRequiresAuthentication() throws Exception { - assertDenied("/actuator/metrics", "/actuator/metrics maps the tool surface and must not be anonymous"); + assertDenied(METRICS_ENDPOINT, METRICS_ENDPOINT + " maps the tool surface and must not be anonymous"); } } diff --git a/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java b/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java index ac9b5e03..88a2f967 100644 --- a/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java +++ b/src/test/java/org/apache/solr/mcp/server/security/McpInspectorCorsTest.java @@ -23,12 +23,16 @@ import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; +import java.util.List; import org.apache.solr.mcp.server.TestcontainersConfiguration; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.server.LocalServerPort; import org.springframework.context.annotation.Import; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; import org.springframework.test.context.ActiveProfiles; import org.testcontainers.junit.jupiter.Testcontainers; @@ -62,48 +66,61 @@ class McpInspectorCorsTest { /** The MCP Inspector UI origin, and the shipped default allowlist entry. */ private static final String INSPECTOR_ORIGIN = "http://localhost:6274"; + /** Single path the MCP Streamable HTTP transport routes through. */ + private static final String MCP_ENDPOINT = "/mcp"; + + /** + * Methods the Streamable HTTP transport needs: POST sends messages, GET opens + * the stream, DELETE ends the session. Dropping any one breaks a different part + * of the transport. + */ + private static final List TRANSPORT_METHODS = List.of(HttpMethod.GET, HttpMethod.POST, + HttpMethod.DELETE); + @LocalServerPort private int port; - private HttpResponse preflight(String origin, String method, String requestHeaders) throws Exception { - HttpRequest.Builder builder = HttpRequest.newBuilder(URI.create("http://localhost:" + port + "/mcp")) - .method("OPTIONS", HttpRequest.BodyPublishers.noBody()).header("Origin", origin) - .header("Access-Control-Request-Method", method); + private HttpResponse preflight(String origin, HttpMethod method, String requestHeaders) throws Exception { + HttpRequest.Builder builder = HttpRequest.newBuilder(URI.create("http://localhost:" + port + MCP_ENDPOINT)) + .method(HttpMethod.OPTIONS.name(), HttpRequest.BodyPublishers.noBody()) + .header(HttpHeaders.ORIGIN, origin).header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, method.name()); if (requestHeaders != null) { - builder.header("Access-Control-Request-Headers", requestHeaders); + builder.header(HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS, requestHeaders); } return HttpClient.newHttpClient().send(builder.build(), HttpResponse.BodyHandlers.ofString()); } @Test void inspectorPreflightIsAllowed() throws Exception { - HttpResponse response = preflight(INSPECTOR_ORIGIN, "POST", "content-type,authorization"); + HttpResponse response = preflight(INSPECTOR_ORIGIN, HttpMethod.POST, + HttpHeaders.CONTENT_TYPE + "," + HttpHeaders.AUTHORIZATION); - assertEquals(200, response.statusCode(), + assertEquals(HttpStatus.OK.value(), response.statusCode(), "The MCP Inspector cannot connect unless its origin passes preflight. Check that " + "mcp.cors.allowed-origins still contains " + INSPECTOR_ORIGIN); - assertEquals(INSPECTOR_ORIGIN, response.headers().firstValue("Access-Control-Allow-Origin").orElse(null), + assertEquals(INSPECTOR_ORIGIN, + response.headers().firstValue(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN).orElse(null), "The specific origin must be echoed back; a wildcard is invalid alongside credentials"); - assertEquals("true", response.headers().firstValue("Access-Control-Allow-Credentials").orElse(null), + assertEquals(Boolean.TRUE.toString(), + response.headers().firstValue(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS).orElse(null), "The Inspector sends the bearer token as a credentialed request"); } @Test void inspectorTransportMethodsAreAllowed() throws Exception { - String allowed = preflight(INSPECTOR_ORIGIN, "POST", null).headers().firstValue("Access-Control-Allow-Methods") - .orElse(""); + String allowed = preflight(INSPECTOR_ORIGIN, HttpMethod.POST, null).headers() + .firstValue(HttpHeaders.ACCESS_CONTROL_ALLOW_METHODS).orElse(""); - // Streamable HTTP: POST sends messages, GET opens the stream, DELETE ends - // the session. Dropping any one breaks a different part of the transport. - for (String method : new String[]{"GET", "POST", "DELETE"}) { - assertTrue(allowed.contains(method), + for (HttpMethod method : TRANSPORT_METHODS) { + assertTrue(allowed.contains(method.name()), () -> "MCP Streamable HTTP needs " + method + "; Allow-Methods was: " + allowed); } } @Test void unknownOriginIsRejected() throws Exception { - assertEquals(403, preflight("http://not-the-inspector.example", "POST", null).statusCode(), + assertEquals(HttpStatus.FORBIDDEN.value(), + preflight("http://not-the-inspector.example", HttpMethod.POST, null).statusCode(), "Origins outside the allowlist must be refused, otherwise the allowlist is decorative"); } }