Fallback to .well-known/oauth-protected-resource on 401s without WWW-Authenticate#268977
Merged
TylerLeonhardt merged 3 commits intomainfrom Sep 30, 2025
Merged
Fallback to .well-known/oauth-protected-resource on 401s without WWW-Authenticate#268977TylerLeonhardt merged 3 commits intomainfrom
TylerLeonhardt merged 3 commits intomainfrom
Conversation
the logic is going to get more complicated, so I want to encapsulate it where it should go.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors OAuth protected resource metadata fetching logic by extracting it from the MCP HTTP client into a reusable function in the OAuth base module. The change encapsulates the resource metadata retrieval logic in preparation for increased complexity.
Key Changes
- Created a new
fetchResourceMetadatafunction in the OAuth base module with proper validation and origin-based header handling - Replaced the private
_getResourceMetadatamethod inMcpHTTPHandlewith a call to the new centralized function - Added comprehensive test coverage for the new function including edge cases and error scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vs/base/common/oauth.ts |
Added fetchResourceMetadata function with origin detection, validation, and error handling |
src/vs/workbench/api/common/extHostMcp.ts |
Replaced private method with call to new centralized function and removed duplicate logic |
src/vs/base/test/common/oauth.test.ts |
Added comprehensive test suite covering success cases, error handling, and edge cases |
| try { | ||
| errorText = await response.text(); | ||
| } catch { | ||
| errorText = 'statusText' in response ? response.statusText : 'Unknown error'; |
There was a problem hiding this comment.
The fallback logic assumes that response might have a statusText property, but the CommonResponse interface doesn't include this property. Consider using a more specific fallback or updating the interface to include statusText if it's expected to be available.
Suggested change
| errorText = 'statusText' in response ? response.statusText : 'Unknown error'; | |
| errorText = 'Unknown error'; |
dmitrivMS
previously approved these changes
Sep 29, 2025
…Authenticate Fixes #268210
lszomoru
approved these changes
Sep 30, 2025
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #268210