feat: implement RFC9728 OAuth Protected Resource Metadata discovery #637
Conversation
WalkthroughAdds RFC9728 protected resource metadata discovery and propagation into client transports and tests; exposes a client-level alias and helpers for authorization-required errors; adds OAuthConfig ProtectedResourceMetadataURL and updates docs/examples to surface discovered metadata URL. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/oauth_client/README.md (1)
7-12: Fix helper name in README example to match exported APIThe RFC9728 section shows:
if metadataURL := client.GetDiscoveredMetadataURL(err); metadataURL != "" { fmt.Printf("Server provided OAuth metadata URL: %s\n", metadataURL) }but the exported helper is
client.GetResourceMetadataURL(err), and the main example file uses that name. Please rename the README snippet toGetResourceMetadataURL(or add a real alias) so documentation matches the actual API.Also applies to: 29-41, 63-76
🧹 Nitpick comments (4)
client/transport/sse.go (2)
166-187: 401 handling and metadata propagation look correct; optionally clean up response channels on errorThe new 401 branches correctly:
- Extract
resource_metadatafromWWW-AuthenticateviaextractResourceMetadataURL.- Return
OAuthAuthorizationRequiredError(whenoauthHandleris present) embeddingAuthorizationRequiredErrorwith the discoveredResourceMetadataURL.- Fall back to
AuthorizationRequiredErrorfor non-OAuth SSE usage.That matches the intended RFC9728 behavior and is consistent across
Start,SendRequest, andSendNotification.One optional improvement for
SendRequest: in the non-success status paths (including this new 401 branch),responses[idKey]remains registered inc.responses, sincedeleteResponseChan()is not called before returning. This was already true for non-401 errors but now applies to 401 as well. If you expect many failed requests, you may want to invokedeleteResponseChan()before returning to avoid leaving stale entries in the map.Also applies to: 443-461, 580-597
143-159: Replace string comparisons witherrors.Is()for sentinel error checkingYou correctly identified that
SendNotificationuseserrors.Is(err, ErrOAuthAuthorizationRequired)at line 551, but bothStart(line 148) andSendRequest(line 394) still use brittle string comparison:if err.Error() == "no valid token available, authorization required" {The sentinel error
ErrOAuthAuthorizationRequiredis defined instreamable_http.go:238and is available insse.go(which imports theerrorspackage). Switching toerrors.Is()makes the code robust to wrapped errors, consistent with theSendNotificationpattern, and aligns with the coding guidelines mandate to useerrors.Is/Aswith sentinel errors.Update lines 148 and 394 to use:
if errors.Is(err, ErrOAuthAuthorizationRequired) {client/oauth_test.go (1)
129-211: Migrate new tests to testify for consistency with codebase testing patternsThe codebase has established testify v1.9.0 as the standard testing library across
_test.gofiles (imports found in 30+ test files includingserver/,mcp/, andclient/packages).
client/oauth_test.gocurrently uses manualif/t.Errorf()assertions in the newTestGetResourceMetadataURLandTestIsAuthorizationRequiredErrortests. Refactor these to usetestify/assertand/ortestify/requirefor consistency:
- Replace
if condition { t.Errorf(...) }patterns withassert.Equal(),assert.Empty(), etc.- Use
requirefor assertions that must pass to continue the test safely.This aligns with the coding guideline and improves readability of test failures across the codebase.
client/transport/streamable_http_oauth_test.go (1)
287-334: Migrate test file to testify for consistency with codebase patternsThe codebase extensively uses
testify/assertandtestify/requireacross test files, butclient/transport/streamable_http_oauth_test.gousest.Errorf()throughout. The newTestExtractResourceMetadataURLtest follows the existing pattern in this file, but to align with the coding guidelines and the established convention across the codebase, you should migrate this test file (or at minimum the new test) to use testify assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/oauth.go(2 hunks)client/oauth_test.go(1 hunks)client/transport/oauth.go(2 hunks)client/transport/sse.go(6 hunks)client/transport/sse_oauth_test.go(1 hunks)client/transport/streamable_http.go(4 hunks)client/transport/streamable_http_oauth_test.go(1 hunks)examples/oauth_client/README.md(3 hunks)examples/oauth_client/main.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/oauth.goexamples/oauth_client/main.goclient/oauth_test.goclient/transport/sse.goclient/transport/streamable_http_oauth_test.goclient/transport/sse_oauth_test.goclient/transport/streamable_http.goclient/transport/oauth.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/oauth_test.goclient/transport/streamable_http_oauth_test.goclient/transport/sse_oauth_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/oauth_test.go
🧬 Code graph analysis (7)
client/oauth.go (1)
client/transport/streamable_http.go (2)
AuthorizationRequiredError(266-268)OAuthAuthorizationRequiredError(280-283)
examples/oauth_client/main.go (1)
client/oauth.go (1)
GetResourceMetadataURL(91-105)
client/oauth_test.go (3)
client/oauth.go (6)
OAuthAuthorizationRequiredError(64-64)OAuthConfig(11-11)AuthorizationRequiredError(61-61)GetResourceMetadataURL(91-105)IsAuthorizationRequiredError(67-70)IsOAuthAuthorizationRequiredError(73-76)client/transport/streamable_http.go (2)
OAuthAuthorizationRequiredError(280-283)AuthorizationRequiredError(266-268)client/transport/oauth.go (2)
NewOAuthHandler(157-169)OAuthConfig(21-44)
client/transport/sse.go (2)
client/oauth.go (2)
AuthorizationRequiredError(61-61)OAuthAuthorizationRequiredError(64-64)client/transport/streamable_http.go (2)
AuthorizationRequiredError(266-268)OAuthAuthorizationRequiredError(280-283)
client/transport/streamable_http_oauth_test.go (3)
client/oauth.go (5)
NewMemoryTokenStore(23-23)Token(14-14)OAuthConfig(11-11)TokenStore(17-17)OAuthAuthorizationRequiredError(64-64)client/transport/oauth.go (4)
NewMemoryTokenStore(99-101)Token(69-82)OAuthConfig(21-44)TokenStore(55-66)client/transport/streamable_http.go (3)
NewStreamableHTTP(131-161)WithHTTPOAuth(65-69)OAuthAuthorizationRequiredError(280-283)
client/transport/sse_oauth_test.go (3)
client/transport/oauth.go (4)
NewMemoryTokenStore(99-101)Token(69-82)OAuthConfig(21-44)TokenStore(55-66)client/transport/sse.go (2)
NewSSE(85-112)WithOAuth(77-81)client/transport/streamable_http.go (1)
OAuthAuthorizationRequiredError(280-283)
client/transport/streamable_http.go (2)
client/oauth.go (2)
AuthorizationRequiredError(61-61)OAuthAuthorizationRequiredError(64-64)client/transport/oauth.go (1)
OAuthHandler(144-154)
🔇 Additional comments (9)
client/transport/sse_oauth_test.go (1)
243-304: SSE OAuth metadata discovery test exercises the right 401/error pathThe test setup and assertions correctly mirror the SSE
Start401 handling (WWW-Authenticate →ResourceMetadataURLonOAuthAuthorizationRequiredError), and the use of a valid token plus forced 401 ensures the discovery path is actually hit. No changes needed here.client/transport/streamable_http_oauth_test.go (1)
222-285: Streamable HTTP OAuth metadata discovery test aligns with 401 handlingThe server fixture, 401 response, and assertion against
OAuthAuthorizationRequiredError.ResourceMetadataURLline up with the newSendRequest401 logic and give solid coverage of the RFC9728 path. Looks good.client/transport/streamable_http.go (2)
240-277: AuthorizationRequiredError and metadata helper are well-structured for RFC9728Defining
ErrAuthorizationRequired,AuthorizationRequiredError(withResourceMetadataURL), and the smallextractResourceMetadataURLhelper gives a clear, reusable surface for 401 handling. TheError/Unwrapmethods follow the sentinel-error pattern, so callers can use botherrors.Is(err, ErrAuthorizationRequired)and higher-level helpers likeIsAuthorizationRequiredError/GetResourceMetadataURL.Given the tests in
client/transport/streamable_http_oauth_test.go, this looks solid.
330-349: Streamable HTTP 401 handling and OAuth error propagation look consistentFor both
SendRequestandSendNotification, the new 401 branches:
- Extract
resource_metadatafromWWW-Authenticate.- Return
OAuthAuthorizationRequiredErrorwith populatedResourceMetadataURLwhenoauthHandleris set.- Otherwise, return
AuthorizationRequiredErrorwith the same URL.Additionally,
sendHTTPnow short-circuits whenGetAuthorizationHeaderreturnsErrOAuthAuthorizationRequiredby constructing anOAuthAuthorizationRequiredError(with empty metadata, since there’s no response yet). This matches how higher layers detect and drive the OAuth flow while still allowingGetResourceMetadataURLto succeed when a real 401 response provided metadata.The behavior is coherent across request and notification paths; I don’t see issues here.
Also applies to: 433-445, 618-637
examples/oauth_client/main.go (1)
111-115: Example correctly surfaces discovered metadata URL via helperUsing
client.GetResourceMetadataURL(err)inside theIsOAuthAuthorizationRequiredErrorbranch is the right way to expose the RFC9728resource_metadataURL to users, and the messaging makes the new behavior clear without changing the control flow. No further changes needed here.client/transport/oauth.go (1)
35-38: ProtectedResourceMetadataURL wiring and discovery priority look correctAdding
ProtectedResourceMetadataURLtoOAuthConfigwith the updatedgetServerMetadatalogic gives a clear precedence:
- Use
AuthServerMetadataURLwhen explicitly configured.- Otherwise, if
ProtectedResourceMetadataURLis set (e.g., from RFC9728resource_metadata), use that.- Otherwise, fall back to
baseURL + "/.well-known/oauth-protected-resource".The subsequent fallbacks to authorization-server discovery and default endpoints remain intact. This aligns with the described RFC9728 behavior and should integrate cleanly with the new error-surfaced metadata.
Also applies to: 358-375
client/oauth.go (3)
60-61: LGTM! Clean exposure of transport-level error.The type alias appropriately exposes the
AuthorizationRequiredErrorat the client package boundary, following the same pattern as other convenience types in this file. The GoDoc comment is clear and properly formatted.
66-70: LGTM! Standard error checking helper.The implementation correctly uses
errors.Asto check forAuthorizationRequiredError, following Go idioms and maintaining consistency with the existingIsOAuthAuthorizationRequiredErrorhelper on lines 72-76.
87-105: LGTM! Well-implemented metadata URL extraction.The function correctly extracts the
ResourceMetadataURLfrom authorization errors with proper handling of both error types. The order of checking (OAuth-specific first, then base) is correct sinceOAuthAuthorizationRequiredErrorembedsAuthorizationRequiredError. The comprehensive GoDoc comment clearly explains the purpose and RFC9728 context. The function safely handles nil errors and missing metadata URLs by returning an empty string.
|
I've reviewed the changes and found a critical logic gap that prevents the discovered metadata URL from actually being used.
@coderabbitai what is your opinion on these findings, particularly regarding how we should safely update the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 661 🏁 Script executed: Length of output: 6895 🏁 Script executed: Length of output: 1067 🏁 Script executed: Length of output: 4885 🏁 Script executed: Length of output: 533 🏁 Script executed: Length of output: 63 🏁 Script executed: Length of output: 387 🏁 Script executed: Length of output: 237 🏁 Script executed: Length of output: 797 🏁 Script executed: Length of output: 335 🏁 Script executed: Length of output: 6811 @ezynda3, your analysis is spot-on. All three concerns represent critical architectural issues in the current implementation. Let me address each: 1. Discovered URL is Never Used ✅ ValidYou're absolutely correct. The example demonstrates this gap perfectly: if metadataURL := client.GetResourceMetadataURL(err); metadataURL != "" {
fmt.Printf("Server provided OAuth metadata URL: %s\n", metadataURL)
fmt.Println("The client will automatically use this URL for OAuth configuration.")
}This comment is misleading. The URL is printed but never used. The code continues with the same
The intended workflow appears to be:
But this is neither documented nor demonstrated in the example. 2.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 782 🏁 Script executed: Length of output: 4082 🏁 Script executed: Length of output: 167 🏁 Script executed: Length of output: 28970 🏁 Script executed: Length of output: 856 @ezynda3, excellent analysis! All three concerns are architecturally critical and demonstrate a fundamental gap in the implementation. Let me validate each: 1. Discovered URL is Never Used ✅ CriticalYour assessment is spot-on. The flow is broken: What happens:
The problem:
Actual workflow required (undocumented): // Current code just prints it:
if metadataURL := client.GetResourceMetadataURL(err); metadataURL != "" {
// Must create NEW client with updated config
newConfig := originalConfig
newConfig.ProtectedResourceMetadataURL = metadataURL
newClient, _ := client.NewClient(/* ... */, client.WithOAuth(newConfig))
// Retry with new client
}2.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23445 🏁 Script executed: Length of output: 3493 🏁 Script executed: Length of output: 1420 🏁 Script executed: Length of output: 387 🏁 Script executed: Length of output: 0 @ezynda3, your analysis is absolutely correct on all three points. Let me address the The sync.Once Problem: Why It's Fundamentally BrokenAfter examining the code and Go's Timeline of the problem:
The Go stdlib has no Reset() - There is not a method to reset the state of sync.Once, and proposals to add one were rejected. Attempting to reset it manually is dangerous and can cause mutex corruption. Root Cause: Architectural MismatchThe RFC9728 discovery workflow is: But the current code assumes: These are incompatible patterns. Concrete SolutionsOption 1: Lazy Discovery (Recommended)Don't call func (h *OAuthHandler) getServerMetadata(ctx context.Context) (*AuthServerMetadata, error) {
h.metadataOnce.Do(func() {
// Check if we have a discovered URL from a previous 401 response
if h.config.ProtectedResourceMetadataURL != "" {
h.fetchMetadataFromURL(ctx, h.config.ProtectedResourceMetadataURL)
return
}
// ... existing logic
})
return h.serverMetadata, h.metadataFetchErr
}Problem: This only works if Option 2: Make OAuthHandler RecreatableSince handlers can't be reset, make creating a new one cheap and easy: // In client code after catching 401
if metadataURL := client.GetResourceMetadataURL(err); metadataURL != "" {
// Create NEW handler with discovered URL
newConfig := oauthConfig // copy
newConfig.ProtectedResourceMetadataURL = metadataURL
// Replace handler in transport
transport.UpdateOAuthHandler(NewOAuthHandler(newConfig))
// Retry connection
client.Connect(ctx)
}Requires: Adding Option 3: Remove sync.Once EntirelyReplace with a mutex-protected lazy initialization that can be reset: type OAuthHandler struct {
config OAuthConfig
httpClient *http.Client
metadataMu sync.RWMutex
serverMetadata *AuthServerMetadata
metadataFetchErr error
metadataFetched bool // NOT sync.Once
// ...
}
func (h *OAuthHandler) getServerMetadata(ctx context.Context) (*AuthServerMetadata, error) {
// Fast path: already fetched
h.metadataMu.RLock()
if h.metadataFetched {
defer h.metadataMu.RUnlock()
return h.serverMetadata, h.metadataFetchErr
}
h.metadataMu.RUnlock()
// Slow path: fetch with write lock
h.metadataMu.Lock()
defer h.metadataMu.Unlock()
// Double-check after acquiring write lock
if h.metadataFetched {
return h.serverMetadata, h.metadataFetchErr
}
// ... fetch logic ...
h.metadataFetched = true
return h.serverMetadata, h.metadataFetchErr
}
func (h *OAuthHandler) ResetMetadata() {
h.metadataMu.Lock()
defer h.metadataMu.Unlock()
h.serverMetadata = nil
h.metadataFetchErr = nil
h.metadataFetched = false
}
func (h *OAuthHandler) UpdateProtectedResourceURL(url string) {
h.config.ProtectedResourceMetadataURL = url
h.ResetMetadata() // Force re-fetch with new URL
}This allows dynamic updates while maintaining thread safety. Regarding the Fragile Header ParsingThe naive string matching with func extractResourceMetadataURL(wwwAuthHeader string) string {
// Use proper auth-param parsing
re := regexp.MustCompile(`\bresource_metadata="([^"\\]*(?:\\.[^"\\]*)*)"`)
matches := re.FindStringSubmatch(wwwAuthHeader)
if len(matches) > 1 {
// Handle escaped quotes in the URL
return strings.ReplaceAll(matches[1], `\"`, `"`)
}
return ""
}Recommended Path Forward
Would you like me to open an issue tracking these architectural changes? |
Add support for discovering OAuth authorization server metadata from WWW-Authenticate headers per RFC9728 Section 5.1. The MCP spec indicates that servers should return a 401 Unauthorized response with a WWW-Authenticate header containing the resource_metadata parameter. This parameter is used to discover the OAuth authorization server metadata. This change adds support for this discovery, allowing clients to automatically extract the OAuth metadata URL from the WWW-Authenticate header and use it to discover the OAuth authorization server configuration, rather than relying on it being on the /.well-known path of the base URL, which is not always the case (for example, https://mcp.linear.app/mcp/.well-known/oauth-protected-resource vs https://mcp.honeycomb.io/.well-known/oauth-protected-resource - note the lack of /mcp in one of these, even though both servers expect the /mcp path in the base URL). Changes: - Add AuthorizationRequiredError base error type with ResourceMetadataURL field - Add OAuthAuthorizationRequiredError that embeds AuthorizationRequiredError - Add ProtectedResourceMetadataURL to OAuthConfig for explicit configuration - Extract resource_metadata parameter from WWW-Authenticate headers in both streamable_http and sse transports - Update getServerMetadata() to use ProtectedResourceMetadataURL when provided - Add helper functions: IsAuthorizationRequiredError(), GetResourceMetadataURL() - Add comprehensive tests for metadata URL extraction and usage - Update OAuth example to demonstrate RFC9728 discovery This allows clients to properly discover OAuth endpoints when servers return 401 responses with WWW-Authenticate headers containing resource_metadata URLs, enabling correct OAuth flows without requiring well-known URL assumptions. RFC9728: https://datatracker.ietf.org/doc/html/rfc9728
sd2k
force-pushed
the
oauth-allow-providing-protected-resource-url
branch
from
9ce87a0 to
238a36d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/transport/oauth.go (1)
349-457: sync.Once prevents using discovered metadata URL.
getServerMetadata()is wrapped inh.metadataOnce.Do(), meaning it executes at most once. If metadata fetching occurs before receiving a 401 response with aresource_metadataparameter:
- The first call to
getServerMetadata()completes (successfully or not).sync.Onceprevents subsequent calls from re-executing the discovery logic.- Even if a
resource_metadataURL is later discovered and set in config (which currently isn't possible), it won't be used becauseDo()won't re-run.To support RFC9728 discovery, replace
sync.Oncewith a mutex-protected fetch flag and add methods likeResetMetadata()to allow safe re-fetching after discovery.Based on PR comments discussion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/oauth.go(2 hunks)client/oauth_test.go(1 hunks)client/transport/oauth.go(2 hunks)client/transport/sse.go(6 hunks)client/transport/sse_oauth_test.go(1 hunks)client/transport/streamable_http.go(4 hunks)client/transport/streamable_http_oauth_test.go(1 hunks)examples/oauth_client/README.md(3 hunks)examples/oauth_client/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/transport/streamable_http_oauth_test.go
- examples/oauth_client/main.go
- client/transport/sse_oauth_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/streamable_http.goclient/oauth_test.goclient/oauth.goclient/transport/sse.goclient/transport/oauth.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/oauth_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/oauth_test.go
🧬 Code graph analysis (3)
client/oauth_test.go (2)
client/oauth.go (6)
OAuthAuthorizationRequiredError(64-64)OAuthConfig(11-11)AuthorizationRequiredError(61-61)GetResourceMetadataURL(91-105)IsAuthorizationRequiredError(67-70)IsOAuthAuthorizationRequiredError(73-76)client/transport/streamable_http.go (2)
OAuthAuthorizationRequiredError(280-283)AuthorizationRequiredError(266-268)
client/oauth.go (1)
client/transport/streamable_http.go (2)
AuthorizationRequiredError(266-268)OAuthAuthorizationRequiredError(280-283)
client/transport/sse.go (2)
client/oauth.go (2)
AuthorizationRequiredError(61-61)OAuthAuthorizationRequiredError(64-64)client/transport/streamable_http.go (2)
AuthorizationRequiredError(266-268)OAuthAuthorizationRequiredError(280-283)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (5)
client/transport/streamable_http.go (3)
264-276: LGTM!
AuthorizationRequiredErroris well-structured with properError()andUnwrap()methods following Go conventions. TheResourceMetadataURLfield correctly captures RFC9728 metadata.
278-291: LGTM!Embedding
AuthorizationRequiredErrorinOAuthAuthorizationRequiredErroris the correct approach, enablingerrors.Asto match both error types and allowing metadata URL extraction from OAuth-specific errors.
330-349: Error construction is consistent but extracted URL is not used.The 401 handling correctly extracts
resource_metadataand constructs errors with the URL in bothSendRequestandSendNotification. The pattern is consistent:
- Call
extractResourceMetadataURL()onWWW-Authenticateheader.- Return
OAuthAuthorizationRequiredError(if handler exists) orAuthorizationRequiredErrorwith the URL.- Set empty URL when no response is available (line 442).
However, per the architectural issues flagged in
oauth.goreview, the discovered URL cannot currently be used because the config is immutable andsync.Onceprevents metadata re-fetching.The error construction is correct, but verify the end-to-end flow once the architectural issues are resolved.
Based on PR comments discussion.
Also applies to: 438-444, 619-638
client/transport/sse.go (1)
168-187: LGTM! Consistent 401 handling with metadata extraction.The RFC9728 metadata discovery is consistently implemented across
Start(),SendRequest(), andSendNotification():
- Extracts
resource_metadatafromWWW-Authenticateheader.- Returns appropriate error type based on OAuth handler presence.
- Embeds discovered URL in
AuthorizationRequiredError.The implementation matches
streamable_http.gofor consistency across transports. However, note the architectural issues flagged inoauth.goreview: the discovered URL cannot currently be used due to immutable config andsync.Onceguard.Based on PR comments discussion.
Also applies to: 443-466, 579-598
client/oauth_test.go (1)
129-211: Tests verify helper functions but miss end-to-end discovery-then-retry flow.The tests at lines 129-211 correctly verify
GetResourceMetadataURL()andIsAuthorizationRequiredError()helper functions. Existing integration tests (TestStreamableHTTP_OAuthMetadataDiscoveryandTestSSE_OAuthMetadataDiscovery) verify metadata extraction from the 401 response, but neither test verifies the complete workflow:
- Extract metadata URL from WWW-Authenticate header
- Fetch metadata from that URL
- Retry request with proper authorization
The
ProtectedResourceMetadataURLfield exists inOAuthConfigbut no metadata fetching or automatic retry logic is currently implemented. Per PR discussion context mentioning architectural issues, adding this integration test may depend on completing the underlying discovery-then-retry mechanism.
| // AuthorizationRequiredError is returned when a 401 Unauthorized response is received | ||
| type AuthorizationRequiredError = transport.AuthorizationRequiredError | ||
|
|
||
| // OAuthAuthorizationRequiredError is returned when OAuth authorization is required | ||
| type OAuthAuthorizationRequiredError = transport.OAuthAuthorizationRequiredError | ||
|
|
||
| // IsAuthorizationRequiredError checks if an error is an AuthorizationRequiredError | ||
| func IsAuthorizationRequiredError(err error) bool { | ||
| var target *AuthorizationRequiredError | ||
| return errors.As(err, &target) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Helper functions are correct but API is incomplete for RFC9728 workflow.
The error helpers correctly extract the discovered metadata URL, but there's no corresponding API to apply the discovered URL:
GetResourceMetadataURL(err)extracts the URL.- No
UpdateOAuthMetadataURL(client, url)or similar to reconfigure the handler.
For a complete RFC9728 implementation, add a helper to update the handler's ProtectedResourceMetadataURL and reset cached metadata, or document that users must create a new client with the discovered URL in the config.
Based on PR comments discussion.
Also applies to: 87-105
🤖 Prompt for AI Agents
In client/oauth.go around lines 60-70 (and also apply same change for the
related section at 87-105), the package exposes helpers to extract a discovered
Protected Resource Metadata URL from errors but lacks an API to apply that URL
to an existing client for RFC9728 flows; add a helper function (e.g.,
UpdateOAuthMetadataURL) that accepts the client instance and the discovered URL,
sets the client's ProtectedResourceMetadataURL field, and clears or resets any
cached metadata/memoized discovery state so the handler will reload metadata
from the new URL; ensure the helper validates the URL, updates the configuration
on the existing client safely (concurrent-safe if the client is used
concurrently), and document that callers should invoke this helper after
extracting the URL from GetResourceMetadataURL(err) to complete the RFC9728
workflow.
| // ProtectedResourceMetadataURL is the URL to the OAuth protected resource metadata | ||
| // per RFC9728. If set, this URL will be used to discover the authorization server. | ||
| // This is typically extracted from the WWW-Authenticate header's resource_metadata parameter. | ||
| ProtectedResourceMetadataURL string |
There was a problem hiding this comment.
Configuration field cannot be updated after discovery.
ProtectedResourceMetadataURL is part of the immutable OAuthConfig. When a metadata URL is discovered from a WWW-Authenticate header (per RFC9728), there is no mechanism to update this field in the handler's config. The discovered URL is extracted into the error but cannot trigger a re-fetch of server metadata because:
OAuthHandler.configis immutable.getServerMetadata()is guarded bysync.Once(line 351), preventing re-execution.
Consider adding UpdateProtectedResourceMetadataURL(url string) and ResetMetadata() methods to allow runtime discovery, or document that this field must be set at configuration time and discovery requires creating a new handler.
Based on PR comments discussion.
🤖 Prompt for AI Agents
In client/transport/oauth.go around lines 35 to 38, ProtectedResourceMetadataURL
is part of an immutable OAuthConfig so a metadata URL discovered at runtime
cannot be applied and getServerMetadata() is prevented from re-running by the
sync.Once guard; add runtime update support by (1) adding an exported method
UpdateProtectedResourceMetadataURL(url string) on the OAuthHandler that updates
a mutable field (or a small internal struct) holding the metadata URL, (2)
adding ResetMetadata() to clear any cached server metadata and reset the
sync.Once (or replace sync.Once with a mutex/atomic guard that supports
resetting), and (3) ensure getServerMetadata() reads the mutable metadata URL
and can re-fetch metadata after ResetMetadata() is called; alternatively,
document clearly that ProtectedResourceMetadataURL must be set at config time
and runtime discovery requires constructing a new handler.
| authHeader, err := c.oauthHandler.GetAuthorizationHeader(ctx) | ||
| if err != nil { | ||
| // If we get an authorization error, return a specific error that can be handled by the client | ||
| if err.Error() == "no valid token available, authorization required" { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use errors.Is instead of string comparison.
Lines 148 and 394 check err.Error() == "no valid token available, authorization required" which is fragile:
- String matching breaks if error message changes.
- Doesn't respect error wrapping.
- Inconsistent with line 553 which correctly uses
errors.Is(err, ErrOAuthAuthorizationRequired).
Replace with:
-if err.Error() == "no valid token available, authorization required" {
+if errors.Is(err, ErrOAuthAuthorizationRequired) {As per coding guidelines: "Error handling: return sentinel errors, wrap with fmt.Errorf, and check with errors.Is/As."
Also applies to: 394-394
🤖 Prompt for AI Agents
In client/transport/sse.go around lines 148 and 394, replace fragile string
equality checks of err.Error() == "no valid token available, authorization
required" with errors.Is(err, ErrOAuthAuthorizationRequired) so wrapped errors
are handled correctly; ensure the package imports the standard errors package if
not already imported and remove the string literal comparison, using the
sentinel ErrOAuthAuthorizationRequired (already used elsewhere) for consistent,
robust error checks.
| // extractResourceMetadataURL extracts the resource_metadata parameter from a WWW-Authenticate header | ||
| // per RFC9728 Section 5.1. Returns empty string if not found. | ||
| // Example: Bearer resource_metadata="https://resource.example.com/.well-known/oauth-protected-resource" | ||
| func extractResourceMetadataURL(wwwAuthHeader string) string { | ||
| // Parse: Bearer resource_metadata="https://..." ... | ||
| const prefix = "resource_metadata=\"" | ||
|
|
||
| idx := strings.Index(wwwAuthHeader, prefix) | ||
| if idx == -1 { | ||
| return "" | ||
| } | ||
|
|
||
| start := idx + len(prefix) | ||
| end := strings.Index(wwwAuthHeader[start:], "\"") | ||
| if end == -1 { | ||
| return "" | ||
| } | ||
|
|
||
| return wwwAuthHeader[start : start+end] | ||
| } |
There was a problem hiding this comment.
Header parsing is fragile and not RFC-compliant.
extractResourceMetadataURL uses naive substring matching (strings.Index) which can incorrectly match:
- Substrings inside other parameter values (e.g.,
Bearer foo="has resource_metadata=\"inside\"") - Parameters inside quoted strings with escapes
- Does not handle RFC 7235/7230 quoted-string semantics and escape sequences
Replace with proper auth-param parsing or a regex that handles quoted strings:
// Example regex approach:
var resourceMetadataRegex = regexp.MustCompile(`\bresource_metadata\s*=\s*"([^"\\]*(?:\\.[^"\\]*)*)"`)
func extractResourceMetadataURL(wwwAuthHeader string) string {
matches := resourceMetadataRegex.FindStringSubmatch(wwwAuthHeader)
if len(matches) < 2 {
return ""
}
// Unescape per RFC 7230 quoted-string rules
return strings.ReplaceAll(matches[1], `\"`, `"`)
}Based on PR comments discussion.
🤖 Prompt for AI Agents
In client/transport/streamable_http.go around lines 243-262, the current
substring-based parsing for resource_metadata is fragile and not RFC-compliant;
replace it with a robust auth-param parser using a regex that matches
resource_metadata with a quoted-string (e.g. a pattern like
\bresource_metadata\s*=\s*"([^"\\]*(?:\\.[^"\\]*)*)" ) to safely capture the
value, then unescape quoted-pair sequences per RFC 7230 (convert `\"` and other
backslash-escaped chars) before returning; ensure the function returns an empty
string when no match is found and add necessary imports (regexp, strings) and
unit tests for edge cases (escaped quotes, other params, missing param).
| ## Features | ||
|
|
||
| - OAuth 2.1 authentication with PKCE support | ||
| - **RFC9728 OAuth Protected Resource Metadata discovery** - Automatically discovers OAuth server metadata from `WWW-Authenticate` headers |
There was a problem hiding this comment.
Documentation overstates automatic discovery capabilities.
The documentation claims the client "automatically discovers" and "automatically extracts" the OAuth metadata URL, but per the PR comments discussion, the discovered URL extracted from WWW-Authenticate is not actually propagated back to the OAuthHandler or used in subsequent requests. The OAuthHandler.config is immutable and getServerMetadata() is guarded by sync.Once, preventing re-fetching even if a metadata URL is discovered.
Clarify that the URL is extracted and made available via the error but requires manual reconfiguration to be used.
Based on PR comments discussion.
Also applies to: 29-32
🤖 Prompt for AI Agents
In examples/oauth_client/README.md around lines 8 and 29-32, the wording
incorrectly states that RFC9728 metadata is "automatically discovers" and
"automatically extracts"/used; update the documentation to state that the
metadata URL is only extracted from WWW-Authenticate and surfaced in the error
(made available) but is not applied to OAuthHandler.config automatically —
callers must manually reconfigure or reinitialize the handler to use the
discovered URL; change the phrasing to “extracts and exposes the metadata URL
via the error; manual reconfiguration is required to apply it” and add a short
note explaining why (immutable config / sync.Once prevents automatic re-fetch).
| This example demonstrates automatic OAuth metadata discovery per [RFC9728](https://datatracker.ietf.org/doc/html/rfc9728). When the MCP server returns a 401 Unauthorized response with a `WWW-Authenticate` header containing the `resource_metadata` parameter, the client automatically extracts and uses this URL to discover the OAuth authorization server configuration. | ||
|
|
||
| The example code demonstrates this with: | ||
|
|
||
| ```go | ||
| // Check if server provided OAuth metadata URL via WWW-Authenticate header (RFC9728) | ||
| if metadataURL := client.GetDiscoveredMetadataURL(err); metadataURL != "" { | ||
| fmt.Printf("Server provided OAuth metadata URL: %s\n", metadataURL) | ||
| } | ||
| ``` | ||
|
|
||
| This makes the OAuth flow more robust and standards-compliant, as the server explicitly tells clients where to find OAuth configuration rather than relying on well-known locations. |
There was a problem hiding this comment.
Example code is misleading about automatic usage.
The example shows extracting the metadata URL but only prints it. It does not demonstrate how to actually use the discovered URL (e.g., creating a new client with ProtectedResourceMetadataURL configured). The statement that this "makes the OAuth flow more robust" is misleading since the discovered URL is not automatically applied to subsequent requests.
Show a complete example of using the discovered URL to reconfigure the client, or clarify that manual reconfiguration is required.
Based on PR comments discussion.
|
@ezynda3 roger, those points are valid, I think this is only working for us because we're using the API slightly weirdly (e.g. attempting to connect without OAuth first, so we already have an |
2 participants
Description
Add support for discovering OAuth authorization server metadata from WWW-Authenticate headers per RFC9728 Section 5.1.
The MCP spec indicates that servers should return a 401 Unauthorized response with a WWW-Authenticate header containing the resource_metadata parameter. This parameter is used to discover the OAuth authorization server metadata.
This change adds support for this discovery, allowing clients to automatically extract the OAuth metadata URL from the WWW-Authenticate header and use it to discover the OAuth authorization server configuration, rather than relying on it being on the /.well-known path of the base URL, which is not always the case (for example,
https://mcp.linear.app/mcp/.well-known/oauth-protected-resource vs https://mcp.honeycomb.io/.well-known/oauth-protected-resource - note the lack of /mcp in one of these, even though both servers expect the /mcp path in the base URL).
Changes:
This allows clients to properly discover OAuth endpoints when servers return 401 responses with WWW-Authenticate headers containing resource_metadata URLs, enabling correct OAuth flows without requiring well-known URL assumptions.
RFC9728: https://datatracker.ietf.org/doc/html/rfc9728
Type of Change
Checklist
MCP Spec Compliance
Additional Information
I'm using this fork locally, will mark as ready-for-review once I'm sure it works correctly.
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.