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

[FrameworkBundle] Allow default action in configuration #57653

Open
wants to merge 6 commits into
base: 7.2
Choose a base branch
from

Conversation

Neirda24
Copy link
Contributor

@Neirda24 Neirda24 commented Jul 4, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Follow-up of #57399 + Fix #48358
License MIT

See symfony/symfony-docs#20019 for documentation.

@carsonbot carsonbot added this to the 7.2 milestone Jul 4, 2024
@carsonbot carsonbot changed the title feat(HtmlSanitizer::Config): allow default action from semantic confi… [FrameworkBundle] feat(HtmlSanitizer::Config): allow default action from semantic confi… Jul 4, 2024
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

We need to check if the HtmlSanitizer component is present in 7.2+ or issue a meaningful error otherwise.

@OskarStark OskarStark changed the title [FrameworkBundle] feat(HtmlSanitizer::Config): allow default action from semantic confi… [FrameworkBundle] feat(HtmlSanitizer::Config): allow default action from semantic configuration Jul 4, 2024
@Neirda24
Copy link
Contributor Author

Neirda24 commented Jul 4, 2024

@xabbuh : done. However I don't know how to update the test in FrameworkExtensionTest to try with sanitizer <7.2 & >=7.2.

@Neirda24
Copy link
Contributor Author

Neirda24 commented Jul 5, 2024

I don't know where and how to fix those errors (related to xml structure). If anyone can point me in the right direction 🙏

@xabbuh
Copy link
Member

xabbuh commented Jul 6, 2024

@Neirda24 In the symfony-1.0.xsd file (located in src/Symfony/Bundle/FrameworkBundle/Resources/config/schema) add <xsd:attribute name="default-action" type="xsd:string" /> as a new entry after line 920.

@Neirda24 Neirda24 changed the title [FrameworkBundle] feat(HtmlSanitizer::Config): allow default action from semantic configuration [FrameworkBundle][HtmlSanitizer] Allow default action in configuration Jul 9, 2024
@Neirda24 Neirda24 requested a review from xabbuh July 9, 2024 18:46
@carsonbot carsonbot changed the title [FrameworkBundle][HtmlSanitizer] Allow default action in configuration [FrameworkBundle] Allow default action in configuration Jul 9, 2024
@@ -2382,6 +2383,10 @@ private function addHtmlSanitizerSection(ArrayNodeDefinition $rootNode, callable
->fixXmlConfig('with_attribute_sanitizer')
->fixXmlConfig('without_attribute_sanitizer')
->children()
->enumNode('default_action')
->info('Defines how the sanitizer must behave by default.')
->values(array_map(static fn (HtmlSanitizerAction $action): string => $action->value, HtmlSanitizerAction::cases()))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->values(array_map(static fn (HtmlSanitizerAction $action): string => $action->value, HtmlSanitizerAction::cases()))
->values(array_column(HtmlSanitizerAction::cases(), 'value'))

Copy link
Member

@nicolas-grekas nicolas-grekas Jul 10, 2024

Choose a reason for hiding this comment

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

but this won't work if the component is not installed, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes good point. Just came here to say #57686 might allow this to be cleaner, but indeed if the enum class is missing what do we do?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

we might not need to make the enum backed in the end, since we cannot use it to generate the XSD nor the config

@@ -3006,6 +3007,17 @@ private function registerHtmlSanitizerConfiguration(array $config, ContainerBuil
$def = $container->register($configId, HtmlSanitizerConfig::class);

// Base
if ($sanitizerConfig['default_action'] ?? false) {
if (!class_exists(HtmlSanitizerAction::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

this is dead code because of the way the config is done at the moment: if there is a value, it must be one of the allowed cases, which are derived from the enum

@@ -918,6 +918,7 @@
<xsd:attribute name="allow-relative-links" type="xsd:boolean" />
<xsd:attribute name="allow-relative-medias" type="xsd:boolean" />
<xsd:attribute name="max-input-length" type="xsd:positiveInteger" />
<xsd:attribute name="default-action" type="xsd:string" />
Copy link
Member

Choose a reason for hiding this comment

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

can't we restrict the possible values to the ones allowed by the enum?

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.

[HtmlSanitizer] Add a blockAll helper
5 participants