Skip to content

[HtmlSanitizer] Add support for sanitizing unlimited length of HTML document #52166

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2023
Merged

[HtmlSanitizer] Add support for sanitizing unlimited length of HTML document #52166

merged 1 commit into from
Oct 19, 2023

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Oct 19, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT

@carsonbot carsonbot added this to the 6.4 milestone Oct 19, 2023
@lyrixx lyrixx force-pushed the sanitizer-no-limit branch from b85d210 to 738450f Compare October 19, 2023 13:30
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 19, 2023 •
edited
Loading

What about withMaxInputLength(PHP_INT_MAX)? Isn't this risky practice?

@lyrixx
Copy link
Member Author

lyrixx commented Oct 19, 2023

What about withMaxInputLength(PHP_INT_MAX)? Isn't this risky practice?

I did that in my app (still on 6.3 😅) But If feels more natural to disable the feature completely if you don't want it.

tgalopin
tgalopin approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@tgalopin tgalopin left a comment

Choose a reason for hiding this comment

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

Given that there is a default at 20 000, I think anyone choosing to change it probably had a a look at why it exists and therefore understands at least a bit the consequences. I'm fine disabling it completely IMO

@stof
Copy link
Member

stof commented Oct 19, 2023

@tgalopin there is no phpdoc at all in HtmlSanitizerConfig that would explain the reason to have a limit though.

@nicolas-grekas
Copy link
Member

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit 5301b00 into symfony:6.4 Oct 19, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 19, 2023
Closed
@lyrixx lyrixx deleted the sanitizer-no-limit branch October 19, 2023 15:35
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@tgalopin tgalopin tgalopin approved these changes

Assignees
No one assigned
Labels
Feature HtmlSanitizer Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
5 participants
@lyrixx @nicolas-grekas @stof @tgalopin @carsonbot