test(security): prove @PreAuthorize and the actuator boundary are enforced - #187
Open
adityamparikh wants to merge 2 commits into
Open
test(security): prove @PreAuthorize and the actuator boundary are enforced#187adityamparikh wants to merge 2 commits into
adityamparikh wants to merge 2 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>
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 currently has no test that exercises it. This adds two, both
validated by mutation.
The gap
McpToolRegistrationTest#everyMcpEndpointIsPreAuthorizedreflects over the service classes andasserts
@PreAuthorizeis present on every MCP entry point. That is a good guard againstforgetting it on a new tool — but it is static, and cannot tell whether the annotation has any
runtime effect.
Demonstrated by commenting out
@EnableMethodSecurityinMethodSecurityConfiguration, whichneuters all 24
@PreAuthorizeannotations and makes every MCP tool callable unauthenticated:@EnableMethodSecuritydisabled/actuator/**→permitAll()So today, if the profile gate or the
http.security.enabledproperty condition ever stoppedmatching, every tool would be open and CI would stay green.
What's added
MethodSecurityEnforcementTest— calls a secured tool through the Spring proxy with anempty
SecurityContextand asserts rejection. Note the exception type: with noAuthenticationat all, Spring raises
AuthenticationCredentialsNotFoundException, notAccessDeniedException(the latter is for an authenticated principal lacking authority). I got that wrong on the first
run and the failure output corrected it.
HttpSecurityFilterChainTest— pins the anonymous-access boundary:/actuator/healthopenfor probes,
/actuator/sbom/applicationand/actuator/metricsclosed. That split is a singlerequestMatchersrule whose justification currently lives only in a code comment — widening itto
permitAll()would expose the dependency tree and the metrics that map the tool surface, andwould break no existing test.
Denial 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: Bearer. Both are correct denials — the propertyworth pinning is that neither is a 200. Asserting 401 exactly would fail the day someone
configures an issuer, which is a good change.
Why this went unnoticed
OtlpExportIntegrationTestis the only test that activates thehttpprofile without settinghttp.security.enabled=false— and it is@Disabledover an unrelated Jetty/LGTM containerissue. Every other http-profile test disables security. So no executing test has ever run with
the security configuration active.
Notes
mainas-is; the security classes are identical on thesb4branch, so thisflows there on the next rebase.
@Tag("integration")+@DisabledInNativeImage, matching the convention forTestcontainers-backed and proxy-dependent tests.
Not covered here
Deliberately out of scope, worth separate issues if wanted: OAuth2 wiring when an issuer is
configured (the Nimbus decoder is eager, so it needs a reachable issuer or a mock), the
validateAudienceClaim(true)behaviour the MCP Authorization spec requires, and the CORSconfiguration.