From 9e2d3ed2d13c0e12bc2e7ae733661ac2c2e97fed Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Thu, 1 Sep 2022 09:21:34 -0700 Subject: [PATCH] refactor(common): drop unnecessary srcset sanitization (#47302) This commit updates runtime and compiler to drop unnecessary `srcset` sanitization. The sanitization was needed previously for old browsers, but all modern browsers can handle `srcset` safely without any additional sanitization. See prior discussion in https://github.com/angular/angular/pull/45182. Resolves #45164. PR Close #47302 --- .../src/schema/dom_security_schema.ts | 22 +++++++++-- packages/core/src/render3/i18n/i18n_parse.ts | 7 +--- .../core/src/sanitization/html_sanitizer.ts | 11 ++---- .../core/src/sanitization/url_sanitizer.ts | 5 --- .../test/sanitization/html_sanitizer_spec.ts | 10 ++++- .../test/sanitization/url_sanitizer_spec.ts | 39 +------------------ 6 files changed, 33 insertions(+), 61 deletions(-) diff --git a/packages/compiler/src/schema/dom_security_schema.ts b/packages/compiler/src/schema/dom_security_schema.ts index 7d1182d49ad..253f00038fc 100644 --- a/packages/compiler/src/schema/dom_security_schema.ts +++ b/packages/compiler/src/schema/dom_security_schema.ts @@ -35,10 +35,24 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} { registerContext(SecurityContext.STYLE, ['*|style']); // NB: no SCRIPT contexts here, they are never allowed due to the parser stripping them. registerContext(SecurityContext.URL, [ - '*|formAction', 'area|href', 'area|ping', 'audio|src', 'a|href', - 'a|ping', 'blockquote|cite', 'body|background', 'del|cite', 'form|action', - 'img|src', 'img|srcset', 'input|src', 'ins|cite', 'q|cite', - 'source|src', 'source|srcset', 'track|src', 'video|poster', 'video|src', + '*|formAction', + 'area|href', + 'area|ping', + 'audio|src', + 'a|href', + 'a|ping', + 'blockquote|cite', + 'body|background', + 'del|cite', + 'form|action', + 'img|src', + 'input|src', + 'ins|cite', + 'q|cite', + 'source|src', + 'track|src', + 'video|poster', + 'video|src', ]); registerContext(SecurityContext.RESOURCE_URL, [ 'applet|code', diff --git a/packages/core/src/render3/i18n/i18n_parse.ts b/packages/core/src/render3/i18n/i18n_parse.ts index 56813212904..2ec4dcdc3ff 100644 --- a/packages/core/src/render3/i18n/i18n_parse.ts +++ b/packages/core/src/render3/i18n/i18n_parse.ts @@ -8,9 +8,9 @@ import '../../util/ng_dev_mode'; import '../../util/ng_i18n_closure_mode'; -import {getTemplateContent, SRCSET_ATTRS, URI_ATTRS, VALID_ATTRS, VALID_ELEMENTS} from '../../sanitization/html_sanitizer'; +import {getTemplateContent, URI_ATTRS, VALID_ATTRS, VALID_ELEMENTS} from '../../sanitization/html_sanitizer'; import {getInertBodyHelper} from '../../sanitization/inert_body'; -import {_sanitizeUrl, sanitizeSrcset} from '../../sanitization/url_sanitizer'; +import {_sanitizeUrl} from '../../sanitization/url_sanitizer'; import {assertDefined, assertEqual, assertGreaterThanOrEqual, assertOneOf, assertString} from '../../util/assert'; import {CharCode} from '../../util/char_code'; import {loadIcuContainerVisitor} from '../instructions/i18n_icu_container_visitor'; @@ -624,9 +624,6 @@ function walkIcuTree( if (URI_ATTRS[lowerAttrName]) { generateBindingUpdateOpCodes( update, attr.value, newIndex, attr.name, 0, _sanitizeUrl); - } else if (SRCSET_ATTRS[lowerAttrName]) { - generateBindingUpdateOpCodes( - update, attr.value, newIndex, attr.name, 0, sanitizeSrcset); } else { generateBindingUpdateOpCodes(update, attr.value, newIndex, attr.name, 0, null); } diff --git a/packages/core/src/sanitization/html_sanitizer.ts b/packages/core/src/sanitization/html_sanitizer.ts index b5121d452a4..f3cdc8bbb39 100644 --- a/packages/core/src/sanitization/html_sanitizer.ts +++ b/packages/core/src/sanitization/html_sanitizer.ts @@ -8,8 +8,9 @@ import {TrustedHTML} from '../util/security/trusted_type_defs'; import {trustedHTMLFromString} from '../util/security/trusted_types'; + import {getInertBodyHelper, InertBodyHelper} from './inert_body'; -import {_sanitizeUrl, sanitizeSrcset} from './url_sanitizer'; +import {_sanitizeUrl} from './url_sanitizer'; function tagSet(tags: string): {[k: string]: boolean} { const res: {[k: string]: boolean} = {}; @@ -64,14 +65,11 @@ export const VALID_ELEMENTS = // Attributes that have href and hence need to be sanitized export const URI_ATTRS = tagSet('background,cite,href,itemtype,longdesc,poster,src,xlink:href'); -// Attributes that have special href set hence need to be sanitized -export const SRCSET_ATTRS = tagSet('srcset'); - const HTML_ATTRS = tagSet( 'abbr,accesskey,align,alt,autoplay,axis,bgcolor,border,cellpadding,cellspacing,class,clear,color,cols,colspan,' + 'compact,controls,coords,datetime,default,dir,download,face,headers,height,hidden,hreflang,hspace,' + 'ismap,itemscope,itemprop,kind,label,lang,language,loop,media,muted,nohref,nowrap,open,preload,rel,rev,role,rows,rowspan,rules,' + - 'scope,scrolling,shape,size,sizes,span,srclang,start,summary,tabindex,target,title,translate,type,usemap,' + + 'scope,scrolling,shape,size,sizes,span,srclang,srcset,start,summary,tabindex,target,title,translate,type,usemap,' + 'valign,value,vspace,width'); // Accessibility attributes as per WAI-ARIA 1.1 (W3C Working Draft 14 December 2018) @@ -92,7 +90,7 @@ const ARIA_ATTRS = tagSet( // can be sanitized, but they increase security surface area without a legitimate use case, so they // are left out here. -export const VALID_ATTRS = merge(URI_ATTRS, SRCSET_ATTRS, HTML_ATTRS, ARIA_ATTRS); +export const VALID_ATTRS = merge(URI_ATTRS, HTML_ATTRS, ARIA_ATTRS); // Elements whose content should not be traversed/preserved, if the elements themselves are invalid. // @@ -177,7 +175,6 @@ class SanitizingHtmlSerializer { let value = elAttr!.value; // TODO(martinprobst): Special case image URIs for data:image/... if (URI_ATTRS[lower]) value = _sanitizeUrl(value); - if (SRCSET_ATTRS[lower]) value = sanitizeSrcset(value); this.buf.push(' ', attrName, '="', encodeEntities(value), '"'); } this.buf.push('>'); diff --git a/packages/core/src/sanitization/url_sanitizer.ts b/packages/core/src/sanitization/url_sanitizer.ts index c97f22d2e91..b2966b472a6 100644 --- a/packages/core/src/sanitization/url_sanitizer.ts +++ b/packages/core/src/sanitization/url_sanitizer.ts @@ -45,8 +45,3 @@ export function _sanitizeUrl(url: string): string { return 'unsafe:' + url; } - -export function sanitizeSrcset(srcset: string): string { - srcset = String(srcset); - return srcset.split(',').map((srcset) => _sanitizeUrl(srcset.trim())).join(', '); -} diff --git a/packages/core/test/sanitization/html_sanitizer_spec.ts b/packages/core/test/sanitization/html_sanitizer_spec.ts index 01d805c9be0..d66953f9a3b 100644 --- a/packages/core/test/sanitization/html_sanitizer_spec.ts +++ b/packages/core/test/sanitization/html_sanitizer_spec.ts @@ -69,9 +69,15 @@ function sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): string { .toEqual(''); }); - it('sanitizes srcset attributes', () => { + it('ignores srcset attributes', () => { + // Modern browsers can handle `srcset` safely without any additional sanitization. expect(sanitizeHtml(defaultDoc, '')) - .toEqual(''); + .toEqual(''); + + // Verify that complex `srcset` with URLs that contain commas are retained as is. + const content = ''; + expect(sanitizeHtml(defaultDoc, content)).toEqual(content); }); it('supports sanitizing plain text', () => { diff --git a/packages/core/test/sanitization/url_sanitizer_spec.ts b/packages/core/test/sanitization/url_sanitizer_spec.ts index 2ff6119dae5..f734d9ae09a 100644 --- a/packages/core/test/sanitization/url_sanitizer_spec.ts +++ b/packages/core/test/sanitization/url_sanitizer_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {_sanitizeUrl, sanitizeSrcset} from '../../src/sanitization/url_sanitizer'; +import {_sanitizeUrl} from '../../src/sanitization/url_sanitizer'; { describe('URL sanitizer', () => { @@ -71,42 +71,5 @@ import {_sanitizeUrl, sanitizeSrcset} from '../../src/sanitization/url_sanitizer it(`valid ${url}`, () => expect(_sanitizeUrl(url)).toMatch(/^unsafe:/)); } }); - - describe('valid srcsets', () => { - const validSrcsets = [ - '', - 'http://angular.io/images/test.png', - 'http://angular.io/images/test.png, http://angular.io/images/test.png', - 'http://angular.io/images/test.png, http://angular.io/images/test.png, http://angular.io/images/test.png', - 'http://angular.io/images/test.png 2x', - 'http://angular.io/images/test.png 2x, http://angular.io/images/test.png 3x', - 'http://angular.io/images/test.png 1.5x', - 'http://angular.io/images/test.png 1.25x', - 'http://angular.io/images/test.png 200w, http://angular.io/images/test.png 300w', - 'https://angular.io/images/test.png, http://angular.io/images/test.png', - 'http://angular.io:80/images/test.png, http://angular.io:8080/images/test.png', - 'http://www.angular.io:80/images/test.png, http://www.angular.io:8080/images/test.png', - 'https://angular.io/images/test.png, https://angular.io/images/test.png', - '//angular.io/images/test.png, //angular.io/images/test.png', - '/images/test.png, /images/test.png', - 'images/test.png, images/test.png', - 'http://angular.io/images/test.png?12345, http://angular.io/images/test.png?12345', - 'http://angular.io/images/test.png?maxage, http://angular.io/images/test.png?maxage', - 'http://angular.io/images/test.png?maxage=234, http://angular.io/images/test.png?maxage=234', - ]; - for (const srcset of validSrcsets) { - it(`valid ${srcset}`, () => expect(sanitizeSrcset(srcset)).toEqual(srcset)); - } - }); - - describe('invalid srcsets', () => { - const invalidSrcsets = [ - 'ht:tp://angular.io/images/test.png', - 'http://angular.io/images/test.png, ht:tp://angular.io/images/test.png', - ]; - for (const srcset of invalidSrcsets) { - it(`valid ${srcset}`, () => expect(sanitizeSrcset(srcset)).toMatch(/unsafe:/)); - } - }); }); }