Allow style in span in markdown

[alexr00]

Part of #40607

Example of markdown that will use this:

<span style="color:${textColor};background-color:#${color};">&nbsp;&nbsp;${text}&nbsp;&nbsp;</span>

Example of what that will look like:

[jrieken]

I like it, not sure if we need a two step approach. Now, because the marked sanitize is gone, extensionn can add iframes, img, div etc. Tho, I am also not sure why these things are listed here in the first place... @mjbvz please comment

[mjbvz]

@jrieken We actually have two sanitization passes currently, one from marked and one from insane. The insane is the more complete sanitizer so we probably don't need the marked one.

What worries me though is that by allowing markdown strings to use style, we open up the surface area both in terms of security and presentation. Now an extension can use arbitrary css in hovers, including loading images or positioning content outside of the hover itself.

Github's sanitizer for markdown previews strips out style attributes for these reasons (and also I believe because style could at one point be used in very old browsers to run scripts and do other super unsafe things)

[jrieken]

@jrieken We actually have two sanitization passes currently, one from marked and one from insane.

Well, that's the point because with this PR we don't have that anymore and the questions is how we get to that again or why our current 2nd pass sanitizer looks/allows for iframe. Where does that come from?

[alexr00]

marked's sanitize is also deprecated, so not sure if it makes sense to rely on it (especially since we have the non-deprecated insane.

I'm also very curious about why we're already letting iframe through insane

Makes sense to be worried about style if it can be used to execute. What about being super restrictive like this? Allowing colors is really the only goal here.

	markedOptions.sanitizer = (html: string): string => {
		const permitted = /^<span style=\"(color\:#[0-9a-fA-F]+;)?(background-color\:#[0-9a-fA-F]+;)?\">[^<>]+<\/\s*span>$/;
		const match = html.match(permitted);
		return match ? html : '';
	};

[pfitzseb]

Sorry to butt in, but would you consider letting through all HTML tags allowed by GitHub's sanitization pipeline?

Those are

h1 h2 h3 h4 h5 h6 h7 h8 br b i strong em a pre code img tt
div ins del sup sub p ol ul table thead tbody tfoot blockquote
dl dt dd kbd q samp var hr ruby rt rp li tr td th s strike summary
details caption figure figcaption
abbr bdo cite dfn mark small span time wbr

afaict (source).

[jrieken]

terms of security and presentation. Now an extension can use arbitrary css in hovers, including loading images or positioning content outside of the hover itself.

@mjbvz wrt to security. Don't we have content security policies in place that protect us from this?

[mjbvz]

@alexr00 Yes you'll need to be careful when removing the first sanitization pass because it may have sanitized things the second didn't. This should be unlikely.

In the case of iframe, I think we do strip out iframes. insane lets you specify both the allowed tags and the allowed attributes. We do not include iframe in the allowed tags

For this issue though, we could try using filter on the style element. I suspect though that this is both going to be tricky to get right (unless you enforce a super restricted format like that regular expression does) and it opens up feature requests to add more style properties in the future.

@jrieken It is always better to have multiple layers of protection and limit the potential attack surface where possible.

@pfitzseb Please open a separate issue for that

[jrieken]

In the case of iframe, I think we do strip out iframes.

@mjbvz you added this line and I don't understand why

vscode/src/vs/base/browser/markdownRenderer.ts

Line 207 in 93c2e7b

'iframe': ['allowfullscreen', 'frameborder', 'src'],

@jrieken It is always better to have multiple layers of protection and limit the potential attack surface where possible.

Sure, but it's better to know what one layer is doing and what the other isn't doing, e.g where we have hole or duplication. So, this was a question if you know what the CSPs do

[alexr00]

For this issue though, we could try using filter on the style element. I suspect though that this is both going to be tricky to get right (unless you enforce a super restricted format like that regular expression does) and it opens up feature requests to add more style properties in the future.

This would still involve deleting the marked sanitize though, which you don't want to do. Is it ok to remove then?

[mjbvz]

