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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HtmlSanitizer] Some minor changes in the config API #44814

Merged
merged 1 commit into from Jan 7, 2022

Conversation

javiereguiluz
Copy link
Member

Q A
Branch? 6.1
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

First of all, thanks to @tgalopin for this superb contribution 馃檱

This PR makes 3 little changes:

(1) Fix two minor typos

(2) Rename allowAllStaticElements() as allowStaticElements() to be consistent with the rest of methods, which don't include the All word.

(3) A proposal to change this default value:

-public function allowElement(string $element, array|string $allowedAttributes = []): static
+public function allowElement(string $element, array|string $allowedAttributes = '*'): static

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:

->allowElement('div')

Forcing to write it as ->allowElement('div', '*') seems cumbersome. The previous behavior (forbid all attributes) would now be like this:

->allowElement('div', [])

@tgalopin
Copy link
Member

tgalopin commented Dec 28, 2021

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.

@javiereguiluz
Copy link
Member Author

I've reverted all the changes related to (3). Thanks.

@tgalopin
Copy link
Member

tgalopin commented Jan 5, 2022

Could you update the README as well with the new method name?

@javiereguiluz
Copy link
Member Author

@tgalopin done! Sorry I forgot about this!

@fabpot
Copy link
Member

fabpot commented Jan 7, 2022

Thank you @javiereguiluz.

@fabpot fabpot merged commit 098ff62 into symfony:6.1 Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants