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

Loading