Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions client/transport/oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ type OAuthHandler struct {
metadataFetchErr error
metadataOnce sync.Once
baseURL string
resourceURL string // RFC 8707 resource indicator; populated from RFC 9728 discovery or baseURL

mu sync.RWMutex // Protects expectedState
expectedState string // Expected state value for CSRF protection
Expand Down Expand Up @@ -217,6 +218,9 @@ func (h *OAuthHandler) refreshToken(ctx context.Context, refreshToken string) (*
if h.config.ClientSecret != "" {
data.Set("client_secret", h.config.ClientSecret)
}
if resource := h.getResourceURL(); resource != "" {
data.Set("resource", resource)
}

req, err := http.NewRequestWithContext(
ctx,
Expand Down Expand Up @@ -310,6 +314,15 @@ func (h *OAuthHandler) SetBaseURL(baseURL string) {
h.baseURL = baseURL
}

// getResourceURL returns the RFC 8707 resource indicator to send to the
// authorization server so it can audience-restrict issued tokens.
func (h *OAuthHandler) getResourceURL() string {
if h.resourceURL != "" {
return h.resourceURL
}
return h.baseURL
Comment on lines +317 to +325

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Explicit auth-metadata config still misses the canonical resource.

getResourceURL() only prefers h.resourceURL, but that field is populated exclusively on the protected-resource discovery path. When AuthServerMetadataURL is set and baseURL is also configured, these new resource parameters fall back to baseURL and never see the protected resource’s advertised canonical identifier. If those differ, the client can send the wrong audience to the authorization server. Please make protected-resource lookup a best-effort step even when auth metadata is supplied directly, and add a regression test for that combination.

Also applies to: 431-434

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@client/transport/oauth.go` around lines 317 - 325, getResourceURL currently
only returns h.resourceURL if populated, but h.resourceURL is only set via the
protected-resource discovery path so when AuthServerMetadataURL is provided and
baseURL is configured the handler never attempts discovery and may use baseURL
as the audience; update OAuthHandler initialization to perform a best-effort
protected-resource lookup even when AuthServerMetadataURL is set (populate
h.resourceURL from the protected-resource's advertised canonical identifier if
available) and keep getResourceURL unchanged so it prefers the discovered value
over h.baseURL; add a regression test exercising the case where
AuthServerMetadataURL and baseURL are both configured but the protected-resource
metadata advertises a different resource to ensure the client sends the
discovered resource as the audience.

}

// GetExpectedState returns the expected state value (for testing purposes)
func (h *OAuthHandler) GetExpectedState() string {
h.mu.RLock()
Expand Down Expand Up @@ -413,6 +426,11 @@ func (h *OAuthHandler) getServerMetadata(ctx context.Context) (*AuthServerMetada
return
}

// Capture the canonical resource identifier for RFC 8707 resource indicators
if protectedResource.Resource != "" {
h.resourceURL = protectedResource.Resource
}

// If no authorization servers are specified, fall back to default endpoints
if len(protectedResource.AuthorizationServers) == 0 {
metadata, err := h.getDefaultEndpoints(baseURL)
Expand Down Expand Up @@ -654,6 +672,10 @@ func (h *OAuthHandler) ProcessAuthorizationResponse(ctx context.Context, code, s
data.Set("code_verifier", codeVerifier)
}

if resource := h.getResourceURL(); resource != "" {
data.Set("resource", resource)
}

req, err := http.NewRequestWithContext(
ctx,
http.MethodPost,
Expand Down Expand Up @@ -734,5 +756,9 @@ func (h *OAuthHandler) GetAuthorizationURL(ctx context.Context, state, codeChall
params.Set("code_challenge_method", "S256")
}

if resource := h.getResourceURL(); resource != "" {
params.Set("resource", resource)
}

return metadata.AuthorizationEndpoint + "?" + params.Encode(), nil
}
167 changes: 167 additions & 0 deletions client/transport/oauth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"net/http"
"net/http/httptest"
"net/url"
"strconv"
"strings"
"testing"
Expand All @@ -15,6 +16,172 @@ import (
"github.com/stretchr/testify/require"
)

// TestOAuthHandler_ResourceIndicator_RFC8707 verifies that the RFC 8707
// "resource" parameter is included in the authorization URL, the
// authorization_code token exchange, and the refresh_token request so the
// authorization server can audience-restrict issued tokens.
func TestOAuthHandler_ResourceIndicator_RFC8707(t *testing.T) {
var tokenExchangeResource, refreshResource string

var serverURL string
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/.well-known/oauth-authorization-server":
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"issuer": serverURL,
"authorization_endpoint": serverURL + "/authorize",
"token_endpoint": serverURL + "/token",
})
case "/token":
_ = r.ParseForm()
switch r.FormValue("grant_type") {
case "authorization_code":
tokenExchangeResource = r.FormValue("resource")
case "refresh_token":
refreshResource = r.FormValue("resource")
}
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"access_token": "at",
"token_type": "Bearer",
"expires_in": 3600,
"refresh_token": "rt",
})
default:
http.NotFound(w, r)
}
}))
serverURL = server.URL
defer server.Close()

const mcpServerURL = "https://mcp.example.com/api"

config := OAuthConfig{
ClientID: "test-client",
RedirectURI: "http://localhost/callback",
TokenStore: NewMemoryTokenStore(),
AuthServerMetadataURL: server.URL + "/.well-known/oauth-authorization-server",
}
handler := NewOAuthHandler(config)
handler.SetBaseURL(mcpServerURL)
ctx := context.Background()

// 1. Authorization URL must carry resource=
authURL, err := handler.GetAuthorizationURL(ctx, "state123", "")
require.NoError(t, err)
parsed, err := url.Parse(authURL)
require.NoError(t, err)
assert.Equal(t, mcpServerURL, parsed.Query().Get("resource"),
"authorization URL must include RFC 8707 resource indicator")

// 2. Token exchange must carry resource=
handler.SetExpectedState("state123")
require.NoError(t, handler.ProcessAuthorizationResponse(ctx, "code", "state123", ""))
assert.Equal(t, mcpServerURL, tokenExchangeResource,
"authorization_code token request must include RFC 8707 resource indicator")

// 3. Refresh must carry resource=
_, err = handler.RefreshToken(ctx, "rt")
require.NoError(t, err)
assert.Equal(t, mcpServerURL, refreshResource,
"refresh_token request must include RFC 8707 resource indicator")
}

// TestOAuthHandler_ResourceIndicator_FromProtectedResourceMetadata verifies that
// when RFC 9728 protected resource metadata advertises a canonical "resource"
// value, that value is preferred over the base URL as the RFC 8707 indicator.
func TestOAuthHandler_ResourceIndicator_FromProtectedResourceMetadata(t *testing.T) {
const canonicalResource = "https://mcp.example.com/"
var gotResource string

var serverURL string
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/.well-known/oauth-protected-resource":
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"resource": canonicalResource,
"authorization_servers": []string{serverURL},
})
case "/.well-known/oauth-authorization-server":
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"issuer": serverURL,
"authorization_endpoint": serverURL + "/authorize",
"token_endpoint": serverURL + "/token",
})
case "/token":
_ = r.ParseForm()
gotResource = r.FormValue("resource")
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"access_token": "at",
"token_type": "Bearer",
})
default:
http.NotFound(w, r)
}
}))
serverURL = server.URL
defer server.Close()

handler := NewOAuthHandler(OAuthConfig{
ClientID: "test-client",
RedirectURI: "http://localhost/callback",
TokenStore: NewMemoryTokenStore(),
})
// baseURL differs from the canonical resource advertised by the server;
// the advertised value must win.
handler.SetBaseURL(server.URL)

_, err := handler.RefreshToken(context.Background(), "rt")
require.NoError(t, err)
assert.Equal(t, canonicalResource, gotResource,
"resource indicator must prefer RFC 9728 'resource' over base URL")
}

// TestOAuthHandler_ResourceIndicator_OmittedWhenUnknown verifies that no
// resource parameter is sent when the handler has no way to determine one.
func TestOAuthHandler_ResourceIndicator_OmittedWhenUnknown(t *testing.T) {
var hadResource bool

var serverURL string
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/.well-known/oauth-authorization-server":
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"issuer": serverURL,
"authorization_endpoint": serverURL + "/authorize",
"token_endpoint": serverURL + "/token",
})
case "/token":
_ = r.ParseForm()
_, hadResource = r.Form["resource"]
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(map[string]any{
"access_token": "at",
"token_type": "Bearer",
})
}
}))
serverURL = server.URL
defer server.Close()

handler := NewOAuthHandler(OAuthConfig{
ClientID: "test-client",
RedirectURI: "http://localhost/callback",
TokenStore: NewMemoryTokenStore(),
AuthServerMetadataURL: server.URL + "/.well-known/oauth-authorization-server",
})
// No SetBaseURL, no protected-resource discovery → resource unknown.

_, err := handler.RefreshToken(context.Background(), "rt")
require.NoError(t, err)
assert.False(t, hadResource, "resource parameter must be omitted when unknown")
}

func TestToken_IsExpired(t *testing.T) {
// Test cases
testCases := []struct {
Expand Down