-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
cody: add callback route and URI handler for vscode-insider #53313
Conversation
// Example: 'vscode-insider://' for VS Code Insider | ||
if (clientURLScheme && nextRequester.customURLScheme === 'vscode') { | ||
const redirectURL = new URL(nextRequester.redirectURL) | ||
redirectURL.protocol = clientURLScheme |
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.
This might be a security vulnerability vector, being a query param vs being hardcoded into the application. I'm just not sure if there's malicious URL schemes out there in the OS that people could take advantage of, but wouldn't be surprised.
We probably want to use specific hardcoded URL, like redirectURL
, but for insiders. We could add a new CODY_INSIDERS
entry, or condition it on the client
param you added?
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.
@vdavid is looking into Jetbrains URL schemes now… there's 15 or so of them, and might need some conditional logic too. Perhaps there'll be some conditional logic there too, instead of a straight lookup table?
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.
This could be an issue because every time we add a new entry to the whitelist, the new entry will not work until the next release.
I guess my question for now is, do we only want to support vscode and vscode-insider only? (and ignore codespaces and remote hosts)
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.
Just added a new entry for vscode-insider
.
If we want to support other clients we will need to open another PR for those
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.
Code looks good.
Let's write out the test plan in text and not rely on following a Loom alone. At the very least you want to see this working in vscode and vscode insiders.
Would be great to add an automated test so we can detect regressions automatically.
Feedback inline about naming.
Close #53312
This PR adds support to handle callback and redirections from VS Code Insider.
Ref: https://code.visualstudio.com/api/advanced-topics/remote-extensions#callbacks-and-uri-handlers
Test plan
See attached video as demo:
https://www.loom.com/share/cb1a6bc634ad4d44b5c1a719baaf2277