Skip to content

Commit 8c88ebb

Browse files
mjbvzmjbvz
committed
Use more controlled sanitizer for attributes
For microsoft#261132
1 parent d6b3b49 commit 8c88ebb

File tree

4 files changed

+130
-68
lines changed

4 files changed

+130
-68
lines changed

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

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,18 @@ function hookDomPurifyHrefAndSrcSanitizer(allowedLinkProtocols: readonly string[
153153
return toDisposable(() => dompurify.removeHook('afterSanitizeAttributes'));
154154
}
155155

156+
/**
157+
* Predicate that checks if an attribute should be kept or removed.
158+
*
159+
* @returns A boolean indicating whether the attribute should be kept or a string with the sanitized value (which implicitly keeps the attribute)
160+
*/
161+
export type SanitizeAttributePredicate = (node: Element, data: { readonly attrName: string; readonly attrValue: string }) => boolean | string;
162+
163+
export interface SanitizeAttributeRule {
164+
readonly attributeName: string;
165+
shouldKeep: SanitizeAttributePredicate;
166+
}
167+
156168
export interface DomSanitizerConfig {
157169
/**
158170
* Configured the allowed html tags.
@@ -166,8 +178,8 @@ export interface DomSanitizerConfig {
166178
* Configured the allowed html attributes.
167179
*/
168180
readonly allowedAttributes?: {
169-
readonly override?: readonly string[];
170-
readonly augment?: readonly string[];
181+
readonly override?: ReadonlyArray<string | SanitizeAttributeRule>;
182+
readonly augment?: ReadonlyArray<string | SanitizeAttributeRule>;
171183
};
172184

173185
/**
@@ -190,11 +202,6 @@ export interface DomSanitizerConfig {
190202
* For example, <p><bad>"text"</bad></p> becomes <p>"<bad>text</bad>"</p>.
191203
*/
192204
readonly replaceWithPlaintext?: boolean;
193-
194-
// TODO: move these into more controlled api
195-
readonly _do_not_use_hooks?: {
196-
readonly uponSanitizeAttribute?: UponSanitizeAttributeCb;
197-
};
198205
}
199206

200207
const defaultDomPurifyConfig = Object.freeze({
@@ -230,16 +237,27 @@ export function sanitizeHtml(untrusted: string, config?: DomSanitizerConfig): Tr
230237
}
231238
}
232239

240+
let resolvedAttributes: Array<string | SanitizeAttributeRule> = [...defaultAllowedAttrs];
233241
if (config?.allowedAttributes) {
234242
if (config.allowedAttributes.override) {
235-
resolvedConfig.ALLOWED_ATTR = [...config.allowedAttributes.override];
243+
resolvedAttributes = [...config.allowedAttributes.override];
236244
}
237245

238246
if (config.allowedAttributes.augment) {
239-
resolvedConfig.ALLOWED_ATTR = [...(resolvedConfig.ALLOWED_ATTR ?? []), ...config.allowedAttributes.augment];
247+
resolvedAttributes = [...resolvedAttributes, ...config.allowedAttributes.augment];
248+
}
249+
}
250+
251+
const allowedAttrNames = new Set(resolvedAttributes.map(attr => typeof attr === 'string' ? attr : attr.attributeName));
252+
const allowedAttrPredicates = new Map<string, SanitizeAttributeRule>();
253+
for (const attr of resolvedAttributes) {
254+
if (typeof attr !== 'string') {
255+
allowedAttrPredicates.set(attr.attributeName, attr);
240256
}
241257
}
242258

259+
resolvedConfig.ALLOWED_ATTR = Array.from(allowedAttrNames);
260+
243261
store.add(hookDomPurifyHrefAndSrcSanitizer(
244262
config?.allowedLinkProtocols?.override ?? [Schemas.http, Schemas.https],
245263
config?.allowedMediaProtocols?.override ?? [Schemas.http, Schemas.https]));
@@ -248,8 +266,21 @@ export function sanitizeHtml(untrusted: string, config?: DomSanitizerConfig): Tr
248266
store.add(addDompurifyHook('uponSanitizeElement', replaceWithPlainTextHook));
249267
}
250268

