[HtmlSanitizer] Some minor changes in the config API #44814
Merged
Conversation
This file contains hidden or 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
|
Thanks for 1! I'm fine with 2, I included the "all" to clarify the fact that all elements would be added, meaning possibly CSS injection issues, clickjacking issues, ... If we think the new name is enough, I'm good with it, it's good to have shorter names. For 3, I have more reserves. I do think the sanitizer shouldn't have behaviors where the default includes everything. Your proposal would for instance allow style attributes by default, which probably shouldn't happen. I think I'm -1 on this 3rd point. |
|
I've reverted all the changes related to (3). Thanks. |
fancyweb
approved these changes
Jan 4, 2022
|
Could you update the README as well with the new method name? |
|
@tgalopin done! Sorry I forgot about this! |
fabpot
approved these changes
Jan 7, 2022
f2cb841 to
84470ef
Compare
|
Thank you @javiereguiluz. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
First of all, thanks to @tgalopin for this superb contribution 馃檱
This PR makes 3 little changes:
(1) Fix two minor typos
(2) Rename
allowAllStaticElements()asallowStaticElements()to be consistent with the rest of methods, which don't include theAllword.(3) A proposal to change this default value:
In my opinion, when you want to allow some element, most of the times you want to allow the standard attributes on them too. So, the following should allow
<div>and their standard attributes:Forcing to write it as
->allowElement('div', '*')seems cumbersome. The previous behavior (forbid all attributes) would now be like this: