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"

[eamodio]

Depending on how "safe" things need to be having something like a onDidExecuteCommand on the decorator feels somewhat natural to the api -- similar in spirit to onDidSelectItem in the QuickPicks. As I mentioned above, the main downside I see is that an extension could be lazy and just forward on commands without inspecting -- re-opening the security hole. But is that a legitimate concern? Extensions could do all sorts of "bad" things.

Where as if we are worried about that security risk, having something like hoverMessageCommands?: string[] (don't love the name) whitelist on a decorator feels like a reasonable compromise -- it is explicit, it only affect extensions that really want this behavior, and it should be fairly safe. It also doesn't feel like a big ask for an extension to provide with the hover the set of commands it expects in a given hover. The biggest downside I see is it is somewhat different than other areas of the api.

As for a single trust switch -- I'm not sure I understand what that means -- would it be per extensions? Would it be on the first executing of a specific command? Would there be a blacklist of commands that would be considered "unsafe" (blacklists are of course problematic)?

[jrieken]

Extensions could do all sorts of "bad" things.

That is correct and there is little we can do. However, there is a scenario in which a good extension can be made do evil thing by special input, e.g. a source file which results in a special hover command. Because of that a list of approached commands won't help much and I'd opt for a global flag (user-setting) that says "I understand and I am OK with the risk"

[eamodio]

Why wouldn't a separate callback or whitelist of commands not help? In the callback case the extension could decide to execute the command or not. And in the whitelist case, there wouldn't even be a command execution unless the extension expressed that the command should be allowed - e.g. in GitLens I would allow the gitlens.showQuickCommitDetails and gitlens.diffWithPrevious commands -- but anything else would be blocked.

And at least that way -- the extension doesn't get "tricked" into doing something it didn't expect

[mjbvz]

I'm not in favor of a user setting to disable security for the same reasons I'm not in favor of a prompt.

My proposal:

1. Keep all commands uris disabled in 1.14. I don't know the history of this code but my understanding is the command uris were never officially documented as being supported in hovers / MarkedStrings.

2. Revisit interactivity in hovers. If we decide that markdown + command uris are really how we want to support interactivity, document this and use a callback or whitelist on the hover/hoverprovider to enable these

[eamodio]

😞

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?

@mjbvz is the above still a risk? Or only command:?

[ArtemGovorov]

Keep all commands uris disabled in 1.14.

We are using command: URIs and disabling them would cripple a useful feature in our extension.

[eamodio]

@mjbvz @jrieken is there any resolution on this? I was planning to add more interactivity into the GitLens hovers (to open quickpicks, open diffs, even revert a line to the previous), but I obviously don't want to put in that effort if the feature is going to get pulled.

[kieferrm]

We'll leave command: urls enabled in hovers for 1.14. We'll disable them in 1.15 but we'll give extension authors an API that they can use to not only specify the content of a hover but also the actions that should be shown. We'll make sure in the UI that users can clearly identify actions and their potential side effects.

[eamodio]

@mjbvz @jrieken @kieferrm Thank you! :)

[eamodio]

@kieferrm is there any chance this commit ca6b39b
from my PR #29082 could get in? It allows links to be inside <code> tags

For example in GitLens I want to add the following command links:

But I'd much rather not lose the monospace code-block styling.

[jrieken]

So, idea is to deprecate MarkedString because it lacks expressiveness, for this issue and also for adding markdown in other places, see #11877.

The proposal is to have a MarkdownString (yeah, stupid name) that looks like this

	export class MarkdownString {
		value: string;
		trusted?: boolean;

		constructor(value?: string);
		appendText(value: string): MarkdownString;
		appendMarkdown(value: string): MarkdownString;
	}

So, basically MarkdownString is the next version of MarkedString and more or less just a wrapper around a string. The trusted flag means that value has been crafted with caution (which we obviously cannot verify). Extensions that just forward contents, like TypeScript, won't set the trusted flag which disables certain features, e.g. command links.

Pros

• Contains the source string and information about this in one place
• Allows to solve other issues we have, see Provide API to return formatted content from suggest and parameter hints #11877
• Aligned with the SnippetString and how we use it

Cons

• A little ugly as it is quite similar to the MarkedString

Another proposal is to have a second enum-style type that contains information about MarkedString usages. Like so

export enum MarkedStringType {
  TrustedMarkdown,
  Markdown,
  Plaintext
}

export class Hover {

  contents: MarkedString[];
  contentsType: MarkedStringType

Pros

• Doesn't enforce us to have two XYZString types
• Fits nicely onto LSP workings
• Also solves Provide API to return formatted content from suggest and parameter hints #11877

Cons

• Bogus property containment as these two pieces of information belong into one logical unit and not into their enclosing unit

[eamodio]

Please forgive me is this is a stupid questions -- but why couldn't MarkedString have the optional trusted?: bool? Since it is currently defined as:
https://github.com/Microsoft/vscode/blob/93a3cfc36c7bfa4ff55390c87c5500d58f24ea0f/src/vs/vscode.d.ts#L1825

So if it was a string or if the trusted property wasn't explicitly set it wouldn't be trusted.

[jrieken]

Yeah, a common pitfall ;-). The { language, value }-bit is actually "desugared" into a code block, so triple backtick, language, newline, value, triple-backtick. That is from a time in which the string portion wasn't supporting markdown but was just plaintext.

So, yes we could add another type-definition { value:string, trusted:boolean } making the MarkedString being a or-type of three. However, that doesn't the solve the problem described in #11877

[dbaeumer]

I personally would go with the MarkdownString type. But for better compatibility I would define MarkedString as

export type MarkedString = string | { language: string; value: string } | MardownString; 

[jrieken]

I personally would go with the MarkdownString type. But for better compatibility I would define MarkedString as

Unsure, it will make the the API look cleaner but I also want to add the @deprecated-tag to MarkedString. Going that way would mean types like this hover: MarkdownString[] | MarkedString[] and in places that today accept strings only documentation: string | MarkdownString. Esp the former is a little ugly...

[dbaeumer]

May be it is not a good idea. For the hover example should that be: hover: MarkdownString | MarkedString[]. If we have a MarkdownString I thing properties don't need to define an array of it. Or ?

[jrieken]

If we have a MarkdownString I thing properties don't need to define an array of it. Or ?

Unsure, I believe we have it because each of them is separated by a horizontal line... Tho, we could also deprecate a level up so

export interface DecorationOptions {
  /**
   * @deprecated
   */ 
  hoverMessage?: MarkedString | MarkedString[];

  hoverMessage?: MarkdownString | MarkdownString[]

Tho it makes it harder to describe what to do. Use hoverMessage with that other type?

[jrieken]

Ugh, this is getting ugly constructor(contents: MarkdownString | MarkdownString[] | MarkedString | MarkedString[], range?: Range);. Tending to go with @dbaeumer's suggestion of mixing the MarkdownString into the MarkedString

[jrieken]

Moving this to September when we turn off command-links unless explicitly enabled via isTrusted

[eamodio]

@jrieken will the api be in August but not enforced until Sept or no api until Sept too?

[jrieken]

Yes, the new MarkdownString will ship for August (it already is in insiders) and the default behaviour for MarkedString will change in September.

[eamodio]

Awesome -- thank you. New GitLens features coming ;)

[jrieken]

Closing this as the new API is out and adopting is being tracked here: #33577

[roblourens]

Can you clarify what the change was for september, and what part of it needs to be verified for this issue?

[eamodio]

Not sure if @jrieken wanted something specific checked, but this works for me in GitLens now:

Those links are all command links now