diff --git a/packages/core/src/util/dom.ts b/packages/core/src/util/dom.ts index 805daa5a92..c8f9845b12 100644 --- a/packages/core/src/util/dom.ts +++ b/packages/core/src/util/dom.ts @@ -6,15 +6,18 @@ * found in the LICENSE file at https://angular.io/license */ -const END_COMMENT = /-->/g; -const END_COMMENT_ESCAPED = '-\u200B-\u200B>'; +const END_COMMENT = /(<|>)/g; +const END_COMMENT_ESCAPED = '\u200B$1\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. - * `". -->`. Above the `"-->"` is meant to be text not - * an end to the comment. This can be created programmatically through DOM APIs. + * Consider: `" 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. (`` 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 index 0376dcc1cf..afc8b9ccd4 100644 --- a/packages/core/test/acceptance/security_spec.ts +++ b/packages/core/test/acceptance/security_spec.ts @@ -11,24 +11,33 @@ import {TestBed} from '@angular/core/testing'; describe('comment node text escaping', () => { - it('should not be possible to do XSS through comment reflect data', () => { - @Component({template: `
`}) - class XSSComp { - xssValue: string = '--> -->'; - } + // 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: `
`}) + class XSSComp { + // ngIf serializes the `xssValue` into a comment for debugging purposes. + xssValue: string = xssValue + ''; + } - 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('-->` - 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('-->` + const script = div.querySelector('script'); + expect(script).toBeFalsy(); + }); }); }); \ No newline at end of file diff --git a/packages/core/test/util/dom_spec.ts b/packages/core/test/util/dom_spec.ts index 8d552ccf2e..dc28711c56 100644 --- a/packages/core/test/util/dom_spec.ts +++ b/packages/core/test/util/dom_spec.ts @@ -14,13 +14,20 @@ 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-\u200b>after'); + expect(escapeCommentText('before-->after')).toEqual('before--\u200b>\u200bafter'); }); it('should escape multiple markers', () => { expect(escapeCommentText('before-->inline-->after')) - .toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after'); + .toEqual('before--\u200b>\u200binline--\u200b>\u200bafter'); }); }); });