feat: implement RFC9728 OAuth Protected Resource Metadata discovery
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:
- 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
Type of Change
- [ ] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [x] MCP spec compatibility implementation
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation update
- [ ] Code refactoring (no functional changes)
- [ ] Performance improvement
- [ ] Tests only (no functional changes)
- [ ] Other (please describe):
Checklist
- [x] My code follows the code style of this project
- [x] I have performed a self-review of my own code
- [x] I have added tests that prove my fix is effective or that my feature works
- [ ] I have updated the documentation accordingly
MCP Spec Compliance
- [x] This PR implements a feature defined in the MCP specification
- [x] Link to relevant spec section: Authorization server location
- [x] Implementation follows the specification exactly
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
- RFC9728 OAuth Protected Resource Metadata discovery support
- Automatic extraction and propagation of OAuth metadata URLs from server WWW-Authenticate headers
- New client helpers to detect authorization-required errors and retrieve discovered metadata
- New config option to explicitly specify protected resource metadata URL
-
Tests
- Added tests covering OAuth metadata discovery and related error paths
-
Documentation
- Updated example README and sample to show automatic metadata discovery and usage
โ๏ธ Tip: You can customize this high-level summary in your review settings.
Walkthrough
Adds 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
| Cohort / File(s) | Summary |
|---|---|
Client error helpers client/oauth.go |
Added public alias AuthorizationRequiredError = transport.AuthorizationRequiredError, IsAuthorizationRequiredError(err error) bool, and GetResourceMetadataURL(err error) string. |
Client tests client/oauth_test.go |
Added TestGetResourceMetadataURL, expanded TestIsAuthorizationRequiredError to cover base and OAuth-wrapped errors and metadata extraction. |
OAuth config / discovery client/transport/oauth.go |
Added ProtectedResourceMetadataURL string to OAuthConfig; getServerMetadata now prefers the explicit config value over the constructed well-known protected-resource URL. |
HTTP transport error plumbing client/transport/streamable_http.go |
Introduced AuthorizationRequiredError (with ResourceMetadataURL), extractResourceMetadataURL() helper, and updated 401 handling to attach resource metadata; extended OAuthAuthorizationRequiredError to embed the base error. |
SSE transport metadata handling client/transport/sse.go |
Updated Start, SendRequest, SendNotification to extract resource_metadata from WWW-Authenticate on 401 and return either OAuthAuthorizationRequiredError (when OAuth configured) or AuthorizationRequiredError with the metadata URL. |
Transport tests client/transport/sse_oauth_test.go, client/transport/streamable_http_oauth_test.go |
Added TestSSE_OAuthMetadataDiscovery, TestStreamableHTTP_OAuthMetadataDiscovery, and TestExtractResourceMetadataURL to validate RFC9728 metadata extraction and edge cases. |
Examples & docs examples/oauth_client/README.md, examples/oauth_client/main.go |
Documented automatic RFC9728 metadata discovery, updated flow steps and example usage showing retrieval of discovered metadata via GetResourceMetadataURL(err). |
Estimated code review effort
๐ฏ 3 (Moderate) | โฑ๏ธ ~25 minutes
- Verify embedding of
AuthorizationRequiredErrorintoOAuthAuthorizationRequiredErrorpreserves previous error unwrapping behavior. - Review
extractResourceMetadataURL()parsing for quoted values, multiple parameters, empty/malformed headers, and DPoP variations. - Check consistent propagation of
ResourceMetadataURLthrough SSE, SendRequest, SendNotification, and token-retrieval failure paths.
Possibly related PRs
- mark3labs/mcp-go#581 โ related changes to server metadata discovery logic in
client/transport/oauth.go(discovery URL selection/order). - mark3labs/mcp-go#296 โ earlier OAuth authorization-error work that this PR extends with a base error type and metadata propagation.
- mark3labs/mcp-go#340 โ concurrent OAuth integration changes touching the same transport code paths and tests.
Suggested labels
type: enhancement, area: mcp spec, status: needs submitter response
Suggested reviewers
- ezynda3
- giridhar-murthy-glean
- pottekkat
Pre-merge checks and finishing touches
โ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | โ ๏ธ Warning | Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
| Description check | โ Inconclusive | The description includes most required template sections with comprehensive details about the RFC9728 implementation, files changed, and MCP spec compliance. However, the documentation update checklist item is unchecked despite README modifications. | Check the 'Documentation update' box in the Type of Change section since examples/oauth_client/README.md was modified, or clarify if README changes should not count as documentation updates. |
โ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | โ Passed | The title accurately and concisely summarizes the main change: implementing RFC9728 OAuth Protected Resource Metadata discovery. |
โจ Finishing touches
- [ ] ๐ Generate docstrings
๐งช Generate unit tests (beta)
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
[!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.
- Provide your own instructions using the
high_level_summary_instructionssetting.- Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.Example instruction:
"Divide the high-level summary into five sections:
- ๐ Description โ Summarize the main change in 50โ60 words, explaining what was done.
- ๐ References โ List relevant issues, discussions, documentation, or related PRs.
- ๐ฆ Dependencies & Requirements โ Mention any new/updated dependencies, environment variable changes, or configuration updates.
- ๐ Contributor Summary โ Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |- โ๏ธ Additional Notes โ Add any extra reviewer context. Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."
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 help to get the list of available commands and usage tips.
I've reviewed the changes and found a critical logic gap that prevents the discovered metadata URL from actually being used.
- Discovered URL is Ignored: While the code extracts the
resource_metadataURL into the error object, it is never communicated back to theOAuthHandler.getServerMetadataonly checksh.config.ProtectedResourceMetadataURL. Since theOAuthHandleris long-lived and its config is immutable, the user (or the transport) has no way to update it with the URL found in the 401 response. sync.OnceLimitation:getServerMetadatais guarded byh.metadataOnce. If metadata fetching was attempted (and failed or used defaults) before the 401 is processed (e.g. if the client tried to preemptively fetch it), setting the new URL won't trigger a re-fetch.- Header Parsing:
extractResourceMetadataURLuses simple string matching (strings.Index). This is fragile and could match substrings within other parameter values.
@coderabbitai what is your opinion on these findings, particularly regarding how we should safely update the OAuthHandler state given the sync.Once constraint?
[!TIP] For best results, initiate chat on the files or code changes.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# Find and examine the OAuthHandler implementation, specifically getServerMetadata
fd -e go . -x grep -l "type OAuthHandler" {} \; | head -5
Length of output: 27
๐ Script executed:
#!/bin/bash
# Examine the OAuthHandler struct and getServerMetadata method
ast-grep --pattern $'type OAuthHandler struct {
$$$
}'
Length of output: 661
๐ Script executed:
#!/bin/bash
# Find getServerMetadata implementation
ast-grep --pattern $'func ($$$) getServerMetadata($$$) ($$$) {
$$$
}'
Length of output: 6895
๐ Script executed:
#!/bin/bash
# Examine extractResourceMetadataURL implementation
ast-grep --pattern $'func extractResourceMetadataURL($$$) $$$'
Length of output: 1067
๐ Script executed:
#!/bin/bash
# Check how the error flows - look at where OAuthAuthorizationRequiredError is created
rg -n -A5 "OAuthAuthorizationRequiredError" client/transport/streamable_http.go client/transport/sse.go
Length of output: 4885
๐ Script executed:
#!/bin/bash
# Check the OAuthHandler's config field and how it's used in getServerMetadata
rg -n -B3 -A10 "func.*getServerMetadata" client/transport/oauth.go
Length of output: 533
๐ Script executed:
#!/bin/bash
# Look at how metadataOnce is used
rg -n "metadataOnce" client/transport/oauth.go
Length of output: 63
๐ Script executed:
#!/bin/bash
# Check extractResourceMetadataURL full implementation
cat client/transport/streamable_http.go | sed -n '/^func extractResourceMetadataURL/,/^}/p'
Length of output: 387
๐ Script executed:
#!/bin/bash
# Check the example to see how the discovered metadata URL is intended to be used
cat examples/oauth_client/main.go | sed -n '/GetResourceMetadataURL/,/^[[:space:]]*$/p'
Length of output: 237
๐ Script executed:
#!/bin/bash
# Look for any mechanism to update handler config or reset sync.Once
rg -n "config.*=" client/transport/oauth.go | head -20
Length of output: 797
๐ Script executed:
#!/bin/bash
# Check if there's any reset or update method on OAuthHandler
ast-grep --pattern $'func ($_ *OAuthHandler) $FUNC($$$) {
$$$
}' | grep -E "(Update|Reset|Set|Reload)"
Length of output: 335
๐ Script executed:
#!/bin/bash
# See the full error handling flow in the example
cat examples/oauth_client/main.go
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 โ Valid
You'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 oauthHandler that has:
- An immutable
configfield (line 145 in oauth.go) - No mechanism to inject the discovered URL
The intended workflow appears to be:
- Catch the error
- Extract
ResourceMetadataURL - Create a new
OAuthConfigwithProtectedResourceMetadataURLset - Create a new client/transport
- Retry
But this is neither documented nor demonstrated in the example.
2. sync.Once Prevents Re-fetching โ
Valid
The metadataOnce.Do() guard at line 351 ensures getServerMetadata only executes once. Even if we could update h.config.ProtectedResourceMetadataURL mid-flight, the metadata fetching logic would never re-run.
Additionally, getServerMetadata might be called before the 401 response is received (e.g., during initial token acquisition), meaning the discovered URL arrives too late.
3. Fragile Header Parsing โ Valid
The extractResourceMetadataURL function uses naive substring matching:
idx := strings.Index(wwwAuthHeader, prefix) // prefix = "resource_metadata=\""
This could match incorrectly if another parameter's value contains that substring:
WWW-Authenticate: Bearer realm="https://example.com?note=resource_metadata=\"tricky\"", resource_metadata="https://actual.url"
The parser would extract "https://example.com?note=resource_metadata=" instead of the actual URL.
Recommendations
For Issue #1 & #2: Architectural Fix
Option A: Pre-discovery Phase (Recommended)
- Add a separate discovery method to attempt fetching metadata before creating the handler
- Populate
OAuthConfig.ProtectedResourceMetadataURLupfront - Then create the handler with complete config
Option B: Resettable Metadata
// Add to OAuthHandler
func (h *OAuthHandler) UpdateProtectedResourceURL(url string) error {
h.mu.Lock()
defer h.mu.Unlock()
h.config.ProtectedResourceMetadataURL = url
h.serverMetadata = nil
h.metadataFetchErr = nil
h.metadataOnce = sync.Once{} // Reset the guard
return nil
}
Option C: Lazy Initialization with Dynamic Config
- Don't call
getServerMetadatauntil absolutely needed - Check for discovered URL in error before first metadata fetch
- Update config before the Once executes
For Issue #3: Robust Header Parsing
Replace the naive string search with proper RFC-compliant parsing:
func extractResourceMetadataURL(wwwAuthHeader string) string {
// Split on commas, accounting for quoted values
params := parseAuthParams(wwwAuthHeader)
if url, ok := params["resource_metadata"]; ok {
return url
}
return ""
}
func parseAuthParams(header string) map[string]string {
params := make(map[string]string)
// Implement proper RFC 7235 auth-param parsing
// Handle quoted-strings, escape sequences, etc.
// Consider using a library or regex with proper state machine
return params
}
Or use regex with proper quoted-string handling:
func extractResourceMetadataURL(wwwAuthHeader string) string {
// Match resource_metadata="..." accounting for escaped quotes
re := regexp.MustCompile(`\bresource_metadata="([^"\\]*(?:\\.[^"\\]*)*)"`)
matches := re.FindStringSubmatch(wwwAuthHeader)
if len(matches) > 1 {
return matches[1]
}
return ""
}
Immediate Action Items
-
Fix the example to either:
- Show creating a new client with discovered URL, OR
- Remove the misleading "automatically use" comment
-
Document the workflow explicitly in the README
-
Consider implementing Option B (reset mechanism) as it's the least disruptive change
What's your preferred approach for addressing the state management issue with sync.Once?
[!TIP] For best results, initiate chat on the files or code changes.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# Examine the OAuthHandler struct and its config field
echo "=== OAuthHandler struct ==="
ast-grep --pattern $'type OAuthHandler struct {
$$$
}'
echo -e "\n=== getServerMetadata implementation ==="
ast-grep --pattern $'func ($_ *OAuthHandler) getServerMetadata() ($$$) {
$$$
}'
echo -e "\n=== extractResourceMetadataURL implementation ==="
ast-grep --pattern $'func extractResourceMetadataURL($$$) string {
$$$
}'
Length of output: 782
๐ Script executed:
#!/bin/bash
# Check how the discovered URL flows through error handling
echo "=== Error creation in streamable_http.go ==="
rg -n -B2 -A8 "OAuthAuthorizationRequiredError\{" client/transport/streamable_http.go
echo -e "\n=== Error creation in sse.go ==="
rg -n -B2 -A8 "OAuthAuthorizationRequiredError\{" client/transport/sse.go
Length of output: 4082
๐ Script executed:
#!/bin/bash
# Look at the example usage to see the intended workflow
echo "=== Example OAuth error handling ==="
sed -n '/if client.IsOAuthAuthorizationRequiredError(err)/,/^[[:space:]]*$/p' examples/oauth_client/main.go | head -30
Length of output: 167
๐ Script executed:
#!/bin/bash
# Check if there's any mechanism to update or reset the handler
echo "=== OAuthHandler methods ==="
ast-grep --pattern $'func ($_ *OAuthHandler) $METHOD($$$) $$$'
Length of output: 28970
๐ Script executed:
#!/bin/bash
# Examine the complete extractResourceMetadataURL implementation with context
echo "=== extractResourceMetadataURL with context ==="
rg -n -B5 -A15 "^func extractResourceMetadataURL" client/transport/streamable_http.go
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 โ Critical
Your assessment is spot-on. The flow is broken:
What happens:
- Line 340 in
streamable_http.go:metadataURLextracted from 401 response - Line 339-342: Placed into
AuthorizationRequiredError{ResourceMetadataURL: metadataURL} - Error returned to caller
- Dead end - no path back to
OAuthHandler
The problem:
OAuthHandler.configis immutable (line 145:config OAuthConfig, not a pointer)getServerMetadataonly checksh.config.ProtectedResourceMetadataURL(line 369)- No setter method exists to update this field
- The example's comment "automatically use this URL" is misleading
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. sync.Once Prevents Re-fetching โ
Critical
Line 351: h.metadataOnce.Do(func() { ensures metadata is fetched exactly once. Even if we could update h.config.ProtectedResourceMetadataURL mid-flight:
- The
Do()function body won't execute again - Metadata remains stale
- No reset mechanism exists
Timing issue: If metadata fetch happens before the 401 (e.g., during initial token acquisition), the discovered URL arrives too late.
3. Fragile Header Parsing โ Valid
Lines 250-251:
idx := strings.Index(wwwAuthHeader, prefix) // prefix = "resource_metadata=\""
Attack vector:
WWW-Authenticate: Bearer realm="https://evil.com?note=resource_metadata=\"injected\"", resource_metadata="https://real.com"
Returns: "https://evil.com?note=resource_metadata=" instead of "https://real.com"
Missing:
- RFC 7235 quoted-string parsing
- Escape sequence handling
- Parameter boundary detection
Recommended Fixes
For Issues #1 & #2: Architecture
Option A: Pre-Discovery (Cleanest)
// Before creating handler
func DiscoverMetadataURL(ctx context.Context, serverURL string) (string, error) {
// Make 401-triggering request, extract resource_metadata
return metadataURL, nil
}
// Then use it
config.ProtectedResourceMetadataURL, _ = DiscoverMetadataURL(ctx, serverURL)
handler := NewOAuthHandler(config)
Option B: Resettable Metadata (Pragmatic)
func (h *OAuthHandler) UpdateProtectedResourceURL(url string) {
h.config.ProtectedResourceMetadataURL = url
h.serverMetadata = nil
h.metadataFetchErr = nil
h.metadataOnce = sync.Once{} // Reset guard
}
Option C: Lazy + Dynamic Config
- Pass discovered URL via context
- Check context before
metadataOnce.Do() - Defer metadata fetch until absolutely needed
For Issue #3: Robust Parsing
func extractResourceMetadataURL(wwwAuthHeader string) string {
// RFC 7235 compliant: match resource_metadata="..." with proper quoted-string handling
re := regexp.MustCompile(`\bresource_metadata\s*=\s*"([^"\\]*(?:\\.[^"\\]*)*)"`)
if matches := re.FindStringSubmatch(wwwAuthHeader); len(matches) > 1 {
// Unescape per RFC 7230 Section 3.2.6
return strings.ReplaceAll(matches[1], `\"`, `"`)
}
return ""
}
Or use a proper HTTP header parser library.
Immediate Actions
- Fix the example: Remove the misleading "automatically use" comment OR demonstrate creating a new client with discovered URL
- Document the workflow: Explicitly state the manual steps required
- Choose architecture fix: My recommendation is Option B (reset mechanism) as it's minimally invasive and backward-compatible
Question for you: Do you prefer Option B (adding a reset method), or would you rather implement Option A (pre-discovery phase) for a cleaner separation of concerns? Option A would be a larger refactor but architecturally superior.
[!TIP] For best results, initiate chat on the files or code changes.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# Examine the OAuthHandler struct and its fields, especially around config and sync.Once
cat client/transport/oauth.go
Length of output: 23445
๐ Script executed:
#!/bin/bash
# Look at the full error handling flow and where OAuthAuthorizationRequiredError is created/handled
cat client/transport/streamable_http.go | grep -A20 -B5 "OAuthAuthorizationRequiredError"
Length of output: 3493
๐ Script executed:
#!/bin/bash
# Check the example to see the full error handling workflow
cat examples/oauth_client/main.go | grep -A30 -B10 "GetResourceMetadataURL"
Length of output: 1420
๐ Script executed:
#!/bin/bash
# Examine extractResourceMetadataURL implementation
cat client/transport/streamable_http.go | sed -n '/^func extractResourceMetadataURL/,/^}/p'
Length of output: 387
๐ Script executed:
#!/bin/bash
# Check if there's any mechanism to recreate or reset the handler
rg -n "NewOAuthHandler\|WithHTTPOAuth\|WithSSEOAuth" client/transport/ --type go -A3
Length of output: 0
@ezynda3, your analysis is absolutely correct on all three points. Let me address the sync.Once constraint specifically, as that's the most critical architectural flaw.
The sync.Once Problem: Why It's Fundamentally Broken
After examining the code and Go's sync.Once semantics, sync.Once isn't suitable for scenarios requiring reinitialization (e.g., reconnecting after discovering new metadata). Here's why the current implementation cannot work:
Timeline of the problem:
- Client attempts to connect โ
getServerMetadata()called metadataOnce.Do()executes, fetches metadata (or fails), locks forever- Server returns 401 with
WWW-Authenticate: Bearer resource_metadata="..." - Extracted URL is placed in error โ but it's too late
- Handler's
getServerMetadata()will never run again
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 Mismatch
The RFC9728 discovery workflow is:
Try connection โ 401 + metadata URL โ Use metadata URL โ Retry
But the current code assumes:
Fetch metadata once โ Use forever
These are incompatible patterns.
Concrete Solutions
Option 1: Lazy Discovery (Recommended)
Don't call getServerMetadata() until absolutely needed AND check for discovered URL first:
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 getServerMetadata() hasn't been called yet. If it was called during initial token fetch, you're locked out.
Option 2: Make OAuthHandler Recreatable
Since 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 UpdateOAuthHandler() method to transports.
Option 3: Remove sync.Once Entirely
Replace 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 Parsing
The naive string matching with strings.Index needs proper RFC 7235 parsing:
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
- Immediate: Fix the misleading example comment
- Short-term: Implement Option 3 (remove
sync.Once) - it's the most robust - Document: Clearly explain the workflow in the README
- Test: Add tests for the metadata-discovery-then-retry flow
Would you like me to open an issue tracking these architectural changes?
@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 AuthorizationRequiredError when we initially construct the handler). I'll mark this as draft and come back to it soon.