fix(oauth): parse spaced resource_metadata params - #859
Conversation
|
Connected to Huly®: MCP_G-424 |
WalkthroughThis PR adds a single test case to ChangesOAuth Protected-Resource Metadata Discovery
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/transport/oauth_test.go`:
- Around line 990-1088: Merge the two tests
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeader and
TestOAuthHandler_GetServerMetadata_UsesResourceMetadataHeaderWithWhitespace into
a single table-driven test: create tests := []struct{name string, headerValue
string}{...} with three cases (resource_metadata="url", resource_metadata =
"url", and "Bearer resource_metadata=\"url\""), set up the httptest.Server and
handler (NewOAuthHandler/OAuthConfig, SetBaseURL) once, and in the loop write
the headerValue into the WWW-Authenticate response based on each case, call
handler.GetServerMetadata, and assert the same flags and metadata for each
iteration; keep the existing variables protectedResourceRequested,
headerResourceMetadataRequested, authServerRequested and the same assertions
inside the loop so behavior is identical.
In `@client/transport/oauth.go`:
- Around line 528-538: The extractor misses a directive when the header begins
with the auth-scheme (e.g., "Bearer resource_metadata=...") because the code
splits params directly and ends up with a key like "Bearer resource_metadata";
update extractResourceMetadataURL to strip the auth-scheme prefix (e.g., detect
and TrimPrefix "Bearer" or remove the first token before parsing params) or
otherwise normalize the header before splitting so keys are just the param
names, then continue the existing key/value logic for "resource_metadata"; also
add a regression unit test that passes a header string exactly like `Bearer
resource_metadata="https://..."` to ensure the URL is returned.
🪄 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: fa9f3f9b-593c-4230-96d5-a175153b51c7
📒 Files selected for processing (2)
client/transport/oauth.goclient/transport/oauth_test.go
47203a0 to
0613e4f
Compare
Summary
This PR now keeps the OAuth protected-resource metadata change focused on one remaining regression test after the upstream OAuth discovery code was refreshed.
It adds coverage for a
WWW-Authenticateparameter with optional whitespace around=:The current
extractResourceMetadataURLsimplementation already handles the broader parser behavior onmain; this PR just locks in that spaced-parameter case so it does not regress.Type of Change
Validation
Ran locally:
go test ./client/transport -run 'TestExtractResourceMetadataURLs|TestOAuthHandler_HandleUnauthorizedResponse'go test ./client/transportChecklist