diff options
author | John Jarvis <jarv@gitlab.com> | 2018-12-26 15:03:47 +0300 |
---|---|---|
committer | John Jarvis <jarv@gitlab.com> | 2018-12-26 15:03:47 +0300 |
commit | aba4b6afbedb5c7f0fa8d742b090d1471f1739b7 (patch) | |
tree | 8e736b6a53054d3716eef6226a391fb8b010058f | |
parent | 9349651f2689b4d4f84acabe71352a4e4856b4d0 (diff) | |
parent | 5f51b335d24518cc4dea79958648dc9559ed51ff (diff) |
Merge branch 'security-11-6-54377-label-milestone-name-xss' into 'security-11-6'
[11.6] Escape label and milestone titles to prevent XSS in GFM autocomplete
See merge request gitlab/gitlabhq!2740
-rw-r--r-- | app/assets/javascripts/gfm_auto_complete.js | 17 | ||||
-rw-r--r-- | changelogs/unreleased/security-11-6-54377-label-milestone-name-xss.yml | 5 | ||||
-rw-r--r-- | spec/features/issues/gfm_autocomplete_spec.rb | 44 |
3 files changed, 59 insertions, 7 deletions
diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index c14eb936930..8178821be3d 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -256,7 +256,7 @@ class GfmAutoComplete { displayTpl(value) { let tmpl = GfmAutoComplete.Loading.template; if (value.title != null) { - tmpl = GfmAutoComplete.Milestones.template; + tmpl = GfmAutoComplete.Milestones.templateFunction(value.title); } return tmpl; }, @@ -323,7 +323,7 @@ class GfmAutoComplete { searchKey: 'search', data: GfmAutoComplete.defaultLoadingData, displayTpl(value) { - let tmpl = GfmAutoComplete.Labels.template; + let tmpl = GfmAutoComplete.Labels.templateFunction(value.color, value.title); if (GfmAutoComplete.isLoading(value)) { tmpl = GfmAutoComplete.Loading.template; } @@ -588,9 +588,11 @@ GfmAutoComplete.Members = { }, }; GfmAutoComplete.Labels = { - template: - // eslint-disable-next-line no-template-curly-in-string - '<li><span class="dropdown-label-box" style="background: ${color}"></span> ${title}</li>', + templateFunction(color, title) { + return `<li><span class="dropdown-label-box" style="background: ${_.escape( + color, + )}"></span> ${_.escape(title)}</li>`; + }, }; // Issues, MergeRequests and Snippets GfmAutoComplete.Issues = { @@ -600,8 +602,9 @@ GfmAutoComplete.Issues = { }; // Milestones GfmAutoComplete.Milestones = { - // eslint-disable-next-line no-template-curly-in-string - template: '<li>${title}</li>', + templateFunction(title) { + return `<li>${_.escape(title)}</li>`; + }, }; GfmAutoComplete.Loading = { template: diff --git a/changelogs/unreleased/security-11-6-54377-label-milestone-name-xss.yml b/changelogs/unreleased/security-11-6-54377-label-milestone-name-xss.yml new file mode 100644 index 00000000000..f2911ce4698 --- /dev/null +++ b/changelogs/unreleased/security-11-6-54377-label-milestone-name-xss.yml @@ -0,0 +1,5 @@ +--- +title: Escape label and milestone titles to prevent XSS in GFM autocomplete +merge_request: 2740 +author: +type: security diff --git a/spec/features/issues/gfm_autocomplete_spec.rb b/spec/features/issues/gfm_autocomplete_spec.rb index d7531d5fcd9..3b7a17ef355 100644 --- a/spec/features/issues/gfm_autocomplete_spec.rb +++ b/spec/features/issues/gfm_autocomplete_spec.rb @@ -3,6 +3,8 @@ require 'rails_helper' describe 'GFM autocomplete', :js do let(:issue_xss_title) { 'This will execute alert<img src=x onerror=alert(2)<img src=x onerror=alert(1)>' } let(:user_xss_title) { 'eve <img src=x onerror=alert(2)<img src=x onerror=alert(1)>' } + let(:label_xss_title) { 'alert label <img src=x onerror="alert(\'Hello xss\');" a'} + let(:milestone_xss_title) { 'alert milestone <img src=x onerror="alert(\'Hello xss\');" a' } let(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') } let(:user) { create(:user, name: '💃speciąl someone💃', username: 'someone.special') } @@ -25,10 +27,14 @@ describe 'GFM autocomplete', :js do simulate_input('#issue-description', "@#{user.name[0...3]}") + wait_for_requests + find('.atwho-view .cur').click click_button 'Save changes' + wait_for_requests + expect(find('.description')).to have_content(user.to_reference) end @@ -47,6 +53,8 @@ describe 'GFM autocomplete', :js do find('#note-body').native.send_keys('#') end + wait_for_requests + expect(page).to have_selector('.atwho-container') page.within '.atwho-container #at-view-issues' do @@ -59,6 +67,8 @@ describe 'GFM autocomplete', :js do find('#note-body').native.send_keys('@ev') end + wait_for_requests + expect(page).to have_selector('.atwho-container') page.within '.atwho-container #at-view-users' do @@ -66,6 +76,22 @@ describe 'GFM autocomplete', :js do end end + it 'opens autocomplete menu for Milestone when field starts with text with item escaping HTML characters' do + create(:milestone, project: project, title: milestone_xss_title) + + page.within '.timeline-content-form' do + find('#note-body').native.send_keys('%') + end + + wait_for_requests + + expect(page).to have_selector('.atwho-container') + + page.within '.atwho-container #at-view-milestones' do + expect(find('li').text).to have_content('alert milestone') + end + end + it 'doesnt open autocomplete menu character is prefixed with text' do page.within '.timeline-content-form' do find('#note-body').native.send_keys('testing') @@ -258,12 +284,28 @@ describe 'GFM autocomplete', :js do let!(:bug) { create(:label, project: project, title: 'bug') } let!(:feature_proposal) { create(:label, project: project, title: 'feature proposal') } + it 'opens autocomplete menu for Labels when field starts with text with item escaping HTML characters' do + create(:label, project: project, title: label_xss_title) + + note = find('#note-body') + + # It should show all the labels on "~". + type(note, '~') + + wait_for_requests + + page.within '.atwho-container #at-view-labels' do + expect(find('.atwho-view-ul').text).to have_content('alert label') + end + end + context 'when no labels are assigned' do it 'shows labels' do note = find('#note-body') # It should show all the labels on "~". type(note, '~') + wait_for_requests expect_labels(shown: [backend, bug, feature_proposal]) # It should show all the labels on "/label ~". @@ -290,6 +332,7 @@ describe 'GFM autocomplete', :js do # It should show all the labels on "~". type(note, '~') + wait_for_requests expect_labels(shown: [backend, bug, feature_proposal]) # It should show only unset labels on "/label ~". @@ -316,6 +359,7 @@ describe 'GFM autocomplete', :js do # It should show all the labels on "~". type(note, '~') + wait_for_requests expect_labels(shown: [backend, bug, feature_proposal]) # It should show no labels on "/label ~". |