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

Motivate extensions to upgrade to MarkdownString #33577

Closed
jrieken opened this Issue Aug 31, 2017 · 17 comments

Comments

Projects
None yet
7 participants
@jrieken
Member

jrieken commented Aug 31, 2017

We will disable command-links in the (deprecated) MarkedString and extension should use the new MarkdownString for command-links.

Background

When using markdown-formatted content, VS Code supports "links" that invoke a command, like so: [Hello](command:myExt.myCommand). This is a powerful feature but has a security concern: With a carefully crafted document and an extension that creates a markdown-string from that content users can be tricked into clicking on what appears to be a link but actually executed a command. Consider the following sample:

  • I happen to know that you use an extension that includes potential dangers commands, e.g. running arbitrary commands in a shell
  • I make you use my library file that contains a jsdoc-comment invoking such a command via a command-link
  • While coding you get a hover saying "click here for more info" which in reality runs a command

untitled

To tackle this, we ask extensions to identify their markdown contents as trusted or not. E.g. TypeScript will say it don't trust markdown contents because it doesn't generate it, it just forwards. Other extensions, esp. those that generate command-links on purpose will mark their contents as trusted. To support that we have introduced a new type, MarkdownString. The MarkdownString can be used wherever the MarkedString can be used and when constructing it, you can say if you trust the contents, e.g.

// trusted
const md = new MarkdownString('[My Cool Feature](command:myTrustedContents)')
md.isTrusted = true;

// not trusted
new MarkdownString(forgeinContentsLikeJsDoc)

@jrieken jrieken self-assigned this Aug 31, 2017

@jrieken jrieken added the debt label Aug 31, 2017

@jrieken jrieken added this to the September 2017 milestone Aug 31, 2017

@jrieken jrieken closed this Sep 7, 2017

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 7, 2017

Member

Extensions that are known to use command-links and that need updating during the September timeframe:

Member

jrieken commented Sep 7, 2017

Extensions that are known to use command-links and that need updating during the September timeframe:

@ArtemGovorov

This comment has been minimized.

Show comment
Hide comment
@ArtemGovorov

ArtemGovorov Sep 7, 2017

Contributor
// trusted
new MarkdownString('[My Cool Feature](command:myTrustedContents)', true)

The MarkdownString constructor only has one parameter. Should it be:

// trusted
const md = new MarkdownString('[My Cool Feature](command:myTrustedContents)');
md.isTrusted = true;

?

Contributor

ArtemGovorov commented Sep 7, 2017

// trusted
new MarkdownString('[My Cool Feature](command:myTrustedContents)', true)

The MarkdownString constructor only has one parameter. Should it be:

// trusted
const md = new MarkdownString('[My Cool Feature](command:myTrustedContents)');
md.isTrusted = true;

?

@castwide

This comment has been minimized.

Show comment
Hide comment
@castwide

castwide Sep 7, 2017

TypeScript does not recognize MarkdownString, either as a stand-alone constant or inside the vscode namespace.

import * as vscode from 'vscode';
new MarkdownString // <- unrecognized
new vscode.MarkdownString // <- unrecognized

I'm using vscode module 1.1.5 and engine 1.5.0. I fetched vscode.d.ts from https://raw.githubusercontent.com/Microsoft/vscode/4fc690be310dd02e0ab6529c0b9bf348a8b26a19/src/vs/vscode.d.ts. Is there something I'm missing?

castwide commented Sep 7, 2017

TypeScript does not recognize MarkdownString, either as a stand-alone constant or inside the vscode namespace.

import * as vscode from 'vscode';
new MarkdownString // <- unrecognized
new vscode.MarkdownString // <- unrecognized

I'm using vscode module 1.1.5 and engine 1.5.0. I fetched vscode.d.ts from https://raw.githubusercontent.com/Microsoft/vscode/4fc690be310dd02e0ab6529c0b9bf348a8b26a19/src/vs/vscode.d.ts. Is there something I'm missing?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 7, 2017

Member

Good catch @ArtemGovorov - me thinking about new ideas that aren't implemented yet... Yeah, today we only have the property. I'll update the samples...

Member

jrieken commented Sep 7, 2017

Good catch @ArtemGovorov - me thinking about new ideas that aren't implemented yet... Yeah, today we only have the property. I'll update the samples...

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 7, 2017

Member

@castwide We will ship the 1.16 release today, then you can get the latest API by setting the engine-field in package.json to 1.16. Run npm i after that and our scripts will download the latest version of vscode.d.ts

Member

jrieken commented Sep 7, 2017

@castwide We will ship the 1.16 release today, then you can get the latest API by setting the engine-field in package.json to 1.16. Run npm i after that and our scripts will download the latest version of vscode.d.ts

@metaleap

This comment has been minimized.

Show comment
Hide comment
@metaleap

metaleap Sep 12, 2017

MarkedString had a property language that was actually supremely handy and that I used quite a bit. MarkdownString no longer has that. Is that coming back by any chance? It wasn't harmful and it actually proved useful. Kinda 'tabula rasa' to just throw it out and have everyone code-bloat / wheel-reinvent their own workaround via markdown's triple-backtick syntax, when vscode used to readily handle it so smoothly before.

metaleap commented Sep 12, 2017

MarkedString had a property language that was actually supremely handy and that I used quite a bit. MarkdownString no longer has that. Is that coming back by any chance? It wasn't harmful and it actually proved useful. Kinda 'tabula rasa' to just throw it out and have everyone code-bloat / wheel-reinvent their own workaround via markdown's triple-backtick syntax, when vscode used to readily handle it so smoothly before.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Sep 13, 2017

