diff options
-rw-r--r-- | app/assets/javascripts/issue_show/index.js | 7 | ||||
-rw-r--r-- | app/assets/javascripts/pages/projects/issues/show.js | 3 | ||||
-rw-r--r-- | changelogs/unreleased/security-acet-issue-details.yml | 5 | ||||
-rw-r--r-- | spec/features/issues/issue_detail_spec.rb | 17 | ||||
-rw-r--r-- | spec/javascripts/issue_show/index_spec.js | 19 |
5 files changed, 47 insertions, 4 deletions
diff --git a/app/assets/javascripts/issue_show/index.js b/app/assets/javascripts/issue_show/index.js index 75dfdedcf1b..d08e8ba0c4b 100644 --- a/app/assets/javascripts/issue_show/index.js +++ b/app/assets/javascripts/issue_show/index.js @@ -1,10 +1,11 @@ import Vue from 'vue'; +import sanitize from 'sanitize-html'; import issuableApp from './components/app.vue'; import '../vue_shared/vue_resource_interceptor'; -document.addEventListener('DOMContentLoaded', () => { +export default function initIssueableApp() { const initialDataEl = document.getElementById('js-issuable-app-initial-data'); - const props = JSON.parse(initialDataEl.innerHTML.replace(/"/g, '"')); + const props = JSON.parse(sanitize(initialDataEl.textContent).replace(/"/g, '"')); return new Vue({ el: document.getElementById('js-issuable-app'), @@ -17,4 +18,4 @@ document.addEventListener('DOMContentLoaded', () => { }); }, }); -}); +} diff --git a/app/assets/javascripts/pages/projects/issues/show.js b/app/assets/javascripts/pages/projects/issues/show.js index 500fbd27340..3f415391aa1 100644 --- a/app/assets/javascripts/pages/projects/issues/show.js +++ b/app/assets/javascripts/pages/projects/issues/show.js @@ -3,9 +3,10 @@ import Issue from '~/issue'; import ShortcutsIssuable from '~/shortcuts_issuable'; import ZenMode from '~/zen_mode'; import '~/notes/index'; -import '~/issue_show/index'; +import initIssueableApp from '~/issue_show'; export default function () { + initIssueableApp(); new Issue(); // eslint-disable-line no-new new ShortcutsIssuable(); // eslint-disable-line no-new new ZenMode(); // eslint-disable-line no-new diff --git a/changelogs/unreleased/security-acet-issue-details.yml b/changelogs/unreleased/security-acet-issue-details.yml new file mode 100644 index 00000000000..64147a9d6e8 --- /dev/null +++ b/changelogs/unreleased/security-acet-issue-details.yml @@ -0,0 +1,5 @@ +--- +title: Sanitize JSON data properly to fix XSS on Issue details page +merge_request: +author: +type: security diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 088ab114df3..76bc93e9766 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/spec/features/issues/issue_detail_spec.rb @@ -18,6 +18,23 @@ describe 'Issue Detail', :js do end end + context 'when issue description has xss snippet' do + before do + issue.update!(description: '![xss" onload=alert(1);//](a)') + sign_in(user) + visit project_issue_path(project, issue) + wait_for_requests + end + + it 'should encode the description to prevent xss issues' do + page.within('.issuable-details .detail-page-description') do + expect(page).to have_selector('img', count: 1) + expect(find('img')['onerror']).to be_nil + expect(find('img')['src']).to end_with('/a') + end + end + end + context 'when edited by a user who is later deleted' do before do sign_in(user) diff --git a/spec/javascripts/issue_show/index_spec.js b/spec/javascripts/issue_show/index_spec.js new file mode 100644 index 00000000000..fa0b426c06c --- /dev/null +++ b/spec/javascripts/issue_show/index_spec.js @@ -0,0 +1,19 @@ +import initIssueableApp from '~/issue_show'; + +describe('Issue show index', () => { + describe('initIssueableApp', () => { + it('should initialize app with no potential XSS attack', () => { + const d = document.createElement('div'); + d.id = 'js-issuable-app-initial-data'; + d.innerHTML = JSON.stringify({ + initialDescriptionHtml: '<img src=x onerror=alert(1)>', + }); + document.body.appendChild(d); + + const alertSpy = spyOn(window, 'alert'); + initIssueableApp(); + + expect(alertSpy).not.toHaveBeenCalled(); + }); + }); +}); |