Merge pull request microsoft#256143 from mjbvz/marvellous-swordfish

Tighten up dom sanitizing rules

Author: mjbvz

src/vs/base/browser/domSanitize.ts

@@ -7,11 +7,6 @@ import { DisposableStore, IDisposable, toDisposable } from '../common/lifecycle.
 import { Schemas } from '../common/network.js';
 import dompurify from './dompurify/dompurify.js';
 
-const defaultSafeProtocols = [
-	Schemas.http,
-	Schemas.https,
-	Schemas.command,
-];
 
 /**
  * List of safe, non-input html tags.
@@ -83,40 +78,27 @@ export const basicMarkupHtmlTags = Object.freeze([
 	'var',
 	'video',
 	'wbr',
-
-	// TODO: Move these out of the default
-	'select',
-	'input',
 ]);
 
 export const defaultAllowedAttrs = Object.freeze([
 	'href',
 	'target',
-	'title',
-	'name',
 	'src',
 	'alt',
+	'title',
+	'for',
+	'name',
 	'role',
 	'tabindex',
-	'width',
-	'height',
-	'align',
 	'x-dispatch',
 	'required',
 	'checked',
 	'placeholder',
 	'type',
 	'start',
-
-	// TODO: See if we can move these out of the default
-	'for',
-	'role',
-	'data-href',
-	'data-command',
-	'data-code',
-	'id',
-	'class',
-	'style',
+	'width',
+	'height',
+	'align',
 ]);
 
 
@@ -134,28 +116,35 @@ function addDompurifyHook(hook: 'uponSanitizeElement' | 'uponSanitizeAttribute',
  * Hooks dompurify using `afterSanitizeAttributes` to check that all `href` and `src`
  * attributes are valid.
  */