Member

MarkedString had a property language that was actually supremely handy and that I used quite a bit. MarkdownString no longer has that.

It has it. The {language, value} part of MarkedString is nothing else than a triple back-tic and text. Like you have code blocks inside markdown, just more verbose. The string portion of both (MarkedString and Markdown) support this, like all the other markdown things (bullet list, links, inline code, italic, bold, etc...). The only reason the verbose-part exists is that in beginning we didn't support any markdown but stuff was just a string or a code block. Ever since we started to support more markdown the verbose variant as been obsolete.

Member

jrieken commented Sep 13, 2017

MarkedString had a property language that was actually supremely handy and that I used quite a bit. MarkdownString no longer has that.

It has it. The {language, value} part of MarkedString is nothing else than a triple back-tic and text. Like you have code blocks inside markdown, just more verbose. The string portion of both (MarkedString and Markdown) support this, like all the other markdown things (bullet list, links, inline code, italic, bold, etc...). The only reason the verbose-part exists is that in beginning we didn't support any markdown but stuff was just a string or a code block. Ever since we started to support more markdown the verbose variant as been obsolete.

@jrieken jrieken modified the milestones: September 2017, October 2017 Sep 25, 2017

@jrieken jrieken assigned seanmcbreen and unassigned jrieken Oct 2, 2017

@KamasamaK

This comment has been minimized.

Show comment
Hide comment
@KamasamaK

KamasamaK Oct 8, 2017

I was looking for an example to replace the language-highlighted version, but found that VS Code's own extensions still use {language, value} (Example).

Are you saying this would be replaced with something like new MarkdownString('```' + language + ' ' + value + '```') (or the template string equivalent)?

KamasamaK commented Oct 8, 2017

I was looking for an example to replace the language-highlighted version, but found that VS Code's own extensions still use {language, value} (Example).

Are you saying this would be replaced with something like new MarkdownString('```' + language + ' ' + value + '```') (or the template string equivalent)?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 9, 2017

Member

Are you saying this would be replaced with something like new MarkdownString('' + language + ' ' + value + '') (or the template string equivalent)?

Almost, newline character after language and the value are missing

Member

jrieken commented Oct 9, 2017

Are you saying this would be replaced with something like new MarkdownString('' + language + ' ' + value + '') (or the template string equivalent)?

Almost, newline character after language and the value are missing

@abe33

This comment has been minimized.

Show comment
Hide comment
@abe33

abe33 Oct 9, 2017

Hi all, Kite's maintainer here, I didn't noticed that issue until recently and I just wanted to let you know that our next release will include the change to use MarkdownString instead of MarkedString.

abe33 commented Oct 9, 2017

Hi all, Kite's maintainer here, I didn't noticed that issue until recently and I just wanted to let you know that our next release will include the change to use MarkdownString instead of MarkedString.

@KamasamaK

This comment has been minimized.

Show comment
Hide comment
@KamasamaK

KamasamaK Oct 9, 2017

@jrieken Thanks for clarifying. You're right, it is a bit verbose. What do you think about adding a new method to MarkdownString called something like appendLanguageBlock?

KamasamaK commented Oct 9, 2017

@jrieken Thanks for clarifying. You're right, it is a bit verbose. What do you think about adding a new method to MarkdownString called something like appendLanguageBlock?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 9, 2017

Member

Yeah, we can do that. Actually, internally we have it already and I just didn't know what all to expose in the API...

Member

jrieken commented Oct 9, 2017

Yeah, we can do that. Actually, internally we have it already and I just didn't know what all to expose in the API...

@KamasamaK

This comment has been minimized.

Show comment
Hide comment
@KamasamaK

KamasamaK Oct 9, 2017

Sounds great! Actually, after thinking about it a bit more, perhaps it should just be appendCodeBlock which optionally takes a language.

KamasamaK commented Oct 9, 2017

Sounds great! Actually, after thinking about it a bit more, perhaps it should just be appendCodeBlock which optionally takes a language.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 9, 2017

Member

done

Member

jrieken commented Oct 9, 2017

done

jrieken added a commit that referenced this issue Oct 9, 2017

@KamasamaK

This comment has been minimized.

Show comment
Hide comment
@KamasamaK

KamasamaK Oct 9, 2017

@jrieken Wow, that was fast! A bit nit picky, but since "code block" is generally written as two words, shouldn't the b be upper case?

KamasamaK commented Oct 9, 2017

@jrieken Wow, that was fast! A bit nit picky, but since "code block" is generally written as two words, shouldn't the b be upper case?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Oct 9, 2017

Member

Yeah, did that on purpose because it somehow felt better...

Member

jrieken commented Oct 9, 2017

Yeah, did that on purpose because it somehow felt better...

sandy081 added a commit that referenced this issue Oct 16, 2017

@jrieken jrieken modified the milestones: October 2017, November 2017 Nov 8, 2017

@seanmcbreen

This comment has been minimized.

Show comment
Hide comment
@seanmcbreen

seanmcbreen Dec 7, 2017

Contributor

I think this issue can now be closed - LMK if that was the wrong call.

Sean
VS Code

Contributor

seanmcbreen commented Dec 7, 2017

I think this issue can now be closed - LMK if that was the wrong call.

Sean
VS Code

@seanmcbreen seanmcbreen closed this Dec 7, 2017

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.