Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 2ccb832fca17a885f5561e11 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
✅ Changeset file added - thank you! |
Review SummaryThis PR fixes the OAuth protected resource metadata URL construction to properly handle path-based routing per RFC 9728 Section 3. The implementation is excellent with strong adherence to Rust best practices. Findings: None - no issues identified Highlights:
Test Coverage Assessment: Comprehensive. The test suite thoroughly validates the URL construction logic across all relevant scenarios with proper naming conventions and one-behavior-per-test structure. Recommendation: ✅ Approve - This is a well-implemented bug fix with no issues found. The code follows all Rust best practices and is ready to merge. Reviewed by Claude Code Sonnet 4.5 |
| ( | ||
| StatusCode::UNAUTHORIZED, | ||
| TypedHeader(WwwAuthenticate::Bearer { | ||
| resource_metadata: resource_metadata_url(), |
There was a problem hiding this comment.
Do you think we should add an integration test that triggers 401, parses WWW-Authenticate, and then asserts that resource_metadata is the exact RFC-style URL for a path-scoped resource?
There was a problem hiding this comment.
Great idea! Added unauthorized_response_contains_path_scoped_resource_metadata_url.
| .route( | ||
| "/.well-known/oauth-protected-resource", | ||
| get(protected_resource), | ||
| ) |
There was a problem hiding this comment.
Would it be best to add a test that GETs the exact resource_metadata path that is in the header and expects 200?
There was a problem hiding this comment.
Yep, good call! Added get_resource_metadata_path_returns_ok which GETs the path-scoped metadata URL and asserts 200 OK.
| let mut url = auth_config.resource.clone(); | ||
| url.set_path("/.well-known/oauth-protected-resource"); | ||
| url | ||
| }; |
There was a problem hiding this comment.
This makes me wonder about those who hard-code the metadata_url in a CI/CD flow or elsewhere, where this may break for them now. Should we add validation coverage for invalid resource values (i.e. non-HTTP(S) scheme or missing host) so startup fails fast?
There was a problem hiding this comment.
Yeah, totally fair concern! Added a scheme check that rejects non-HTTP resource URLs at startup.
Fixes #533
The
resource_metadataURL in theWWW-Authenticateheader and the.well-knownendpoint route were ignoring the path component of the configuredresourceURL. Per RFC 9728 Section 3, the well-known URI must be formed by inserting/.well-known/oauth-protected-resourcebetween the host and path components. This fixes OAuth metadata discovery for MCP servers deployed behind reverse proxies with path-based routing, where clients like VS Code and Claude could not authenticate.