Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HtmlSanitizer] AddblockBodyElements that will block all known elements by default.#48851
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
Closed
Neirda24 wants to merge1,103 commits intosymfony:6.3fromNeirda24:ticket_48358-html_sanitizer-block_all_elements
Closed
[HtmlSanitizer] AddblockBodyElements that will block all known elements by default.#48851
Neirda24 wants to merge1,103 commits intosymfony:6.3fromNeirda24:ticket_48358-html_sanitizer-block_all_elements
Uh oh!
There was an error while loading.Please reload this page.
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
carsonbot commentedJan 2, 2023
Hey! Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 for features". Cheers! Carsonbot |
Neirda24 commentedJan 2, 2023
| @@ -2186,6 +2186,10 @@ private function addHtmlSanitizerSection(ArrayNodeDefinition $rootNode, callable | |||
| ->info('Allows all static elements and attributes from the W3C Sanitizer API standard.') | |||
| ->defaultFalse() | |||
| ->end() | |||
| ->booleanNode('block_body_elements') | |||
| ->info('Blocks all static body elements and remove attributes.') | |||
| ->defaultFalse() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to open a discussion on whether or not it should be default to true.
| @@ -8,6 +8,7 @@ | |||
| <config xmlns="http://symfony.com/schema/dic/symfony" http-method-override="false"> | |||
| <html-sanitizer> | |||
| <sanitizer name="custom" | |||
| block-body-elements="true" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is failing in the tests because of the xml schema I guess
carsonbot commentedJan 3, 2023
Hey! I think@tgalopin has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
Hi@Neirda24 ! This is an interesting idea. I understand why there is a need and it would be worth finding a solution to it indeed. My two concerns for the moments are:
Otherwise I like the idea, thanks very much! |
@tgalopin : available to iterate on this. Agree that the naming isn't the best one. I find your approach of default_behaviour quite good and would love to have a spin at it with you. As for the default I think it is still experimental, if so isn't there a special rule for experimental components ? If not at least make it the default behaviour on forms ? |
It is indeed experimental, but even with experimental status we try to avoid BC breaks when possible. I don't think it's worth it to introduce a BC break here, especially if we add the configuration in the recipe (meaning new installs would get the block behavior setup) Regarding naming,@nicolas-grekas any opinion? |
BTW@tgalopin : I tried to stick to the naming already in place. To me it is very important that we stay consistent. According to the component itself, what this PR provides is to automatically block all known elements (vs purge or just sanitizing) |
…olicy` in XSD (MatTheCat)This PR was merged into the 5.4 branch.Discussion----------[FrameworkBundle] Rename limiter’s `strategy` to `policy` in XSD| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fixsymfony#49671| License | MIT| Doc PR | N/Asymfony#38664 renamed `strategy` to `policy` but did not update the XSD.Commits-------c19711c [FrameworkBundle] Rename limiter’s `strategy` to `policy` in XSD
…nteger for Profiler (Aliance)This PR was merged into the 5.4 branch.Discussion----------[HttpKernel] Change limit argument from string to integer for Profiler| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fixsymfony#49656| License | MITCommits-------fb9b0d0 Change limit argument from string to integer.
…hiebault)This PR was squashed before being merged into the 6.3 branch.Discussion----------[Form] Improve exception for unsubmitted form| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets |Fixsymfony#49619| License | MIT| Doc PR | -This PR improves the error message when a form is submitted but its submission is not verified.Commits-------51d3038 [Form] Improve exception for unsubmitted form
…equest::create (neclimdul)This PR was merged into the 6.3 branch.Discussion----------[HttpFoundation] Deprecate passing invalid URI to Request::createFixes:symfony#47084Passing an invalid URI to Request::create triggers an undefined code path. In PHP7 the false value returned by parse_url would quietly be treated as a an array through type coercion leading to unexpected results. In PHP8 this triggers a deprecation exposing the bug.| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | yes| New feature? | no| Deprecations? | yes| Tickets |Fixsymfony#47084| License | MITCommits-------bce4c27 [HttpFoundation] Deprecate passing invalid URI to Request::create
…he HTML error page (lyrixx)This PR was squashed before being merged into the 6.3 branch.Discussion----------[ErrorHander] Display exception properties in the HTML error page| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets |Fixsymfony#49613| License | MIT| Doc PR |------<details><summary>Code to reproduce</summary>```phpclass MyException extends \Exception{ public function __construct() { parent ::__construct('some_message', 0, new MyException2()); } public string $myMessage = 'some_message'; public string $myCode = 'some_code'; private string $privateStuff = 'private_stuff';}class MyException2 extends \Exception{ private string $anotherPrivateStuff = 'another_private_stuff';}```</details>Commits-------b041d06 [ErrorHander] Display exception properties in the HTML error page
* 5.4: Fix some Composer keywords [FrameworkBundle] Rename limiter’s `strategy` to `policy` in XSD [VarDumper] Fixed dumping of CutStub Fix test Change limit argument from string to integer. [Messenger] Fix `evaluate()` calls in `WorkerTest` [Mailer] STDOUT blocks infinitely under Windows when STDERR is filled
* 6.2: Fix some Composer keywords [FrameworkBundle] Rename limiter’s `strategy` to `policy` in XSD [VarDumper] Fixed dumping of CutStub Fix test Change limit argument from string to integer. [Messenger] Fix `evaluate()` calls in `WorkerTest` [Mailer] STDOUT blocks infinitely under Windows when STDERR is filled
* 5.4: Fix some Composer keywords
* 6.2: Fix some Composer keywords Fix some Composer keywords Fix some Composer keywords
…ration of "enabled" node
…dStrengthValidator (nicolas-grekas)This PR was merged into the 6.3 branch.Discussion----------[Validator] Improve entropy estimation in PasswordStrengthValidator| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Improves a bit the estimation of the entropy./cc `@Spomky`Commits-------99c09ff [Validator] Improve entropy estimation in PasswordStrengthValidator
…Stark)This PR was merged into the 6.3 branch.Discussion----------[Notifier] [SimpleTextin] Fix license year| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Commits-------cd23623 [Notifier][SimpleTextin] Fix license year
…yguedidi)This PR was merged into the 6.3 branch.Discussion----------Apply align_multiline_comment PHP-CS-Fixer rule| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets || License | MIT| Doc PR |After a discussion with `@stof`, this rule should be part of the ``@Symfony`` PHP-CS-Fixer ruleset.This PR apply the rule on the Symfony code base.seePHP-CS-Fixer/PHP-CS-Fixer#6875 for the PR on PHP-CS-FixerCommits-------6e984f0 Apply align_multiline_comment PHP-CS-Fixer rule
…idi)This PR was merged into the 6.3 branch.Discussion----------Apply operator_linebreak PHP-CS-Fixer rule| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets || License | MIT| Doc PR |After a discussion with `@stof`, this rule should be part of the ``@Symfony`` PHP-CS-Fixer ruleset.This PR apply the rule on the Symfony code base.seePHP-CS-Fixer/PHP-CS-Fixer#6877 for the PR on PHP-CS-FixerCommits-------6edc748 Apply operator_linebreak PHP-CS-Fixer rule
…r rule (yguedidi)This PR was merged into the 6.3 branch.Discussion----------Apply no_null_property_initialization PHP-CS-Fixer rule| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets || License | MIT| Doc PR |After a discussion with `@stof`, this rule should be part of the ``@Symfony`` PHP-CS-Fixer ruleset.This PR apply the rule on the Symfony code base.seePHP-CS-Fixer/PHP-CS-Fixer#6876 for the PR on PHP-CS-FixerCommits-------2320c2a Apply no_null_property_initialization PHP-CS-Fixer rule
… debug toolbar (PhilETaylor)This PR was squashed before being merged into the 6.3 branch.Discussion----------[WebProfilerBundle] Add clickable entry view to debug toolbar| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | yes| Deprecations? | no| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Just a proposed idea.One of the great parts of the tool bar is the Clickable IDE integration to quickly jump to the Controller method that the page is rendered from.example:<img width="409" alt="ScreenShot-2023-03-31-21 01 07" src="https://user-images.githubusercontent.com/400092/229218485-e07d2588-9421-453a-a4cb-93c0d34f13e1.png">One thing I find myself doing often is needing to get to the rendered view (a twig returned by the controller method with `return $this->render('template.twig.html,[]);` etc. To do that I need to Hover the toolbar left corner, move up and click the controller, wait for phpStorm to load the controller file, then scroll down to find the rendered view, then go and find it manually in the tree (or with the [phpStorm Symfony plugin (amazing plugin)](https://plugins.jetbrains.com/plugin/7219-symfony-support) use the gutter icon to select the view).After this PR the first template used in the stack will be rendered into to the twig debug toolbar (much like the controller in the request toolbar) and will be clickable if the [IDE integration is enabled](https://symfony.com/doc/current/reference/configuration/framework.html#ide) meaning less clicks to get from the webpage to the twig template to make changes in HTML quickly.Just a thought, would make less clicks...thoughts?<img width="551" alt="ScreenShot-2023-03-31-20 46 24" src="https://user-images.githubusercontent.com/400092/229217786-e516c8a5-817d-4767-8d85-60521265b1a0.png">Commits-------1c6152e [WebProfilerBundle] Add clickable entry view to debug toolbar
…rsion 6.3 (nicolas-grekas)This PR was merged into the 6.3 branch.Discussion----------[Console] Add Les-Tilleuls.coop as a backer of version 6.3| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Thanks to them! 🙏/cc `@chalasr` `@dunglas` *et al.*Commits-------caa6fe1 [Console] Add Les-Tilleuls.coop as a backer of version 6.3
…n 6.3 (nicolas-grekas)This PR was merged into the 6.3 branch.Discussion----------[Security] Add SymfonyCasts as a backer of version 6.3| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Thanks them! 🙏/cc `@weaverryan` *et al.*Commits-------31f65d4 [Security] Add SymfonyCasts as a backer of version 6.3
…ub.com:Neirda24/symfony into ticket_48358-html_sanitizer-block_all_elements
Sorry I'm not used to rebases and I think it went wrong somewhere 😓 |
Moved to#49920 |
Sign up for freeto 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.
Uh oh!
There was an error while loading.Please reload this page.
Add a way to block all body elements. Currently without any setup, the
purgemode is the default.Without the framework :
With the framework :