fixup! fix(core): ignore comment nodes under unsafe elements

angular · Oct 23, 2018 · 838f55e · 838f55e
1 parent 37ac9ae
commit 838f55e
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 30 deletions.
diff --git a/packages/core/src/sanitization/html_sanitizer.ts b/packages/core/src/sanitization/html_sanitizer.ts
@@ -98,16 +98,17 @@ class SanitizingHtmlSerializer {
     // However this code never accesses properties off of `document` before deleting its contents
     // again, so it shouldn't be vulnerable to DOM clobbering.
     let current: Node = el.firstChild !;
+    let elementValid = true;
     while (current) {
       if (current.nodeType === Node.ELEMENT_NODE) {
-        this.startElement(current as Element);
-      } else if (current.nodeType === Node.TEXT_NODE && !isCommentNode(current)) {
+        elementValid = this.startElement(current as Element);
+      } else if (current.nodeType === Node.TEXT_NODE) {
         this.chars(current.nodeValue !);
       } else {
         // Strip non-element, non-text nodes.
         this.sanitizedSomething = true;
       }
-      if (current.firstChild) {
+      if (elementValid && current.firstChild) {
         current = current.firstChild !;
         continue;
       }
@@ -130,11 +131,19 @@ class SanitizingHtmlSerializer {
     return this.buf.join('');
   }
 
-  private startElement(element: Element) {
+  /**
+   * Outputs only valid Elements.
+   *
+   * Invalid elements are skipped.
+   *
+   * @param element element to sanitize
+   * Returns true if the element is valid.
+   */
+  private startElement(element: Element): boolean {
     const tagName = element.nodeName.toLowerCase();
     if (!VALID_ELEMENTS.hasOwnProperty(tagName)) {
       this.sanitizedSomething = true;
-      return;
+      return false;
     }
     this.buf.push('<');
     this.buf.push(tagName);
@@ -154,6 +163,7 @@ class SanitizingHtmlSerializer {
       this.buf.push(' ', attrName, '="', encodeEntities(value), '"');
     }
     this.buf.push('>');
+    return true;
   }
 
   private endElement(current: Element) {
@@ -263,12 +273,3 @@ function getTemplateContent(el: Node): Node|null {
 function isTemplateElement(el: Node): el is HTMLTemplateElement {
   return el.nodeType === Node.ELEMENT_NODE && el.nodeName === 'TEMPLATE';
 }
-
-/**
- * Comment nodes inside unsafe elements are converted to TEXT nodes.
- * Therefore nodeType for these nodes is returned as Node.TEXT_NODE.
- */
-function isCommentNode(el: Node): boolean {
-  const nodeText = el.nodeValue !;
-  return nodeText.startsWith('<!--') && nodeText.endsWith('-->');
-}
diff --git a/packages/core/test/linker/security_integration_spec.ts b/packages/core/test/linker/security_integration_spec.ts
@@ -239,7 +239,7 @@ function declareTests({useJit}: {useJit: boolean}) {
 
         ci.ctxProp = 'ha <script>evil()</script>';
         fixture.detectChanges();
-        expect(getDOM().getInnerHTML(e)).toEqual('ha evil()');
+        expect(getDOM().getInnerHTML(e)).toEqual('ha ');
 
         ci.ctxProp = 'also <img src="x" onerror="evil()"> evil';
         fixture.detectChanges();

diff --git a/packages/core/test/sanitization/html_sanitizer_spec.ts b/packages/core/test/sanitization/html_sanitizer_spec.ts
@@ -36,8 +36,8 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
           .toEqual('<p>Hello <br> World</p>');
     });
 
-    it('supports namespaced elements',
-       () => { expect(_sanitizeHtml(defaultDoc, 'a<my:hr/><my:div>b</my:div>c')).toEqual('abc'); });
+    it('supports removal of namespaced elements',
+       () => { expect(_sanitizeHtml(defaultDoc, 'a<my:hr/><my:div>b</my:div>c')).toEqual('a'); });
 
     it('supports namespaced attributes', () => {
       expect(_sanitizeHtml(defaultDoc, '<a xlink:href="something">t</a>'))
@@ -85,15 +85,37 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
           .toEqual('<p alt="% &amp; &#34; !">Hello</p>');  // NB: quote encoded as ASCII &#34;.
     });
 
-    describe('should strip dangerous elements', () => {
+    describe('should strip dangerous elements and its content', () => {
       const dangerousTags = [
-        'frameset', 'form', 'param', 'object', 'embed', 'textarea', 'input', 'button', 'option',
-        'select', 'script', 'style', 'link', 'base', 'basefont'
+        'form',
+        'object',
+        'textarea',
+        'button',
+        'option',
+        'select',
+        'script',
+        'style',
       ];
 
       for (const tag of dangerousTags) {
         it(`${tag}`,
-           () => { expect(_sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual('evil!'); });
+           () => { expect(_sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual(''); });
+      }
+      const dangerousSelfClosingTags = [
+        'frameset',
+        'embed',
+        'input',
+        'param',
+        'base',
+        'basefont',
+        'param',
+        'link',
+      ];
+
+      for (const tag of dangerousSelfClosingTags) {
+        it(`${tag}`, () => {
+          expect(_sanitizeHtml(defaultDoc, `before<${tag}>After`)).toEqual('beforeAfter');
+        });
       }
 
       it(`swallows frame entirely`, () => {
@@ -111,12 +133,11 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
       }
     });
 
-    it('ignores comments inside non-safe elements', () => {
+    it('ignores content of style elements', () => {
       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('&lt;!-- something--&gt;hi');
+      expect(_sanitizeHtml(defaultDoc, '<style>\<\!-- something--\>hi</style>')).toEqual('');
       expect(logMsgs.join('\n')).toMatch(/sanitizing HTML stripped some content/);
     });
 
@@ -160,12 +181,7 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
        () => {
          expect(_sanitizeHtml(
                     defaultDoc, '<svg><p><style><img src="</style><img src=x onerror=alert(1)//">'))
-             .toEqual(
-                 isDOMParserAvailable() ?
-                     // PlatformBrowser output
-                     '<p>&lt;img src=&#34;<img src="x"></p>' :
-                     // PlatformServer output
-                     '<p><img src="&lt;/style&gt;&lt;img src=x onerror=alert(1)//"></p>');
+             .toEqual('');
        });
 
     if (browserDetection.isWebkit) {