Allow `command:` links in hover decorations

[eamodio]

• VSCode Version: Code - Insiders 1.14.0-insider (77bf512, 2017-06-19T08:59:44.265Z)
• OS Version: Windows_NT ia32 10.0.16215

---

It would be great to be able to execute commands from hover decorations. It would provide for powerful navigation options as well as open up many other uses.

For example, in GitLens I would love to have the shas eb411003 & 078960e8 be links that would open the commit details command. And the Changes be a link to open the diff with previous command.

[eamodio]

@mjbvz I should have dug deeper, but it looks like this changed very recently here: fe8ea99

[mjbvz]

@eamodio Yes there's a bit of history to that change. In summary: command: uris are a security concern and they were previously enabled in many more places than they should have been, including in hovers. The concern is that commands have not been validated for safety which may allow an evildoer to craft some evil source code that could trick a user into executing a vscode command:

/**
 * Does stuff
 *
 * [Learn More](command:runShellComamnd?rm%20-rf%20%25)
 */
export function doStuff() { ... }

fe8ea99 attempted to fix this

I don't know if there is a safe way to selectively re-enable this functionality. Let me know if you have any thoughts on this

[eamodio]

Ah - I see. Could maybe the extension provide a whitelist of commands that should be allowed in a hover? Because having commands in hovers can be quite powerful, and I'd hate to completely lose that ability (even though I didn't think it worked because I was always trying to wrap a link around a code block which caused the action to never trigger).

Or maybe have an command callback on a hover that would allow an extension to see the command being attempted and take its own action? That might be nicer than a whitelist -- but it could be abused if an extension was lazy and just blindly forwarded commands.

[mjbvz]

@jrieken Any thoughts on supporting some form of interactivity in hovers? I'm inclined to keep hovers limited to static content.

If we did want to support actions in the hover, the HoverProvider may be a good place to handle these.

[eamodio]

I would argue that there was (and still is) interactivity in hovers -- you have just taken away a part of it (for perfectly reasonable security concerns). But I would hope that the feature that used to work (which could have the security concerns mitigated) would not be lost altogether -- just changed for security.

As for hover decorations vs hover providers -- again I would argue that this feature has already existed and will continue to exist for both external links as well as internal file links.

Also, I want to point out that not only have command links been lost, but also custom TextDocumentContentProvider links -- are those also of the same security risk?

[jrieken]

@jrieken Any thoughts on supporting some form of interactivity in hovers? I'm inclined to keep hovers limited to static content.

Yeah, I think is critical that we are taking features aways. This has been working and advertised since at least a year. Why don't we do something similar to script execution and the preview command? There we didn't just disable script execution but prompted the user in some way. I think taking the big hammer to solve this problem won't make people happy.

[mjbvz]

I only recall seeing command uris documented for html previews. That they worked in hovers seemed like a side effect of using the OpenerService to handle links. Was this really a feature that we officially supported?

In the markdown preview, we disabled scripts by default and then provided a setting to enable them again. Prompting works fine for the markdown preview because the markdown content comes from the user. For UX, I'm not in favor of a prompt based solution here because the content is coming from an extension which the user has already trusted. How should they know which commands to trust and which not to? A whitelist of valid commands for given hover would be better.

Unless a large number of users will be negatively impacted when some existing extensions stop working, we need to ship 1.14 with the current change that uses a whitelist of valid link schemes, or at the very least entirely disable command uris in hovers. If we ever do want to interactive functionality in hovers, we should look into a proper solution that does not use marked strings and command uris

[jrieken]

I didn't say prompt allow user in some way to keep/restore the old behaviour. Generally, the dislike is expressed when things are taken away. How about making it a user-setting? I don't think that whitelisting necessary works because my good and proper extension might be (ab)used by a third party to with special crafted contents. So, I'd its a generic, single trust switch which defaults to "safe"