Skip to content

Commit

Permalink
fix(core): fix possible XSS attack in development through SSR
Browse files Browse the repository at this point in the history
This is a follow up fix for
894286d.

It turns out that comments can be closed in several ways:
- `<!-->`
- `<!-- -->`
- `<!-- --!>`

All of the above are valid ways to close comment per:
https://html.spec.whatwg.org/multipage/syntax.html#comments

The new fix surrounds `<` and `>` with zero width space so that it
renders in the same way, but it prevents the comment to be closed eagerly.
  • Loading branch information
mhevery committed Jan 22, 2021
1 parent dc06873 commit e4d6420
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 6 deletions.
13 changes: 8 additions & 5 deletions packages/core/src/util/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
* `<!-- 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.
* Consider: `<!-- The way you close a comment is with ">", and "->" at the begging 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
*
* ```
* div.innerHTML = div.innerHTML
Expand All @@ -26,7 +29,7 @@ const END_COMMENT_ESCAPED = '-\u200B-\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
2 changes: 1 addition & 1 deletion packages/core/test/acceptance/security_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('comment node text escaping', () => {
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>';
xssValue: string = '>--> --!><!-- --><script>"evil"</script>';
}

TestBed.configureTestingModule({declarations: [XSSComp]});
Expand Down

0 comments on commit e4d6420

Please sign in to comment.