Add RFC 8707 resource indicator support to OAuth flow#725
Open
MariaChrysafis wants to merge 2 commits intomark3labs:mainfrom
Open
Add RFC 8707 resource indicator support to OAuth flow#725MariaChrysafis wants to merge 2 commits intomark3labs:mainfrom
MariaChrysafis wants to merge 2 commits intomark3labs:mainfrom
Conversation
Contributor
WalkthroughAdded an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MariaChrysafis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/transport/oauth.go (2)
214-220:⚠️ Potential issue | 🟠 MajorRefresh token request misses
resourceparameter.Line 214-220 builds refresh payload without
resource, while authorization-code exchange includes it (Line 655-657). This can cause inconsistent token behavior for resource-scoped flows.🔧 Proposed fix
data := url.Values{} data.Set("grant_type", "refresh_token") data.Set("refresh_token", refreshToken) data.Set("client_id", h.config.ClientID) + if metadata.Resource != "" { + data.Set("resource", metadata.Resource) + } if h.config.ClientSecret != "" { data.Set("client_secret", h.config.ClientSecret) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth.go` around lines 214 - 220, The refresh-token payload built in the refresh flow (in the function/method that constructs data := url.Values{} and sets grant_type, refresh_token, client_id, client_secret) is missing the optional "resource" parameter, causing mismatch with the authorization-code exchange which includes it; update that refresh-token request builder to also set "resource" when h.config.Resource (or the equivalent config field used in the auth-code flow) is non-empty so the same resource-scoped token semantics are preserved (mirror the same check used around lines 655-657).
417-450:⚠️ Potential issue | 🟠 Major
protectedResource.Resourceis parsed but never propagated toserverMetadata.After decoding on Line 411-415, discovery paths return metadata without copying the protected-resource identifier. That makes the new
resourcepropagation effectively no-op in common discovery/fallback paths.🔧 Proposed fix
@@ // If no authorization servers are specified, fall back to default endpoints if len(protectedResource.AuthorizationServers) == 0 { metadata, err := h.getDefaultEndpoints(baseURL) if err != nil { h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", err) return } + if metadata.Resource == "" && protectedResource.Resource != "" { + metadata.Resource = protectedResource.Resource + } h.serverMetadata = metadata return } @@ // Try OAuth Authorization Server Metadata first h.fetchMetadataFromURL(ctx, authServerURL+"/.well-known/oauth-authorization-server") if h.serverMetadata != nil { + if h.serverMetadata.Resource == "" && protectedResource.Resource != "" { + h.serverMetadata.Resource = protectedResource.Resource + } return } @@ // If OAuth Authorization Server Metadata discovery fails, try OpenID Connect discovery h.fetchMetadataFromURL(ctx, authServerURL+"/.well-known/openid-configuration") if h.serverMetadata != nil { + if h.serverMetadata.Resource == "" && protectedResource.Resource != "" { + h.serverMetadata.Resource = protectedResource.Resource + } return } @@ metadata, err := h.getDefaultEndpoints(authServerURL) if err != nil { h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", err) return } + if metadata.Resource == "" && protectedResource.Resource != "" { + metadata.Resource = protectedResource.Resource + } h.serverMetadata = metadata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth.go` around lines 417 - 450, The parsed protectedResource.Resource is never copied into the discovered metadata, so serverMetadata lacks the protected-resource identifier; update all assignment sites where h.serverMetadata is set (after fetchMetadataFromURL returns and after getDefaultEndpoints returns) to copy/propgate protectedResource.Resource into the metadata before assigning (e.g., set metadata.Resource = protectedResource.Resource or the appropriate field on the Metadata struct), and ensure the same propagation happens in both the default-endpoints branch and the OAuth/OpenID discovery branches so h.serverMetadata always carries the resource identifier; retain existing error handling using metadataFetchErr and reuse getDefaultEndpoints/fetchMetadataFromURL as currently called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/transport/oauth.go`:
- Around line 214-220: The refresh-token payload built in the refresh flow (in
the function/method that constructs data := url.Values{} and sets grant_type,
refresh_token, client_id, client_secret) is missing the optional "resource"
parameter, causing mismatch with the authorization-code exchange which includes
it; update that refresh-token request builder to also set "resource" when
h.config.Resource (or the equivalent config field used in the auth-code flow) is
non-empty so the same resource-scoped token semantics are preserved (mirror the
same check used around lines 655-657).
- Around line 417-450: The parsed protectedResource.Resource is never copied
into the discovered metadata, so serverMetadata lacks the protected-resource
identifier; update all assignment sites where h.serverMetadata is set (after
fetchMetadataFromURL returns and after getDefaultEndpoints returns) to
copy/propgate protectedResource.Resource into the metadata before assigning
(e.g., set metadata.Resource = protectedResource.Resource or the appropriate
field on the Metadata struct), and ensure the same propagation happens in both
the default-endpoints branch and the OAuth/OpenID discovery branches so
h.serverMetadata always carries the resource identifier; retain existing error
handling using metadataFetchErr and reuse
getDefaultEndpoints/fetchMetadataFromURL as currently called.
Contributor
Author
|
@ezynda3 can you take a look? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/transport/oauth.go`:
- Around line 567-570: In RegisterClient, the if block checking
metadata.Resource (the line with if metadata.Resource != "" { and the
regRequest["resource"] assignment) is missing its closing brace which breaks
compilation; add a closing brace immediately after the regRequest["resource"] =
metadata.Resource line so the metadata.Resource conditional is properly closed
before the subsequent if h.config.ClientURI != "" { block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d061eac6-f3f4-411e-9037-7a32e8100d6b
📒 Files selected for processing (1)
client/transport/oauth.go
Comment on lines
+567
to
570
| // Add resource parameter if available (RFC 8707) | ||
| if metadata.Resource != "" { | ||
| regRequest["resource"] = metadata.Resource | ||
| if h.config.ClientURI != "" { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, inspect the modified region around RegisterClient
echo "=== Lines 560-580 of client/transport/oauth.go ==="
nl -ba client/transport/oauth.go 2>/dev/null | sed -n '560,580p' || echo "File not found or error"
# Perform brace-balance check scoped to RegisterClient
echo -e "\n=== Brace balance check in RegisterClient ==="
python3 - <<'PY'
from pathlib import Path
try:
src = Path("client/transport/oauth.go").read_text()
start_idx = src.find("func (h *OAuthHandler) RegisterClient")
if start_idx == -1:
print("RegisterClient function not found")
else:
end_idx = src.find("// ErrInvalidState", start_idx)
if end_idx == -1:
end_idx = src.find("func", start_idx + 10)
block = src[start_idx:end_idx]
opens = block.count("{")
closes = block.count("}")
print(f"RegisterClient brace count: open={opens}, close={closes}")
if opens != closes:
print(f"❌ Mismatch detected: unclosed block exists")
else:
print(f"✓ Braces are balanced")
except Exception as e:
print(f"Error: {e}")
PYRepository: mark3labs/mcp-go
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the oauth.go file
echo "=== Locating oauth.go ==="
find . -name "oauth.go" -type f
# Read the actual file and show lines around 567-570
echo -e "\n=== Content around lines 560-580 ==="
FILE=$(find . -name "oauth.go" -type f | head -1)
if [ -n "$FILE" ]; then
cat -n "$FILE" | sed -n '560,580p'
echo -e "\n=== Extended context: lines 565-575 ==="
cat -n "$FILE" | sed -n '565,575p'
fiRepository: mark3labs/mcp-go
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="./client/transport/oauth.go"
echo "=== File info ==="
wc -l "$FILE"
echo -e "\n=== Checking if RegisterClient exists ==="
grep -n "func.*RegisterClient" "$FILE"
echo -e "\n=== Reading RegisterClient function ==="
START_LINE=$(grep -n "func.*RegisterClient" "$FILE" | cut -d: -f1)
if [ -n "$START_LINE" ]; then
# Read 100 lines starting from RegisterClient
sed -n "${START_LINE},$((START_LINE + 100))p" "$FILE" | cat -n
fiRepository: mark3labs/mcp-go
Length of output: 4195
Critical: Missing closing brace breaks compilation in RegisterClient.
Line 568 opens if metadata.Resource != "" { but it is never closed before the next if statement at line 570. The first if block requires a closing brace after line 569.
Suggested fix
// Add resource parameter if available (RFC 8707)
if metadata.Resource != "" {
regRequest["resource"] = metadata.Resource
+ }
if h.config.ClientURI != "" {
regRequest["client_uri"] = h.config.ClientURI
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Add resource parameter if available (RFC 8707) | |
| if metadata.Resource != "" { | |
| regRequest["resource"] = metadata.Resource | |
| if h.config.ClientURI != "" { | |
| // Add resource parameter if available (RFC 8707) | |
| if metadata.Resource != "" { | |
| regRequest["resource"] = metadata.Resource | |
| } | |
| if h.config.ClientURI != "" { | |
| regRequest["client_uri"] = h.config.ClientURI | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/transport/oauth.go` around lines 567 - 570, In RegisterClient, the if
block checking metadata.Resource (the line with if metadata.Resource != "" { and
the regRequest["resource"] assignment) is missing its closing brace which breaks
compilation; add a closing brace immediately after the regRequest["resource"] =
metadata.Resource line so the metadata.Resource conditional is properly closed
before the subsequent if h.config.ClientURI != "" { block.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Add RFC 8707 (Resource Indicators for OAuth 2.0) support to the OAuth flow.
When the authorization server metadata includes a resource field, it is now passed through as a parameter in:
This ensures tokens are properly scoped to the specific resource being accessed, which is required by authorization servers that protect multiple resources.
Type of Change
MCP spec compatibility implementation
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit