From 2d8f93edbdb9fae6e18053896ea5a8396230a85c Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sat, 15 Nov 2025 11:48:13 -0600 Subject: [PATCH 1/7] Fix RFC 7591 scope serialization format Dynamic client registration was sending scopes as a JSON array instead of a space-delimited string, violating RFC 7591 Section 2. Changes: - Add RequestScopeList type with custom MarshalJSON method - Convert scope arrays to space-delimited strings per RFC 7591 - Update DynamicClientRegistrationRequest to use RequestScopeList - Update tests to verify RFC 7591 compliance - Remove TODO comments about the format violation The ScopeList type for responses already handles both formats (string and array) for maximum provider compatibility. Fixes #2596 Signed-off-by: Claude --- pkg/auth/oauth/dynamic_registration.go | 33 ++++++- pkg/auth/oauth/dynamic_registration_test.go | 101 ++++++++++++++++---- 2 files changed, 112 insertions(+), 22 deletions(-) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index 9eb23e02c..8ea7f6846 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -27,17 +27,40 @@ const ResponseTypeCode = "code" // TokenEndpointAuthMethodNone is the token endpoint auth method for none const TokenEndpointAuthMethodNone = "none" +// RequestScopeList represents the "scope" field in a dynamic client registration request. +// Per RFC 7591 Section 2, this must be marshaled as a space-delimited string, not a JSON array. +// +// Examples of marshaled output: +// +// []string{"openid", "profile", "email"} → "openid profile email" +// []string{"openid"} → "openid" +// nil or []string{} → omitted (via omitempty) +type RequestScopeList []string + +// MarshalJSON implements custom encoding for RequestScopeList. It converts the slice +// of scopes into a space-delimited string as required by RFC 7591 Section 2. +func (r RequestScopeList) MarshalJSON() ([]byte, error) { + // Handle nil or empty slice - return null so omitempty works + if len(r) == 0 { + return []byte("null"), nil + } + + // Join scopes with spaces and marshal as a string + scopeString := strings.Join(r, " ") + return json.Marshal(scopeString) +} + // DynamicClientRegistrationRequest represents the request for dynamic client registration (RFC 7591) type DynamicClientRegistrationRequest struct { // Required field according to RFC 7591 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 RequestScopeList `json:"scope,omitempty"` } // NewDynamicClientRegistrationRequest creates a new dynamic client registration request diff --git a/pkg/auth/oauth/dynamic_registration_test.go b/pkg/auth/oauth/dynamic_registration_test.go index 3d0392b2d..1cb45db3b 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"`, }, } @@ -441,6 +439,75 @@ func TestDynamicClientRegistrationResponse_Validation(t *testing.T) { // TestIsLocalhost is already defined in oidc_test.go +// TestRequestScopeList_MarshalJSON tests that the RequestScopeList marshaling works correctly +// and produces RFC 7591 compliant space-delimited strings. +func TestRequestScopeList_MarshalJSON(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + scopes RequestScopeList + wantJSON string + wantOmit bool // If true, expect null (which triggers omitempty) + }{ + { + name: "nil scopes => null (omitempty will hide)", + scopes: nil, + wantJSON: `null`, + wantOmit: true, + }, + { + name: "empty slice => null (omitempty will hide)", + scopes: RequestScopeList{}, + wantJSON: `null`, + wantOmit: true, + }, + { + name: "single scope => string", + scopes: RequestScopeList{"openid"}, + wantJSON: `"openid"`, + }, + { + name: "two scopes => space-delimited string", + scopes: RequestScopeList{"openid", "profile"}, + wantJSON: `"openid profile"`, + }, + { + name: "three scopes => space-delimited string", + scopes: RequestScopeList{"openid", "profile", "email"}, + wantJSON: `"openid profile email"`, + }, + { + name: "scopes with special characters", + scopes: RequestScopeList{"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 + if tt.wantOmit { + type testStruct struct { + Scope RequestScopeList `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 null scope field") + } + }) + } +} + // TestScopeList_UnmarshalJSON tests that the ScopeList unmarshaling works correctly. func TestScopeList_UnmarshalJSON(t *testing.T) { t.Parallel() From ba472c75047cbdc364b7334b544c766a6be56ce8 Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:06:31 -0600 Subject: [PATCH 2/7] Add INFO logging to demonstrate RFC 7591 scope marshaling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds informative logging to show the transformation of RequestScopeList from Go slice to RFC 7591 compliant space-delimited string format. Tested successfully against Atlassian MCP OAuth server: - Input: [openid profile email] - Output: "openid profile email" (space-delimited string) - Result: Successful dynamic client registration 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/auth/oauth/dynamic_registration.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index 8ea7f6846..274a444ed 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -42,12 +42,17 @@ type RequestScopeList []string func (r RequestScopeList) MarshalJSON() ([]byte, error) { // Handle nil or empty slice - return null so omitempty works if len(r) == 0 { + logger.Infof("RFC 7591: Marshaling empty RequestScopeList -> null (will be omitted)") return []byte("null"), nil } // Join scopes with spaces and marshal as a string scopeString := strings.Join(r, " ") - return json.Marshal(scopeString) + result, err := json.Marshal(scopeString) + if err == nil { + logger.Infof("RFC 7591: Marshaled RequestScopeList %v -> %q (space-delimited string)", []string(r), scopeString) + } + return result, err } // DynamicClientRegistrationRequest represents the request for dynamic client registration (RFC 7591) From 5343be02f4ae468c9a4aeb2752b97ad50d1a3bfc Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:17:42 -0600 Subject: [PATCH 3/7] Change RFC 7591 scope marshaling logging to DEBUG level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The INFO level logging was useful for validating RFC 7591 compliance during testing, but DEBUG is more appropriate for production use. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/auth/oauth/dynamic_registration.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index 274a444ed..51a741b3c 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -42,7 +42,7 @@ type RequestScopeList []string func (r RequestScopeList) MarshalJSON() ([]byte, error) { // Handle nil or empty slice - return null so omitempty works if len(r) == 0 { - logger.Infof("RFC 7591: Marshaling empty RequestScopeList -> null (will be omitted)") + logger.Debugf("RFC 7591: Marshaling empty RequestScopeList -> null (will be omitted)") return []byte("null"), nil } @@ -50,7 +50,7 @@ func (r RequestScopeList) MarshalJSON() ([]byte, error) { scopeString := strings.Join(r, " ") result, err := json.Marshal(scopeString) if err == nil { - logger.Infof("RFC 7591: Marshaled RequestScopeList %v -> %q (space-delimited string)", []string(r), scopeString) + logger.Debugf("RFC 7591: Marshaled RequestScopeList %v -> %q (space-delimited string)", []string(r), scopeString) } return result, err } From 5e93d6df99b2ad0fc8bfc639fe3651006399dbed Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:28:15 -0600 Subject: [PATCH 4/7] Simplify RequestScopeList.MarshalJSON by removing unnecessary null check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation returned null for empty slices to work with omitempty, but testing revealed that omitempty checks the Go value (empty slice) BEFORE calling MarshalJSON. This means: 1. Empty/nil slices are omitted by omitempty at the struct level 2. MarshalJSON is never called for empty slices 3. The null return was dead code Simplified implementation: - Removed unnecessary empty slice check and null return - MarshalJSON now only handles non-empty slices - Added comment explaining omitempty handles empty case - Updated tests to reflect actual behavior (empty -> "") All tests pass, including RFC 7591 compliance validation against Atlassian's production OAuth server. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/auth/oauth/dynamic_registration.go | 9 ++------- pkg/auth/oauth/dynamic_registration_test.go | 14 ++++++++------ 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index 51a741b3c..bcdaa7963 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -39,14 +39,9 @@ type RequestScopeList []string // MarshalJSON implements custom encoding for RequestScopeList. It converts the slice // of scopes into a space-delimited string as required by RFC 7591 Section 2. +// Note: Empty slices are handled by omitempty at the struct level before this is called. func (r RequestScopeList) MarshalJSON() ([]byte, error) { - // Handle nil or empty slice - return null so omitempty works - if len(r) == 0 { - logger.Debugf("RFC 7591: Marshaling empty RequestScopeList -> null (will be omitted)") - return []byte("null"), nil - } - - // Join scopes with spaces and marshal as a string + // Join scopes with spaces and marshal as a string (RFC 7591 Section 2) scopeString := strings.Join(r, " ") result, err := json.Marshal(scopeString) if err == nil { diff --git a/pkg/auth/oauth/dynamic_registration_test.go b/pkg/auth/oauth/dynamic_registration_test.go index 1cb45db3b..a0aaf2ee4 100644 --- a/pkg/auth/oauth/dynamic_registration_test.go +++ b/pkg/auth/oauth/dynamic_registration_test.go @@ -448,18 +448,18 @@ func TestRequestScopeList_MarshalJSON(t *testing.T) { name string scopes RequestScopeList wantJSON string - wantOmit bool // If true, expect null (which triggers omitempty) + wantOmit bool // If true, expect omitempty to hide the field }{ { - name: "nil scopes => null (omitempty will hide)", + name: "nil scopes => empty string (omitempty will hide at struct level)", scopes: nil, - wantJSON: `null`, + wantJSON: `""`, wantOmit: true, }, { - name: "empty slice => null (omitempty will hide)", + name: "empty slice => empty string (omitempty will hide at struct level)", scopes: RequestScopeList{}, - wantJSON: `null`, + wantJSON: `""`, wantOmit: true, }, { @@ -495,6 +495,8 @@ func TestRequestScopeList_MarshalJSON(t *testing.T) { 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 RequestScopeList `json:"scope,omitempty"` @@ -502,7 +504,7 @@ func TestRequestScopeList_MarshalJSON(t *testing.T) { s := testStruct{Scope: tt.scopes} structJSON, err := json.Marshal(s) require.NoError(t, err) - assert.Equal(t, "{}", string(structJSON), "omitempty should hide null scope field") + assert.Equal(t, "{}", string(structJSON), "omitempty should hide empty scope field") } }) } From b01a08a36e62700b85d0449407294b6178acc853 Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:30:05 -0600 Subject: [PATCH 5/7] Add detailed comment explaining omitempty mechanism with MarshalJSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clarifies that: - Go's encoding/json evaluates omitempty by checking the Go value - Empty slices (len == 0) are checked BEFORE calling MarshalJSON - MarshalJSON is never invoked for empty slices - Therefore no need to return null or handle empty case This documentation helps future developers understand the interaction between custom marshalers and omitempty, preventing confusion about why we don't check for empty slices. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/auth/oauth/dynamic_registration.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index bcdaa7963..b247311b8 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -39,7 +39,14 @@ type RequestScopeList []string // MarshalJSON implements custom encoding for RequestScopeList. It converts the slice // of scopes into a space-delimited string as required by RFC 7591 Section 2. -// Note: Empty slices are handled by omitempty at the struct level before this is called. +// +// 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 (r RequestScopeList) MarshalJSON() ([]byte, error) { // Join scopes with spaces and marshal as a string (RFC 7591 Section 2) scopeString := strings.Join(r, " ") From c5fd7b37f0636b881a84f5d124c84318f3c7b865 Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:43:56 -0600 Subject: [PATCH 6/7] Per PR feedback: add validation to prevent scopes with spaces --- pkg/auth/oauth/dynamic_registration.go | 8 ++++++ pkg/auth/oauth/dynamic_registration_test.go | 27 +++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index b247311b8..351ab7915 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -195,6 +195,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 a0aaf2ee4..862b9356e 100644 --- a/pkg/auth/oauth/dynamic_registration_test.go +++ b/pkg/auth/oauth/dynamic_registration_test.go @@ -354,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 { From eeeb8a64fff6278180c1f5d037ee38209268ce4c Mon Sep 17 00:00:00 2001 From: Jon Christiansen <467023+theJC@users.noreply.github.com> Date: Sun, 16 Nov 2025 18:21:03 -0600 Subject: [PATCH 7/7] Remove RequestScopeList type and use ScopeList for both requests and responses --- pkg/auth/oauth/dynamic_registration.go | 81 ++++++++++----------- pkg/auth/oauth/dynamic_registration_test.go | 18 ++--- 2 files changed, 47 insertions(+), 52 deletions(-) diff --git a/pkg/auth/oauth/dynamic_registration.go b/pkg/auth/oauth/dynamic_registration.go index 351ab7915..fa3f4fb98 100644 --- a/pkg/auth/oauth/dynamic_registration.go +++ b/pkg/auth/oauth/dynamic_registration.go @@ -27,47 +27,17 @@ const ResponseTypeCode = "code" // TokenEndpointAuthMethodNone is the token endpoint auth method for none const TokenEndpointAuthMethodNone = "none" -// RequestScopeList represents the "scope" field in a dynamic client registration request. -// Per RFC 7591 Section 2, this must be marshaled as a space-delimited string, not a JSON array. -// -// Examples of marshaled output: -// -// []string{"openid", "profile", "email"} → "openid profile email" -// []string{"openid"} → "openid" -// nil or []string{} → omitted (via omitempty) -type RequestScopeList []string - -// MarshalJSON implements custom encoding for RequestScopeList. 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 (r RequestScopeList) MarshalJSON() ([]byte, error) { - // Join scopes with spaces and marshal as a string (RFC 7591 Section 2) - scopeString := strings.Join(r, " ") - result, err := json.Marshal(scopeString) - if err == nil { - logger.Debugf("RFC 7591: Marshaled RequestScopeList %v -> %q (space-delimited string)", []string(r), scopeString) - } - return result, err -} - // DynamicClientRegistrationRequest represents the request for dynamic client registration (RFC 7591) type DynamicClientRegistrationRequest struct { // Required field according to RFC 7591 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 RequestScopeList `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 @@ -87,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. diff --git a/pkg/auth/oauth/dynamic_registration_test.go b/pkg/auth/oauth/dynamic_registration_test.go index 862b9356e..6b564a5d9 100644 --- a/pkg/auth/oauth/dynamic_registration_test.go +++ b/pkg/auth/oauth/dynamic_registration_test.go @@ -466,14 +466,14 @@ func TestDynamicClientRegistrationResponse_Validation(t *testing.T) { // TestIsLocalhost is already defined in oidc_test.go -// TestRequestScopeList_MarshalJSON tests that the RequestScopeList marshaling works correctly +// TestScopeList_MarshalJSON tests that the ScopeList marshaling works correctly // and produces RFC 7591 compliant space-delimited strings. -func TestRequestScopeList_MarshalJSON(t *testing.T) { +func TestScopeList_MarshalJSON(t *testing.T) { t.Parallel() tests := []struct { name string - scopes RequestScopeList + scopes ScopeList wantJSON string wantOmit bool // If true, expect omitempty to hide the field }{ @@ -485,28 +485,28 @@ func TestRequestScopeList_MarshalJSON(t *testing.T) { }, { name: "empty slice => empty string (omitempty will hide at struct level)", - scopes: RequestScopeList{}, + scopes: ScopeList{}, wantJSON: `""`, wantOmit: true, }, { name: "single scope => string", - scopes: RequestScopeList{"openid"}, + scopes: ScopeList{"openid"}, wantJSON: `"openid"`, }, { name: "two scopes => space-delimited string", - scopes: RequestScopeList{"openid", "profile"}, + scopes: ScopeList{"openid", "profile"}, wantJSON: `"openid profile"`, }, { name: "three scopes => space-delimited string", - scopes: RequestScopeList{"openid", "profile", "email"}, + scopes: ScopeList{"openid", "profile", "email"}, wantJSON: `"openid profile email"`, }, { name: "scopes with special characters", - scopes: RequestScopeList{"read:user", "write:repo"}, + scopes: ScopeList{"read:user", "write:repo"}, wantJSON: `"read:user write:repo"`, }, } @@ -526,7 +526,7 @@ func TestRequestScopeList_MarshalJSON(t *testing.T) { // so empty slices are omitted regardless of what MarshalJSON returns. if tt.wantOmit { type testStruct struct { - Scope RequestScopeList `json:"scope,omitempty"` + Scope ScopeList `json:"scope,omitempty"` } s := testStruct{Scope: tt.scopes} structJSON, err := json.Marshal(s)