Merge pull request microsoft#256773 from mjbvz/repulsive-ape

More clean up for markdown renderer interfaces

Author: mjbvz

src/vs/base/browser/domSanitize.ts

@@ -153,7 +153,7 @@ function hookDomPurifyHrefAndSrcSanitizer(allowedLinkProtocols: readonly string[
 	return toDisposable(() => dompurify.removeHook('afterSanitizeAttributes'));
 }
 
-export interface SanitizeOptions {
+export interface DomSanitizerConfig {
 	/**
 	 * Configured the allowed html tags.
 	 */
@@ -209,7 +209,7 @@ const defaultDomPurifyConfig = Object.freeze({
  *
  * @returns A sanitized string of html.
  */
-export function sanitizeHtml(untrusted: string, config?: SanitizeOptions): TrustedHTML {
+export function sanitizeHtml(untrusted: string, config?: DomSanitizerConfig): TrustedHTML {
 	const store = new DisposableStore();
 	try {
 		const resolvedConfig: dompurify.Config = { ...defaultDomPurifyConfig };
@@ -258,6 +258,6 @@ export function sanitizeHtml(untrusted: string, config?: SanitizeOptions): Trust
 /**
  * Sanitizes the given `value` and reset the given `node` with it.
  */
-export function safeSetInnerHtml(node: HTMLElement, untrusted: string, config?: SanitizeOptions): void {
+export function safeSetInnerHtml(node: HTMLElement, untrusted: string, config?: DomSanitizerConfig): void {
 	node.innerHTML = sanitizeHtml(untrusted, config) as any;
 }

src/vs/base/browser/markdownRenderer.ts

@@ -26,29 +26,40 @@ import { StandardKeyboardEvent } from './keyboardEvent.js';
 import { StandardMouseEvent } from './mouseEvent.js';
 import { renderLabelWithIcons } from './ui/iconLabel/iconLabels.js';
 
-export interface MarkedOptions extends Readonly<Omit<marked.MarkedOptions, 'extensions' | 'baseUrl'>> {
-	readonly markedExtensions?: marked.MarkedExtension[];
-}
-
+/**
+ * Options for the rendering of markdown with {@link renderMarkdown}.
+ */
 export interface MarkdownRenderOptions extends FormattedTextRenderOptions {
 	readonly codeBlockRenderer?: (languageId: string, value: string) => Promise<HTMLElement>;
 	readonly codeBlockRendererSync?: (languageId: string, value: string, raw?: string) => HTMLElement;
 	readonly asyncRenderCallback?: () => void;
+
 	readonly fillInIncompleteTokens?: boolean;
-	readonly remoteImageIsAllowed?: (uri: URI) => boolean;
 
-	readonly sanitizerOptions?: ISanitizerOptions;
+	readonly sanitizerConfig?: MarkdownSanitizerConfig;
 
-	readonly markedOptions?: MarkedOptions;
+	readonly markedOptions?: MarkdownRendererMarkedOptions;
+	readonly markedExtensions?: marked.MarkedExtension[];
 }
 
-export interface ISanitizerOptions {
+/**
+ * Subset of options passed to `Marked` for rendering markdown.
+ */
+export interface MarkdownRendererMarkedOptions {
+	readonly gfm?: boolean;
+	readonly breaks?: boolean;
+}
+
+export interface MarkdownSanitizerConfig {
 	readonly replaceWithPlaintext?: boolean;
 	readonly allowedTags?: {
 		readonly override: readonly string[];
 	};
 	readonly customAttrSanitizer?: (attrName: string, attrValue: string) => boolean | string;
-	readonly allowedProductProtocols?: readonly string[];
+	readonly allowedLinkSchemes?: {
+		readonly augment: readonly string[];
+	};
+	readonly remoteImageIsAllowed?: (uri: URI) => boolean;
 }
 
 const defaultMarkedRenderers = Object.freeze({
@@ -110,14 +121,14 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
 	const disposables = new DisposableStore();
 	let isDisposed = false;
 
-	const markedInstance = new marked.Marked(...(options.markedOptions?.markedExtensions ?? []));
+	const markedInstance = new marked.Marked(...(options.markedExtensions ?? []));
 	const { renderer, codeBlocks, syncCodeBlocks } = createMarkdownRenderer(markedInstance, options, markdown);
 	const value = preprocessMarkdownString(markdown);
 
 	let renderedMarkdown: string;
 	if (options.fillInIncompleteTokens) {
 		// The defaults are applied by parse but not lexer()/parser(), and they need to be present
-		const opts: MarkedOptions = {
+		const opts: marked.MarkedOptions = {
 			...markedInstance.defaults,
 			...options.markedOptions,
 			renderer
@@ -136,12 +147,12 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
 	}
 
 	const htmlParser = new DOMParser();
-	const markdownHtmlDoc = htmlParser.parseFromString(sanitizeRenderedMarkdown({ isTrusted: markdown.isTrusted, ...options.sanitizerOptions }, renderedMarkdown) as unknown as string, 'text/html');
+	const markdownHtmlDoc = htmlParser.parseFromString(sanitizeRenderedMarkdown(renderedMarkdown, markdown.isTrusted ?? false, options.sanitizerConfig) as unknown as string, 'text/html');
 
 	rewriteRenderedLinks(markdown, options, markdownHtmlDoc.body);
 
 	const element = target ?? document.createElement('div');
-	element.innerHTML = sanitizeRenderedMarkdown({ isTrusted: markdown.isTrusted, ...options.sanitizerOptions }, markdownHtmlDoc.body.innerHTML) as unknown as string;
+	element.innerHTML = sanitizeRenderedMarkdown(markdownHtmlDoc.body.innerHTML, markdown.isTrusted ?? false, options.sanitizerConfig) as unknown as string;
 
 	if (codeBlocks.length > 0) {
 		Promise.all(codeBlocks).then((tuples) => {
@@ -222,9 +233,9 @@ function rewriteRenderedLinks(markdown: IMarkdownString, options: MarkdownRender
 
 			el.setAttribute('src', massageHref(markdown, href, true));
 
-			if (options.remoteImageIsAllowed) {
+			if (options.sanitizerConfig?.remoteImageIsAllowed) {
 				const uri = URI.parse(href);
-				if (uri.scheme !== Schemas.file && uri.scheme !== Schemas.data && !options.remoteImageIsAllowed(uri)) {
+				if (uri.scheme !== Schemas.file && uri.scheme !== Schemas.data && !options.sanitizerConfig.remoteImageIsAllowed(uri)) {
 					el.replaceWith(DOM.$('', undefined, el.outerHTML));
 				}
 			}
@@ -280,7 +291,7 @@ function createMarkdownRenderer(marked: marked.Marked, options: MarkdownRenderOp
 		// Note: we always pass the output through dompurify after this so that we don't rely on
 		// marked for real sanitization.
 		renderer.html = ({ text }) => {
-			if (options.sanitizerOptions?.replaceWithPlaintext) {
+			if (options.sanitizerConfig?.replaceWithPlaintext) {
 				return escape(text);
 			}
 
@@ -401,17 +412,15 @@ function resolveWithBaseUri(baseUri: URI, href: string): string {
 	}
 }
 
-interface IInternalSanitizerOptions extends ISanitizerOptions {
-	readonly isTrusted?: boolean | MarkdownStringTrustedOptions;
-}
 
 const selfClosingTags = ['area', 'base', 'br', 'col', 'command', 'embed', 'hr', 'img', 'input', 'keygen', 'link', 'meta', 'param', 'source', 'track', 'wbr'];
 
 function sanitizeRenderedMarkdown(
-	options: IInternalSanitizerOptions,
 	renderedMarkdown: string,
+	isTrusted: boolean | MarkdownStringTrustedOptions,
+	options: MarkdownSanitizerConfig = {},
 ): TrustedHTML {
-	const sanitizerConfig = getSanitizerOptions(options);
+	const sanitizerConfig = getSanitizerOptions(isTrusted, options);
 	return domSanitize.sanitizeHtml(renderedMarkdown, sanitizerConfig);
 }
 
@@ -448,7 +457,7 @@ export const allowedMarkdownHtmlAttributes = [
 	'class',
 ];
 
-function getSanitizerOptions(options: IInternalSanitizerOptions): domSanitize.SanitizeOptions {
+function getSanitizerOptions(isTrusted: boolean | MarkdownStringTrustedOptions, options: MarkdownSanitizerConfig): domSanitize.DomSanitizerConfig {
 	const allowedLinkSchemes = [
 		Schemas.http,
 		Schemas.https,
@@ -460,12 +469,12 @@ function getSanitizerOptions(options: IInternalSanitizerOptions): domSanitize.Sa
 		Schemas.vscodeNotebookCell
 	];
 
-	if (options.isTrusted) {
+	if (isTrusted) {
 		allowedLinkSchemes.push(Schemas.command);
 	}
 
-	if (options.allowedProductProtocols) {
-		allowedLinkSchemes.push(...options.allowedProductProtocols);
+	if (options.allowedLinkSchemes?.augment) {
+		allowedLinkSchemes.push(...options.allowedLinkSchemes.augment);
 	}
 
 	return {
@@ -606,7 +615,7 @@ export function renderAsPlaintext(str: IMarkdownString | string, options?: {
 	}
 
 	const html = marked.parse(value, { async: false, renderer: options?.includeCodeBlocksFences ? plainTextWithCodeBlocksRenderer.value : plainTextRenderer.value });
-	return sanitizeRenderedMarkdown({ isTrusted: false }, html)
+	return sanitizeRenderedMarkdown(html, /* isTrusted */ false, {})
 		.toString()
 		.replace(/&(#\d+|[a-zA-Z]+);/g, m => unescapeInfo.get(m) ?? m)
 		.trim();

src/vs/base/browser/ui/button/button.ts

@@ -24,7 +24,7 @@ import { localize } from '../../../../nls.js';
 import type { IManagedHover } from '../hover/hover.js';
 import { getBaseLayerHoverDelegate } from '../hover/hoverDelegate2.js';
 import { IActionProvider } from '../dropdown/dropdown.js';
-import { safeSetInnerHtml, SanitizeOptions } from '../../domSanitize.js';
+import { safeSetInnerHtml, DomSanitizerConfig } from '../../domSanitize.js';
 
 export interface IButtonOptions extends Partial<IButtonStyles> {
 	readonly title?: boolean | string;
@@ -79,7 +79,7 @@ export interface IButtonWithDescription extends IButton {
 }
 
 // Only allow a very limited set of inline html tags
-const buttonSanitizerOptions = Object.freeze<SanitizeOptions>({
+const buttonSanitizerConfig = Object.freeze<DomSanitizerConfig>({
 	allowedTags: {
 		override: ['b', 'i', 'u', 'code', 'span'],
 	},
@@ -253,7 +253,7 @@ export class Button extends Disposable implements IButton {
 			// Don't include outer `<p>`
 			const root = rendered.element.querySelector('p')?.innerHTML;
 			if (root) {
-				safeSetInnerHtml(labelElement, root, buttonSanitizerOptions);
+				safeSetInnerHtml(labelElement, root, buttonSanitizerConfig);
 			} else {
 				reset(labelElement);
 			}
@@ -654,7 +654,7 @@ export class ButtonWithIcon extends Button {
 
 			const root = rendered.element.querySelector('p')?.innerHTML;
 			if (root) {
-				safeSetInnerHtml(this._mdlabelElement, root, buttonSanitizerOptions);
+				safeSetInnerHtml(this._mdlabelElement, root, buttonSanitizerConfig);
 			} else {
 				reset(this._mdlabelElement);
 			}

src/vs/workbench/contrib/chat/browser/chatContentParts/chatMarkdownContentPart.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as dom from '../../../../../base/browser/dom.js';
-import { allowedMarkdownHtmlAttributes, MarkedOptions } from '../../../../../base/browser/markdownRenderer.js';
+import { allowedMarkdownHtmlAttributes, MarkdownRendererMarkedOptions } from '../../../../../base/browser/markdownRenderer.js';
 import { StandardMouseEvent } from '../../../../../base/browser/mouseEvent.js';
 import { HoverPosition } from '../../../../../base/browser/ui/hover/hoverWidget.js';
 import { DomScrollableElement } from '../../../../../base/browser/ui/scrollbar/scrollableElement.js';
@@ -118,16 +118,13 @@ export class ChatMarkdownContentPart extends Disposable implements IChatContentP
 				: [];
 
 			// Don't set to 'false' for responses, respect defaults
-			const markedOpts: MarkedOptions = isRequestVM(element) ? {
+			const markedOpts: MarkdownRendererMarkedOptions = isRequestVM(element) ? {
 				gfm: true,
 				breaks: true,
-				markedExtensions,
-			} : {
-				markedExtensions,
-			};
+			} : {};
 
 			const result = this._register(renderer.render(markdown.content, {
-				sanitizerOptions: MarkedKatexSupport.getSanitizerOptions({
+				sanitizerConfig: MarkedKatexSupport.getSanitizerOptions({
 					allowedTags: allowedChatMarkdownHtmlTags,
 					allowedAttributes: allowedMarkdownHtmlAttributes,
 				}),
@@ -240,6 +237,7 @@ export class ChatMarkdownContentPart extends Disposable implements IChatContentP
 				},
 				asyncRenderCallback: () => this._onDidChangeHeight.fire(),
 				markedOptions: markedOpts,
+				markedExtensions,
 			}, this.domNode));
 
 			const markdownDecorationsRenderer = instantiationService.createInstance(ChatMarkdownDecorationsRenderer);

src/vs/workbench/contrib/chat/browser/chatMarkdownRenderer.ts

@@ -73,14 +73,14 @@ export class ChatMarkdownRenderer extends MarkdownRenderer {
 	override render(markdown: IMarkdownString | undefined, options?: MarkdownRenderOptions, outElement?: HTMLElement): IMarkdownRenderResult {
 		options = {
 			...options,
-			remoteImageIsAllowed: (_uri) => false,
-			sanitizerOptions: {
+			sanitizerConfig: {
 				replaceWithPlaintext: true,
 				allowedTags: {
 					override: allowedChatMarkdownHtmlTags,
 				},
-				...options?.sanitizerOptions,
-				allowedProductProtocols: [product.urlProtocol]
+				...options?.sanitizerConfig,
+				allowedLinkSchemes: { augment: [product.urlProtocol] },
+				remoteImageIsAllowed: (_uri) => false,
 			}
 		};
 

src/vs/workbench/contrib/markdown/browser/markedKatexSupport.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { importAMDNodeModule, resolveAmdNodeModulePath } from '../../../../amdX.js';
-import { ISanitizerOptions } from '../../../../base/browser/markdownRenderer.js';
+import { MarkdownSanitizerConfig } from '../../../../base/browser/markdownRenderer.js';
 import { CodeWindow } from '../../../../base/browser/window.js';
 import { Lazy } from '../../../../base/common/lazy.js';
 import type * as marked from '../../../../base/common/marked/marked.js';
@@ -14,7 +14,7 @@ export class MarkedKatexSupport {
 	public static getSanitizerOptions(baseConfig: {
 		readonly allowedTags: readonly string[];
 		readonly allowedAttributes: readonly string[];
-	}): ISanitizerOptions {
+	}): MarkdownSanitizerConfig {
 		return {
 			allowedTags: {
 				override: [

src/vs/workbench/contrib/markdown/test/browser/markdownKatexSupport.test.ts

@@ -17,13 +17,11 @@ suite('Markdown Katex Support Test', () => {
 	async function renderMarkdownWithKatex(str: string) {
 		const katex = await MarkedKatexSupport.loadExtension(getWindow(document), {});
 		const rendered = store.add(renderMarkdown(new MarkdownString(str), {
-			sanitizerOptions: MarkedKatexSupport.getSanitizerOptions({
+			sanitizerConfig: MarkedKatexSupport.getSanitizerOptions({
 				allowedTags: basicMarkupHtmlTags,
 				allowedAttributes: defaultAllowedAttrs,
 			}),
-			markedOptions: {
-				markedExtensions: [katex],
-			}
+			markedExtensions: [katex],
 		}));
 		return rendered;
 	}