-
Notifications
You must be signed in to change notification settings - Fork 401
Ensure <li>s are wrapped in a <ol> or <ul> after sanitizing HTML #13544
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
Conversation
This matters when displaying the versions page, where each version is in a <li> already, and developers can have release notes containing <li> of their own.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13544 +/- ##
=======================================
Coverage 98.28% 98.29%
=======================================
Files 267 268 +1
Lines 10630 10647 +17
Branches 3247 3258 +11
=======================================
+ Hits 10448 10465 +17
Misses 169 169
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
}); | ||
}); | ||
|
||
it('doesnt wrap `<li>` inside a `<ul>` if not necessary with menu', () => { |
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.
Can we have tests for multiple <li>
and when the closing </li>
is missing?
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.
f196bb9 ; I added handling of multiple <li>
close together while I was at it
Couldn't we fix the DB instead then? That kind of hook applied on everything we sanitize feels risky to me. |
We could fix it for current content and hope it doesn't come back but we don't have a strong guarantee. My reasoning was, why wouldn't we want that applied to everything we sanitize ? This is invalid HTML, you have to have a |
I spent a bit more time investigating what a migration would look like and I still think this is the safer approach. I'm checking whether |
Fixes mozilla/addons#14989
Context
This matters when displaying the versions page, where each version is in a
<li>
already, and developers can have release notes containing<li>
of their own. This can be seen:(Look at the output carefully, the HTML from the footer is duplicated, the "clone" appearing either in the sidebar or on top of the existing footer)
Testing
You have to manually craft HTML with
<li>
and no parent<ul>
and get it passed to our sanitize functions to test this. That is harder to achieve now that we have markdown, so best to modify the entry in the database, or pick one that was already broken, or test with https://localhost:3000/en-US/firefox/addon/ratopuse-weasp-vulturkal/versions/ with the patch applied and unapplied to see the difference. Or look at the unit tests.