fix(oauth): Honor resource_metadata in WWW-Authenticate fallback - #794
fix(oauth): Honor resource_metadata in WWW-Authenticate fallback#794Gujiassh wants to merge 2 commits into
Conversation
|
Connected to Huly®: MCP_G-362 |
WalkthroughAdds RFC 9728 fallback to OAuth metadata discovery: on non-200 protected-resource responses, parse Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 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 528-558: In extractResourceMetadataURL, the parsing only finds the
exact substring "resource_metadata=" and thus misses cases with optional
whitespace; update the logic to robustly parse auth-params from
wwwAuthenticateHeaders by splitting each header into comma-separated params,
trimming spaces, locating a param whose key (after trimming) equals
"resource_metadata" and then extracting its value (handling optional surrounding
quotes and trimming whitespace); operate on the wwwAuthenticateHeaders slice and
ensure you handle values with/without quotes and ignore empty values so
resource_metadata is correctly returned even when whitespace surrounds the '='.
🪄 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: 879c3ec9-590f-4ae7-a89a-34886bcd2a87
📒 Files selected for processing (2)
client/transport/oauth.goclient/transport/oauth_test.go
| authMetadataURL, err := buildWellKnownURL(authServerURL, "oauth-authorization-server") | ||
| if err == nil { | ||
| h.fetchMetadataFromURL(ctx, authMetadataURL) | ||
| if h.serverMetadata != nil { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| openidMetadataURL, err := buildWellKnownURL(authServerURL, "openid-configuration") | ||
| if err == nil { | ||
| h.fetchMetadataFromURL(ctx, openidMetadataURL) | ||
| if h.serverMetadata != nil { |
There was a problem hiding this comment.
Don't latch the first metadata error in this fallback chain.
Lines 409-417 call fetchMetadataFromURL, which writes h.metadataFetchErr immediately. If oauth-authorization-server fails with a transport/decode error but openid-configuration succeeds, h.serverMetadata gets populated but getServerMetadata() still returns the stale first error. That breaks this new RFC 9728 fallback whenever the first metadata document is malformed but a later fallback is usable. Please keep per-attempt errors local and only assign metadataFetchErr after all fallbacks fail.
|
Hi @Gujiassh. Thanks for the PR, could you have a look at and address the coderabbit comments? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/transport/oauth_test.go (1)
990-1088: Refactor the two header-variant tests into one table-driven test.These cases differ only by header formatting and are a strong fit for a
tests := []struct{...}loop.As per coding guidelines, "**/*_test.go`: Testing: ... Write 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 990 - 1088, Combine TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeader and TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeaderWithWhitespace into one table-driven test: create a tests := []struct{name string; headerValue string} with two entries for the header without and with whitespace, then loop over them and run each case (t.Run(tc.name,...)) setting the WWW-Authenticate header to tc.headerValue inside the httptest server; keep the same assertions and reuse NewOAuthHandler, handler.SetBaseURL, and handler.GetServerMetadata to verify protectedResourceRequested, headerResourceMetadataRequested, authServerRequested and the returned metadata.
🤖 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 990-1088: The tests
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeader and
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeaderWithWhitespace
currently only assert that endpoints were hit; to harden them, record the
sequence of requested paths (e.g., append r.URL.Path to a requestOrder slice
inside the httptest.Server handler) and after calling
handler.GetServerMetadata(ctx) assert that requestOrder equals the exact
expected sequence ["/.well-known/oauth-protected-resource",
"/.well-known/oauth-protected-resource/googledrive",
"/.well-known/oauth-authorization-server/oauth/googledrive"] to enforce
precedence (protected-resource → header resource_metadata URL → derived
auth-server metadata); update both tests and keep existing boolean flags and
metadata assertions.
---
Nitpick comments:
In `@client/transport/oauth_test.go`:
- Around line 990-1088: Combine
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeader and
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeaderWithWhitespace into
one table-driven test: create a tests := []struct{name string; headerValue
string} with two entries for the header without and with whitespace, then loop
over them and run each case (t.Run(tc.name,...)) setting the WWW-Authenticate
header to tc.headerValue inside the httptest server; keep the same assertions
and reuse NewOAuthHandler, handler.SetBaseURL, and handler.GetServerMetadata to
verify protectedResourceRequested, headerResourceMetadataRequested,
authServerRequested and the returned metadata.
🪄 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: 78559602-c97b-4fe6-96ab-8f15777ca7de
📒 Files selected for processing (2)
client/transport/oauth.goclient/transport/oauth_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/oauth.go
| func TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeader(t *testing.T) { | ||
| protectedResourceRequested := false | ||
| headerResourceMetadataRequested := false | ||
| authServerRequested := false | ||
|
|
||
| var server *httptest.Server | ||
| server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| switch r.URL.Path { | ||
| case "/.well-known/oauth-protected-resource": | ||
| protectedResourceRequested = true | ||
| w.Header().Set("WWW-Authenticate", `Bearer error="invalid_request", resource_metadata="`+server.URL+`/.well-known/oauth-protected-resource/googledrive"`) | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
| case "/.well-known/oauth-protected-resource/googledrive": | ||
| headerResourceMetadataRequested = true | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(OAuthProtectedResource{ | ||
| AuthorizationServers: []string{server.URL + "/oauth/googledrive"}, | ||
| }) | ||
| case "/.well-known/oauth-authorization-server/oauth/googledrive": | ||
| authServerRequested = true | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(AuthServerMetadata{ | ||
| Issuer: server.URL + "/oauth/googledrive", | ||
| AuthorizationEndpoint: server.URL + "/oauth/googledrive/authorize", | ||
| TokenEndpoint: server.URL + "/oauth/googledrive/token", | ||
| RegistrationEndpoint: server.URL + "/oauth/googledrive/register", | ||
| }) | ||
| default: | ||
| w.WriteHeader(http.StatusNotFound) | ||
| } | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| handler := NewOAuthHandler(OAuthConfig{ | ||
| ClientID: "test-client", | ||
| RedirectURI: "http://localhost/callback", | ||
| TokenStore: NewMemoryTokenStore(), | ||
| }) | ||
| handler.SetBaseURL(server.URL) | ||
|
|
||
| metadata, err := handler.GetServerMetadata(context.Background()) | ||
| require.NoError(t, err) | ||
| assert.True(t, protectedResourceRequested) | ||
| assert.True(t, headerResourceMetadataRequested) | ||
| assert.True(t, authServerRequested) | ||
| assert.Equal(t, server.URL+"/oauth/googledrive", metadata.Issuer) | ||
| assert.Equal(t, server.URL+"/oauth/googledrive/authorize", metadata.AuthorizationEndpoint) | ||
| assert.Equal(t, server.URL+"/oauth/googledrive/token", metadata.TokenEndpoint) | ||
| } | ||
|
|
||
| func TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeaderWithWhitespace(t *testing.T) { | ||
| protectedResourceRequested := false | ||
| headerResourceMetadataRequested := false | ||
| authServerRequested := false | ||
|
|
||
| var server *httptest.Server | ||
| server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| switch r.URL.Path { | ||
| case "/.well-known/oauth-protected-resource": | ||
| protectedResourceRequested = true | ||
| w.Header().Add("WWW-Authenticate", `Bearer error="invalid_request", resource_metadata = "`+server.URL+`/.well-known/oauth-protected-resource/googledrive"`) | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
| case "/.well-known/oauth-protected-resource/googledrive": | ||
| headerResourceMetadataRequested = true | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(OAuthProtectedResource{ | ||
| AuthorizationServers: []string{server.URL + "/oauth/googledrive"}, | ||
| }) | ||
| case "/.well-known/oauth-authorization-server/oauth/googledrive": | ||
| authServerRequested = true | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(AuthServerMetadata{ | ||
| Issuer: server.URL + "/oauth/googledrive", | ||
| AuthorizationEndpoint: server.URL + "/oauth/googledrive/authorize", | ||
| TokenEndpoint: server.URL + "/oauth/googledrive/token", | ||
| RegistrationEndpoint: server.URL + "/oauth/googledrive/register", | ||
| }) | ||
| default: | ||
| w.WriteHeader(http.StatusNotFound) | ||
| } | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| handler := NewOAuthHandler(OAuthConfig{ | ||
| ClientID: "test-client", | ||
| RedirectURI: "http://localhost/callback", | ||
| TokenStore: NewMemoryTokenStore(), | ||
| }) | ||
| handler.SetBaseURL(server.URL) | ||
|
|
||
| metadata, err := handler.GetServerMetadata(context.Background()) | ||
| require.NoError(t, err) | ||
| assert.True(t, protectedResourceRequested) | ||
| assert.True(t, headerResourceMetadataRequested) | ||
| assert.True(t, authServerRequested) | ||
| assert.Equal(t, server.URL+"/oauth/googledrive", metadata.Issuer) | ||
| assert.Equal(t, server.URL+"/oauth/googledrive/authorize", metadata.AuthorizationEndpoint) | ||
| assert.Equal(t, server.URL+"/oauth/googledrive/token", metadata.TokenEndpoint) | ||
| } |
There was a problem hiding this comment.
Harden this regression by asserting exact discovery sequence.
Right now, the tests only check that key endpoints were hit. They don’t fail if default fallback endpoints are called unnecessarily. Capture requested paths and assert strict order to lock in precedence (protected-resource → header resource_metadata URL → derived auth-server metadata).
🔎 Suggested test hardening
+ requestedPaths := make([]string, 0, 4)
server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ requestedPaths = append(requestedPaths, r.URL.Path)
switch r.URL.Path {
case "/.well-known/oauth-protected-resource":
...
case "/.well-known/oauth-protected-resource/googledrive":
...
case "/.well-known/oauth-authorization-server/oauth/googledrive":
...
default:
w.WriteHeader(http.StatusNotFound)
}
}))
...
+ assert.Equal(t, []string{
+ "/.well-known/oauth-protected-resource",
+ "/.well-known/oauth-protected-resource/googledrive",
+ "/.well-known/oauth-authorization-server/oauth/googledrive",
+ }, requestedPaths)🤖 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 990 - 1088, The tests
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeader and
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeaderWithWhitespace
currently only assert that endpoints were hit; to harden them, record the
sequence of requested paths (e.g., append r.URL.Path to a requestOrder slice
inside the httptest.Server handler) and after calling
handler.GetServerMetadata(ctx) assert that requestOrder equals the exact
expected sequence ["/.well-known/oauth-protected-resource",
"/.well-known/oauth-protected-resource/googledrive",
"/.well-known/oauth-authorization-server/oauth/googledrive"] to enforce
precedence (protected-resource → header resource_metadata URL → derived
auth-server metadata); update both tests and keep existing boolean flags and
metadata assertions.
|
Hey @Gujiassh — heads up, two more PRs have landed after yours implementing the same RFC 9728 §5.1 You were first to tackle this, so I want to make sure you're in the loop. A few notes on how this PR compares: Different trigger point — this PR parses Parser — the Missing from all three but present in others:
Given that #804 and #808 are both more complete and there's already a conversation happening about combining the best parts of those two, it probably makes sense to close this one in favor of whichever combined PR comes out of that discussion. Your early work on this clearly helped frame the problem though — the test scenarios you wrote (especially the whitespace-in-header variant) are useful validation. |
2 participants
Summary
Validation
Refs #697
Summary by CodeRabbit