Tighten up dom sanitizing rules

This change adds restrictions to our dom sanitizer rules. The goal is to make the core set of rules more restrictive and require callers to opt into more permissive tags

Main changes:

- Remove input tags being allowed by default
- Remove some markdown specific / custom attributes from the default list
- Remove id, class, and style attrs from default layer
- Split up link/media allowed commands
- Restructure api to make it more clear how you can override or augment tags/attrs

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',
+			],
+		}
 	});
 }