fix: Strip HTML comments from security advisory HTML before sanitization#17037
Open
stevejalim wants to merge 1 commit intomainfrom
Open
fix: Strip HTML comments from security advisory HTML before sanitization#17037stevejalim wants to merge 1 commit intomainfrom
stevejalim wants to merge 1 commit intomainfrom
Conversation
Some older advisories (e.g. mfsa2009-33) contain HTML comments with placeholder content commented out, generally CVE references. However, the the justhtml sanitizer escapes these into visible `<!-- … -->` text on the rendered page, making them visible. The fix is a new strip_html_comments() function that removes comments before sanitization runs, preventing them from being escaped into visible page content.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17037 +/- ##
=======================================
Coverage 81.49% 81.50%
=======================================
Files 171 171
Lines 9203 9206 +3
=======================================
+ Hits 7500 7503 +3
Misses 1703 1703 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a rendering issue in older security advisories where HTML comments (<!-- ... -->) were being escaped by the justhtml sanitizer and shown as visible text on the page. It introduces a preprocessing step to remove HTML comments before sanitization, and adds targeted tests to prevent regressions.
Changes:
- Add
strip_html_comments()helper to remove HTML comments from advisory HTML prior to sanitization. - Update
sanitize_advisory_html()to callstrip_html_comments()before running the allowlist sanitizer. - Add unit tests covering comment stripping (single-line, multiline, multiple, empty) and an integration-style sanitization test for comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
bedrock/security/management/commands/update_security_advisories.py |
Adds HTML-comment stripping and wires it into advisory HTML sanitization. |
bedrock/security/tests/test_commands.py |
Adds coverage for comment stripping and ensures sanitized output doesn’t leak escaped comment text. |
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
Sign up for free
to 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.
Some older advisories (e.g. mfsa2009-33) contain HTML comments with placeholder content commented out, generally CVE references.
However, the the justhtml sanitizer escapes these into visible
<!-- … -->text on the rendered page, making them visible.The fix is a new strip_html_comments() function that removes comments before sanitization runs, preventing them from being escaped into visible page content.
If this changeset needs to go into the FXC codebase, please add the
WMO and FXClabel.Issue / Bugzilla link
https://mozilla-hub.atlassian.net/browse/WT-668
Testing
make preflightto get current data<and>have been escaped, so don't mark the start of an ignorable comment.python manage.py update_security_advisoriesto re-import the data