mcp-go icon indicating copy to clipboard operation
mcp-go copied to clipboard

feat: implement RFC9728 OAuth Protected Resource Metadata discovery

Open sd2k opened this issue 1 month ago โ€ข 6 comments

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.

sd2k avatar Nov 19 '25 14:11 sd2k

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 AuthorizationRequiredError into OAuthAuthorizationRequiredError preserves previous error unwrapping behavior.
  • Review extractResourceMetadataURL() parsing for quoted values, multiple parameters, empty/malformed headers, and DPoP variations.
  • Check consistent propagation of ResourceMetadataURL through 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_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. ๐Ÿ“ Description โ€” Summarize the main change in 50โ€“60 words, explaining what was done.
  2. ๐Ÿ““ References โ€” List relevant issues, discussions, documentation, or related PRs.
  3. ๐Ÿ“ฆ Dependencies & Requirements โ€” Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. ๐Ÿ“Š Contributor Summary โ€” Include a Markdown table showing contributions: | Contributor | Lines Added | Lines Removed | Files Changed |
  5. โœ”๏ธ 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.

โค๏ธ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 19 '25 14:11 coderabbitai[bot]

I've reviewed the changes and found a critical logic gap that prevents the discovered metadata URL from actually being used.

  1. Discovered URL is Ignored: While the code extracts the resource_metadata URL into the error object, it is never communicated back to the OAuthHandler. getServerMetadata only checks h.config.ProtectedResourceMetadataURL. Since the OAuthHandler is 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.
  2. sync.Once Limitation: getServerMetadata is guarded by h.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.
  3. Header Parsing: extractResourceMetadataURL uses 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?

ezynda3 avatar Nov 22 '25 15:11 ezynda3

[!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 config field (line 145 in oauth.go)
  • No mechanism to inject the discovered URL

The intended workflow appears to be:

  1. Catch the error
  2. Extract ResourceMetadataURL
  3. Create a new OAuthConfig with ProtectedResourceMetadataURL set
  4. Create a new client/transport
  5. 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)

  1. Add a separate discovery method to attempt fetching metadata before creating the handler
  2. Populate OAuthConfig.ProtectedResourceMetadataURL upfront
  3. 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 getServerMetadata until 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

  1. Fix the example to either:

    • Show creating a new client with discovered URL, OR
    • Remove the misleading "automatically use" comment
  2. Document the workflow explicitly in the README

  3. 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?

coderabbitai[bot] avatar Nov 22 '25 15:11 coderabbitai[bot]

[!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: metadataURL extracted 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.config is immutable (line 145: config OAuthConfig, not a pointer)
  • getServerMetadata only checks h.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

  1. Fix the example: Remove the misleading "automatically use" comment OR demonstrate creating a new client with discovered URL
  2. Document the workflow: Explicitly state the manual steps required
  3. 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.

coderabbitai[bot] avatar Nov 22 '25 15:11 coderabbitai[bot]

[!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:

  1. Client attempts to connect โ†’ getServerMetadata() called
  2. metadataOnce.Do() executes, fetches metadata (or fails), locks forever
  3. Server returns 401 with WWW-Authenticate: Bearer resource_metadata="..."
  4. Extracted URL is placed in error โ†’ but it's too late
  5. 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

  1. Immediate: Fix the misleading example comment
  2. Short-term: Implement Option 3 (remove sync.Once) - it's the most robust
  3. Document: Clearly explain the workflow in the README
  4. Test: Add tests for the metadata-discovery-then-retry flow

Would you like me to open an issue tracking these architectural changes?

coderabbitai[bot] avatar Nov 22 '25 16:11 coderabbitai[bot]

@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.

sd2k avatar Nov 24 '25 08:11 sd2k