Revert "fix(core): fix possible XSS attack in development through SSR (#40525)" (#40533)

thePunderWoman · thePunderWoman · commit c64a56fbccbf · 2021-01-22T16:44:34.000-08:00
This reverts commit bb3b315. Reason for Revert: Issues with Google3 TAP Failures PR Close #40533
diff --git a/packages/core/src/util/dom.ts b/packages/core/src/util/dom.ts
@@ -6,18 +6,15 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-const END_COMMENT = /(<|>)/g;
-const END_COMMENT_ESCAPED = '\u200B$1\u200B';
+const END_COMMENT = /-->/g;
+const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
 
 /**
  * Escape the content of the strings so that it can be safely inserted into a comment node.
  *
  * The issue is that HTML does not specify any way to escape comment end text inside the comment.
- * Consider: `<!-- The way you close a comment is with ">", and "->" at the beginning or by "-->" or
- * "--!>" at the end. -->`. Above the `"-->"` is meant to be text not an end to the comment. This
- * can be created programmatically through DOM APIs. (`<!--` are also disallowed.)
- *
- * see: https://html.spec.whatwg.org/multipage/syntax.html#comments
+ * `<!-- The way you close a comment is with "-->". -->`. Above the `"-->"` is meant to be text not
+ * an end to the comment. This can be created programmatically through DOM APIs.
  *
  * ```
  * div.innerHTML = div.innerHTML
@@ -29,7 +26,7 @@ const END_COMMENT_ESCAPED = '\u200B$1\u200B';
  * may contain such text and expect them to be safe.)
  *
  * This function escapes the comment text by looking for the closing char sequence `-->` and replace
- * it with `--_>_` where the `_` is a zero width space `\u200B`. The result is that if a comment
+ * it with `-_-_>` where the `_` is a zero width space `\u200B`. The result is that if a comment
  * contains `-->` text it will render normally but it will not cause the HTML parser to close the
  * comment.
  *
diff --git a/packages/core/test/acceptance/security_spec.ts b/packages/core/test/acceptance/security_spec.ts
@@ -11,33 +11,24 @@ import {TestBed} from '@angular/core/testing';
 
 
 describe('comment node text escaping', () => {
-  // see: https://html.spec.whatwg.org/multipage/syntax.html#comments
-  ['>',         // self closing
-   '-->',       // standard closing
-   '--!>',      // alternate closing
-   '<!-- -->',  // embedded comment.
-  ].forEach((xssValue) => {
-    it('should not be possible to do XSS through comment reflect data when writing: ' + xssValue,
-       () => {
-         @Component({template: `<div><span *ngIf="xssValue"></span><div>`})
-         class XSSComp {
-           // ngIf serializes the `xssValue` into a comment for debugging purposes.
-           xssValue: string = xssValue + '<script>"evil"</script>';
-         }
+  it('should not be possible to do XSS through comment reflect data', () => {
+    @Component({template: `<div><span *ngIf="xssValue"></span><div>`})
+    class XSSComp {
+      xssValue: string = '--> --><script>"evil"</script>';
+    }
 
-         TestBed.configureTestingModule({declarations: [XSSComp]});
-         const fixture = TestBed.createComponent(XSSComp);
-         fixture.detectChanges();
-         const div = fixture.nativeElement.querySelector('div') as HTMLElement;
-         // Serialize into a string to mimic SSR serialization.
-         const html = div.innerHTML;
-         // This must be escaped or we have XSS.
-         expect(html).not.toContain('--><script');
-         // Now parse it back into DOM (from string)
-         div.innerHTML = html;
-         // Verify that we did not accidentally deserialize the `<script>`
-         const script = div.querySelector('script');
-         expect(script).toBeFalsy();
-       });
+    TestBed.configureTestingModule({declarations: [XSSComp]});
+    const fixture = TestBed.createComponent(XSSComp);
+    fixture.detectChanges();
+    const div = fixture.nativeElement.querySelector('div') as HTMLElement;
+    // Serialize into a string to mimic SSR serialization.
+    const html = div.innerHTML;
+    // This must be escaped or we have XSS.
+    expect(html).not.toContain('--><script');
+    // Now parse it back into DOM (from string)
+    div.innerHTML = html;
+    // Verify that we did not accidentally deserialize the `<script>`
+    const script = div.querySelector('script');
+    expect(script).toBeFalsy();
   });
 });
diff --git a/packages/core/test/util/dom_spec.ts b/packages/core/test/util/dom_spec.ts
@@ -14,20 +14,13 @@ describe('comment node text escaping', () => {
       expect(escapeCommentText('text')).toEqual('text');
     });
 
-    it('should escape "<" or ">"', () => {
-      expect(escapeCommentText('<')).toEqual('\u200b<\u200b');
-      expect(escapeCommentText('<<')).toEqual('\u200b<\u200b\u200b<\u200b');
-      expect(escapeCommentText('>')).toEqual('\u200b>\u200b');
-      expect(escapeCommentText('>>')).toEqual('\u200b>\u200b\u200b>\u200b');
-    });
-
     it('should escape end marker', () => {
-      expect(escapeCommentText('before-->after')).toEqual('before--\u200b>\u200bafter');
+      expect(escapeCommentText('before-->after')).toEqual('before-\u200b-\u200b>after');
     });
 
     it('should escape multiple markers', () => {
       expect(escapeCommentText('before-->inline-->after'))
-          .toEqual('before--\u200b>\u200binline--\u200b>\u200bafter');
+          .toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after');
     });
   });
 });
