From b9bf7249a2da1e537614631b7124b1c019ed981f Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Thu, 20 Aug 2026 12:17:33 -0400 Subject: [PATCH 1/2] 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/2] 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"); + } +}