Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq

8 files changed, 90 insertions, 11 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6e5296e231e..54ea014a679 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,13 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 11.4.6 (2018-11-18)
+
+### Security (1 change)
+
+- Escape user fullname while rendering autocomplete template to prevent XSS.
+
+
 ## 11.4.5 (2018-11-04)
 
 ### Fixed (4 changes, 1 of them is from the community)
@@ -271,6 +278,13 @@ entry.
 - Check frozen string in style builds. (gfyoung)
 
 
+## 11.3.10 (2018-11-18)
+
+### Security (1 change)
+
+- Escape user fullname while rendering autocomplete template to prevent XSS.
+
+
 ## 11.3.9 (2018-10-31)
 
 ### Security (1 change)
diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js
index 00b3d283570..6f8b73564d0 100644
--- a/app/assets/javascripts/gfm_auto_complete.js
+++ b/app/assets/javascripts/gfm_auto_complete.js
@@ -151,10 +151,16 @@ class GfmAutoComplete {
     // Team Members
     $input.atwho({
       at: '@',
+      alias: 'users',
       displayTpl(value) {
         let tmpl = GfmAutoComplete.Loading.template;
-        if (value.username != null) {
-          tmpl = GfmAutoComplete.Members.template;
+        const { avatarTag, username, title } = value;
+        if (username != null) {
+          tmpl = GfmAutoComplete.Members.templateFunction({
+            avatarTag,
+            username,
+            title,
+          });
         }
         return tmpl;
       },
@@ -565,8 +571,9 @@ GfmAutoComplete.Emoji = {
 };
 // Team Members
 GfmAutoComplete.Members = {
-  // eslint-disable-next-line no-template-curly-in-string
-  template: '<li>${avatarTag} ${username} <small>${title}</small></li>',
+  templateFunction({ avatarTag, username, title }) {
+    return `<li>${avatarTag} ${username} <small>${_.escape(title)}</small></li>`;
+  },
 };
 GfmAutoComplete.Labels = {
   template:
diff --git a/changelogs/unreleased/security-2717-xss-username-autocomplete.yml b/changelogs/unreleased/security-2717-xss-username-autocomplete.yml
new file mode 100644
index 00000000000..d9b1015eeb4
--- /dev/null
+++ b/changelogs/unreleased/security-2717-xss-username-autocomplete.yml
@@ -0,0 +1,5 @@
+---
+title: Escape user fullname while rendering autocomplete template to prevent XSS
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/sh-fix-issue-54189.yml b/changelogs/unreleased/sh-fix-issue-54189.yml
new file mode 100644
index 00000000000..eee743aa5d9
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-issue-54189.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent templated services from being imported
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index 9790818ecaf..b40eac3de9a 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -154,6 +154,8 @@ excluded_attributes:
     - :encrypted_token_iv
     - :encrypted_url
     - :encrypted_url_iv
+  services:
+    - :template
 
 methods:
   labels:
diff --git a/spec/features/issues/gfm_autocomplete_spec.rb b/spec/features/issues/gfm_autocomplete_spec.rb
index 605860b90cd..7c591dacce5 100644
--- a/spec/features/issues/gfm_autocomplete_spec.rb
+++ b/spec/features/issues/gfm_autocomplete_spec.rb
@@ -1,6 +1,10 @@
 require 'rails_helper'
 
 describe 'GFM autocomplete', :js do
+  let(:issue_xss_title) { 'This will execute alert<img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
+  let(:user_xss_title) { 'eve <img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
+
+  let(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') }
   let(:user)    { create(:user, name: '💃speciąl someone💃', username: 'someone.special') }
   let(:project) { create(:project) }
   let(:label) { create(:label, project: project, title: 'special+') }
@@ -9,6 +13,8 @@ describe 'GFM autocomplete', :js do
 
   before do
     project.add_maintainer(user)
+    project.add_maintainer(user_xss)
+
     sign_in(user)
     visit project_issue_path(project, issue)
 
@@ -35,9 +41,8 @@ describe 'GFM autocomplete', :js do
     expect(page).to have_selector('.atwho-container')
   end
 
-  it 'opens autocomplete menu when field starts with text with item escaping HTML characters' do
-    alert_title = 'This will execute alert<img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;'
-    create(:issue, project: project, title: alert_title)
+  it 'opens autocomplete menu for Issues when field starts with text with item escaping HTML characters' do
+    create(:issue, project: project, title: issue_xss_title)
 
     page.within '.timeline-content-form' do
       find('#note-body').native.send_keys('#')
@@ -46,7 +51,19 @@ describe 'GFM autocomplete', :js do
     expect(page).to have_selector('.atwho-container')
 
     page.within '.atwho-container #at-view-issues' do
-      expect(page.all('li').first.text).to include(alert_title)
+      expect(page.all('li').first.text).to include(issue_xss_title)
+    end
+  end
+
+  it 'opens autocomplete menu for Username when field starts with text with item escaping HTML characters' do
+    page.within '.timeline-content-form' do
+      find('#note-body').native.send_keys('@ev')
+    end
+
+    expect(page).to have_selector('.atwho-container')
+
+    page.within '.atwho-container #at-view-users' do
+      expect(find('li').text).to have_content(user_xss.username)
     end
   end
 
@@ -107,7 +124,7 @@ describe 'GFM autocomplete', :js do
 
     wait_for_requests
 
-    expect(find('#at-view-64')).to have_selector('.cur:first-of-type')
+    expect(find('#at-view-users')).to have_selector('.cur:first-of-type')
   end
 
   it 'includes items for assignee dropdowns with non-ASCII characters in name' do
@@ -120,7 +137,7 @@ describe 'GFM autocomplete', :js do
 
     wait_for_requests
 
-    expect(find('#at-view-64')).to have_content(user.name)
+    expect(find('#at-view-users')).to have_content(user.name)
   end
 
   it 'selects the first item for non-assignee dropdowns if a query is entered' do
diff --git a/spec/lib/gitlab/import_export/project.light.json b/spec/lib/gitlab/import_export/project.light.json
index ba2248073f5..2971ca0f0f8 100644
--- a/spec/lib/gitlab/import_export/project.light.json
+++ b/spec/lib/gitlab/import_export/project.light.json
@@ -101,6 +101,28 @@
       ]
     }
   ],
+  "services": [
+    {
+      "id": 100,
+      "title": "JetBrains TeamCity CI",
+      "project_id": 5,
+      "created_at": "2016-06-14T15:01:51.315Z",
+      "updated_at": "2016-06-14T15:01:51.315Z",
+      "active": false,
+      "properties": {},
+      "template": true,
+      "push_events": true,
+      "issues_events": true,
+      "merge_requests_events": true,
+      "tag_push_events": true,
+      "note_events": true,
+      "job_events": true,
+      "type": "TeamcityService",
+      "category": "ci",
+      "default": false,
+      "wiki_page_events": true
+    }
+  ],
   "snippets": [],
   "hooks": []
 }
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index 365bfae0d88..7171e12a849 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -297,7 +297,8 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
                       issues: 1,
                       labels: 1,
                       milestones: 1,
-                      first_issue_labels: 1
+                      first_issue_labels: 1,
+                      services: 1
 
       context 'project.json file access check' do
         it 'does not read a symlink' do
@@ -382,6 +383,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         project_tree_restorer.instance_variable_set(:@path, "spec/lib/gitlab/import_export/project.light.json")
       end
 
+      it 'does not import any templated services' do
+        restored_project_json
+
+        expect(project.services.where(template: true).count).to eq(0)
+      end
+
       it 'imports labels' do
         create(:group_label, name: 'Another label', group: project.group)