Fix RFC 7591 scope serialization format

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

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()