From 969af25c8cd1547377b958ecabc1b89ae5eda036 Mon Sep 17 00:00:00 2001 From: guglielmoc Date: Mon, 4 May 2026 15:26:14 +0000 Subject: [PATCH 1/5] feat: implement scope accumulation in AuthorizationCode and ClientCredentials handlers to support incremental step-up authorization --- auth/authorization_code.go | 36 +++++ auth/authorization_code_test.go | 166 ++++++++++++++++++++++++ auth/extauth/client_credentials.go | 36 +++++ auth/extauth/client_credentials_test.go | 130 +++++++++++++++++++ 4 files changed, 368 insertions(+) diff --git a/auth/authorization_code.go b/auth/authorization_code.go index 758b88e3..71faa9a4 100644 --- a/auth/authorization_code.go +++ b/auth/authorization_code.go @@ -14,6 +14,7 @@ import ( "net/url" "slices" "strings" + "sync" "github.com/modelcontextprotocol/go-sdk/internal/util" "github.com/modelcontextprotocol/go-sdk/oauthex" @@ -111,6 +112,9 @@ type AuthorizationCodeHandler struct { // tokenSource is the token source to use for authorization. tokenSource oauth2.TokenSource + + mu sync.Mutex + requestedScopes []string } var _ OAuthHandler = (*AuthorizationCodeHandler)(nil) @@ -263,6 +267,14 @@ func (h *AuthorizationCodeHandler) Authorize(ctx context.Context, req *http.Requ scps = prm.ScopesSupported } + // Accumulate scopes: union previously requested scopes with the newly + // challenged scopes so that step-up authorization does not lose + // permissions granted in earlier rounds (SEP-2350). + h.mu.Lock() + scps = unionScopes(h.requestedScopes, scps) + h.requestedScopes = scps + h.mu.Unlock() + cfg := &oauth2.Config{ ClientID: resolvedClientConfig.clientID, ClientSecret: resolvedClientConfig.clientSecret, @@ -318,6 +330,30 @@ func errorFromChallenges(cs []oauthex.Challenge) string { return "" } +// unionScopes returns the union of existing and challenged scopes, +// preserving order (existing first, then new challenged scopes). +func unionScopes(existing, challenged []string) []string { + if len(existing) == 0 { + return challenged + } + if len(challenged) == 0 { + return existing + } + seen := make(map[string]struct{}, len(existing)) + result := make([]string, len(existing), len(existing)+len(challenged)) + copy(result, existing) + for _, s := range existing { + seen[s] = struct{}{} + } + for _, s := range challenged { + if _, ok := seen[s]; !ok { + seen[s] = struct{}{} + result = append(result, s) + } + } + return result +} + // getProtectedResourceMetadata returns the protected resource metadata. // If no metadata was found or the fetched metadata fails security checks, // it returns an error. diff --git a/auth/authorization_code_test.go b/auth/authorization_code_test.go index 92b6faf8..c6050a59 100644 --- a/auth/authorization_code_test.go +++ b/auth/authorization_code_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "net/http/httputil" + "net/url" "strings" "testing" @@ -111,6 +112,110 @@ func TestAuthorize(t *testing.T) { } } +func TestAuthorize_ScopeAccumulation(t *testing.T) { + authServer := oauthtest.NewFakeAuthorizationServer(oauthtest.Config{ + RegistrationConfig: &oauthtest.RegistrationConfig{ + PreregisteredClients: map[string]oauthtest.ClientInfo{ + "test_client_id": { + Secret: "test_client_secret", + RedirectURIs: []string{"http://localhost:12345/callback"}, + }, + }, + }, + }) + authServer.Start(t) + + resourceMux := http.NewServeMux() + resourceServer := httptest.NewServer(resourceMux) + t.Cleanup(resourceServer.Close) + resourceURL := resourceServer.URL + "/resource" + + resourceMux.Handle("/.well-known/oauth-protected-resource/resource", ProtectedResourceMetadataHandler(&oauthex.ProtectedResourceMetadata{ + Resource: resourceURL, + AuthorizationServers: []string{authServer.URL()}, + })) + + var capturedAuthURLs []string + + handler, err := NewAuthorizationCodeHandler(&AuthorizationCodeHandlerConfig{ + RedirectURL: "http://localhost:12345/callback", + PreregisteredClient: &oauthex.ClientCredentials{ + ClientID: "test_client_id", + ClientSecretAuth: &oauthex.ClientSecretAuth{ + ClientSecret: "test_client_secret", + }, + }, + AuthorizationCodeFetcher: func(ctx context.Context, args *AuthorizationArgs) (*AuthorizationResult, error) { + capturedAuthURLs = append(capturedAuthURLs, args.URL) + client := &http.Client{ + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + } + resp, err := client.Get(args.URL) + if err != nil { + return nil, fmt.Errorf("failed to visit auth URL: %v", err) + } + defer resp.Body.Close() + location, err := resp.Location() + if err != nil { + return nil, fmt.Errorf("failed to get location header: %v", err) + } + return &AuthorizationResult{ + Code: location.Query().Get("code"), + State: location.Query().Get("state"), + }, nil + }, + }) + if err != nil { + t.Fatalf("NewAuthorizationCodeHandler failed: %v", err) + } + + // First authorization: 401 with scope="read" + req := httptest.NewRequest(http.MethodGet, resourceURL, nil) + resp := &http.Response{ + StatusCode: http.StatusUnauthorized, + Header: make(http.Header), + Body: http.NoBody, + } + resp.Header.Set("WWW-Authenticate", + fmt.Sprintf(`Bearer scope="read", resource_metadata="%s/.well-known/oauth-protected-resource/resource"`, resourceServer.URL)) + if err := handler.Authorize(context.Background(), req, resp); err != nil { + t.Fatalf("First Authorize failed: %v", err) + } + + // Verify first auth URL requested only "read". + firstURL, err := url.Parse(capturedAuthURLs[0]) + if err != nil { + t.Fatalf("Failed to parse first auth URL: %v", err) + } + if got := firstURL.Query().Get("scope"); got != "read" { + t.Errorf("First auth scope = %q, want %q", got, "read") + } + + // Second authorization: 403 insufficient_scope with scope="write" + req2 := httptest.NewRequest(http.MethodGet, resourceURL, nil) + resp2 := &http.Response{ + StatusCode: http.StatusForbidden, + Header: make(http.Header), + Body: http.NoBody, + } + resp2.Header.Set("WWW-Authenticate", + fmt.Sprintf(`Bearer error="insufficient_scope", scope="write", resource_metadata="%s/.well-known/oauth-protected-resource/resource"`, resourceServer.URL)) + if err := handler.Authorize(context.Background(), req2, resp2); err != nil { + t.Fatalf("Second Authorize failed: %v", err) + } + + // Verify second auth URL accumulated both scopes. + secondURL, err := url.Parse(capturedAuthURLs[1]) + if err != nil { + t.Fatalf("Failed to parse second auth URL: %v", err) + } + if got := secondURL.Query().Get("scope"); got != "read write" { + t.Errorf("Second auth scope = %q, want %q", got, "read write") + } +} + func TestAuthorize_ForbiddenUnhandledError(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "http://example.com/resource", nil) resp := &http.Response{ @@ -738,6 +843,67 @@ func TestApplicationTypeInference(t *testing.T) { } } +func TestUnionScopes(t *testing.T) { + tests := []struct { + name string + existing []string + challenged []string + want []string + }{ + { + name: "both empty", + existing: nil, + challenged: nil, + want: nil, + }, + { + name: "existing only", + existing: []string{"read"}, + challenged: nil, + want: []string{"read"}, + }, + { + name: "challenged only", + existing: nil, + challenged: []string{"write"}, + want: []string{"write"}, + }, + { + name: "disjoint scopes", + existing: []string{"read"}, + challenged: []string{"write"}, + want: []string{"read", "write"}, + }, + { + name: "overlapping scopes", + existing: []string{"read", "write"}, + challenged: []string{"write", "admin"}, + want: []string{"read", "write", "admin"}, + }, + { + name: "identical scopes", + existing: []string{"read", "write"}, + challenged: []string{"read", "write"}, + want: []string{"read", "write"}, + }, + { + name: "preserves order", + existing: []string{"b", "a"}, + challenged: []string{"c", "a"}, + want: []string{"b", "a", "c"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := unionScopes(tt.existing, tt.challenged) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("unionScopes() mismatch (-want +got):\n%s", diff) + } + }) + } +} + // validConfig for test to create an AuthorizationCodeHandler using its constructor. // Values that are relevant to the test should be set explicitly. func validConfig() *AuthorizationCodeHandlerConfig { diff --git a/auth/extauth/client_credentials.go b/auth/extauth/client_credentials.go index fefa29b9..06863a9b 100644 --- a/auth/extauth/client_credentials.go +++ b/auth/extauth/client_credentials.go @@ -12,6 +12,7 @@ import ( "net/url" "slices" "strings" + "sync" "github.com/modelcontextprotocol/go-sdk/auth" "github.com/modelcontextprotocol/go-sdk/oauthex" @@ -47,6 +48,9 @@ type ClientCredentialsHandlerConfig struct { type ClientCredentialsHandler struct { config *ClientCredentialsHandlerConfig tokenSource oauth2.TokenSource + + mu sync.Mutex + requestedScopes []string } // Compile-time check that ClientCredentialsHandler implements auth.OAuthHandler. @@ -127,6 +131,14 @@ func (h *ClientCredentialsHandler) Authorize(ctx context.Context, req *http.Requ scopes = prm.ScopesSupported } + // Accumulate scopes: union previously requested scopes with the newly + // challenged scopes so that step-up authorization does not lose + // permissions granted in earlier rounds (SEP-2350). + h.mu.Lock() + scopes = unionScopes(h.requestedScopes, scopes) + h.requestedScopes = scopes + h.mu.Unlock() + // Step 3: Exchange client credentials for an access token. creds := h.config.Credentials cfg := &clientcredentials.Config{ @@ -229,6 +241,30 @@ func scopesFromChallenges(cs []oauthex.Challenge) []string { return nil } +// unionScopes returns the union of existing and challenged scopes, +// preserving order (existing first, then new challenged scopes). +func unionScopes(existing, challenged []string) []string { + if len(existing) == 0 { + return challenged + } + if len(challenged) == 0 { + return existing + } + seen := make(map[string]struct{}, len(existing)) + result := make([]string, len(existing), len(existing)+len(challenged)) + copy(result, existing) + for _, s := range existing { + seen[s] = struct{}{} + } + for _, s := range challenged { + if _, ok := seen[s]; !ok { + seen[s] = struct{}{} + result = append(result, s) + } + } + return result +} + // selectTokenAuthMethod selects the preferred token endpoint auth method based on // the authorization server's supported methods. Prefers client_secret_post over // client_secret_basic per the OAuth 2.1 draft. diff --git a/auth/extauth/client_credentials_test.go b/auth/extauth/client_credentials_test.go index 91ae7c04..fe46c48e 100644 --- a/auth/extauth/client_credentials_test.go +++ b/auth/extauth/client_credentials_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/modelcontextprotocol/go-sdk/auth" "github.com/modelcontextprotocol/go-sdk/internal/oauthtest" "github.com/modelcontextprotocol/go-sdk/oauthex" @@ -255,6 +256,135 @@ func TestClientCredentialsHandler_Authorize(t *testing.T) { }) } +func TestClientCredentialsHandler_ScopeAccumulation(t *testing.T) { + authServer := oauthtest.NewFakeAuthorizationServer(oauthtest.Config{ + MetadataEndpointConfig: &oauthtest.MetadataEndpointConfig{ + ServeOAuthInsertedEndpoint: true, + }, + RegistrationConfig: &oauthtest.RegistrationConfig{ + PreregisteredClients: map[string]oauthtest.ClientInfo{ + "test-client": {Secret: "test-secret"}, + }, + }, + ClientCredentialsConfig: &oauthtest.ClientCredentialsConfig{ + Enabled: true, + }, + }) + authServer.Start(t) + + resourceMux := http.NewServeMux() + resourceServer := httptest.NewServer(resourceMux) + t.Cleanup(resourceServer.Close) + resourceURL := resourceServer.URL + "/resource" + + resourceMux.Handle("/.well-known/oauth-protected-resource/resource", auth.ProtectedResourceMetadataHandler(&oauthex.ProtectedResourceMetadata{ + Resource: resourceURL, + AuthorizationServers: []string{authServer.URL()}, + })) + + handler, err := NewClientCredentialsHandler(validClientCredentialsConfig()) + if err != nil { + t.Fatal(err) + } + + // First authorization: 401 with scope="read" + resp := &http.Response{ + StatusCode: http.StatusUnauthorized, + Header: make(http.Header), + Body: http.NoBody, + } + resp.Header.Set("WWW-Authenticate", `Bearer scope="read"`) + req := httptest.NewRequest("GET", resourceURL, nil) + if err := handler.Authorize(t.Context(), req, resp); err != nil { + t.Fatalf("First Authorize failed: %v", err) + } + + // Verify handler tracked the requested scopes. + handler.mu.Lock() + firstScopes := append([]string{}, handler.requestedScopes...) + handler.mu.Unlock() + wantFirst := []string{"read"} + if diff := cmp.Diff(wantFirst, firstScopes); diff != "" { + t.Errorf("After first Authorize, requestedScopes mismatch (-want +got):\n%s", diff) + } + + // Second authorization: 401 with scope="write" (simulating step-up) + resp2 := &http.Response{ + StatusCode: http.StatusUnauthorized, + Header: make(http.Header), + Body: http.NoBody, + } + resp2.Header.Set("WWW-Authenticate", `Bearer scope="write"`) + req2 := httptest.NewRequest("GET", resourceURL, nil) + if err := handler.Authorize(t.Context(), req2, resp2); err != nil { + t.Fatalf("Second Authorize failed: %v", err) + } + + // Verify handler accumulated both scopes. + handler.mu.Lock() + secondScopes := append([]string{}, handler.requestedScopes...) + handler.mu.Unlock() + wantSecond := []string{"read", "write"} + if diff := cmp.Diff(wantSecond, secondScopes); diff != "" { + t.Errorf("After second Authorize, requestedScopes mismatch (-want +got):\n%s", diff) + } +} + +func TestUnionScopes(t *testing.T) { + tests := []struct { + name string + existing []string + challenged []string + want []string + }{ + { + name: "both empty", + existing: nil, + challenged: nil, + want: nil, + }, + { + name: "existing only", + existing: []string{"read"}, + challenged: nil, + want: []string{"read"}, + }, + { + name: "challenged only", + existing: nil, + challenged: []string{"write"}, + want: []string{"write"}, + }, + { + name: "disjoint scopes", + existing: []string{"read"}, + challenged: []string{"write"}, + want: []string{"read", "write"}, + }, + { + name: "overlapping scopes", + existing: []string{"read", "write"}, + challenged: []string{"write", "admin"}, + want: []string{"read", "write", "admin"}, + }, + { + name: "identical scopes", + existing: []string{"read", "write"}, + challenged: []string{"read", "write"}, + want: []string{"read", "write"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := unionScopes(tt.existing, tt.challenged) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("unionScopes() mismatch (-want +got):\n%s", diff) + } + }) + } +} + func TestSelectTokenAuthMethod(t *testing.T) { tests := []struct { name string From 0115c95ad1e2c04b2f1a3c3fb52168d0324bbea4 Mon Sep 17 00:00:00 2001 From: guglielmoc Date: Wed, 6 May 2026 13:20:07 +0000 Subject: [PATCH 2/5] refactor: replace map-based deduplication with slices.Contains in authorization code flow --- auth/authorization_code.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/auth/authorization_code.go b/auth/authorization_code.go index 71faa9a4..0c913f12 100644 --- a/auth/authorization_code.go +++ b/auth/authorization_code.go @@ -339,15 +339,10 @@ func unionScopes(existing, challenged []string) []string { if len(challenged) == 0 { return existing } - seen := make(map[string]struct{}, len(existing)) result := make([]string, len(existing), len(existing)+len(challenged)) copy(result, existing) - for _, s := range existing { - seen[s] = struct{}{} - } for _, s := range challenged { - if _, ok := seen[s]; !ok { - seen[s] = struct{}{} + if !slices.Contains(result, s) { result = append(result, s) } } From eeedb71686fb8f000a6fe4e87686e74cd0d8675c Mon Sep 17 00:00:00 2001 From: guglielmoc Date: Wed, 6 May 2026 15:29:04 +0000 Subject: [PATCH 3/5] refactor: replace manual map-based deduplication with slices.Contains in client credentials provider --- auth/extauth/client_credentials.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/auth/extauth/client_credentials.go b/auth/extauth/client_credentials.go index 06863a9b..34bcd627 100644 --- a/auth/extauth/client_credentials.go +++ b/auth/extauth/client_credentials.go @@ -250,15 +250,10 @@ func unionScopes(existing, challenged []string) []string { if len(challenged) == 0 { return existing } - seen := make(map[string]struct{}, len(existing)) result := make([]string, len(existing), len(existing)+len(challenged)) copy(result, existing) - for _, s := range existing { - seen[s] = struct{}{} - } for _, s := range challenged { - if _, ok := seen[s]; !ok { - seen[s] = struct{}{} + if !slices.Contains(result, s) { result = append(result, s) } } From 1002d49b995208a9e2f0a69efd1bd458aca274e2 Mon Sep 17 00:00:00 2001 From: guglielmoc Date: Wed, 6 May 2026 15:56:00 +0000 Subject: [PATCH 4/5] feat: ensure offline_access scope is added before unioning requested scopes and refactor test helper invocation --- auth/authorization_code.go | 15 ++++++++------- auth/authorization_code_test.go | 16 +++++++++++++--- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/auth/authorization_code.go b/auth/authorization_code.go index e8a74539..9cafb8df 100644 --- a/auth/authorization_code.go +++ b/auth/authorization_code.go @@ -284,13 +284,6 @@ func (h *AuthorizationCodeHandler) Authorize(ctx context.Context, req *http.Requ scps = prm.ScopesSupported } - // Accumulate scopes: union previously requested scopes with the newly - // challenged scopes so that step-up authorization does not lose - // permissions granted in earlier rounds (SEP-2350). - h.mu.Lock() - scps = unionScopes(h.requestedScopes, scps) - h.requestedScopes = scps - h.mu.Unlock() // SEP-2207: when the client desires refresh tokens and the Authorization // Server advertises offline_access support, add it to the requested scopes. if h.config.RequestRefreshToken && @@ -299,6 +292,14 @@ func (h *AuthorizationCodeHandler) Authorize(ctx context.Context, req *http.Requ scps = append(scps, "offline_access") } + // Accumulate scopes: union previously requested scopes with the newly + // challenged scopes so that step-up authorization does not lose + // permissions granted in earlier rounds (SEP-2350). + h.mu.Lock() + scps = unionScopes(h.requestedScopes, scps) + h.requestedScopes = scps + h.mu.Unlock() + cfg := &oauth2.Config{ ClientID: resolvedClientConfig.clientID, ClientSecret: resolvedClientConfig.clientSecret, diff --git a/auth/authorization_code_test.go b/auth/authorization_code_test.go index 7c7d6a98..439465ab 100644 --- a/auth/authorization_code_test.go +++ b/auth/authorization_code_test.go @@ -892,6 +892,19 @@ func TestUnionScopes(t *testing.T) { existing: []string{"b", "a"}, challenged: []string{"c", "a"}, want: []string{"b", "a", "c"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := unionScopes(tt.existing, tt.challenged) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("unionScopes() mismatch (-want +got):\n%s", diff) + } + }) + } +} + func TestAuthorize_OfflineAccessScope(t *testing.T) { tests := []struct { name string @@ -929,9 +942,6 @@ func TestAuthorize_OfflineAccessScope(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := unionScopes(tt.existing, tt.challenged) - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("unionScopes() mismatch (-want +got):\n%s", diff) authServer := oauthtest.NewFakeAuthorizationServer(oauthtest.Config{ ScopesSupported: tt.asScopesSupported, RegistrationConfig: &oauthtest.RegistrationConfig{ From 370e55f7bbd24f2a5f973ac96a58b125afbe438b Mon Sep 17 00:00:00 2001 From: guglielmoc Date: Wed, 6 May 2026 17:15:00 +0000 Subject: [PATCH 5/5] test: update authorization code tests to terminate early after capturing auth URLs --- auth/authorization_code_test.go | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/auth/authorization_code_test.go b/auth/authorization_code_test.go index 439465ab..419a8389 100644 --- a/auth/authorization_code_test.go +++ b/auth/authorization_code_test.go @@ -148,24 +148,7 @@ func TestAuthorize_ScopeAccumulation(t *testing.T) { }, AuthorizationCodeFetcher: func(ctx context.Context, args *AuthorizationArgs) (*AuthorizationResult, error) { capturedAuthURLs = append(capturedAuthURLs, args.URL) - client := &http.Client{ - CheckRedirect: func(req *http.Request, via []*http.Request) error { - return http.ErrUseLastResponse - }, - } - resp, err := client.Get(args.URL) - if err != nil { - return nil, fmt.Errorf("failed to visit auth URL: %v", err) - } - defer resp.Body.Close() - location, err := resp.Location() - if err != nil { - return nil, fmt.Errorf("failed to get location header: %v", err) - } - return &AuthorizationResult{ - Code: location.Query().Get("code"), - State: location.Query().Get("state"), - }, nil + return nil, fmt.Errorf("stop after capturing URL") }, }) if err != nil { @@ -181,8 +164,9 @@ func TestAuthorize_ScopeAccumulation(t *testing.T) { } resp.Header.Set("WWW-Authenticate", fmt.Sprintf(`Bearer scope="read", resource_metadata="%s/.well-known/oauth-protected-resource/resource"`, resourceServer.URL)) - if err := handler.Authorize(context.Background(), req, resp); err != nil { - t.Fatalf("First Authorize failed: %v", err) + err = handler.Authorize(context.Background(), req, resp) + if err == nil || !strings.Contains(err.Error(), "stop after capturing URL") { + t.Fatalf("First Authorize expected error containing 'stop after capturing URL', got: %v", err) } // Verify first auth URL requested only "read". @@ -203,8 +187,9 @@ func TestAuthorize_ScopeAccumulation(t *testing.T) { } resp2.Header.Set("WWW-Authenticate", fmt.Sprintf(`Bearer error="insufficient_scope", scope="write", resource_metadata="%s/.well-known/oauth-protected-resource/resource"`, resourceServer.URL)) - if err := handler.Authorize(context.Background(), req2, resp2); err != nil { - t.Fatalf("Second Authorize failed: %v", err) + err = handler.Authorize(context.Background(), req2, resp2) + if err == nil || !strings.Contains(err.Error(), "stop after capturing URL") { + t.Fatalf("Second Authorize expected error containing 'stop after capturing URL', got: %v", err) } // Verify second auth URL accumulated both scopes.