-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix building auth metadata paths #779
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the suggestion @Kludex. Refactored the |
src/mcp/server/auth/routes.py
Outdated
) | ||
token_url = modify_url_path(issuer_url, lambda path: append_path(path, TOKEN_PATH)) |
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.
I think we can delete this modify_url_path
tho. str(issuer_url)
should work.
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.
I was trying to modify as little code as possible, but you're right, this can be simplified by just casting the URL to a string and merging it with the endpoint (authorize, token, etc...)
I decided to convert the merged endpoint into a AnyHttpUrl
pydantic class because the OAuthMetadata
class is expecting AnyHttpUrl
URLs
Let me know what you think! @Kludex
Summary
Fixes OAuth endpoint URL construction to always join paths correctly, preventing missing or double slashes regardless of the issuer URL format. Removes unnecessary use of
lstrip("/")
, which caused endpoints like$MY_ISSUERauthorize
instead of the expected$MY_ISSUER/authorize
.Motivation and Context
The output of
/.well-known/oauth-authorization-server
was incorrect when the issuer URL included a path or when using URLs likehttp://localhost:8000
.Expected:
Actual:
This was due to the use of
lstrip("/")
when joining paths, which strips the leading slash and results in malformed URLs.Note:
There is already PR #770 attempting to address this, but it does not handle all cases (e.g., URLs like
http://localhost:8000
or issuer URLs with existing paths).How Has This Been Tested?
Tested with various issuer URLs, including:
Verified the output of:
Example output:
All endpoints are now correctly formed, with no missing or double slashes.
Breaking Changes
No breaking changes
Types of changes
Checklist
Additional context
Related PRs
lstrip
in each OAuth endpoint #770 (works partially)@hongkunyoo @pcarleton @Kludex