test(security): prove @PreAuthorize and the actuator boundary are enforced - #187
Open
adityamparikh wants to merge 7 commits into
Open
test(security): prove @PreAuthorize and the actuator boundary are enforced#187adityamparikh wants to merge 7 commits into
adityamparikh wants to merge 7 commits into
Conversation
…orced 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 <aditya.m.parikh@gmail.com>
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 <aditya.m.parikh@gmail.com>
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 <aditya.m.parikh@gmail.com>
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011VuxVJU4FuPBPkb8ye7oTF Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
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.<M>.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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011VuxVJU4FuPBPkb8ye7oTF Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
MethodSecurityEnforcementTest used a MOCK environment while the other two used RANDOM_PORT, so it built a second context and a second Solr container. All three now use the same annotation set, opt out of Docker Compose explicitly (compose.yaml is about to gain Keycloak), and the HTTP-driven tests reuse one static HttpClient. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CiUHyyXLTo9ATdgg8eRFZJ Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The security configuration has no test that exercises it at runtime. This adds three, validated by mutation.
The gap
McpToolRegistrationTest#everyMcpEndpointIsPreAuthorizedasserts@PreAuthorizeis present on every MCP entry point, but it is static and cannot tell whether the annotation has any effect. That matters here becauseHttpSecurityConfigurationleaves/mcponpermitAll()so the MCP layer can dispatch; method security is the only thing between an anonymous caller and every tool.@EnableMethodSecuritycommented out/actuator/**widened topermitAll()mcp.cors.allowed-originsdefaulted to*What's added
All three run under the
httpprofile with security left at its default, share one Spring context (RANDOM_PORT, Docker Compose disabled), and run in the native image as well as the JVM.MethodSecurityEnforcementTestcalls a secured tool through the Spring proxy with an emptySecurityContextand assertsAuthenticationCredentialsNotFoundException(no principal at all;AccessDeniedExceptionis for an authenticated principal lacking authority), then the authenticated counterpart succeeds.HttpSecurityFilterChainTestpins the anonymous-access boundary:/actuator/healthopen for probes,/actuator/sbom/applicationand/actuator/metricsclosed with exactly 403. With no issuer configured there is no authentication entry point, so Spring's default isAccessDeniedExceptionrendered as 403; pinning it means a change to the entry-point wiring (which turns the same request into a 401 withWWW-Authenticate: Bearer) shows up as a deliberate test change rather than passing silently.McpInspectorCorsTestpins the CORS contract the Inspector depends on: its originhttp://localhost:6274is the shipped default ofmcp.cors.allowed-origins, and*alongsideallowCredentials(true)rejects every origin rather than allowing all.spring-security-testis added to the catalog for@WithMockUser; it is not otherwise on the classpath.Verification
./gradlew buildon Java 25: 411 tests, 0 failures, 7 skipped (the OTLP suite, skipped onmainuntil test(observability): re-enable OtlpExportIntegrationTest #198)../gradlew nativeTest -Pnativeon GraalVM CE 25.0.2 at an earlier head with the same test classes: 234 successful, 0 failed, 142 skipped, unchanged skip count from the branch point. Spring's AOT-generated proxies for@PreAuthorizeare build-time artifacts, unlike Mockito's, so these tests need no@DisabledInNativeImage.Not covered here
OAuth2 wiring when an issuer is configured (the Nimbus decoder is eager, so it needs a reachable issuer or a mock), and the
validateAudienceClaim(true)behaviour the MCP Authorization spec requires.🤖 Generated with Claude Code
https://claude.ai/code/session_01CiUHyyXLTo9ATdgg8eRFZJ