-
Notifications
You must be signed in to change notification settings - Fork 874
fix(oauth): Honor resource_metadata in WWW-Authenticate fallback #794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -987,6 +987,56 @@ func TestOAuthHandler_GetServerMetadata_PathAwareDiscovery(t *testing.T) { | |
| assert.Equal(t, server.URL+"/oauth/googledrive/token", metadata.TokenEndpoint) | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
|
Comment on lines
+990
to
+1088
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ( 🔎 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 |
||
|
|
||
| // TestOAuthHandler_RefreshToken_GitHubErrorIn200Response tests that we properly detect | ||
| // GitHub's non-spec-compliant behavior of returning HTTP 200 with error details in the JSON body | ||
| func TestOAuthHandler_RefreshToken_GitHubErrorIn200Response(t *testing.T) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't latch the first metadata error in this fallback chain.
Lines 409-417 call
fetchMetadataFromURL, which writesh.metadataFetchErrimmediately. Ifoauth-authorization-serverfails with a transport/decode error butopenid-configurationsucceeds,h.serverMetadatagets populated butgetServerMetadata()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 assignmetadataFetchErrafter all fallbacks fail.