251-
if (config?._do_not_use_hooks?.uponSanitizeAttribute) {
252-
store.add(addDompurifyHook('uponSanitizeAttribute', config._do_not_use_hooks.uponSanitizeAttribute));
269+
if (allowedAttrPredicates.size) {
270+
store.add(addDompurifyHook('uponSanitizeAttribute', (node, e) => {
271+
const predicate = allowedAttrPredicates.get(e.attrName);
272+
if (predicate) {
273+
const result = predicate.shouldKeep(node, e);
274+
if (typeof result === 'string') {
275+
e.keepAttr = true;
276+
e.attrValue = result;
277+
} else {
278+
e.keepAttr = result;
279+
}
280+
} else {
281+
e.keepAttr = allowedAttrNames.has(e.attrName);
282+
}
283+
}));
253284
}
254285

255286
return dompurify.sanitize(untrusted, {

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

Lines changed: 35 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@ export interface MarkdownSanitizerConfig {
5656
readonly allowedTags?: {
5757
readonly override: readonly string[];
5858
};
59-
readonly customAttrSanitizer?: (attrName: string, attrValue: string) => boolean | string;
59+
readonly allowedAttributes?: {
60+
readonly override: ReadonlyArray<string | domSanitize.SanitizeAttributeRule>;
61+
};
6062
readonly allowedLinkSchemes?: {
6163
readonly augment: readonly string[];
6264
};
@@ -448,14 +450,12 @@ export const allowedMarkdownHtmlTags = Object.freeze([
448450
'input', // Allow inputs for rendering checkboxes. Other types of inputs are removed and the inputs are always disabled
449451
]);
450452

451-
export const allowedMarkdownHtmlAttributes = [
453+
export const allowedMarkdownHtmlAttributes = Object.freeze<Array<string | domSanitize.SanitizeAttributeRule>>([
452454
'align',
453455
'autoplay',
454456
'alt',
455-
'checked',
456457
'colspan',
457458
'controls',
458-
'disabled',
459459
'draggable',
460460
'height',
461461
'href',
@@ -470,16 +470,42 @@ export const allowedMarkdownHtmlAttributes = [
470470
'type',
471471
'width',
472472
'start',
473+
474+
// Input (For disabled inputs)
475+
'checked',
476+
'disabled',
473477
'value',
474478

475479
// Custom markdown attributes
476480
'data-code',
477481
'data-href',
478482

479-
// These attributes are sanitized in the hooks
480-
'style',
481-
'class',
482-
];
483+
// Only allow very specific styles
484+
{
485+
attributeName: 'style',
486+
shouldKeep: (element, data) => {
487+
if (element.tagName === 'SPAN') {
488+
if (data.attrName === 'style') {
489+
return /^(color\:(#[0-9a-fA-F]+|var\(--vscode(-[a-zA-Z0-9]+)+\));)?(background-color\:(#[0-9a-fA-F]+|var\(--vscode(-[a-zA-Z0-9]+)+\));)?(border-radius:[0-9]+px;)?$/.test(data.attrValue);
490+
}
491+
}
492+
return false;
493+
}
494+
},
495+
496+
// Only allow codicons for classes
497+
{
498+
attributeName: 'class',
499+
shouldKeep: (element, data) => {
500+
if (element.tagName === 'SPAN') {
501+
if (data.attrName === 'class') {
502+
return /^codicon codicon-[a-z\-]+( codicon-modifier-[a-z\-]+)?$/.test(data.attrValue);
503+
}
504+
}
505+
return false;
506+
},
507+
},
508+
]);
483509

484510
function getDomSanitizerConfig(isTrusted: boolean | MarkdownStringTrustedOptions, options: MarkdownSanitizerConfig): domSanitize.DomSanitizerConfig {
485511
const allowedLinkSchemes = [
@@ -510,7 +536,7 @@ function getDomSanitizerConfig(isTrusted: boolean | MarkdownStringTrustedOptions
510536
override: options.allowedTags?.override ?? allowedMarkdownHtmlTags
511537
},
512538
allowedAttributes: {
513-
override: allowedMarkdownHtmlAttributes,
539+
override: options.allowedAttributes?.override ?? allowedMarkdownHtmlAttributes,
514540
},
515541
allowedLinkProtocols: {
516542
override: allowedLinkSchemes,
@@ -527,45 +553,6 @@ function getDomSanitizerConfig(isTrusted: boolean | MarkdownStringTrustedOptions
527553
]
528554
},
529555
replaceWithPlaintext: options.replaceWithPlaintext,
530-
_do_not_use_hooks: {
531-
uponSanitizeAttribute: (element, e) => {
532-
if (options.customAttrSanitizer) {
533-
const result = options.customAttrSanitizer(e.attrName, e.attrValue);
534-
if (typeof result === 'string') {
535-
if (result) {
536-
e.attrValue = result;
537-
e.keepAttr = true;
538-
} else {
539-
e.keepAttr = false;
540-
}
541-
} else {
542-
e.keepAttr = result;
543-
}
544-
return;
545-
}
546-
547-
if (e.attrName === 'style' || e.attrName === 'class') {
548-
if (element.tagName === 'SPAN') {
549-
if (e.attrName === 'style') {
550-
e.keepAttr = /^(color\:(#[0-9a-fA-F]+|var\(--vscode(-[a-zA-Z0-9]+)+\));)?(background-color\:(#[0-9a-fA-F]+|var\(--vscode(-[a-zA-Z0-9]+)+\));)?(border-radius:[0-9]+px;)?$/.test(e.attrValue);
551-
return;
552-
} else if (e.attrName === 'class') {
553-
e.keepAttr = /^codicon codicon-[a-z\-]+( codicon-modifier-[a-z\-]+)?$/.test(e.attrValue);
554-
return;
555-
}
556-
}
557-
558-
e.keepAttr = false;
559-
return;
560-
} else if (element.tagName === 'INPUT' && element.attributes.getNamedItem('type')?.value === 'checkbox') {
561-
if ((e.attrName === 'type' && e.attrValue === 'checkbox') || e.attrName === 'disabled' || e.attrName === 'checked') {
562-
e.keepAttr = true;
563-
return;
564-
}
565-
e.keepAttr = false;
566-
}
567-
},
568-
}
569556
};
570557
}
571558

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

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { ensureNoDisposablesAreLeakedInTestSuite } from '../common/utils.js';
7-
import { sanitizeHtml } from '../../browser/domSanitize.js';
86
import * as assert from 'assert';
7+
import { sanitizeHtml } from '../../browser/domSanitize.js';
98
import { Schemas } from '../../common/network.js';
9+
import { ensureNoDisposablesAreLeakedInTestSuite } from '../common/utils.js';
1010

1111
suite('DomSanitize', () => {
1212

@@ -114,6 +114,45 @@ suite('DomSanitize', () => {
114114
assert.ok(str.includes('src="data:image/png;base64,'));
115115
});
116116

117+
test('Supports dynamic attribute sanitization', () => {
118+
const html = '<div title="a" other="1">text1</div><div title="b" other="2">text2</div>';
119+
const result = sanitizeHtml(html, {
120+
allowedAttributes: {
121+
override: [
122+
{
123+
attributeName: 'title',
124+
shouldKeep: (_el, data) => {
125+
return data.attrValue.includes('b');
126+
}
127+
}
128+
]
129+
}
130+
});
131+
const str = result.toString();
132+
assert.strictEqual(str, '<div>text1</div><div title="b">text2</div>');
133+
});
134+
135+
test('Supports changing attributes in dynamic sanitization', () => {
136+
const html = '<div title="abc" other="1">text1</div><div title="xyz" other="2">text2</div>';
137+
const result = sanitizeHtml(html, {
138+
allowedAttributes: {
139+
override: [
140+
{
141+
attributeName: 'title',
142+
shouldKeep: (_el, data) => {
143+
if (data.attrValue === 'abc') {
144+
return false;
145+
}
146+
return data.attrValue + data.attrValue;
147+
}
148+
}
149+
]
150+
}
151+
});
152+
// xyz title should be preserved and doubled
153+
assert.strictEqual(result.toString(), '<div>text1</div><div title="xyzxyz">text2</div>');
154+
});
155+
117156
suite('replaceWithPlaintext', () => {
118157

119158
test('replaces unsupported tags with plaintext representation', () => {

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,19 @@ export class MarkedKatexSupport {
2222
...trustedMathMlTags,
2323
]
2424
},
25-
customAttrSanitizer: (attrName, attrValue) => {
26-
if (attrName === 'class') {
27-
return true; // TODO: allows all classes for now since we don't have a list of possible katex classes
28-
} else if (attrName === 'style') {
29-
return this.sanitizeKatexStyles(attrValue);
30-
}
25+
allowedAttributes: {
26+
override: [
27+
...baseConfig.allowedAttributes,
3128

32-
return baseConfig.allowedAttributes.includes(attrName);
29+
// Allow all classes since we don't have a list of allowed katex classes
30+
'class',
31+
32+
// Sanitize allowed styles for katex
33+
{
34+
attributeName: 'style',
35+
shouldKeep: (_el, data) => this.sanitizeKatexStyles(data.attrValue),
36+
},
37+
]
3338
},
3439
};
3540
}

0 commit comments

Comments
 (0)