@jrieken Those are the default settings for insane (just copied over so I could add a few more attributes). The allowed tags are separate from the allowed attributes. The current setting just says: if we allow iframes (which we don't) here are the allowed attributes for them. We can remove the line since it has no effect

@alexr00 I'm good with removing marked.sanitize. The setting is deprecated and has known issues, which is why we added insane.

My concern is just that allowing CSS brings up a number of other considerations:

• Which css rules do we allow? Do we try sanitizing the css? If we do have a whitelist of allowed css, how do we handle feature requests to support additional properties?

• Do we want extensions to control the styling insides hovers at all? Right now we control the presentation and can try our best to make sure the hovers look ok with all themes

• Enabling css expands the potential attack surface. If someone finds an exploit in Chromium related to CSS, it could potentially now effect VS Code too.

My feeling is that we should think through this problem more and find some other potential use cases. Issues labels seem like a very specific use case

[alexr00]

Which css rules do we allow? Do we try sanitizing the css? If we do have a whitelist of allowed css, how do we handle feature requests to support additional properties?

I think we can say that we only allow colors to be set, as allowing any positioning or sizing would allow extensions to break hovers.

Do we want extensions to control the styling insides hovers at all? Right now we control the presentation and can try our best to make sure the hovers look ok with all themes

The issue labels are one definite use case. Another is allowing codicons to be colored. The current svg solution I have for the issue labels looks bad and is a hack.

Enabling css expands the potential attack surface. If someone finds an exploit in Chromium related to CSS, it could potentially now effect VS Code too.

Do our CSPs do anything for this? If we only allow a foreground color and a background color it seems pretty safe.

[bradennapier]

Wouldn't rendering the content in a iframe and/or utilizing css containment such as contain: strict on the wrapping element help with a lot of these things?

[bradennapier]

I was actually reminded of another method that I have used for markdown in the past which is to just use an external css file that is included in then provide support using custom approved html elements instead.

This can be quite a nice solution for this and allows defining some styles people can use in the markdown they provide you without exposing all styling.

> <important /> This API will be superseded by an all-new API in June 2020. It is not recommended to start 
> new development projects using this API. Documentation for the new API will be available prior to its release.

I used css variables in this implementation since we did also allow setting them via html but that would then require the style prop so prob not ideal for this situation :). perhaps attr() https://developer.mozilla.org/en-US/docs/Web/CSS/attr

  > <warn style='--title: "Disclaimer"' /> 
  > This is a custom disclaimer blockquote

---

I am not sure if the shortcut of not wrapping it was something custom or not, it has been awhile, but it renders:

<blockquote>
<p><important> This API will be superseded by an all-new API in June 2020. It is not recommended to start 
new development projects using this API. Documentation for the new API will be available prior to its release.</important></p>
</blockquote>

:root {
  --note: steelblue;
  --important: red;
  --warn: orange;
  --tip: rgb(50, 189, 91);
  --info: rgb(30, 153, 147);
}

blockquote {
  color: #333333 !important;
  position: relative;
  box-shadow: 0px 0px 1px rgba(0, 0, 0, 0.2);
  background-color: #fcfcfc;
  padding: 20px !important;
  margin-top: 15px !important;
  margin-bottom: 15px !important;
  border-left: 0 !important;
}

blockquote p {
  margin: 0 !important;
  margin-bottom: 5px !important;
  margin-top: 5px !important;
  padding-left: 5px !important;
}

blockquote li {
  margin-top: 15px;
}

blockquote::before {
  display: block;
  content: "";
  position: absolute;
  left: 0;
  top: 0;
  bottom: 0;
  width: 3px;
}

blockquote::before {
  background-color: steelblue;
}

blockquote note::after,
blockquote tip::after,
blockquote important::after,
blockquote warn::after,
blockquote custom::after {
  display: block;
  content: "";
  position: absolute;
  left: 0;
  top: 0;
  bottom: 0;
  width: 3px;
}

blockquote important::before {
  content: var(--title, "IMPORTANT");
  color: var(--color, var(--important));
}

blockquote important::after {
  background-color: var(--color, var(--important));
}

[jrieken]

🚢it