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

fix: code scanning alert no. 1226: Incomplete multi-character sanitization #31358

Closed
wants to merge 2 commits into from

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Dec 9, 2024

Fixes https://github.com/apache/superset/security/code-scanning/1226

To fix the problem, we should ensure that all HTML tags are removed from the input string, even if they are nested or malformed. One way to achieve this is to repeatedly apply the regular expression replacement until no more replacements can be performed. This approach ensures that all instances of HTML tags are removed.

We will modify the removeHTMLTags function to apply the regular expression replacement in a loop until the input string no longer changes. This ensures that all HTML tags are removed, regardless of their structure.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@rusackas rusackas changed the title Fix code scanning alert no. 1226: Incomplete multi-character sanitization fix: code scanning alert no. 1226: Incomplete multi-character sanitization Dec 9, 2024
@rusackas rusackas marked this pull request as ready for review December 9, 2024 21:01
let newstr = str;
do {
previous = newstr;
newstr = newstr.replace(/<[^>]*>/g, '');

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on
library input
may run slow on strings starting with '<' and with many repetitions of '<'.
Copy link
Member Author

Choose a reason for hiding this comment

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

@dpgaspar this PR was opened by github as a fix for a CodeQL issue, and the solution it gives is another CodeQL issue. Curious if you have a take on whether we're better or worse off with this change :P

Copy link
Member Author

Choose a reason for hiding this comment

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

@dpgaspar this PR was opened by github as a fix for a CodeQL issue, and the solution it gives is another CodeQL issue. Curious if you have a take on whether we're better or worse off with this change :P

@sadpandajoe sadpandajoe requested a review from dpgaspar December 10, 2024 18:19
@rusackas rusackas closed this Jan 11, 2025
@rusackas rusackas deleted the alert-autofix-1226 branch January 11, 2025 01:16
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.

1 participant