feat(elicitation): Add URL elicitation support. SEP-1036 #994
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements URL elicitation support (SEP-1036), enabling servers to request user interaction through URLs (e.g., OAuth flows) in addition to the existing form-based elicitation. The implementation includes a new component for displaying URL elicitation requests with security warnings, updates to client capabilities, and comprehensive test coverage.
Key Changes
- Added URL elicitation mode alongside the existing form mode, with distinct handling for each request type
- Implemented security warnings for non-HTTPS protocols and internationalized domain names
- Extended client capabilities to advertise support for both form and URL elicitation modes
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/components/ElicitationUrlRequest.tsx | New component for URL elicitation with security warnings, multiple action buttons, and clipboard integration |
| client/src/components/ElicitationTab.tsx | Updated to handle multiple elicitation modes with type guards and render appropriate components |
| client/src/components/ElicitationFormRequest.tsx | Renamed from ElicitationRequest and updated to work with typed form requests |
| client/src/components/tests/ElicitationUrlRequest.test.tsx | Comprehensive test suite covering URL validation, warnings, user interactions, and edge cases |
| client/src/components/tests/ElicitationFormRequest.test.tsx | Updated test imports and types to reflect component rename |
| client/src/lib/hooks/useConnection.ts | Updated client capabilities to include form and url elicitation support, added ElicitationCompleteNotificationSchema |
| client/src/lib/hooks/tests/useConnection.test.tsx | Updated test expectations for new elicitation capabilities structure |
| client/src/App.tsx | Updated to pass all elicitation request parameters including mode, url, and elicitationId |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!parsedUrl) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
While window.open includes "noopener,noreferrer" security flags (good practice), there's no validation to prevent opening potentially dangerous URL schemes like "javascript:", "data:", or "file:". Consider adding scheme validation to only allow http: and https: protocols before opening the URL, even though the warning system alerts users about non-HTTPS URLs.
| if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") { | |
| return; | |
| } |
There was a problem hiding this comment.
Hi @pbezglasny , looks like you resolved this but the handleAcceptAndOpen function still doesn't validate URL schemes before opening. Should we add the suggested filter for only allowing http or https?
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Thanks for bring this in. |
olaservo
added
the
spec compliance
olaservo
left a comment
olaservo
left a comment
There was a problem hiding this comment.
Thanks for the PR and sorry for the wait in reviewing. I left a few comments. Also since the Everything Server doesn't support URL mode yet (tracked in modelcontextprotocol/servers#3034) I think we'd want to add it there first, before releasing this change, since we usually prefer having parity there to support testing with that server.
| } | ||
|
|
||
| if (parsedUrl.hostname.includes("xn--")) { | ||
| warnings.push("This URL contains internationalized (non-ASCII) characters"); |
There was a problem hiding this comment.
Looks like the warning message text doesn't match between code and tests:
- Code (
ElicitationUrlRequest.tsx:73):"This URL contains internationalized (non-ASCII) characters" - Tests:
"This URL contains internationalized characters"
Please update one to match the other.
| @@ -0,0 +1,165 @@ | |||
| import { | |||
There was a problem hiding this comment.
Looks like CI is failing due to formatting issues, please run npx prettier --write client/src/components/ElicitationUrlRequest.tsx to resolve.
| if (!parsedUrl) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Hi @pbezglasny , looks like you resolved this but the handleAcceptAndOpen function still doesn't validate URL schemes before opening. Should we add the suggested filter for only allowing http or https?
3 participants
Summary
Implemented support of URL Elicitation SEP-1036. This bring next changes:
notifications/elicitation/completeto notification tab! This PR does not change behaviour in case URLElicitationRequiredError. The error will only shown in error popup.
Video:
Screen.Recording.2026-01-01.at.20.37.42.mov
Type of Change
Changes Made
Code changes:
ElicitationUrlRequestRelated Issues
Issue: #929
Testing
Tested with python elicitation server
Test Results and/or Instructions
Screenshots are encouraged to share your testing results for this change.
Checklist
npm run prettier-fix)Breaking Changes
Additional Context
I'm not typescript developer, feel free to close or update PR if needed. This changes was done to test url elicitation for rust sdk.