Skip to content
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.

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

Do not ignore attributes allowed globally together with 'style' (#237) #238

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

corebonts
Copy link

Also, allowStyling() internally allows the 'style' attribute, so it is not necessary to ignore it.

Gabor Garancsi added 2 commits October 27, 2021 18:09
…P#237)

Also, allowStyling() internally allows the 'style' attribute, so it is not
necessary to ignore it.
@mikesamuel
Copy link
Contributor

Thanks for adding a testcase. The check for style as the zero-th element seems good to change, but what prompted this?

Instead of using null for the policy, and checking == null, can we check for the identity policy? My vague recollection was that .join was pretty good about just returning x when joining x with the identity policy.

@corebonts
Copy link
Author

First, thanks for reviewing it.
But sorry, it's not clear, what prompted what? The change that calls allowStyling() when "style" property is allowed or the change that uses now the contains check instead of the zero-th element check?

For the first, I don't know, it's someone else's change and I don't know the reason behind it. For me it also feels a bit magical.
For the latter one, we had failing tests in our product when we updated to the latest sanitizer.

And for your comment about the nullcheck, you're right, I will change that.

this.policy = attrPolicy;
} else {
this.policy = AttributePolicy.Util.join(this.policy, attrPolicy);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants