feat: add WWW-Authenticate header parsing for OAuth metadata discovery - #804
Closed
MariaChrysafis wants to merge 7 commits into
Closed
feat: add WWW-Authenticate header parsing for OAuth metadata discovery#804MariaChrysafis wants to merge 7 commits into
MariaChrysafis wants to merge 7 commits into
Conversation
|
Connected to Huly®: MCP_G-371 |
Contributor
WalkthroughReplaces one-time discovery with mutex-guarded re-fetchable state, adds resource_metadata URL parsing and a setter, refactors authorization-server discovery order to prefer explicit metadata/protected-resource discovery, and surfaces raw WWW-Authenticate header values in OAuth authorization errors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MariaChrysafis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
client/transport/oauth_test.go (1)
218-230: Please add coverage for the newresource_metadatadiscovery flow.This only validates the legacy base-URL fallback. The risky part of this PR is the
WWW-Authenticateparser plus re-discovery path, and there is still no regression test for multi-challenge headers or issuer URLs with path components.As per coding guidelines,
**/*_test.go: Testing: use table-driven tests using a tests := []struct{ name, ... } pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth_test.go` around lines 218 - 230, The test only asserts legacy base-URL fallback and misses coverage for the new resource_metadata discovery and WWW-Authenticate re-discovery logic; add table-driven tests in oauth_test.go (using a tests := []struct{name string, redirectURI string, wwwAuthenticate string, expectedAuthEndpoint string, expectedTokenEndpoint string, expectError bool} pattern) that call handler.GetServerMetadata and cover: (1) resource_metadata discovery flow triggered by a WWW-Authenticate header containing a resource_metadata URI, (2) multi-challenge headers (multiple WWW-Authenticate values) ensuring the parser picks the correct challenge, and (3) issuer URLs that include path components so expected endpoints include that path; reference the GetServerMetadata method and the WWW-Authenticate parsing/re-discovery behavior when creating test cases and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/transport/oauth.go`:
- Around line 474-528: The parser currently treats everything after a scheme as
key=value and errors on token68 forms (e.g., "Basic abc123"), causing the rest
of the header (like Bearer hints) to be skipped; update the loop in the auth
challenge parser (the function building wwwAuthChallenge) to detect token68
values when there's no '=' before the next comma/end: when keyEnd == -1 or when
the substring before the next comma contains no '=', treat that substring as a
token68 value, store it in params under a reserved key like "token68"
(lowercased), advance paramsStr past that token (and any comma) and continue
parsing instead of returning an error; adjust the downstream checks that
currently assume key/value pairs (the no value / comma-after-value checks) so
they allow token68 entries and don't abort the whole header parse.
- Around line 551-557: The branch that calls h.fetchMetadataFromURL when
h.config.AuthServerMetadataURL != "" can return (nil, nil) if
fetchMetadataFromURL leaves both h.serverMetadata and h.metadataFetchErr unset;
update this branch to treat a nil h.serverMetadata as an error: after calling
fetchMetadataFromURL, if h.metadataFetchErr != nil return it, else if
h.serverMetadata == nil set or return a new error (e.g. fmt.Errorf with context
mentioning AuthServerMetadataURL and non-200 response) so callers never receive
(nil, nil); reference the symbols h.fetchMetadataFromURL, h.metadataFetchErr,
h.serverMetadata and h.config.AuthServerMetadataURL when implementing the check.
- Around line 661-668: The code builds discovery endpoints by string
concatenation (authServerURL + "/.well-known/...") which breaks for issuers with
paths; parse authServerURL (using net/url.Parse) and construct discovery URLs by
joining the issuer path onto the well-known prefix (e.g.
path.Join("/.well-known/oauth-authorization-server", parsed.Path) and
path.Join("/.well-known/openid-configuration", parsed.Path)), then call
h.fetchMetadataFromURL with the assembled URL(s); keep using h.serverMetadata
and h.metadataFetchErr logic as-is (references: h.fetchMetadataFromURL,
authServerURL, h.serverMetadata, h.metadataFetchErr).
In `@client/transport/streamable_http.go`:
- Around line 248-251: The OAuthAuthorizationRequiredError currently stores
WWWAuthenticate as a string losing multiple WWW-Authenticate header values;
change the struct field WWWAuthenticate to []string on
OAuthAuthorizationRequiredError and update all call sites (in streamable_http.go
and sse.go) that currently use resp.Header.Get("WWW-Authenticate") to use
resp.Header.Values("WWW-Authenticate") to populate that []string; ensure any
code that reads WWWAuthenticate now handles a slice (e.g., iterating or joining
values) and update any tests or error formatting that assumed a single string.
---
Nitpick comments:
In `@client/transport/oauth_test.go`:
- Around line 218-230: The test only asserts legacy base-URL fallback and misses
coverage for the new resource_metadata discovery and WWW-Authenticate
re-discovery logic; add table-driven tests in oauth_test.go (using a tests :=
[]struct{name string, redirectURI string, wwwAuthenticate string,
expectedAuthEndpoint string, expectedTokenEndpoint string, expectError bool}
pattern) that call handler.GetServerMetadata and cover: (1) resource_metadata
discovery flow triggered by a WWW-Authenticate header containing a
resource_metadata URI, (2) multi-challenge headers (multiple WWW-Authenticate
values) ensuring the parser picks the correct challenge, and (3) issuer URLs
that include path components so expected endpoints include that path; reference
the GetServerMetadata method and the WWW-Authenticate parsing/re-discovery
behavior when creating test cases and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c178ad3-e9b8-41f5-bb15-1df8c7dbfe0c
📒 Files selected for processing (4)
client/transport/oauth.goclient/transport/oauth_test.goclient/transport/sse.goclient/transport/streamable_http.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
client/transport/oauth.go (3)
473-476:⚠️ Potential issue | 🟠 MajorHandle
token68challenges instead of aborting the entire header parse.This parser assumes everything after the auth scheme is
key=valueparams. A valid header likeBasic abc123, Bearer resource_metadata="..."will fail onBasic abc123, andParseResourceMetadataURLreturns""without ever seeing the later Bearer hint.When
keyEnd <= 0(no=found before the next comma or end), treat the substring as a token68 value and continue parsing rather than returning an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth.go` around lines 473 - 476, The parser currently treats any params substring without an '=' (keyEnd <= 0) as a fatal error, which breaks headers that include token68 challenges like "Basic abc123" and prevents ParseResourceMetadataURL from seeing later Bearer hints; modify the parsing in oauth.go so that when keyEnd <= 0 you treat paramsStr as a token68 token (skip or record it) and continue parsing the rest of the header instead of returning fmt.Errorf; update the logic around keyEnd, paramsStr and the construction of wwwAuthChallenge to skip over/consume the token68 segment and keep parsing subsequent comma-separated challenges so ParseResourceMetadataURL can find the Bearer resource_metadata hint.
541-547:⚠️ Potential issue | 🔴 CriticalDon't return success with nil metadata for an explicit metadata URL.
If
fetchMetadataFromURLgets a non-200 response (lines 671-673), it leaves bothserverMetadataandmetadataFetchErrunset. This branch then returns(nil, nil), and callers dereferencemetadata.TokenEndpointimmediately after.Proposed fix
if h.config.AuthServerMetadataURL != "" { h.fetchMetadataFromURL(ctx, h.config.AuthServerMetadataURL) + if h.serverMetadata == nil && h.metadataFetchErr == nil { + h.metadataFetchErr = fmt.Errorf( + "metadata discovery returned no document from %s", + h.config.AuthServerMetadataURL, + ) + } if h.metadataFetchErr != nil { return nil, h.metadataFetchErr } return h.serverMetadata, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth.go` around lines 541 - 547, The branch that handles h.config.AuthServerMetadataURL can return (nil, nil) when fetchMetadataFromURL gets a non-200 response because fetchMetadataFromURL doesn't set serverMetadata or metadataFetchErr; update the logic so that after calling h.fetchMetadataFromURL(ctx, h.config.AuthServerMetadataURL) you check if h.metadataFetchErr != nil OR h.serverMetadata == nil and return an explicit error (or wrap the non-200 response as metadataFetchErr) instead of returning (nil, nil); adjust fetchMetadataFromURL to set h.metadataFetchErr on non-200 responses (or ensure the caller treats nil serverMetadata as an error) so callers won't dereference metadata.TokenEndpoint when metadata is nil.
636-651:⚠️ Potential issue | 🟠 MajorBuild well-known discovery URLs from the issuer origin, not by string concatenation.
authServerURL + "/.well-known/..."only works when the issuer has no path. For issuers likehttps://idp.example.com/oauth2/default, RFC 8414 requires the discovery URL to behttps://idp.example.com/.well-known/oauth-authorization-server/oauth2/default.Proposed fix
func (h *OAuthHandler) discoverAuthServerMetadata(ctx context.Context, authServerURL string) bool { + parsed, err := url.Parse(authServerURL) + if err != nil { + return false + } + + // Construct well-known URL per RFC 8414: /.well-known/<suffix>/<path> + oauthWellKnown := &url.URL{ + Scheme: parsed.Scheme, + Host: parsed.Host, + Path: "/.well-known/oauth-authorization-server" + parsed.Path, + } - h.fetchMetadataFromURL(ctx, authServerURL+"/.well-known/oauth-authorization-server") + h.fetchMetadataFromURL(ctx, oauthWellKnown.String()) if h.serverMetadata != nil { h.metadataFetchErr = nil return true } - h.fetchMetadataFromURL(ctx, authServerURL+"/.well-known/openid-configuration") + oidcWellKnown := &url.URL{ + Scheme: parsed.Scheme, + Host: parsed.Host, + Path: "/.well-known/openid-configuration" + parsed.Path, + } + h.fetchMetadataFromURL(ctx, oidcWellKnown.String()) if h.serverMetadata != nil { h.metadataFetchErr = nil return true } return false }
🧹 Nitpick comments (1)
client/transport/oauth.go (1)
429-436: Minor: Byte vs rune indexing in escape detection.Line 431 uses
header[i-1](byte access) whileiis a rune offset fromrange. For multi-byte UTF-8 characters, this could access the wrong byte. In practice this is unlikely since WWW-Authenticate headers are ASCII per RFC 9110, but the pattern is fragile.Consider using byte iteration if only ASCII is expected:
for i := 0; i < len(header); i++ { if header[i] == '"' { if i > 0 && header[i-1] != '\\' { inQuotes = !inQuotes } // ... } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth.go` around lines 429 - 436, The loop over header uses range (rune indexing) but checks header[i-1] as a byte; change the loop to a byte-indexed for i := 0; i < len(header); i++ { ... } and replace rune comparisons with header[i] == '"' and header[i] == ',' so the escape check (i>0 && header[i-1] != '\\') correctly examines the previous byte; keep the existing special-case for i==0 returning the error and preserve the inQuotes toggle and comma-splitting logic around the same header variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/transport/oauth.go`:
- Around line 375-391: The ParseResourceMetadataURL function signature is wrong
for how WWW-Authenticate headers are represented: change
ParseResourceMetadataURL to accept []string (not string) and update its
implementation to iterate over the slice and call parseWWWAuthenticate for each
header (or pass the slice directly to parseWWWAuthenticate if it already expects
[]string); also ensure OAuthAuthorizationRequiredError construction or the code
that creates oauthErr extracts and sets the resource metadata URL by calling the
updated ParseResourceMetadataURL with oauthErr.WWWAuthenticate and, if
non-empty, invoking oauthErr.Handler.SetResourceMetadataURL so the
resource_metadata header is captured automatically (or document the required
caller behavior in the error type).
---
Duplicate comments:
In `@client/transport/oauth.go`:
- Around line 473-476: The parser currently treats any params substring without
an '=' (keyEnd <= 0) as a fatal error, which breaks headers that include token68
challenges like "Basic abc123" and prevents ParseResourceMetadataURL from seeing
later Bearer hints; modify the parsing in oauth.go so that when keyEnd <= 0 you
treat paramsStr as a token68 token (skip or record it) and continue parsing the
rest of the header instead of returning fmt.Errorf; update the logic around
keyEnd, paramsStr and the construction of wwwAuthChallenge to skip over/consume
the token68 segment and keep parsing subsequent comma-separated challenges so
ParseResourceMetadataURL can find the Bearer resource_metadata hint.
- Around line 541-547: The branch that handles h.config.AuthServerMetadataURL
can return (nil, nil) when fetchMetadataFromURL gets a non-200 response because
fetchMetadataFromURL doesn't set serverMetadata or metadataFetchErr; update the
logic so that after calling h.fetchMetadataFromURL(ctx,
h.config.AuthServerMetadataURL) you check if h.metadataFetchErr != nil OR
h.serverMetadata == nil and return an explicit error (or wrap the non-200
response as metadataFetchErr) instead of returning (nil, nil); adjust
fetchMetadataFromURL to set h.metadataFetchErr on non-200 responses (or ensure
the caller treats nil serverMetadata as an error) so callers won't dereference
metadata.TokenEndpoint when metadata is nil.
---
Nitpick comments:
In `@client/transport/oauth.go`:
- Around line 429-436: The loop over header uses range (rune indexing) but
checks header[i-1] as a byte; change the loop to a byte-indexed for i := 0; i <
len(header); i++ { ... } and replace rune comparisons with header[i] == '"' and
header[i] == ',' so the escape check (i>0 && header[i-1] != '\\') correctly
examines the previous byte; keep the existing special-case for i==0 returning
the error and preserve the inQuotes toggle and comma-splitting logic around the
same header variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6dbb7289-a420-4af2-8f8e-72ed0db2a1c1
📒 Files selected for processing (1)
client/transport/oauth.go
Contributor
Author
9 tasks
Contributor
|
Hey @MariaChrysafis — just flagging that #808 by @Dennisadira landed 3 days after this PR and implements the same RFC 9728 §5.1 feature. I left a detailed comparison on #808 (#808 (comment)) breaking down the design differences. The short version: your PR has the stronger challenge parser (aligned with the official go-sdk) and the re-discovery mechanism, but #808 adds two security validations that are important and currently missing here:
Both of these are spec-required when using an advertised (i.e., untrusted) PRM URL vs. the origin-constructed Would be great if you two could sync up — see the comparison table on #808 for the full breakdown. Happy to help land whichever approach you settle on. |
16 tasks
Contributor
16 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
WWW-Authenticateheader support to the OAuth flow, aligning metadata discovery with RFC 9728 (Protected Resource Metadata) and RFC 9110 §11.6.1 (challenge parsing).When a 401 Unauthorized response includes a
WWW-Authenticate: Bearer resource_metadata="..."header, callers can now extract theresource_metadataURL and feed it back to the OAuthHandler to guide metadata discovery — matching the approach used by the official MCP Go SDK.Changes
WWWAuthenticatefield, populated from 401 responses in both StreamableHTTP and SSE transports.resource_metadataURL from aWWW-Authenticateheader using a full RFC 9110 §11.6.1 challenge parser (ported from the official MCP Go SDK).sync.Oncewithsync.Mutexto allow re-discovery when aresource_metadatahint is provided. Also fixed a latent bug wheremetadataFetchErrcould persist after a successful fallback.Type of Change
Checklist
MCP Spec Compliance
Additional Information
github.com/modelcontextprotocol/go-sdk, MIT/Apache-2.0 licensed), with attribution comments in the source.resource_metadatahint can do so via ParseResourceMetadataURL + SetResourceMetadataURL.Summary by CodeRabbit