Skip to content

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 master-global-snippet-refresh branch-global-snippet branch-global-snippet-refresh
Project Snippets master-project-snippet master-project-snippet-refresh branch-project-snippet branch-project-snippet-refresh
Commit master-commit LoC Comment master-commit-line-refresh Comment on commit master-commit-overall-refresh branch-commit LoC Comment branch-commit-line-refresh Comment on commit branch-commit-overall-refresh

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):

  1. Go to a commit
  2. 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).
  3. 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.

Edited by Thomas Randolph

Merge request reports