NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:6.1fromjaviereguiluz:tweak_html_sanitizer

Jan 7, 2022

Merged

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

fabpot merged 1 commit intosymfony:6.1fromjaviereguiluz:tweak_html_sanitizer

Jan 7, 2022

Conversation

Copy link

Member

javiereguiluz commentedDec 28, 2021

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) RenameallowAllStaticElements() asallowStaticElements() to be consistent with the rest of methods, which don't include theAll 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', [])

javiereguiluz added this to the6.1 milestone

Dec 28, 2021

carsonbot added the Status: Needs Review label

Dec 28, 2021

fancyweb added the HtmlSanitizer label

Dec 28, 2021

Copy link

Contributor

tgalopin commentedDec 28, 2021•
edited by nicolas-grekas
Loading

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.