diff --git a/app/assets/javascripts/deprecated_notes.js b/app/assets/javascripts/deprecated_notes.js index 8019a10a042ac7856a8e7dd5a9e8c0283bc15434..7503df9194bbd3a8612ad7cdee73e9e0e0df8550 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,36 @@ 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); + 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 + 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. + */ + 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(); diff --git a/spec/frontend/notes/deprecated_notes_spec.js b/spec/frontend/notes/deprecated_notes_spec.js index 4756a107a15770e546f57d81e452423995929c1e..6d3bc19bd4530552f4eb8279e658ea95bc329756 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)); + }); }); });