Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jessicajaniuk wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions packages/core/src/util/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
*
Expand Down
45 changes: 18 additions & 27 deletions packages/core/test/acceptance/security_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
11 changes: 2 additions & 9 deletions packages/core/test/util/dom_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});