auth: add integration tests for security best practices conformance #457
Conversation
Add HTTP middleware integration tests to validate MCP Security Best Practices conformance: - Invalid tokens (e.g., wrong audience/unknown issuer) return 401, set WWW-Authenticate, and do not invoke the handler - Missing required scopes return 403 and set WWW-Authenticate Valid tokens succeed (200) - No token passthrough: downstream requests do not receive the client Authorization header These tests improve confidence that the SDK’s bearer auth middleware enforces authentication and authorization correctly and avoids token passthrough risks, per: https://modelcontextprotocol.io/specification/2025-06-18/basic/security_best_practices
|
No objections, I just want to read the doc first. It may be a couple of days. |
| wrapped := RequireBearerToken(verifier, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Simulate proxy-like behavior: perform a downstream request without | ||
| // forwarding the client's Authorization header. | ||
| resp, err := http.Get(downstream.URL) |
There was a problem hiding this comment.
Not saying anything is wrong with the PR, just pointing out that the SDK doesn't (and maybe shouldn't try to) prevent the pattern that I've seen when people forward tokens downstream which would roughly be:
req, err := http.NewRequest(http.MethodGet, downstream.URL, nil)
if err != nil {
t.Fatalf("failed to create downstream request: %v", err)
}
// Read and forward the Authorization header from the incoming request
if auth := r.Header.Get("Authorization"); auth != "" {
req.Header.Set("Authorization", auth)
}
resp, err := http.DefaultClient.Do(req)
if err != nil {
t.Fatalf("downstream request failed: %v", err)
}
resp.Body.Close()There was a problem hiding this comment.
(sorry clicked submit too soon)
what I meant to say is that it's fine to test that the SDK doesn't do the wrong thing on its own, but the SDK can't prevent the user from doing the wrong thing if they wish to.
|
@samthanawalla, could you please help with this review? |
| wantCalled bool | ||
| }{ | ||
| {name: "invalid-aud", token: "bad-aud", wantCode: http.StatusUnauthorized, wantCalled: false}, | ||
| {name: "unknown-issuer", token: "unknown-issuer", wantCode: http.StatusUnauthorized, wantCalled: false}, |
There was a problem hiding this comment.
Is invalid-aud and unknown-issuer the same test case?
| if tt.wantCode == http.StatusUnauthorized || tt.wantCode == http.StatusForbidden { | ||
| want := "Bearer resource_metadata=" + resourceMetadata | ||
| if rw.Header().Get("WWW-Authenticate") != want { | ||
| t.Fatalf("unexpected WWW-Authenticate header: %q", rw.Header().Get("WWW-Authenticate")) |
There was a problem hiding this comment.
print got and want and store w.Header().Get("WWW-Authenticate") as got
|
I don't understand something. The spec says "MCP servers MUST NOT accept any tokens that were not explicitly issued for the MCP server." But the decision on whether to accept the token or not is left up to the TokenVerifier and is out of scope for our SDK. So how can we test this behavior? |
5 participants
This PR adds integration tests to catch Security Best Practices 2.2 Token Passthrough.
Add HTTP middleware integration tests to validate MCP Security Best Practices conformance:
401, setWWW-Authenticate, and do not invoke the handler403and setWWW-Authenticate. Valid tokens succeed (200)Contributes towards #12345