fix(core): traverse and sanitize content of unsafe elements #28804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gkalpak
added
type: bug/fix
action: review
|
I marked this as |
gkalpak
force-pushed
the
fix-core-sanitizer-regression
branch
from
d52e3fd
to
37b6644
Compare
JoostK
reviewed
Feb 18, 2019
| // Typically, `<invalid>Some content</invalid>` would traverse (and in this case preserve) | ||
| // `Some content`, but strip `invalid-element` opening/closing tags. For some elements, though, we | ||
| // don't want to preserve the content, if the elements themselves are going to be removed. | ||
| const SKIP_TRAVERSING_CONTENT_IF_INVALID_ELEMENTS = tagSet('script,style'); |
There was a problem hiding this comment.
Should template be included here?
There was a problem hiding this comment.
🤷♂️
Probably, I guess 😁 Added.
gkalpak
force-pushed
the
fix-core-sanitizer-regression
branch
from
37b6644
to
d787173
Compare
alfaproject
reviewed
Feb 18, 2019
| // Typically, `<invalid>Some content</invalid>` would traverse (and in this case preserve) | ||
| // `Some content`, but strip `invalid-element` opening/closing tags. For some elements, though, we | ||
| // don't want to preserve the content, if the elements themselves are going to be removed. | ||
| const SKIP_TRAVERSING_CONTENT_IF_INVALID_ELEMENTS = tagSet('script,style,template'); |
There was a problem hiding this comment.
I know you didn't do tagSet, but why not pass an array of elements directly instead of splitting. Surely it would reduce overhead and maybe even help with minification
There was a problem hiding this comment.
I was trying to remain consistent with the rest of the file 🤔
gkalpak
force-pushed
the
fix-core-sanitizer-regression
branch
2 times, most recently
from
c3635bf
to
b66a95e
Compare
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
gkalpak
force-pushed
the
fix-core-sanitizer-regression
branch
from
b66a95e
to
9068876
Compare
mhevery
approved these changes
Feb 23, 2019
mhevery
added
action: merge
benlesh
pushed a commit
that referenced
this pull request
Feb 26, 2019
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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Docs have been added / updated (for bug fixes / features)PR Type
What is the current behavior?
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.Issue Number: #28427
What is the new behavior?
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
styleandscript.Does this PR introduce a breaking change?
...this PR does introduce a change in behavior, but only to fix a previous change in behavior that was a regression.
Other information
Related to #25879 and #26007.
Fixes #28427.