cargdev/angular
1
0
Fork 0
Code Issues 17 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(core): make HTML sanitizer return TrustedHTML (#39218)

Browse Source 
Make Angular's HTML sanitizer return a TrustedHTML, as its output is
trusted not to cause XSS vulnerabilities when used in a context where a
browser may parse and evaluate HTML. Also update tests to reflect the
new behaviour.

PR Close #39218
This commit is contained in:
Bjarki
2020-10-10 00:27:29 +00:00
committed by Andrew Kushnir
parent e8d47c2d41
commit 9ec2bad4dc
4 changed files with 56 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
packages/core/src/sanitization/html_sanitizer.ts
View File

@ -7,6 +7,8 @@
*/
import {isDevMode} from '../util/is_dev_mode';
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';
@ -242,7 +244,7 @@ let inertBodyHelper: InertBodyHelper;
* Sanitizes the given unsafe, untrusted HTML fragment, and returns HTML text that is safe to add to
* the DOM in a browser environment.
*/
export function _sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): string {
export function _sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): TrustedHTML|string {
let inertBodyElement: HTMLElement|null = null;
try {
inertBodyHelper = inertBodyHelper || getInertBodyHelper(defaultDoc);
@ -274,7 +276,7 @@ export function _sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): string
'WARNING: sanitizing HTML stripped some content, see http://g.co/ng/security#xss');
}
return safeHtml;
return trustedHTMLFromString(safeHtml);
} finally {
// In case anything goes wrong, clear out inertElement to reset the entire DOM structure.
if (inertBodyElement) {

91
packages/core/test/sanitization/html_sanitizer_spec.ts
View File

@ -11,6 +11,10 @@ import {browserDetection} from '@angular/platform-browser/testing/src/browser_ut
import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
function sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): string {
return _sanitizeHtml(defaultDoc, unsafeHtmlInput).toString();
}
{
describe('HTML sanitizer', () => {
let defaultDoc: any;
@ -29,73 +33,73 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
});
it('serializes nested structures', () => {
expect(_sanitizeHtml(defaultDoc, '<div alt="x"><p>a</p>b<b>c<a alt="more">d</a></b>e</div>'))
expect(sanitizeHtml(defaultDoc, '<div alt="x"><p>a</p>b<b>c<a alt="more">d</a></b>e</div>'))
.toEqual('<div alt="x"><p>a</p>b<b>c<a alt="more">d</a></b>e</div>');
expect(logMsgs).toEqual([]);
});
it('serializes self closing elements', () => {
expect(_sanitizeHtml(defaultDoc, '<p>Hello <br> World</p>'))
expect(sanitizeHtml(defaultDoc, '<p>Hello <br> World</p>'))
.toEqual('<p>Hello <br> World</p>');
});
it('supports namespaced elements', () => {
expect(_sanitizeHtml(defaultDoc, 'a<my:hr/><my:div>b</my:div>c')).toEqual('abc');
expect(sanitizeHtml(defaultDoc, 'a<my:hr/><my:div>b</my:div>c')).toEqual('abc');
});
it('supports namespaced attributes', () => {
expect(_sanitizeHtml(defaultDoc, '<a xlink:href="something">t</a>'))
expect(sanitizeHtml(defaultDoc, '<a xlink:href="something">t</a>'))
.toEqual('<a xlink:href="something">t</a>');
expect(_sanitizeHtml(defaultDoc, '<a xlink:evil="something">t</a>')).toEqual('<a>t</a>');
expect(_sanitizeHtml(defaultDoc, '<a xlink:href="javascript:foo()">t</a>'))
expect(sanitizeHtml(defaultDoc, '<a xlink:evil="something">t</a>')).toEqual('<a>t</a>');
expect(sanitizeHtml(defaultDoc, '<a xlink:href="javascript:foo()">t</a>'))
.toEqual('<a xlink:href="unsafe:javascript:foo()">t</a>');
});
it('supports HTML5 elements', () => {
expect(_sanitizeHtml(defaultDoc, '<main><summary>Works</summary></main>'))
expect(sanitizeHtml(defaultDoc, '<main><summary>Works</summary></main>'))
.toEqual('<main><summary>Works</summary></main>');
});
it('supports ARIA attributes', () => {
expect(_sanitizeHtml(defaultDoc, '<h1 role="presentation" aria-haspopup="true">Test</h1>'))
expect(sanitizeHtml(defaultDoc, '<h1 role="presentation" aria-haspopup="true">Test</h1>'))
.toEqual('<h1 role="presentation" aria-haspopup="true">Test</h1>');
expect(_sanitizeHtml(defaultDoc, '<i aria-label="Info">Info</i>'))
expect(sanitizeHtml(defaultDoc, '<i aria-label="Info">Info</i>'))
.toEqual('<i aria-label="Info">Info</i>');
expect(_sanitizeHtml(defaultDoc, '<img src="pteranodon.jpg" aria-details="details">'))
expect(sanitizeHtml(defaultDoc, '<img src="pteranodon.jpg" aria-details="details">'))
.toEqual('<img src="pteranodon.jpg" aria-details="details">');
});
it('sanitizes srcset attributes', () => {
expect(_sanitizeHtml(defaultDoc, '<img srcset="/foo.png 400px, javascript:evil() 23px">'))
expect(sanitizeHtml(defaultDoc, '<img srcset="/foo.png 400px, javascript:evil() 23px">'))
.toEqual('<img srcset="/foo.png 400px, unsafe:javascript:evil() 23px">');
});
it('supports sanitizing plain text', () => {
expect(_sanitizeHtml(defaultDoc, 'Hello, World')).toEqual('Hello, World');
expect(sanitizeHtml(defaultDoc, 'Hello, World')).toEqual('Hello, World');
});
it('ignores non-element, non-attribute nodes', () => {
expect(_sanitizeHtml(defaultDoc, '<!-- comments? -->no.')).toEqual('no.');
expect(_sanitizeHtml(defaultDoc, '<?pi nodes?>no.')).toEqual('no.');
expect(sanitizeHtml(defaultDoc, '<!-- comments? -->no.')).toEqual('no.');
expect(sanitizeHtml(defaultDoc, '<?pi nodes?>no.')).toEqual('no.');
expect(logMsgs.join('\n')).toMatch(/sanitizing HTML stripped some content/);
});
it('supports sanitizing escaped entities', () => {
expect(_sanitizeHtml(defaultDoc, '&#128640;')).toEqual('&#128640;');
expect(sanitizeHtml(defaultDoc, '&#128640;')).toEqual('&#128640;');
expect(logMsgs).toEqual([]);
});
it('does not warn when just re-encoding text', () => {
expect(_sanitizeHtml(defaultDoc, '<p>Hellö Wörld</p>'))
expect(sanitizeHtml(defaultDoc, '<p>Hellö Wörld</p>'))
.toEqual('<p>Hell&#246; W&#246;rld</p>');
expect(logMsgs).toEqual([]);
});
it('escapes entities', () => {
expect(_sanitizeHtml(defaultDoc, '<p>Hello &lt; World</p>'))
expect(sanitizeHtml(defaultDoc, '<p>Hello &lt; World</p>'))
.toEqual('<p>Hello &lt; World</p>');
expect(_sanitizeHtml(defaultDoc, '<p>Hello < World</p>')).toEqual('<p>Hello &lt; World</p>');
expect(_sanitizeHtml(defaultDoc, '<p alt="% &amp; &quot; !">Hello</p>'))
expect(sanitizeHtml(defaultDoc, '<p>Hello < World</p>')).toEqual('<p>Hello &lt; World</p>');
expect(sanitizeHtml(defaultDoc, '<p alt="% &amp; &quot; !">Hello</p>'))
.toEqual('<p alt="% &amp; &#34; !">Hello</p>'); // NB: quote encoded as ASCII &#34;.
});
@ -110,7 +114,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
];
for (const tag of dangerousTags) {
it(tag, () => {
expect(_sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual('evil!');
expect(sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual('evil!');
});
}
@ -125,7 +129,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
];
for (const tag of dangerousSelfClosingTags) {
it(tag, () => {
expect(_sanitizeHtml(defaultDoc, `before<${tag}>After`)).toEqual('beforeAfter');
expect(sanitizeHtml(defaultDoc, `before<${tag}>After`)).toEqual('beforeAfter');
});
}
@ -136,7 +140,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
];
for (const tag of dangerousSkipContentTags) {
it(tag, () => {
expect(_sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual('');
expect(sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual('');
});
}
@ -144,7 +148,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
// `<frame>` is special, because different browsers treat it differently (e.g. remove it
// altogether). // We just verify that (one way or another), there is no `<frame>` element
// after sanitization.
expect(_sanitizeHtml(defaultDoc, `<frame>evil!</frame>`)).not.toContain('<frame>');
expect(sanitizeHtml(defaultDoc, `<frame>evil!</frame>`)).not.toContain('<frame>');
});
});
@ -153,45 +157,45 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
for (const attr of dangerousAttrs) {
it(`${attr}`, () => {
expect(_sanitizeHtml(defaultDoc, `<a ${attr}="x">evil!</a>`)).toEqual('<a>evil!</a>');
expect(sanitizeHtml(defaultDoc, `<a ${attr}="x">evil!</a>`)).toEqual('<a>evil!</a>');
});
}
});
it('ignores content of script elements', () => {
expect(_sanitizeHtml(defaultDoc, '<script>var foo="<p>bar</p>"</script>')).toEqual('');
expect(_sanitizeHtml(defaultDoc, '<script>var foo="<p>bar</p>"</script><div>hi</div>'))
expect(sanitizeHtml(defaultDoc, '<script>var foo="<p>bar</p>"</script>')).toEqual('');
expect(sanitizeHtml(defaultDoc, '<script>var foo="<p>bar</p>"</script><div>hi</div>'))
.toEqual('<div>hi</div>');
expect(_sanitizeHtml(defaultDoc, '<style>\<\!-- something--\>hi</style>')).toEqual('');
expect(sanitizeHtml(defaultDoc, '<style>\<\!-- something--\>hi</style>')).toEqual('');
});
it('ignores content of style elements', () => {
expect(_sanitizeHtml(defaultDoc, '<style><!-- foobar --></style><div>hi</div>'))
expect(sanitizeHtml(defaultDoc, '<style><!-- foobar --></style><div>hi</div>'))
.toEqual('<div>hi</div>');
expect(_sanitizeHtml(defaultDoc, '<style><!-- foobar --></style>')).toEqual('');
expect(_sanitizeHtml(defaultDoc, '<style>\<\!-- something--\>hi</style>')).toEqual('');
expect(sanitizeHtml(defaultDoc, '<style><!-- foobar --></style>')).toEqual('');
expect(sanitizeHtml(defaultDoc, '<style>\<\!-- something--\>hi</style>')).toEqual('');
expect(logMsgs.join('\n')).toMatch(/sanitizing HTML stripped some content/);
});
it('should strip unclosed iframe tag', () => {
expect(_sanitizeHtml(defaultDoc, '<iframe>')).toEqual('');
expect(sanitizeHtml(defaultDoc, '<iframe>')).toEqual('');
expect([
'&lt;iframe&gt;',
// Double-escaped on IE
'&amp;lt;iframe&amp;gt;'
]).toContain(_sanitizeHtml(defaultDoc, '<iframe><iframe>'));
]).toContain(sanitizeHtml(defaultDoc, '<iframe><iframe>'));
expect([
'&lt;script&gt;evil();&lt;/script&gt;',
// Double-escaped on IE
'&amp;lt;script&amp;gt;evil();&amp;lt;/script&amp;gt;'
]).toContain(_sanitizeHtml(defaultDoc, '<iframe><script>evil();</script>'));
]).toContain(sanitizeHtml(defaultDoc, '<iframe><script>evil();</script>'));
});
it('should ignore extraneous body tags', () => {
expect(_sanitizeHtml(defaultDoc, '</body>')).toEqual('');
expect(_sanitizeHtml(defaultDoc, 'foo</body>bar')).toEqual('foobar');
expect(_sanitizeHtml(defaultDoc, 'foo<body>bar')).toEqual('foobar');
expect(_sanitizeHtml(defaultDoc, 'fo<body>ob</body>ar')).toEqual('foobar');
expect(sanitizeHtml(defaultDoc, '</body>')).toEqual('');
expect(sanitizeHtml(defaultDoc, 'foo</body>bar')).toEqual('foobar');
expect(sanitizeHtml(defaultDoc, 'foo<body>bar')).toEqual('foobar');
expect(sanitizeHtml(defaultDoc, 'fo<body>ob</body>ar')).toEqual('foobar');
});
it('should not enter an infinite loop on clobbered elements', () => {
@ -200,18 +204,17 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
// Anyway what we want to test is that browsers do not enter an infinite loop which would
// result in a timeout error for the test.
try {
_sanitizeHtml(defaultDoc, '<form><input name="parentNode" /></form>');
sanitizeHtml(defaultDoc, '<form><input name="parentNode" /></form>');
} catch (e) {
// depending on the browser, we might ge an exception
}
try {
_sanitizeHtml(defaultDoc, '<form><input name="nextSibling" /></form>');
sanitizeHtml(defaultDoc, '<form><input name="nextSibling" /></form>');
} catch (e) {
// depending on the browser, we might ge an exception
}
try {
_sanitizeHtml(
defaultDoc, '<form><div><div><input name="nextSibling" /></div></div></form>');
sanitizeHtml(defaultDoc, '<form><div><div><input name="nextSibling" /></div></div></form>');
} catch (e) {
// depending on the browser, we might ge an exception
}
@ -220,7 +223,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
// See
// https://github.com/cure53/DOMPurify/blob/a992d3a75031cb8bb032e5ea8399ba972bdf9a65/src/purify.js#L439-L449
it('should not allow JavaScript execution when creating inert document', () => {
const output = _sanitizeHtml(defaultDoc, '<svg><g onload="window.xxx = 100"></g></svg>');
const output = sanitizeHtml(defaultDoc, '<svg><g onload="window.xxx = 100"></g></svg>');
const window = defaultDoc.defaultView;
if (window) {
expect(window.xxx).toBe(undefined);
@ -232,7 +235,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
// See https://github.com/cure53/DOMPurify/releases/tag/0.6.7
it('should not allow JavaScript hidden in badly formed HTML to get through sanitization (Firefox bug)',
() => {
expect(_sanitizeHtml(
expect(sanitizeHtml(
defaultDoc, '<svg><p><style><img src="</style><img src=x onerror=alert(1)//">'))
.toEqual(
isDOMParserAvailable() ?
@ -245,7 +248,7 @@ import {isDOMParserAvailable} from '../../src/sanitization/inert_body';
if (browserDetection.isWebkit) {
it('should prevent mXSS attacks', function() {
// In Chrome Canary 62, the ideographic space character is kept as a stringified HTML entity
expect(_sanitizeHtml(defaultDoc, '<a href="&#x3000;javascript:alert(1)">CLICKME</a>'))
expect(sanitizeHtml(defaultDoc, '<a href="&#x3000;javascript:alert(1)">CLICKME</a>'))
.toMatch(/<a href="unsafe:(&#12288;)?javascript:alert\(1\)">CLICKME<\/a>/);
});
}

8
packages/core/test/sanitization/sanitization_spec.ts
View File

@ -29,11 +29,11 @@ describe('sanitization', () => {
}
}
it('should sanitize html', () => {
expect(ɵɵsanitizeHtml('<div></div>')).toEqual('<div></div>');
expect(ɵɵsanitizeHtml(new Wrap('<div></div>'))).toEqual('<div></div>');
expect(ɵɵsanitizeHtml('<img src="javascript:true">'))
expect(ɵɵsanitizeHtml('<div></div>').toString()).toEqual('<div></div>');
expect(ɵɵsanitizeHtml(new Wrap('<div></div>')).toString()).toEqual('<div></div>');
expect(ɵɵsanitizeHtml('<img src="javascript:true">').toString())
.toEqual('<img src="unsafe:javascript:true">');
expect(ɵɵsanitizeHtml(new Wrap('<img src="javascript:true">')))
expect(ɵɵsanitizeHtml(new Wrap('<img src="javascript:true">')).toString())
.toEqual('<img src="unsafe:javascript:true">');
expect(() => ɵɵsanitizeHtml(bypassSanitizationTrustUrl('<img src="javascript:true">')))
.toThrowError(/Required a safe HTML, got a URL/);

2
packages/platform-browser/src/security/dom_sanitization_service.ts
View File

@ -162,7 +162,7 @@ export class DomSanitizerImpl extends DomSanitizer {
if (allowSanitizationBypassOrThrow(value, BypassType.Html)) {
return unwrapSafeValue(value);
}
return _sanitizeHtml(this._doc, String(value));
return _sanitizeHtml(this._doc, String(value)).toString();
case SecurityContext.STYLE:
if (allowSanitizationBypassOrThrow(value, BypassType.Style)) {
return unwrapSafeValue(value);