feat: implement RFC9728 OAuth Protected Resource Metadata discovery (take 2) - #730
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds structured authorization-required error types and helpers, parses RFC 9728 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 |
sd2k
marked this pull request as ready for review
March 19, 2026 09:02
![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: 1
🧹 Nitpick comments (2)
client/transport/streamable_http_oauth_test.go (1)
369-420: Consider documenting the malformed quote handling behavior.The test case "Malformed resource_metadata (no closing quote)" at lines 396-399 shows the parser extracts the URL even when the closing quote is missing. While this is a reasonable lenient parsing choice, consider adding a brief comment explaining this is intentional graceful degradation rather than a bug.
{ name: "Malformed resource_metadata (no closing quote)", + // Intentionally lenient: extract value even without closing quote wwwAuth: `Bearer resource_metadata="https://example.com/metadata`, expectedURL: "https://example.com/metadata", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http_oauth_test.go` around lines 369 - 420, Add a short comment clarifying that the parser intentionally tolerates a missing closing quote when extracting resource_metadata, so the test case "Malformed resource_metadata (no closing quote)" represents deliberate lenient behavior rather than a bug; place this comment either immediately above the test case in TestExtractResourceMetadataURL or in the extractResourceMetadataURL function (referencing extractResourceMetadataURL) to document the graceful-degradation parsing decision.client/transport/streamable_http.go (1)
714-735: LGTM with optional refactor note.The 401 handling logic is correctly implemented and consistent with
SendRequest. The duplication between these two methods is minor, but you could consider extracting a helper likehandle401Response(resp *http.Response) errorif this pattern needs to be used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http.go` around lines 714 - 735, Duplicate 401 handling logic should be extracted into a helper to avoid repetition: create a function (e.g., handle401Response or handleUnauthorizedResponse) that takes the *http.Response, calls extractResourceMetadataURL(resp.Header.Get("WWW-Authenticate")), updates c.oauthHandler via c.oauthHandler.SetProtectedResourceMetadataURL(metadataURL) when c.oauthHandler != nil, and returns either an *OAuthAuthorizationRequiredError (populating Handler and AuthorizationRequiredError{ResourceMetadataURL: metadataURL}) when c.oauthHandler != nil or an *AuthorizationRequiredError{ResourceMetadataURL: metadataURL} otherwise; replace the duplicated block in this file and in SendRequest with a call to that helper.
🤖 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 307-316: SetProtectedResourceMetadataURL mutates shared state
(h.config.ProtectedResourceMetadataURL, h.serverMetadata, h.metadataFetchErr,
h.metadataOnce) without synchronization while getServerMetadata concurrently
reads them inside metadataOnce.Do(), causing a data race; fix by protecting
those fields with a mutex: add or reuse a mutex (e.g., extend existing h.mu or
create h.metadataMu), take the lock in SetProtectedResourceMetadataURL when
updating the fields and also acquire the same lock in getServerMetadata before
checking/using metadataOnce (and release before calling metadataOnce.Do() if
needed to avoid deadlock), ensuring consistent access to h.serverMetadata,
h.metadataFetchErr and h.metadataOnce.
---
Nitpick comments:
In `@client/transport/streamable_http_oauth_test.go`:
- Around line 369-420: Add a short comment clarifying that the parser
intentionally tolerates a missing closing quote when extracting
resource_metadata, so the test case "Malformed resource_metadata (no closing
quote)" represents deliberate lenient behavior rather than a bug; place this
comment either immediately above the test case in TestExtractResourceMetadataURL
or in the extractResourceMetadataURL function (referencing
extractResourceMetadataURL) to document the graceful-degradation parsing
decision.
In `@client/transport/streamable_http.go`:
- Around line 714-735: Duplicate 401 handling logic should be extracted into a
helper to avoid repetition: create a function (e.g., handle401Response or
handleUnauthorizedResponse) that takes the *http.Response, calls
extractResourceMetadataURL(resp.Header.Get("WWW-Authenticate")), updates
c.oauthHandler via c.oauthHandler.SetProtectedResourceMetadataURL(metadataURL)
when c.oauthHandler != nil, and returns either an
*OAuthAuthorizationRequiredError (populating Handler and
AuthorizationRequiredError{ResourceMetadataURL: metadataURL}) when
c.oauthHandler != nil or an *AuthorizationRequiredError{ResourceMetadataURL:
metadataURL} otherwise; replace the duplicated block in this file and in
SendRequest with a call to that helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2968eb1e-723c-4c98-9cba-6c146ed57d69
📒 Files selected for processing (10)
client/oauth.goclient/oauth_test.goclient/transport/oauth.goclient/transport/oauth_test.goclient/transport/sse.goclient/transport/sse_oauth_test.goclient/transport/sse_test.goclient/transport/streamable_http.goclient/transport/streamable_http_oauth_test.goclient/transport/streamable_http_test.go
Contributor
|
@sd2k hey sorry I let this fall through. Could you have a look at the conflicts and I can get this merged as soon as it's done. |
Parse the resource_metadata parameter from WWW-Authenticate headers on 401 responses per RFC 9728 Section 5.1. This allows clients to discover the correct OAuth authorization server for MCP servers that don't follow the simple well-known URL convention (e.g. mcp.linear.app, mcp.honeycomb.io). Changes: - Add AuthorizationRequiredError with ResourceMetadataURL field - Add extractResourceMetadataURL() to parse WWW-Authenticate headers - Add ProtectedResourceMetadataURL to OAuthConfig for explicit configuration - Update getServerMetadata() to prefer explicit metadata URL over constructed one - Add IsAuthorizationRequiredError() and GetResourceMetadataURL() public helpers - Update all 401 handling paths in both StreamableHTTP and SSE transports Rebased on v0.44.1 from original commit on combined-fork branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…RFC7235 parser Address three upstream review concerns for the RFC 9728 implementation: 1. Feed discovered resource_metadata URL from WWW-Authenticate headers back to OAuthHandler via new SetProtectedResourceMetadataURL method, which resets cached metadata and sync.Once to enable re-discovery. 2. Replace naive strings.Index parser with RFC 7235 compliant parseAuthParams that properly handles comma-separated key=value pairs, quoted-string values with backslash escaping, and unquoted token values. 3. Add comprehensive tests for parser, metadata feedback on both StreamableHTTP and SSE transports, and SetProtectedResourceMetadataURL behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SetProtectedResourceMetadataURL and getServerMetadata access shared fields (serverMetadata, metadataFetchErr, metadataOnce) without synchronization. Add metadataMu to prevent data races when a 401 handler resets metadata while another goroutine is fetching it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sd2k
force-pushed
the
feat/rfc9728-take-2
branch
from
April 9, 2026 13:12
3d775c6 to
f513879
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 394-404: The code currently sets protectedResourceURL using
h.config.ProtectedResourceMetadataURL or baseURL and later on a non-200
discovery response still falls back to baseURL-derived discovery; update the
logic in the discovery failure path so that if
h.config.ProtectedResourceMetadataURL was explicitly provided (inspect
protectedResourceURL vs empty or introduce a usedExplicit bool) you do NOT fall
back to baseURL-derived discovery but instead return the discovery error (or
attempt a fallback derived from the explicit URL’s origin), otherwise continue
to construct from baseURL as before; locate references to protectedResourceURL,
h.config.ProtectedResourceMetadataURL and the non-200 discovery handling code
near the discovery response handling and change the fallback behavior
accordingly.
- Line 151: The discovery cache can race with concurrent SetBaseURL calls
because extractBaseURL() reads h.baseURL without holding metadataMu; update the
code so baseURL is protected by the same mutex: include h.baseURL (or the field
in config that holds it) under metadataMu, i.e. acquire metadataMu in
extractBaseURL() and any reader paths (including the discovery/cache code around
serverMetadata, metadataFetchErr, metadataOnce, and
config.ProtectedResourceMetadataURL) and also hold the mutex in SetBaseURL when
mutating baseURL; alternatively, if you prefer not to add locking, document
SetBaseURL as init-only in the comment near SetBaseURL and metadataMu, and add
tests or comments ensuring callers won’t change baseURL after initialization.
🪄 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: a8553cec-200c-4a8f-bce8-543ad6b030a1
📒 Files selected for processing (1)
client/transport/oauth.go
…resource_metadata failure Address two CodeRabbit review findings: 1. SetBaseURL now acquires metadataMu to prevent races with concurrent discovery via extractBaseURL(). 2. When ProtectedResourceMetadataURL is explicitly set (via RFC 9728 WWW-Authenticate header), don't fall back to baseURL-derived discovery on failure — return the error instead, since the server specifically indicated where to find metadata. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/transport/streamable_http.go (1)
371-376:⚠️ Potential issue | 🟠 MajorThe OAuth error is not discoverable as the generic authorization-required error.
OAuthAuthorizationRequiredErrorembedsAuthorizationRequiredError, but itsUnwrap()returnsErrOAuthAuthorizationRequiredinstead ofErrAuthorizationRequired. Without a customIs()method,errors.Is(err, ErrAuthorizationRequired)will return false for OAuth authorization errors, breaking generic 401 handling that expects to catch both OAuth and non-OAuth auth failures via the base sentinel.🔧 Add custom Is() method
func (e *OAuthAuthorizationRequiredError) Unwrap() error { return ErrOAuthAuthorizationRequired } + +func (e *OAuthAuthorizationRequiredError) Is(target error) bool { + return target == ErrOAuthAuthorizationRequired || target == ErrAuthorizationRequired +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http.go` around lines 371 - 376, The Unwrap implementation on OAuthAuthorizationRequiredError currently returns ErrOAuthAuthorizationRequired which prevents errors.Is from matching the base sentinel; update the error type so generic checks succeed by either changing OAuthAuthorizationRequiredError.Unwrap() to return ErrAuthorizationRequired (the base sentinel) or implement a custom Is(target error) bool on OAuthAuthorizationRequiredError that returns true when target == ErrAuthorizationRequired or target == ErrOAuthAuthorizationRequired; refer to the OAuthAuthorizationRequiredError type, its Unwrap method, and the sentinel ErrAuthorizationRequired/ErrOAuthAuthorizationRequired to make this change.
🤖 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_test.go`:
- Around line 1454-1475: The test currently only checks that serverMetadata and
metadataFetchErr were cleared after calling
handler.SetProtectedResourceMetadataURL but doesn't verify the sync.Once was
reset; call handler.GetServerMetadata again after setting the new URL (using
metadataServer.URL + "/updated/..." as before) and assert that the subsequent
discovery actually queries the updated protected-resource URL and returns
metadata (or an error that demonstrates the new URL was used), ensuring
handler.GetServerMetadata and the updated
handler.config.ProtectedResourceMetadataURL reflect the new endpoint and that
discovery ran again.
In `@client/transport/sse.go`:
- Around line 192-199: Replace the string equality check on err.Error() with an
errors.Is comparison against the sentinel ErrOAuthAuthorizationRequired returned
by GetAuthorizationHeader, i.e. check if errors.Is(err,
ErrOAuthAuthorizationRequired) before returning the
OAuthAuthorizationRequiredError (the same change should be applied to the other
identical branch), so the code in the GetAuthorizationHeader error paths
reliably produces the typed OAuthAuthorizationRequiredError even if the error is
wrapped or its text changes.
In `@client/transport/streamable_http_oauth_test.go`:
- Around line 427-429: The test case named "Whitespace around equals" in
streamable_http_oauth_test.go uses `wwwAuth` with `resource_metadata="..."` but
should include actual whitespace around the equals; update the `wwwAuth` value
in that test case to include spaces (e.g., `resource_metadata =
"https://example.com/meta"`) so the parser is exercised for whitespace, leaving
`expectedURL` unchanged; locate the case by the test name string and modify the
wwwAuth input accordingly.
In `@client/transport/streamable_http.go`:
- Around line 286-292: parseAuthParams currently slices rest immediately after
'=' (rest = rest[eqIdx+1:]) which causes values like `resource_metadata = "..."`
to be misparsed when there is optional whitespace (BWS/OWS) around the '=';
update the code in parseAuthParams to consume optional whitespace after the '='
before deciding whether the value is a quoted-string or token: after computing
rest = rest[eqIdx+1:], skip any spaces and tabs (e.g. while len(rest)>0 &&
(rest[0]==' '||rest[0]=='\t') { rest = rest[1:] }) and then proceed to call
parseQuotedString(rest) or parse the token; also ensure you treat an empty rest
after trimming as an error rather than returning an empty token.
- Around line 343-347: The current extractResourceMetadataURL(wwwAuthHeader
string) only checks a single WWW-Authenticate header string and misses
resource_metadata if it appears in later challenges; update it to scan all
WWW-Authenticate header values and return the first non-empty resource_metadata.
Either change extractResourceMetadataURL to accept a []string (or create a new
helper) that iterates over resp.Header.Values("WWW-Authenticate"), calls
parseAuthParams on each header value, and returns the first
params["resource_metadata"] found; then update the call sites (the 401 handling
places that currently use Header.Get and call extractResourceMetadataURL) to use
resp.Header.Values("WWW-Authenticate") and pass the full slice (or iterate and
call the new helper) so resource_metadata from any WWW-Authenticate challenge is
discovered.
---
Outside diff comments:
In `@client/transport/streamable_http.go`:
- Around line 371-376: The Unwrap implementation on
OAuthAuthorizationRequiredError currently returns ErrOAuthAuthorizationRequired
which prevents errors.Is from matching the base sentinel; update the error type
so generic checks succeed by either changing
OAuthAuthorizationRequiredError.Unwrap() to return ErrAuthorizationRequired (the
base sentinel) or implement a custom Is(target error) bool on
OAuthAuthorizationRequiredError that returns true when target ==
ErrAuthorizationRequired or target == ErrOAuthAuthorizationRequired; refer to
the OAuthAuthorizationRequiredError type, its Unwrap method, and the sentinel
ErrAuthorizationRequired/ErrOAuthAuthorizationRequired to make this change.
🪄 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: d7346b48-dbe9-4a1d-b1db-aa10a6c1fcd7
📒 Files selected for processing (10)
client/oauth.goclient/oauth_test.goclient/transport/oauth.goclient/transport/oauth_test.goclient/transport/sse.goclient/transport/sse_oauth_test.goclient/transport/sse_test.goclient/transport/streamable_http.goclient/transport/streamable_http_oauth_test.goclient/transport/streamable_http_test.go
✅ Files skipped from review due to trivial changes (2)
- client/transport/sse_oauth_test.go
- client/transport/streamable_http_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/oauth.go
- client/oauth_test.go
- client/transport/oauth.go
- Use errors.Is sentinel check instead of string matching in SSE auth error handling (sse.go) - Handle BWS (optional whitespace) around '=' in parseAuthParams per RFC 7235 (streamable_http.go) - Scan all WWW-Authenticate headers via Header.Values() instead of only the first via Header.Get() (streamable_http.go, sse.go) - Fix "Whitespace around equals" test to actually contain whitespace (streamable_http_oauth_test.go) - Improve SetProtectedResourceMetadataURL test to verify sync.Once reset by calling GetServerMetadata again and checking re-discovery (oauth_test.go) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # client/transport/oauth.go # client/transport/oauth_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 153-154: resourceURL is not protected by metadataMu leading to
races; update the code so all accesses to resourceURL are guarded by the same
metadataMu contract: make writes (e.g., in
discovery/SetProtectedResourceMetadataURL) hold metadataMu and add a small
accessor getResourceURL() that locks metadataMu, returns the resourceURL string,
and use that getter wherever resourceURL is read
(token/authorization/registration request construction) to ensure consistent
synchronized reads and document the thread-safety in the comment alongside
metadataMu and resourceURL.
🪄 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: 8cae4a08-6e3d-47ed-8529-c4d7f8d98068
📒 Files selected for processing (2)
client/transport/oauth.goclient/transport/oauth_test.go
✅ Files skipped from review due to trivial changes (1)
- client/transport/oauth_test.go
The RFC 8707 resourceURL field (from main) was added alongside the metadataMu lock (from this branch) but never wired into its contract. Reads at 4 call sites raced with writes during concurrent SetProtectedResourceMetadataURL rediscovery. Add a getResourceURL accessor, route all reads through it, and clear resourceURL when the metadata URL is replaced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Hey @sd2k — again, sorry for letting this slip through the cracks. Looking at the landscape now, there are three other PRs (#794, #804, #808) that all implement the same RFC 9728 §5.1 feature, but yours was first and has the best overall design — the I'm planning to merge this one and close the others. Could you rebase to resolve the conflicts and add these two things from #808?
Happy to help review once it's updated. |
9 tasks
ezynda3
added
status: duplicate
This was referenced Apr 20, 2026
# Conflicts: # client/transport/oauth.go
Without validation, a compromised or misconfigured resource can redirect metadata discovery to an attacker's endpoint via the WWW-Authenticate resource_metadata parameter. HandleUnauthorizedResponse now rejects candidates whose scheme/host does not match the base URL, as well as non-absolute references whose empty scheme/host would otherwise bypass the check via two-empty-strings comparison. extractResourceMetadataURLs handles a single header value carrying multiple Bearer challenges so an attacker-controlled first entry cannot mask a legitimate later one. parseAuthParamValue refuses truncated quoted-strings outright rather than returning partial values. Transport call sites now route through HandleUnauthorizedResponse so validation applies uniformly to SSE and StreamableHTTP. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…3/§7.3) An advertised PRM URL may not share an origin with the protected resource, so the PRM response is the only thing that binds the metadata to the resource the client addressed. Two new checks, scoped to the advertised path only (origin-constructed well-known discovery is already same-origin-bound): - reject responses that omit the resource field — without it there is no binding - reject responses whose declared resource does not match the base URL — a compromised resource must not be able to point the client at another resource's OAuth metadata resourceIdentifiersEqual compares resource identifiers per RFC 9728 §3.3: case-insensitive scheme/host, trailing-slash-tolerant path, strict query/fragment/userinfo. Unparseable inputs fall back to exact string equality. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ests golangci-lint's usetesting linter flags context.Background() in tests because t.Context() is preferable: it is automatically cancelled when the test ends, preventing leaked goroutines and hanging sub-tests. Mechanical swap in files touched by this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Contributor
|
@sd2k linting should be fixed now |
Contributor
Author
|
@ezynda3 Cool thanks, merged main in and lints are good to go again. |
16 tasks
This was referenced May 10, 2026
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
3 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
Parse the resource_metadata parameter from WWW-Authenticate headers on
401 responses per RFC 9728 Section 5.1. This allows clients to discover
the correct OAuth authorization server for MCP servers that don't follow
the simple well-known URL convention (e.g. mcp.linear.app, mcp.honeycomb.io).
Changes:
Replaces #637.
RFC9728: datatracker.ietf.org/doc/html/rfc9728
Type of Change
Checklist
MCP Spec Compliance
Additional Information
This is basically #637 but rebased on main and updated to include the fixes suggested by Coderabbit.
Summary by CodeRabbit