fix!(mcp): implement router-level logger injection for MCP auth#3067
fix!(mcp): implement router-level logger injection for MCP auth#3067duwenxin99 wants to merge 2 commits intomainfrom
Conversation
57de20f to
42f2d07
Compare
There was a problem hiding this comment.
Code Review
This pull request enhances the generic authentication service by allowing custom introspection endpoints, methods, and parameter names, specifically to support Google's token validation. It also introduces a fallback for the audience claim and ensures the MCP middleware fails closed on unexpected errors. Feedback indicates a security regression due to the removal of the active claim check in the introspection response, and several instances where Go naming conventions for acronyms were not followed.
I am having trouble creating individual review comments. Click here to see my feedback.
internal/auth/generic/generic.go (346-356)
The check for the active claim in the introspection response has been removed. According to RFC 7662, the active field is REQUIRED and indicates whether the token is currently valid. Removing this check is a security regression as it allows tokens that the provider explicitly identifies as inactive (e.g., revoked or expired) to be accepted if they still contain other claims.
To support providers like Google that do not return this field, you should use a pointer to a boolean (*bool) to detect if the field was present in the JSON, and only reject the token if it is explicitly false.
var introspectResp struct {
Active *bool "json:\"active\"" // Use pointer to detect presence
Scope string "json:\"scope\""
Aud json.RawMessage "json:\"aud\""
Audience json.RawMessage "json:\"audience\""
Exp int64 "json:\"exp\""
}
if err := json.Unmarshal(body, &introspectResp); err != nil {
return &MCPAuthError{Code: http.StatusInternalServerError, Message: fmt.Sprintf("failed to parse introspection response: %v", err), ScopesRequired: a.ScopesRequired}
}
// Verify active status if the field is present (RFC 7662 requirement)
if introspectResp.Active != nil && !*introspectResp.Active {
return &MCPAuthError{Code: http.StatusUnauthorized, Message: "token is not active", ScopesRequired: a.ScopesRequired}
}internal/auth/generic/generic.go (62)
In Go, acronyms like URL should be all-caps (e.g., introspectionURL). This maintains consistency with jwksURL on the same line and follows the project's existing style.
jwksURL, introspectionURL, err := discoverOIDCConfig(httpClient, cfg.AuthorizationServer)
References
- Go style guide recommends that acronyms should be all-caps to maintain consistency. (link)
internal/auth/generic/generic.go (164)
Following Go naming conventions, this field should be named introspectionURL.
introspectionURL string
References
- Go style guide recommends that acronyms should be all-caps to maintain consistency. (link)
averikitsch
left a comment
There was a problem hiding this comment.
@Yuan325 can you be final approver.
Fix: #3076