html sanitizer fix to remove comment nodes that are child nodes of unsafe elements #25879

shinok01 · 2018-09-09T06:31:40Z

PR Checklist

Please check if your PR fulfills the following requirements:

[X ] The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
[X ] Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Input HTML string to be sanitized -

<style><!-- something --></style>

Output returned is encoded comment string like

Issue Number: N/A

What is the new behavior?

Each text node is checked if its opening and closing symbols match the format of an HTML comment.
If they match, then the node is ignored.

Does this PR introduce a breaking change?

[ ] Yes
[X ] No

Other information

googlebot · 2018-09-09T06:31:43Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

shinok01 · 2018-09-09T06:34:30Z

Covered by Google Corporate CLA ( shinok@)

packages/core/test/sanitization/html_sanitizer_spec.ts

mhevery · 2018-09-10T17:10:53Z

packages/core/src/sanitization/html_sanitizer.ts

+ * 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 {


This seems like a very error-prone way to check for comments. It also seems like a fix in a wrong place since it seems to be fixing the symptom rather than the core issue.

benlesh · 2018-09-19T16:15:35Z

Related issue: #26007

IgorMinar

lgtm with @mhevery's fixes

googlebot · 2018-10-23T03:37:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

Comment nodes that are child nodes of unsafe elements are identified as text nodes. This results in the comment node being returned as an encoded string. Add a check to ignore such comment nodes.

googlebot · 2018-10-23T03:46:26Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-10-23T03:46:59Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ngbot · 2018-10-23T04:26:46Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "continuous-integration/travis-ci/pr" is failing
     missing required status "ci/circleci: build"
     missing required status "ci/circleci: lint"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

mhevery · 2018-10-23T04:35:41Z

Global: http://test/OCL:218284351:BASE:218358502:1540313868926:e7b80dcb

Many g3 failures.

alxhub · 2018-10-23T15:32:53Z

This fails g3 presubmit (something called the safevalues_compatibility_test). Kicking back to @shinok01 - please prepare an internal CL with the required updates to g3.

Comment nodes that are child nodes of unsafe elements are identified as text nodes. This results in the comment node being returned as an encoded string. Add a check to ignore such comment nodes. PR Close angular#25879

In the past, the sanitizer would remove unsafe elements, but still traverse and sanitize (and potentially preserve) their content. This was problematic in the case of `<style></style>` tags, whose content would be converted to HTML text nodes. In order to fix this, the sanitizer's behavior was changed in angular#25879 to ignore the content of _all_ unsafe elements. While this fixed the problem with `<style></style>` tags, it unnecessarily removed the contents for _any_ unsafe element. This was an unneeded breaking change. This commit partially restores the old sanitizer behavior (namely traversing content of unsafe elements), but introduces a list of elements whose content should not be traversed if the elements themselves are considered unsafe. Currently, this list contains `style` and `script`. Related to angular#25879 and angular#26007. Fixes angular#28427

In the past, the sanitizer would remove unsafe elements, but still traverse and sanitize (and potentially preserve) their content. This was problematic in the case of `<style></style>` tags, whose content would be converted to HTML text nodes. In order to fix this, the sanitizer's behavior was changed in angular#25879 to ignore the content of _all_ unsafe elements. While this fixed the problem with `<style></style>` tags, it unnecessarily removed the contents for _any_ unsafe element. This was an unneeded breaking change. This commit partially restores the old sanitizer behavior (namely traversing content of unsafe elements), but introduces a list of elements whose content should not be traversed if the elements themselves are considered unsafe. Currently, this list contains `style`, `script` and `template`. Related to angular#25879 and angular#26007. Fixes angular#28427

In the past, the sanitizer would remove unsafe elements, but still traverse and sanitize (and potentially preserve) their content. This was problematic in the case of `<style></style>` tags, whose content would be converted to HTML text nodes. In order to fix this, the sanitizer's behavior was changed in #25879 to ignore the content of _all_ unsafe elements. While this fixed the problem with `<style></style>` tags, it unnecessarily removed the contents for _any_ unsafe element. This was an unneeded breaking change. This commit partially restores the old sanitizer behavior (namely traversing content of unsafe elements), but introduces a list of elements whose content should not be traversed if the elements themselves are considered unsafe. Currently, this list contains `style`, `script` and `template`. Related to #25879 and #26007. Fixes #28427 PR Close #28804

angular-automatic-lock-bot · 2019-09-14T05:52:45Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: no label Sep 9, 2018

mihalskiy approved these changes Sep 10, 2018

View reviewed changes

mhevery suggested changes Sep 10, 2018

View reviewed changes

benlesh added the hotlist: google label Sep 18, 2018

IgorMinar approved these changes Oct 23, 2018

View reviewed changes

mhevery approved these changes Oct 23, 2018

View reviewed changes

mhevery added cla: yes and removed cla: no labels Oct 23, 2018

mhevery added the target: patch This PR is targeted for the next patch release label Oct 23, 2018

IgorMinar added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Oct 23, 2018

fix(core): ignore comment nodes under unsafe elements

41cd44b

Comment nodes that are child nodes of unsafe elements are identified as text nodes. This results in the comment node being returned as an encoded string. Add a check to ignore such comment nodes.

mhevery force-pushed the shinok-html-sanitizer-fix branch from 838f55e to 41cd44b Compare October 23, 2018 03:46

googlebot added cla: no and removed cla: yes labels Oct 23, 2018

mhevery added cla: yes and removed cla: no labels Oct 23, 2018

mhevery added the action: merge The PR is ready for merge by the caretaker label Oct 23, 2018

alxhub assigned shinok01 Oct 23, 2018

alxhub added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 23, 2018

mhevery closed this in d5cbcef Oct 25, 2018

mhevery mentioned this pull request Dec 20, 2018

Child nodes of unsafe HTML nodes returned in sanitized output - displayed as raw HTML tags #26007

Closed

gkalpak mentioned this pull request Jan 29, 2019

Unexpected sanitizer breaking change in 7.1.0-beta.1 #28427

Closed

gkalpak mentioned this pull request Feb 18, 2019

fix(core): traverse and sanitize content of unsafe elements #28804

Closed

6 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

html sanitizer fix to remove comment nodes that are child nodes of unsafe elements #25879

html sanitizer fix to remove comment nodes that are child nodes of unsafe elements #25879

shinok01 commented Sep 9, 2018 •

edited by IgorMinar

googlebot commented Sep 9, 2018

shinok01 commented Sep 9, 2018

mhevery Sep 10, 2018

benlesh commented Sep 19, 2018

IgorMinar left a comment

googlebot commented Oct 23, 2018

googlebot commented Oct 23, 2018

googlebot commented Oct 23, 2018

ngbot bot commented Oct 23, 2018

mhevery commented Oct 23, 2018 •

edited

alxhub commented Oct 23, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

html sanitizer fix to remove comment nodes that are child nodes of unsafe elements #25879

html sanitizer fix to remove comment nodes that are child nodes of unsafe elements #25879

Conversation

shinok01 commented Sep 9, 2018 • edited by IgorMinar

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Sep 9, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

shinok01 commented Sep 9, 2018

mhevery Sep 10, 2018

Choose a reason for hiding this comment

benlesh commented Sep 19, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

googlebot commented Oct 23, 2018

googlebot commented Oct 23, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Oct 23, 2018

ngbot bot commented Oct 23, 2018

mhevery commented Oct 23, 2018 • edited

alxhub commented Oct 23, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

shinok01 commented Sep 9, 2018 •

edited by IgorMinar

mhevery commented Oct 23, 2018 •

edited