-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: url based client metadata registration (SEP 991) #1098
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
Conversation
commit: |
pcarleton
left a comment
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.
overall looks good.
one tweak on non-https url.
Also I'd like to get a conformance test in before we merge, opened an issue here:
modelcontextprotocol/conformance#34
src/client/auth.ts
Outdated
| } | ||
| const supportsUrlBasedClientId = metadata?.client_id_metadata_document_supported === true; | ||
| const clientMetadataUrl = provider.clientMetadataUrl; | ||
| const shouldUseUrlBasedClientId = supportsUrlBasedClientId && clientMetadataUrl && isHttpsUrl(clientMetadataUrl); |
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.
thinking about this: if it's not an HTTPS url, throwing an error would be better I think. It would be confusing and surprising if I was meaning to provide a non-HTTPS URL and it silently fell back to DCR, or failed later on for no available methods.
| grant_types: ['authorization_code', 'refresh_token'], | ||
| response_types: ['code'], | ||
| token_endpoint_auth_method: 'client_secret_post', | ||
| scope: 'mcp:tools' |
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.
👍 confirmed this works on the demo server too. I think we added this when scope selection was more broken early on.
e4db8dc to
5bcf53f
Compare
closes #1052
Motivation and Context
How Has This Been Tested?
npx tsx src/examples/client/simpleOAuthClient.ts
'https://stytch-as-demo.val.run/mcp'
'https://pcarleton--c1073a0b670949da87f3911be7feb5d5.web.val.run/mcp.json'
Breaking Changes
Types of changes
Checklist
Additional context