Sanitize `diff_discussion_html` in `deprecated_notes.js`
What does this MR do and why?
For #386279 (closed).
Adds DOMPurify's sanitize
before injecting a client-side-only comment into the page.
This JavaScript is not used when rendering comments that are present when the page loads, so this basically only prevents a user from XSSing themselves.
Where
The only place I found deprecated_notes.js
being used was init_deprecated_notes.js
.
The only places I found init_deprecated_notes.js
being used were:
- Global snippets (
snippet/snippet_show.js
<=pages/snippets/show/index.js
) - Project snippets (
snippet/snippet_show.js
<=pages/projects/snippets/show/index.js
) - Commit (
pages/projects/commit/show/index.js
)
Notably, the snippets pages do not seem to ever get to the HTML output portion of deprecated_notes.js
, which means they are unaffected by this change.
Screenshots or screen recordings
There is a lot of broken behavior on the pages where deprecated_notes.js
is used, so there are a lot of videos and screenshots here to track across master
and the branch in this MR.
For testing the sanitizer, I pasted in the "Dirty HTML" from DOMPurify's demo page.
master |
master after comment and refresh |
This branch | This branch after comment and refresh | |
---|---|---|---|---|
Global Snippets | master-global-snippet | branch-global-snippet | ||
Project Snippets | master-project-snippet | branch-project-snippet | ||
Commit | master-commit | LoC Comment Comment on commit | branch-commit | LoC Comment Comment on commit |
How to set up and validate locally
Since I don't think snippets actually use the deprecated notes despite importing the code (or at least they're not getting to the HTML output / sanitization part):
- Go to a commit
- Leave a comment on the commit on a line of code.
- While the "overall commit comment" MAY use the same
deprecated_notes.js
code, the error when submitting the comment always prevents the DOM injection from actually happening, so it's never actually sanitized (or used).
- While the "overall commit comment" MAY use the same
- You must submit the comment; previews are loaded from the server and do not use JavaScript DOM injection from this (
deprecated_notes.js
) file.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.