Fix OAuth metadata discovery for path-based server URLs#718
Fix OAuth metadata discovery for path-based server URLs#718roygabriel wants to merge 4 commits intomark3labs:mainfrom
Conversation
MCP servers deployed at sub-paths (e.g. /googledrive) fail OAuth discovery because the client strips the path when constructing the OAuth base URL. This causes well-known metadata requests to hit the wrong endpoints and DCR to fail with 404s. Three fixes applied: 1. Include the URL path when setting the OAuth base URL in both StreamableHTTP and SSE transports (was discarding everything after the host). 2. Construct well-known URLs per RFC 8414 Section 3 by inserting the suffix between host and path instead of appending to the base URL (buildWellKnownURL helper). 3. Parse the resource_metadata parameter from WWW-Authenticate headers on 401 responses per RFC 9728 Section 5.1 and store it for subsequent metadata discovery. Also adds ProtectedResourceMetadataURL to OAuthConfig for explicit configuration and SetProtectedResourceMetadataURL on OAuthHandler for runtime discovery.
Table-driven tests for buildWellKnownURL (various path depths, suffixes, edge cases) and extractResourceMetadataURL (standard headers, multiple params, empty/malformed input). Integration tests verifying getServerMetadata succeeds with path-based URLs, explicit ProtectedResourceMetadataURL config, and runtime-discovered metadata URLs. Transport-level tests confirming WWW-Authenticate parsing on 401 responses and base URL path preservation for both StreamableHTTP and SSE transports.
WalkthroughAdds RFC 8414/9728-compliant OAuth metadata discovery: new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
469-538:⚠️ Potential issue | 🟠 MajorError from failed metadata fetch leaks through successful fallback paths.
When
fetchMetadataFromURLfails with a transport error (settingh.metadataFetchErr), subsequent fallback methods may succeed and seth.serverMetadata. However, whengetServerMetadatacompletes, it checksh.metadataFetchErrat lines 541-542 and returns the stale error despite successful recovery. This occurs across multiple fallback chains (lines 475→480, 516→527, 527→533).Clear
h.metadataFetchErrbefore attempting each fallback to prevent accumulated errors from masking successful recovery:Example from first fallback
authServerWellKnown, wkErr := buildWellKnownURL(baseURL, "oauth-authorization-server") if wkErr != nil { h.metadataFetchErr = fmt.Errorf("failed to build auth server URL: %w", wkErr) return } + h.metadataFetchErr = nil h.fetchMetadataFromURL(ctx, authServerWellKnown) if h.serverMetadata != nil { return } + h.metadataFetchErr = nil metadata, defErr := h.getDefaultEndpoints(baseURL)Apply this pattern before each fallback attempt (lines ~475, ~480, ~516, ~527, ~533).
🤖 Fix all issues with AI agents
In `@client/transport/oauth.go`:
- Around line 322-326: The field protectedResourceMetadataURL is written by
SetProtectedResourceMetadataURL and read by getServerMetadata without
synchronization, causing a data race; fix by guarding all accesses with the
existing mutex mu (or convert the field to an atomic.Value) — acquire mu.Lock()
in SetProtectedResourceMetadataURL before setting protectedResourceMetadataURL
and mu.Unlock() after, and in getServerMetadata ensure you read
protectedResourceMetadataURL while holding mu (including any read performed
inside the metadataOnce.Do closure) so both concurrent writes and reads are
synchronized; update any helper reads of that field to use the same mu to avoid
races.
🧹 Nitpick comments (3)
client/transport/sse_oauth_test.go (1)
243-280: Consider usingtestify/assertandtestify/requireper coding guidelines.The new tests use manual
t.Fatalf/t.Errorfassertions, while the coding guidelines specify usingtestify/assertandtestify/require. Also, line 253 silently discards the error fromSaveToken— userequire.NoErrorinstead.Suggested diff
+ require.NoError(t, tokenStore.SaveToken(context.Background(), &Token{ - _ = tokenStore.SaveToken(context.Background(), &Token{ AccessToken: "stale-token", TokenType: "Bearer", ExpiresAt: time.Now().Add(1 * time.Hour), - }) + }))As per coding guidelines,
**/*_test.go: "Testing: use testify/assert and testify/require".client/transport/streamable_http_oauth_test.go (1)
222-261: Same feedback as SSE test: prefertestifyand don't discardSaveTokenerror.Line 232 silently discards the
SaveTokenerror. Userequire.NoErrorto catch unexpected failures. Also consider usingtestify/assertandtestify/requireper coding guidelines.As per coding guidelines,
**/*_test.go: "Testing: use testify/assert and testify/require".client/transport/oauth.go (1)
354-371: SimplisticWWW-Authenticateparsing may break on edge cases.The
strings.Cut-based parser works for straightforward headers but would incorrectly matchresource_metadata="if it appeared as a substring of another parameter's value (e.g., inside a quotedrealm). This is unlikely in practice for OAuth/Bearer challenges, but a slightly more robust approach would match on a word boundary (e.g., ensure the character beforeresource_metadatais a space or comma).For now this is acceptable given the narrow use case; flagging for awareness.
SetProtectedResourceMetadataURL can be called concurrently from multiple 401 handlers while getServerMetadata reads the field inside metadataOnce.Do. Protect both the write and read with the existing mu RWMutex to prevent a data race.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/transport/oauth.go (1)
474-491:⚠️ Potential issue | 🟠 MajorStale
metadataFetchErrfrom an intermediate fetch can shadow a successful fallback.
fetchMetadataFromURL(line 480) may seth.metadataFetchErron a hard failure (network error, decode error). If that happens buth.serverMetadataremains nil, the code falls through togetDefaultEndpoints(line 485) which succeeds and setsh.serverMetadata. However,getServerMetadatachecksh.metadataFetchErr != nilfirst at line 546, so the successfully-obtained metadata is never returned.The same pattern recurs at lines 515-543 (after PRM-based auth server discovery).
🐛 Proposed fix: clear metadataFetchErr before each fallback
if resp.StatusCode != http.StatusOK { authServerWellKnown, wkErr := buildWellKnownURL(baseURL, "oauth-authorization-server") if wkErr != nil { h.metadataFetchErr = fmt.Errorf("failed to build auth server URL: %w", wkErr) return } h.fetchMetadataFromURL(ctx, authServerWellKnown) if h.serverMetadata != nil { return } // If that also fails, fall back to default endpoints + h.metadataFetchErr = nil // clear intermediate errors before fallback metadata, defErr := h.getDefaultEndpoints(baseURL)Apply the same pattern before each subsequent fallback step in the PRM-based discovery path (lines 526-543):
h.fetchMetadataFromURL(ctx, authServerWellKnown) if h.serverMetadata != nil { return } // If OAuth Authorization Server Metadata discovery fails, try OpenID Connect discovery + h.metadataFetchErr = nil oidcWellKnown, wkErr := buildWellKnownURL(authServerURL, "openid-configuration") ... h.fetchMetadataFromURL(ctx, oidcWellKnown) if h.serverMetadata != nil { return } // If both discovery methods fail, use default endpoints + h.metadataFetchErr = nil metadata, defErr := h.getDefaultEndpoints(authServerURL)Alternatively, swap the check order at line 546 so
serverMetadatatakes precedence:- if h.metadataFetchErr != nil { - return nil, h.metadataFetchErr - } - return h.serverMetadata, nil + if h.serverMetadata != nil { + return h.serverMetadata, nil + } + return nil, h.metadataFetchErr
🧹 Nitpick comments (1)
client/transport/oauth.go (1)
360-374: FragileWWW-Authenticateparsing may miss valid headers.The parsing is case-sensitive (
resource_metadata="), but RFC 9728 / RFC 7235 treat auth-param names as case-insensitive. A server sendingResource_Metadata="..."or adding whitespace around=would not be matched. Consider a case-insensitive search or a small regex.♻️ Optional: case-insensitive matching
func extractResourceMetadataURL(wwwAuthenticate string) string { if wwwAuthenticate == "" { return "" } - const param = `resource_metadata="` - _, after, found := strings.Cut(wwwAuthenticate, param) + lower := strings.ToLower(wwwAuthenticate) + const param = `resource_metadata="` + idx := strings.Index(lower, param) + if idx == -1 { + return "" + } + after := wwwAuthenticate[idx+len(param):] - if !found { - return "" - } value, _, found := strings.Cut(after, `"`) if !found { return "" } return value }
RFC 7235 treats auth-param names as case-insensitive. Lower-case the header before searching for the parameter name while preserving original URL casing in the extracted value.
|
Fixed in 8f11e4a. |
There was a problem hiding this comment.
Actionable comments posted: 1
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)
477-493:⚠️ Potential issue | 🟡 Minor
fetchMetadataFromURLsilently swallows errors when the response is non-200, butmetadataFetchErrfrom a prior failed fetch leaks into the fallback check.When the PRM fetch at line 468 returns non-200, control falls to line 477.
fetchMetadataFromURLis called at line 482. If that also returns non-200,fetchMetadataFromURL(line 574-575) silently returns without settingserverMetadata— but it also doesn't clearmetadataFetchErr. If an earlier step (e.g., the PRM request creation or the PRM HTTP call itself) had setmetadataFetchErr, the check on line 483 (h.serverMetadata != nil) correctly falls through, but the stalemetadataFetchErrpersists. The final fallback on line 487 (getDefaultEndpoints) would succeed and setserverMetadata, and then at line 548 the non-nilmetadataFetchErrfrom an earlier step is returned despiteserverMetadatabeing valid.Consider clearing
h.metadataFetchErrbefore callingfetchMetadataFromURLon fallback paths, or restructuring so thatmetadataFetchErris only authoritative whenserverMetadatais nil.Proposed fix
// If we can't get the protected resource metadata, try OAuth Authorization Server discovery if resp.StatusCode != http.StatusOK { authServerWellKnown, wkErr := buildWellKnownURL(baseURL, "oauth-authorization-server") if wkErr != nil { h.metadataFetchErr = fmt.Errorf("failed to build auth server URL: %w", wkErr) return } + h.metadataFetchErr = nil // Clear any earlier transient error before fallback attempt h.fetchMetadataFromURL(ctx, authServerWellKnown)And similarly before the other
fetchMetadataFromURLcalls (lines 523, 534). Alternatively, change the return condition:- if h.metadataFetchErr != nil { + if h.serverMetadata == nil && h.metadataFetchErr != nil { return nil, h.metadataFetchErr }
🤖 Fix all issues with AI agents
In `@client/transport/oauth.go`:
- Around line 517-545: The fallback chain may leave h.metadataFetchErr set from
a prior fetch attempt even after a later fetch succeeds; before each call to
fetchMetadataFromURL (the calls that use authServerWellKnown and oidcWellKnown)
clear h.metadataFetchErr (set to nil) so a previous error doesn't persist, and
also ensure when h.serverMetadata is assigned (after getDefaultEndpoints or a
successful fetch) any prior metadataFetchErr is cleared; update the logic around
fetchMetadataFromURL, buildWellKnownURL, getDefaultEndpoints, and
getServerMetadata to rely on h.serverMetadata for success and only set
metadataFetchErr when no valid metadata is available.
🧹 Nitpick comments (2)
client/transport/oauth.go (2)
360-376:extractResourceMetadataURLfails whenresource_metadatais not the first parameter and earlier parameters contain quoted values with embedded"(escaped quotes).The
strings.Cutafter lowercasing finds the first occurrence ofresource_metadata="in the header. If an earlier parameter's quoted value contains an escaped or nested occurrence of that substring, you'd extract the wrong value. More practically, if the parameter value itself contains a"that is\"-escaped per RFC 7230, the closing-quotestrings.Cuton line 371 would terminate prematurely.That said, real-world WWW-Authenticate headers for OAuth are simple enough that this is unlikely to be an issue in practice. Worth a comment noting the limitation.
Additionally, the function is unexported but has no unit test coverage for edge cases like multiple auth schemes (
Bearer …, Basic …) or missing closing quote. The PR summary mentions tests exist — consider adding a case for a malformed/missing closing quote to ensure the empty-string fallback works.
424-546:metadataOnce.Domakes discovery non-retryable after transient failures.This is pre-existing behavior, but the expanded fallback chain makes it more consequential: if a transient network error occurs during any step inside the
sync.Onceclosure,metadataFetchErris permanently set and no future call can retry. For a long-lived client, this means a single blip during startup permanently breaks OAuth. Consider using a pattern likesync.Oncewith a reset capability (e.g., replace with a manualsync.Mutex+donebool that can be cleared on transient errors) in a follow-up.
| // Try OAuth Authorization Server Metadata first (RFC 8414) | ||
| authServerWellKnown, wkErr := buildWellKnownURL(authServerURL, "oauth-authorization-server") | ||
| if wkErr != nil { | ||
| h.metadataFetchErr = fmt.Errorf("failed to build auth server well-known URL: %w", wkErr) | ||
| return | ||
| } | ||
| h.fetchMetadataFromURL(ctx, authServerWellKnown) | ||
| if h.serverMetadata != nil { | ||
| return | ||
| } | ||
|
|
||
| // If OAuth Authorization Server Metadata discovery fails, try OpenID Connect discovery | ||
| h.fetchMetadataFromURL(ctx, authServerURL+"/.well-known/openid-configuration") | ||
| oidcWellKnown, wkErr := buildWellKnownURL(authServerURL, "openid-configuration") | ||
| if wkErr != nil { | ||
| h.metadataFetchErr = fmt.Errorf("failed to build OIDC well-known URL: %w", wkErr) | ||
| return | ||
| } | ||
| h.fetchMetadataFromURL(ctx, oidcWellKnown) | ||
| if h.serverMetadata != nil { | ||
| return | ||
| } | ||
|
|
||
| // If both discovery methods fail, use default endpoints based on the authorization server URL | ||
| metadata, err := h.getDefaultEndpoints(authServerURL) | ||
| if err != nil { | ||
| h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", err) | ||
| metadata, defErr := h.getDefaultEndpoints(authServerURL) | ||
| if defErr != nil { | ||
| h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", defErr) | ||
| return | ||
| } | ||
| h.serverMetadata = metadata |
There was a problem hiding this comment.
Same stale-error concern applies to the auth-server → OIDC → default fallback chain.
Lines 523 and 534 call fetchMetadataFromURL which may set metadataFetchErr on request-creation or HTTP errors. If the first call (RFC 8414) fails with an HTTP error that sets metadataFetchErr, and the second call (OIDC) succeeds, metadataFetchErr is never cleared, so getServerMetadata would return an error despite having valid metadata. Same pattern as noted above.
Proposed fix — clear error before each fallback attempt
authServerWellKnown, wkErr := buildWellKnownURL(authServerURL, "oauth-authorization-server")
if wkErr != nil {
h.metadataFetchErr = fmt.Errorf("failed to build auth server well-known URL: %w", wkErr)
return
}
+ h.metadataFetchErr = nil
h.fetchMetadataFromURL(ctx, authServerWellKnown)
if h.serverMetadata != nil {
return
}
// If OAuth Authorization Server Metadata discovery fails, try OpenID Connect discovery
oidcWellKnown, wkErr := buildWellKnownURL(authServerURL, "openid-configuration")
if wkErr != nil {
h.metadataFetchErr = fmt.Errorf("failed to build OIDC well-known URL: %w", wkErr)
return
}
+ h.metadataFetchErr = nil
h.fetchMetadataFromURL(ctx, oidcWellKnown)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Try OAuth Authorization Server Metadata first (RFC 8414) | |
| authServerWellKnown, wkErr := buildWellKnownURL(authServerURL, "oauth-authorization-server") | |
| if wkErr != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to build auth server well-known URL: %w", wkErr) | |
| return | |
| } | |
| h.fetchMetadataFromURL(ctx, authServerWellKnown) | |
| if h.serverMetadata != nil { | |
| return | |
| } | |
| // If OAuth Authorization Server Metadata discovery fails, try OpenID Connect discovery | |
| h.fetchMetadataFromURL(ctx, authServerURL+"/.well-known/openid-configuration") | |
| oidcWellKnown, wkErr := buildWellKnownURL(authServerURL, "openid-configuration") | |
| if wkErr != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to build OIDC well-known URL: %w", wkErr) | |
| return | |
| } | |
| h.fetchMetadataFromURL(ctx, oidcWellKnown) | |
| if h.serverMetadata != nil { | |
| return | |
| } | |
| // If both discovery methods fail, use default endpoints based on the authorization server URL | |
| metadata, err := h.getDefaultEndpoints(authServerURL) | |
| if err != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", err) | |
| metadata, defErr := h.getDefaultEndpoints(authServerURL) | |
| if defErr != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", defErr) | |
| return | |
| } | |
| h.serverMetadata = metadata | |
| // Try OAuth Authorization Server Metadata first (RFC 8414) | |
| authServerWellKnown, wkErr := buildWellKnownURL(authServerURL, "oauth-authorization-server") | |
| if wkErr != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to build auth server well-known URL: %w", wkErr) | |
| return | |
| } | |
| h.metadataFetchErr = nil | |
| h.fetchMetadataFromURL(ctx, authServerWellKnown) | |
| if h.serverMetadata != nil { | |
| return | |
| } | |
| // If OAuth Authorization Server Metadata discovery fails, try OpenID Connect discovery | |
| oidcWellKnown, wkErr := buildWellKnownURL(authServerURL, "openid-configuration") | |
| if wkErr != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to build OIDC well-known URL: %w", wkErr) | |
| return | |
| } | |
| h.metadataFetchErr = nil | |
| h.fetchMetadataFromURL(ctx, oidcWellKnown) | |
| if h.serverMetadata != nil { | |
| return | |
| } | |
| // If both discovery methods fail, use default endpoints based on the authorization server URL | |
| metadata, defErr := h.getDefaultEndpoints(authServerURL) | |
| if defErr != nil { | |
| h.metadataFetchErr = fmt.Errorf("failed to get default endpoints: %w", defErr) | |
| return | |
| } | |
| h.serverMetadata = metadata |
🤖 Prompt for AI Agents
In `@client/transport/oauth.go` around lines 517 - 545, The fallback chain may
leave h.metadataFetchErr set from a prior fetch attempt even after a later fetch
succeeds; before each call to fetchMetadataFromURL (the calls that use
authServerWellKnown and oidcWellKnown) clear h.metadataFetchErr (set to nil) so
a previous error doesn't persist, and also ensure when h.serverMetadata is
assigned (after getDefaultEndpoints or a successful fetch) any prior
metadataFetchErr is cleared; update the logic around fetchMetadataFromURL,
buildWellKnownURL, getDefaultEndpoints, and getServerMetadata to rely on
h.serverMetadata for success and only set metadataFetchErr when no valid
metadata is available.
1 participant
Description
MCP servers deployed at sub-paths (e.g.,
https://server.smithery.ai/googledrive) fail OAuth discovery because the client strips the path component from the server URL when constructing the OAuth base URL. This causes well-known metadata requests to hit wrong endpoints and DCR to fail with 404s.Three bugs fixed:
streamable_http.goandsse.godiscard the URL path when setting the OAuth base URL, losing the sub-path entirely.oauth.goappends.well-known/to the end of the base URL instead of inserting it between host and path per RFC 8414 Section 3.OAuthAuthorizationRequiredErrorwithout parsing theresource_metadatahint from theWWW-Authenticateheader per RFC 9728 Section 5.1.Fixes #697
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Files changed:
client/transport/oauth.goProtectedResourceMetadataURLconfig field,buildWellKnownURLhelper (RFC 8414),extractResourceMetadataURLhelper (RFC 9728),SetProtectedResourceMetadataURLmethod, updatedgetServerMetadatadiscovery flowclient/transport/streamable_http.goclient/transport/sse.goclient/transport/oauth_test.goclient/transport/streamable_http_oauth_test.goclient/transport/sse_oauth_test.goGo version: 1.25.7 linux/amd64
Verification: All existing tests pass,
go vetclean,golangci-lintreports 0 issues.Known gap: RFC 9728 Section 3.3 states that clients should validate retrieved protected resource metadata (e.g., confirming the
resourcefield matches the server URL). This PR does not add that validation. The existing codebase did not perform it either, so this is not a regression. It could be addressed in a follow-up.Summary by CodeRabbit
New Features
Bug Fixes
Tests