Fix RFC 7591 scope serialization format #2597
Open
+156
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 stacklok#2596 Signed-off-by: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2597 +/- ##
==========================================
+ Coverage 55.87% 55.88% +0.01%
==========================================
Files 311 311
Lines 29479 29488 +9
==========================================
+ Hits 16471 16480 +9
Misses 11569 11569
Partials 1439 1439 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
reviewed
Nov 15, 2025
| } | ||
|
|
||
| // Join scopes with spaces and marshal as a string | ||
| scopeString := strings.Join(r, " ") |
Contributor
There was a problem hiding this comment.
I know we haven't done that before but reading the code I wonder if we should check if the scopes have spaces in them and reject if yes?
Contributor
Author
There was a problem hiding this comment.
Good idea. I've added validation to prevent this from happening.
jhrozek
reviewed
Nov 15, 2025
| func (r RequestScopeList) MarshalJSON() ([]byte, error) { | ||
| // Handle nil or empty slice - return null so omitempty works | ||
| if len(r) == 0 { | ||
| return []byte("null"), nil |
Contributor
There was a problem hiding this comment.
is this to make Go's omitempty work?
Contributor
Author
There was a problem hiding this comment.
Excellent question. Ive subsequently dug into omitempty's implementation a bit further to understand how it would interact with this code, and was able to be simplify.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
ToolHive's dynamic client registration was sending the
scopeparameter as a JSON array instead of a space-delimited string, violating RFC 7591 Section 2.This caused registration failures with OAuth providers that strictly validate RFC 7591 compliance.
Changes
RequestScopeListtype with customMarshalJSONmethod to serialize scopes as space-delimited stringsDynamicClientRegistrationRequest.Scopesfield to useRequestScopeListTestRequestScopeList_MarshalJSONto verify RFC complianceRFC 7591 Compliance
Per RFC 7591 Section 2:
Before:
{ "scope": ["openid", "profile", "email"] }After (RFC 7591 compliant):
{ "scope": "openid profile email" }Real-World Testing
Tested successfully against Atlassian's Rovo MCP Server](https://support.atlassian.com/atlassian-rovo-mcp-server/docs/using-with-other-supported-mcp-clients/) and its OAuth server (
https://cf.mcp.atlassian.com):RFC 7591 Scope Marshaling Proof
This proves the fix correctly:
[openid profile email]to JSON string"openid profile email"The authorization URL also confirms correct format:
scope=openid+profile+email(URL-encoded space-delimited).Testing
TestRequestScopeList_MarshalJSONvalidates marshaling behaviorTestDynamicClientRegistrationRequest_ScopeSerializationverifies RFC complianceCompatibility
The response handling (
ScopeListtype) already supports both formats for maximum compatibility with various OAuth providers.Question
Do any mainstream mcp servers only support non-spec / array of strings behavior? If so, does toolhive customers need a config control over the format to allow customers to force non-spec behavior?
Fixes #2596
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com