diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index 9eb23e02c..fa3f4fb98 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -33,11 +33,11 @@ type DynamicClientRegistrationRequest struct { RedirectURIs []string `json:"redirect_uris"` // Essential fields for OAuth flow - ClientName string `json:"client_name,omitempty"` - TokenEndpointAuthMethod string `json:"token_endpoint_auth_method,omitempty"` - GrantTypes []string `json:"grant_types,omitempty"` - ResponseTypes []string `json:"response_types,omitempty"` - Scopes []string `json:"scope,omitempty"` + ClientName string `json:"client_name,omitempty"` + TokenEndpointAuthMethod string `json:"token_endpoint_auth_method,omitempty"` + GrantTypes []string `json:"grant_types,omitempty"` + ResponseTypes []string `json:"response_types,omitempty"` + Scopes ScopeList `json:"scope,omitempty"` } // NewDynamicClientRegistrationRequest creates a new dynamic client registration request @@ -57,18 +57,43 @@ func NewDynamicClientRegistrationRequest(scopes []string, callbackPort int) *Dyn return registrationRequest } -// ScopeList represents the "scope" field in a dynamic client registration response. -// Some servers return this as a space-delimited string per RFC 7591, while others -// return it as a JSON array of strings. This type normalizes both into a []string. +// ScopeList represents the "scope" field in both dynamic client registration requests and responses. // -// Examples of supported inputs: +// Marshaling (requests): Per RFC 7591 Section 2, scopes are serialized as a space-delimited string. +// Examples: +// - []string{"openid", "profile", "email"} → "openid profile email" +// - []string{"openid"} → "openid" +// - nil or []string{} → omitted (via omitempty) // -// "openid profile email" → []string{"openid", "profile", "email"} -// ["openid","profile","email"] → []string{"openid", "profile", "email"} -// null → nil -// "" or ["", " "] → nil +// Unmarshaling (responses): Some servers return scopes as a space-delimited string per RFC 7591, +// while others return a JSON array. This type normalizes both formats into []string. +// Examples: +// - "openid profile email" → []string{"openid", "profile", "email"} +// - ["openid","profile","email"] → []string{"openid", "profile", "email"} +// - null → nil +// - "" or ["", " "] → nil type ScopeList []string +// MarshalJSON implements custom encoding for ScopeList. It converts the slice +// of scopes into a space-delimited string as required by RFC 7591 Section 2. +// +// Important: This method does NOT handle empty slices. Go's encoding/json package +// evaluates omitempty by checking if the Go value is "empty" (len(slice) == 0) +// BEFORE calling MarshalJSON. Empty slices are omitted at the struct level, so this +// method is never invoked for empty slices. This means we don't need to return null +// or handle the empty case - omitempty does it for us automatically. +// +// See: https://pkg.go.dev/encoding/json (omitempty checks zero values before marshaling) +func (s ScopeList) MarshalJSON() ([]byte, error) { + // Join scopes with spaces and marshal as a string (RFC 7591 Section 2) + scopeString := strings.Join(s, " ") + result, err := json.Marshal(scopeString) + if err == nil { + logger.Debugf("RFC 7591: Marshaled ScopeList %v -> %q (space-delimited string)", []string(s), scopeString) + } + return result, err +} + // UnmarshalJSON implements custom decoding for ScopeList. It supports both // string and array encodings of the "scope" field, trimming whitespace and // normalizing empty values to nil for consistent semantics. @@ -165,6 +190,14 @@ func validateAndSetDefaults(request *DynamicClientRegistrationRequest) error { return fmt.Errorf("at least one redirect URI is required") } + // Validate that individual scope values don't contain spaces (RFC 6749 Section 3.3) + // Scopes must be space-separated tokens, so spaces within a scope value are invalid + for _, scope := range request.Scopes { + if strings.Contains(scope, " ") { + return fmt.Errorf("invalid scope value %q: scope values cannot contain spaces (RFC 6749)", scope) + } + } + // Set default values if not provided if request.ClientName == "" { request.ClientName = ToolHiveMCPClientName diff --git a/pkg/auth/oauth/dynamic_registration_test.go b/pkg/auth/oauth/dynamic_registration_test.go index 3d0392b2d..6b564a5d9 100644 --- a/pkg/auth/oauth/dynamic_registration_test.go +++ b/pkg/auth/oauth/dynamic_registration_test.go @@ -204,21 +204,13 @@ func TestNewDynamicClientRegistrationRequest(t *testing.T) { } } -func TestDynamicClientRegistrationRequest_EmptyScopeSerialization(t *testing.T) { +func TestDynamicClientRegistrationRequest_ScopeSerialization(t *testing.T) { t.Parallel() - // NOTE: This test documents current behavior where non-empty scopes are serialized - // as JSON arrays (e.g., ["openid", "profile"]), which violates RFC 7591 Section 2 - // requirement for space-delimited strings (e.g., "openid profile"). - // - // The critical behavior tested here is that empty/nil scopes result in the scope - // field being omitted entirely (omitempty), which is RFC 7591 compliant since - // the scope parameter is optional per RFC 7591 Section 2. - // - // TODO: The RFC 7591 format violation for non-empty scopes should be addressed - // in a separate PR to serialize as space-delimited strings. This keeps the - // MCP well-known URI discovery compliance fix cleanly separated from the - // RFC 7591 scope serialization format fix. + // This test verifies RFC 7591 Section 2 compliance for scope serialization. + // Per the spec, scopes MUST be serialized as a space-delimited string, not a JSON array. + // Empty/nil scopes should result in the scope field being omitted entirely (omitempty), + // which is RFC 7591 compliant since the scope parameter is optional. tests := []struct { name string @@ -237,16 +229,22 @@ func TestDynamicClientRegistrationRequest_EmptyScopeSerialization(t *testing.T) shouldOmitScope: true, }, { - name: "single scope should include scope field as array", + name: "single scope should be space-delimited string per RFC 7591", scopes: []string{"openid"}, shouldOmitScope: false, - expectedScopeJSON: `"scope":["openid"]`, // TODO: Should be "scope":"openid" per RFC 7591 + expectedScopeJSON: `"scope":"openid"`, }, { - name: "multiple scopes should include scope field as array", + name: "multiple scopes should be space-delimited string per RFC 7591", scopes: []string{"openid", "profile"}, shouldOmitScope: false, - expectedScopeJSON: `"scope":["openid","profile"]`, // TODO: Should be "scope":"openid profile" per RFC 7591 + expectedScopeJSON: `"scope":"openid profile"`, + }, + { + name: "three scopes should be space-delimited string per RFC 7591", + scopes: []string{"openid", "profile", "email"}, + shouldOmitScope: false, + expectedScopeJSON: `"scope":"openid profile email"`, }, } @@ -356,6 +354,33 @@ func TestRegisterClientDynamically(t *testing.T) { }, expectedError: true, }, + { + name: "invalid request - scope with spaces", + request: &DynamicClientRegistrationRequest{ + ClientName: "Test Client", + RedirectURIs: []string{"http://localhost:8080/callback"}, + Scopes: []string{"openid", "profile email", "another"}, + }, + expectedError: true, + }, + { + name: "invalid request - scope with leading space", + request: &DynamicClientRegistrationRequest{ + ClientName: "Test Client", + RedirectURIs: []string{"http://localhost:8080/callback"}, + Scopes: []string{" openid"}, + }, + expectedError: true, + }, + { + name: "invalid request - scope with trailing space", + request: &DynamicClientRegistrationRequest{ + ClientName: "Test Client", + RedirectURIs: []string{"http://localhost:8080/callback"}, + Scopes: []string{"openid "}, + }, + expectedError: true, + }, } for _, tt := range tests { @@ -441,6 +466,77 @@ func TestDynamicClientRegistrationResponse_Validation(t *testing.T) { // TestIsLocalhost is already defined in oidc_test.go +// TestScopeList_MarshalJSON tests that the ScopeList marshaling works correctly +// and produces RFC 7591 compliant space-delimited strings. +func TestScopeList_MarshalJSON(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + scopes ScopeList + wantJSON string + wantOmit bool // If true, expect omitempty to hide the field + }{ + { + name: "nil scopes => empty string (omitempty will hide at struct level)", + scopes: nil, + wantJSON: `""`, + wantOmit: true, + }, + { + name: "empty slice => empty string (omitempty will hide at struct level)", + scopes: ScopeList{}, + wantJSON: `""`, + wantOmit: true, + }, + { + name: "single scope => string", + scopes: ScopeList{"openid"}, + wantJSON: `"openid"`, + }, + { + name: "two scopes => space-delimited string", + scopes: ScopeList{"openid", "profile"}, + wantJSON: `"openid profile"`, + }, + { + name: "three scopes => space-delimited string", + scopes: ScopeList{"openid", "profile", "email"}, + wantJSON: `"openid profile email"`, + }, + { + name: "scopes with special characters", + scopes: ScopeList{"read:user", "write:repo"}, + wantJSON: `"read:user write:repo"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + jsonBytes, err := json.Marshal(tt.scopes) + require.NoError(t, err, "marshaling should succeed") + + jsonStr := string(jsonBytes) + assert.Equal(t, tt.wantJSON, jsonStr, "marshaled JSON should match expected format") + + // Verify omitempty behavior in a struct + // Note: omitempty checks the Go value (empty slice) before calling MarshalJSON, + // so empty slices are omitted regardless of what MarshalJSON returns. + if tt.wantOmit { + type testStruct struct { + Scope ScopeList `json:"scope,omitempty"` + } + s := testStruct{Scope: tt.scopes} + structJSON, err := json.Marshal(s) + require.NoError(t, err) + assert.Equal(t, "{}", string(structJSON), "omitempty should hide empty scope field") + } + }) + } +} + // TestScopeList_UnmarshalJSON tests that the ScopeList unmarshaling works correctly. func TestScopeList_UnmarshalJSON(t *testing.T) { t.Parallel()