From f5886e1145f023903f01da8efa22bde0de2404a7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 26 Jan 2023 21:25:01 -0700 Subject: [PATCH 1/3] Sanitize deprecated note HTML before inserting into DOM --- app/assets/javascripts/deprecated_notes.js | 23 ++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/deprecated_notes.js b/app/assets/javascripts/deprecated_notes.js index 8019a10a042ac7..3438cb61bc574e 100644 --- a/app/assets/javascripts/deprecated_notes.js +++ b/app/assets/javascripts/deprecated_notes.js @@ -17,6 +17,7 @@ import { escape, uniqueId } from 'lodash'; import Vue from 'vue'; import { renderGFM } from '~/behaviors/markdown/render_gfm'; import { createAlert, VARIANT_INFO } from '~/flash'; +import { sanitize } from '~/lib/dompurify'; import '~/lib/utils/jquery_at_who'; import AjaxCache from '~/lib/utils/ajax_cache'; import { loadingIconForLegacyJS } from '~/loading_icon_for_legacy_js'; @@ -517,8 +518,26 @@ export default class Notes { if (discussionContainer.length === 0) { if (noteEntity.diff_discussion_html) { const discussionElement = document.createElement('table'); - // eslint-disable-next-line no-unsanitized/method - discussionElement.insertAdjacentHTML('afterbegin', noteEntity.diff_discussion_html); + /* + DOMPurify will strip table-less /, so to get it to stop deleting + nodes (since our note HTML starts with a table-less ), we need to wrap + the noteEntity discussion HTML in a to perform the other + sanitization. + */ + const internalNote = sanitize(`
${noteEntity.diff_discussion_html}
`, { + RETURN_DOM: true, + }); + discussionElement.insertAdjacentElement( + 'afterbegin', + /* + Since we wrapped the in a , we need to extract the back out. + DOMPurify returns a Body Element, so we have to start there, then get the + wrapping table, and then get the content we actually want. + Curiously, DOMPurify **ADDS** a totally novel , so we're actually + inserting a completely as-yet-unseen element here. + */ + internalNote.querySelector('table').firstChild, + ); renderGFM(discussionElement); const $discussion = $(discussionElement).unwrap(); -- GitLab From da94347a8413205ae45c1f73b6bc4cade4fc7474 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 31 Jan 2023 14:19:36 -0700 Subject: [PATCH 2/3] Add tests confirming that DOMPurify is working as documented --- spec/frontend/notes/deprecated_notes_spec.js | 46 +++++++++++++++----- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/spec/frontend/notes/deprecated_notes_spec.js b/spec/frontend/notes/deprecated_notes_spec.js index 4756a107a15770..6d3bc19bd45305 100644 --- a/spec/frontend/notes/deprecated_notes_spec.js +++ b/spec/frontend/notes/deprecated_notes_spec.js @@ -28,6 +28,10 @@ window.gl = window.gl || {}; gl.utils = gl.utils || {}; gl.utils.disableButtonIfEmptyField = () => {}; +function wrappedDiscussionNote(note) { + return `
${note}
`; +} + // the following test is unreliable and failing in main 2-3 times a day // see https://gitlab.com/gitlab-org/gitlab/issues/206906#note_290602581 // eslint-disable-next-line jest/no-disabled-tests @@ -436,22 +440,40 @@ describe.skip('Old Notes (~/deprecated_notes.js)', () => { ); }); - it('should append to row selected with line_code', () => { - $form.length = 0; - note.discussion_line_code = 'line_code'; - note.diff_discussion_html = ''; + describe('HTML output', () => { + let line; - const line = document.createElement('div'); - line.id = note.discussion_line_code; - document.body.appendChild(line); + beforeEach(() => { + $form.length = 0; + note.discussion_line_code = 'line_code'; + note.diff_discussion_html = ''; - // Override mocks for this single test - $form.closest.mockReset(); - $form.closest.mockReturnValue($form); + line = document.createElement('div'); + line.id = note.discussion_line_code; + document.body.appendChild(line); - Notes.prototype.renderDiscussionNote.call(notes, note, $form); + // Override mocks for these tests + $form.closest.mockReset(); + $form.closest.mockReturnValue($form); + }); - expect(line.nextSibling.outerHTML).toEqual(note.diff_discussion_html); + it('should append to row selected with line_code', () => { + Notes.prototype.renderDiscussionNote.call(notes, note, $form); + + expect(line.nextSibling.outerHTML).toEqual( + wrappedDiscussionNote(note.diff_discussion_html), + ); + }); + + it('sanitizes the output html without stripping leading or elements', () => { + const sanitizedDiscussion = 'I am a dolphin!'; + note.diff_discussion_html = + 'I am a dolphin!'; + + Notes.prototype.renderDiscussionNote.call(notes, note, $form); + + expect(line.nextSibling.outerHTML).toEqual(wrappedDiscussionNote(sanitizedDiscussion)); + }); }); }); -- GitLab From 02faaf7da2fde7a313376ae999b48bcc048b857e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 3 Feb 2023 14:51:40 -0700 Subject: [PATCH 3/3] Handle both "normal" discussions and image diff discussions Image diff discussions don't use a table row, which causes DOMPurify to strip the outer table since the child is a
. This causes the .firstChild selector to fail on the result of .querySelector( "table" ), since that always returns null. An alternative solution here is to ONLY modify line that selects the element to append, by modifying the expression based on whether the note is `on_image` or not: ``` internalNote = sanitize(`${noteEntity.diff_discussion_html}
`, { RETURN_DOM: true, }); discussionElement.insertAdjacentElement( 'afterbegin', noteEntity.on_image ? internalNote.firstChild : internalNote.querySelector( "table" ).firstChild ); ``` However, this is pretty "clever" (in a bad way) because it depends on the reader knowing that DOMPurify will magically strip the element if its children aren't s. The if/else here makes it clear that there are two potential parsing modes: one for image comments, and one for other comments. --- app/assets/javascripts/deprecated_notes.js | 36 ++++++++++++++-------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/deprecated_notes.js b/app/assets/javascripts/deprecated_notes.js index 3438cb61bc574e..7503df9194bbd3 100644 --- a/app/assets/javascripts/deprecated_notes.js +++ b/app/assets/javascripts/deprecated_notes.js @@ -518,17 +518,19 @@ export default class Notes { if (discussionContainer.length === 0) { if (noteEntity.diff_discussion_html) { const discussionElement = document.createElement('table'); - /* - DOMPurify will strip table-less /), we need to wrap - the noteEntity discussion HTML in a
, so to get it to stop deleting - nodes (since our note HTML starts with a table-less
to perform the other - sanitization. - */ - const internalNote = sanitize(`
${noteEntity.diff_discussion_html}
`, { - RETURN_DOM: true, - }); - discussionElement.insertAdjacentElement( - 'afterbegin', + let internalNote; + let discussionDOM; + + if (!noteEntity.on_image) { + /* + DOMPurify will strip table-less /, so to get it to stop deleting + nodes (since our note HTML starts with a table-less ), we need to wrap + the noteEntity discussion HTML in a to perform the other + sanitization. + */ + internalNote = sanitize(`
${noteEntity.diff_discussion_html}
`, { + RETURN_DOM: true, + }); /* Since we wrapped the in a , we need to extract the back out. DOMPurify returns a Body Element, so we have to start there, then get the @@ -536,8 +538,16 @@ export default class Notes { Curiously, DOMPurify **ADDS** a totally novel , so we're actually inserting a completely as-yet-unseen element here. */ - internalNote.querySelector('table').firstChild, - ); + discussionDOM = internalNote.querySelector('table').firstChild; + } else { + // Image comments don't need
manipulation, they're already
s + internalNote = sanitize(noteEntity.diff_discussion_html, { + RETURN_DOM: true, + }); + discussionDOM = internalNote.firstChild; + } + + discussionElement.insertAdjacentElement('afterbegin', discussionDOM); renderGFM(discussionElement); const $discussion = $(discussionElement).unwrap(); -- GitLab