-function hookDomPurifyHrefAndSrcSanitizer(allowedProtocols: readonly string[] | '*', allowDataImages = false): IDisposable {
+function hookDomPurifyHrefAndSrcSanitizer(allowedLinkProtocols: readonly string[] | '*', allowedMediaProtocols: readonly string[]): IDisposable {
 	// https://github.com/cure53/DOMPurify/blob/main/demos/hooks-scheme-allowlist.html
 	// build an anchor to map URLs to
 	const anchor = document.createElement('a');
 
+	function validateLink(value: string, allowedProtocols: readonly string[] | '*'): boolean {
+		if (allowedProtocols === '*') {
+			return true; // allow all protocols
+		}
+
+		anchor.href = value;
+		return allowedProtocols.includes(anchor.protocol.replace(/:$/, ''));
+	}
+
 	dompurify.addHook('afterSanitizeAttributes', (node) => {
 		// check all href/src attributes for validity
 		for (const attr of ['href', 'src']) {
 			if (node.hasAttribute(attr)) {
 				const attrValue = node.getAttribute(attr) as string;
-				if (attr === 'href' && attrValue.startsWith('#')) {
-					// Allow fragment links
-					continue;
-				}
+				if (attr === 'href') {
 
-				anchor.href = attrValue;
-				if (allowedProtocols !== '*' && !allowedProtocols.includes(anchor.protocol.replace(/:$/, ''))) {
-					if (allowDataImages && attr === 'src' && anchor.href.startsWith('data:')) {
-						continue;
+					if (!attrValue.startsWith('#') && !validateLink(attrValue, allowedLinkProtocols)) {
+						node.removeAttribute(attr);
 					}
 
-					node.removeAttribute(attr);
+				} else {// 'src'
+					if (!validateLink(attrValue, allowedMediaProtocols)) {
+						node.removeAttribute(attr);
+					}
 				}
 			}
 		}
@@ -165,16 +154,35 @@ function hookDomPurifyHrefAndSrcSanitizer(allowedProtocols: readonly string[] |
 }
 
 export interface SanitizeOptions {
-	readonly overrideAllowedTags?: readonly string[];
-	readonly overrideAllowedAttributes?: readonly string[];
+	/**
+	 * Configured the allowed html tags.
+	 */
+	readonly allowedTags?: {
+		readonly override?: readonly string[];
+		readonly augment?: readonly string[];
+	};
+
+	/**
+	 * Configured the allowed html attributes.
+	 */
+	readonly allowedAttributes?: {
+		readonly override?: readonly string[];
+		readonly augment?: readonly string[];
+	};
 
 	/**
-	 * List of allowed protocols for `href` and `src` attributes.
-	 *
-	 * If this is
+	 * List of allowed protocols for `href` attributes.
 	 */
-	readonly overrideAllowedProtocols?: readonly string[] | '*';
-	readonly allowDataImages?: boolean;
+	readonly allowedLinkProtocols?: {
+		readonly override?: readonly string[] | '*';
+	};
+
+	/**
+	 * List of allowed protocols for `src` attributes.
+	 */
+	readonly allowedMediaProtocols?: {
+		readonly override?: readonly string[];
+	};
 
 	readonly hooks?: {
 		readonly uponSanitizeElement?: UponSanitizeElementCb;
@@ -205,16 +213,29 @@ export function sanitizeHtml(untrusted: string, config?: SanitizeOptions): Trust
 	try {
 		const resolvedConfig: dompurify.Config = { ...defaultDomPurifyConfig };
 
-		if (config?.overrideAllowedTags) {
-			resolvedConfig.ALLOWED_TAGS = [...config.overrideAllowedTags];
+		if (config?.allowedTags) {
+			if (config.allowedTags.override) {
+				resolvedConfig.ALLOWED_TAGS = [...config.allowedTags.override];
+			}
+
+			if (config?.allowedTags?.augment) {
+				resolvedConfig.ALLOWED_TAGS = [...(resolvedConfig.ALLOWED_TAGS ?? []), ...config.allowedTags.augment];
+			}
 		}
 
-		if (config?.overrideAllowedAttributes) {
-			resolvedConfig.ALLOWED_ATTR = [...config.overrideAllowedAttributes];
+		if (config?.allowedAttributes) {
+			if (config.allowedAttributes.override) {
+				resolvedConfig.ALLOWED_ATTR = [...config.allowedAttributes.override];
+			}
+
+			if (config?.allowedAttributes?.augment) {
+				resolvedConfig.ALLOWED_ATTR = [...(resolvedConfig.ALLOWED_ATTR ?? []), ...config.allowedAttributes.augment];
+			}
 		}
 
-		const allowedProtocols = config?.overrideAllowedProtocols ?? defaultSafeProtocols;
-		store.add(hookDomPurifyHrefAndSrcSanitizer(allowedProtocols, config?.allowDataImages));
+		store.add(hookDomPurifyHrefAndSrcSanitizer(
+			config?.allowedLinkProtocols?.override ?? [Schemas.http, Schemas.https],
+			config?.allowedMediaProtocols?.override ?? [Schemas.http, Schemas.https]));
 
 		if (config?.hooks?.uponSanitizeElement) {
 			store.add(addDompurifyHook('uponSanitizeElement', config?.hooks.uponSanitizeElement));

src/vs/base/browser/markdownRenderer.ts

@@ -419,8 +419,6 @@ export const allowedMarkdownHtmlAttributes = [
 	'class',
 	'colspan',
 	'controls',
-	'data-code',
-	'data-href',
 	'disabled',
 	'draggable',
 	'height',
@@ -437,17 +435,20 @@ export const allowedMarkdownHtmlAttributes = [
 	'width',
 	'start',
 
+	// Custom markdown attributes
+	'data-code',
+	'data-href',
+
 	// These attributes are sanitized in the hooks
 	'style',
 	'class',
 ];
 
 function getSanitizerOptions(options: IInternalSanitizerOptions): domSanitize.SanitizeOptions {
-	const allowedSchemes = [
+	const allowedLinkSchemes = [
 		Schemas.http,
 		Schemas.https,
 		Schemas.mailto,
-		Schemas.data,
 		Schemas.file,
 		Schemas.vscodeFileResource,
 		Schemas.vscodeRemote,
@@ -456,21 +457,38 @@ function getSanitizerOptions(options: IInternalSanitizerOptions): domSanitize.Sa
 	];
 
 	if (options.isTrusted) {
-		allowedSchemes.push(Schemas.command);
+		allowedLinkSchemes.push(Schemas.command);
 	}
 
 	if (options.allowedProductProtocols) {
-		allowedSchemes.push(...options.allowedProductProtocols);
+		allowedLinkSchemes.push(...options.allowedProductProtocols);
 	}
 
 	return {
 		// allowedTags should included everything that markdown renders to.
 		// Since we have our own sanitize function for marked, it's possible we missed some tag so let dompurify make sure.
 		// HTML tags that can result from markdown are from reading https://spec.commonmark.org/0.29/
 		// HTML table tags that can result from markdown are from https://github.github.com/gfm/#tables-extension-
-		overrideAllowedTags: options.allowedTags ?? domSanitize.basicMarkupHtmlTags,
-		overrideAllowedAttributes: allowedMarkdownHtmlAttributes,
-		overrideAllowedProtocols: allowedSchemes,
+		allowedTags: {
+			override: options.allowedTags ?? domSanitize.basicMarkupHtmlTags
+		},
+		allowedAttributes: {
+			override: allowedMarkdownHtmlAttributes,
+		},
+		allowedLinkProtocols: {
+			override: allowedLinkSchemes,
+		},
+		allowedMediaProtocols: {
+			override: [
+				Schemas.http,
+				Schemas.https,
+				Schemas.data,
+				Schemas.file,
+				Schemas.vscodeFileResource,
+				Schemas.vscodeRemote,
+				Schemas.vscodeRemoteResource,
+			]
+		},
 		hooks: {
 			uponSanitizeAttribute: (element, e) => {
 				if (options.customAttrSanitizer) {

src/vs/base/test/browser/domSanitize.test.ts

@@ -6,6 +6,7 @@
 import { ensureNoDisposablesAreLeakedInTestSuite } from '../common/utils.js';
 import { sanitizeHtml } from '../../browser/domSanitize.js';
 import * as assert from 'assert';
+import { Schemas } from '../../common/network.js';
 
 suite('DomSanitize', () => {
 
@@ -34,19 +35,26 @@ suite('DomSanitize', () => {
 	});
 
 	test('allows custom tags via config', () => {
-		const html = '<custom-tag>hello</custom-tag>';
-		const result = sanitizeHtml(html, {
-			overrideAllowedTags: ['custom-tag']
-		});
-		const str = result.toString();
-
-		assert.ok(str.includes('<custom-tag>hello</custom-tag>'));
+		{
+			const html = '<div>removed</div><custom-tag>hello</custom-tag>';
+			const result = sanitizeHtml(html, {
+				allowedTags: { override: ['custom-tag'] }
+			});
+			assert.strictEqual(result.toString(), 'removed<custom-tag>hello</custom-tag>');
+		}
+		{
+			const html = '<div>kept</div><augmented-tag>world</augmented-tag>';
+			const result = sanitizeHtml(html, {
+				allowedTags: { augment: ['augmented-tag'] }
+			});
+			assert.strictEqual(result.toString(), '<div>kept</div><augmented-tag>world</augmented-tag>');
+		}
 	});
 
 	test('allows custom attributes via config', () => {
 		const html = '<div custom-attr="value">content</div>';
 		const result = sanitizeHtml(html, {
-			overrideAllowedAttributes: ['custom-attr']
+			allowedAttributes: { override: ['custom-attr'] }
 		});
 		const str = result.toString();
 
@@ -98,7 +106,9 @@ suite('DomSanitize', () => {
 
 	test('allows data images when enabled', () => {
 		const html = '<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8/5+hHgAHggJ/PchI7wAAAABJRU5ErkJggg==">';
-		const result = sanitizeHtml(html, { allowDataImages: true });
+		const result = sanitizeHtml(html, {
+			allowedMediaProtocols: { override: [Schemas.data] }
+		});
 		const str = result.toString();
 
 		assert.ok(str.includes('src="data:image/png;base64,'));

src/vs/workbench/contrib/issue/browser/issueFormService.ts

@@ -93,7 +93,23 @@ export class IssueFormService implements IIssueFormService {
 			// removes preset monaco-workbench
 			auxiliaryWindow.container.remove();
 			auxiliaryWindow.window.document.body.appendChild(div);
-			safeInnerHtml(div, BaseHtml());
+			safeInnerHtml(div, BaseHtml(), {
+				// Also allow input elements
+				allowedTags: {
+					augment: [
+						'input',
+						'select',
+						'checkbox',
+					]
+				},
+				allowedAttributes: {
+					augment: [
+						'id',
+						'class',
+						'style',
+					]
+				}
+			});
 
 			this.issueReporterWindow = auxiliaryWindow.window;
 		} else {

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

@@ -163,20 +163,28 @@ const allowedProtocols = [
 ];
 
 function sanitize(documentContent: string, allowAllProtocols = false): TrustedHTML {
+	// TODO: Move most of these options to the callers
 	return sanitizeHtml(documentContent, {
-		overrideAllowedProtocols: allowAllProtocols ? '*' : allowedProtocols,
-		overrideAllowedTags: [
-			...basicMarkupHtmlTags,
-			'input',
-			'checkbox',
-			'checklist',
-		],
-		overrideAllowedAttributes: [
-			...allowedMarkdownHtmlAttributes,
-			'data-command', 'name', 'id', 'role', 'tabindex',
-			'x-dispatch',
-			'required', 'checked', 'placeholder', 'when-checked', 'checked-on',
-		],
+		allowedLinkProtocols: {
+			override: allowAllProtocols ? '*' : allowedProtocols,
+		},
+		allowedTags: {
+			override: [
+				...basicMarkupHtmlTags,
+				'input',
+				'select',
+				'checkbox',
+				'checklist',
+			],
+		},
+		allowedAttributes: {
+			override: [
+				...allowedMarkdownHtmlAttributes,
+				'data-command', 'name', 'id', 'role', 'tabindex',
+				'x-dispatch',
+				'required', 'checked', 'placeholder', 'when-checked', 'checked-on',
+			],
+		}
 	});
 }