From 9ec2bad4dc2b965fca5ae0f4051ed250dc616f65 Mon Sep 17 00:00:00 2001
From: Bjarki <bjarki@google.com>
Date: Sat, 10 Oct 2020 00:27:29 +0000
Subject: [PATCH] refactor(core): make HTML sanitizer return TrustedHTML
 (#39218)

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
---
 .../core/src/sanitization/html_sanitizer.ts   |  6 +-
 .../test/sanitization/html_sanitizer_spec.ts  | 91 ++++++++++---------
 .../test/sanitization/sanitization_spec.ts    |  8 +-
 .../src/security/dom_sanitization_service.ts  |  2 +-
 4 files changed, 56 insertions(+), 51 deletions(-)

diff --git a/packages/core/src/sanitization/html_sanitizer.ts b/packages/core/src/sanitization/html_sanitizer.ts
index ab7bbbe836..b0b30311a0 100644
--- a/packages/core/src/sanitization/html_sanitizer.ts
+++ b/packages/core/src/sanitization/html_sanitizer.ts
@@ -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) {
diff --git a/packages/core/test/sanitization/html_sanitizer_spec.ts b/packages/core/test/sanitization/html_sanitizer_spec.ts
index d577ce2c4d..aae4a18421 100644
--- a/packages/core/test/sanitization/html_sanitizer_spec.ts
+++ b/packages/core/test/sanitization/html_sanitizer_spec.ts
@@ -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>/);
       });
     }
diff --git a/packages/core/test/sanitization/sanitization_spec.ts b/packages/core/test/sanitization/sanitization_spec.ts
index 389e28b52a..e144427a6d 100644
--- a/packages/core/test/sanitization/sanitization_spec.ts
+++ b/packages/core/test/sanitization/sanitization_spec.ts
@@ -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/);
diff --git a/packages/platform-browser/src/security/dom_sanitization_service.ts b/packages/platform-browser/src/security/dom_sanitization_service.ts
index 69c08cb201..1521a93f39 100644
--- a/packages/platform-browser/src/security/dom_sanitization_service.ts
+++ b/packages/platform-browser/src/security/dom_sanitization_service.ts
@@ -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);