Merge branch 'security-357-tracing-stored-xss' into 'master'

[master] Stored XSS in Operation Page

See merge request gitlab/gitlab-ee!722

12 files changed, 195 insertions, 13 deletions

diff --git a/db/schema.rb b/db/schema.rb
index d4c5b762fd4..0f77d58724c 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181112103239) do
+ActiveRecord::Schema.define(version: 20181116100917) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/ee/app/models/project_tracing_setting.rb b/ee/app/models/project_tracing_setting.rb
index 97e157f241c..78d872a61fe 100644
--- a/ee/app/models/project_tracing_setting.rb
+++ b/ee/app/models/project_tracing_setting.rb
@@ -5,6 +5,8 @@ class ProjectTracingSetting < ActiveRecord::Base
 
   validates :external_url, length: { maximum: 255 }, public_url: true
 
+  before_validation :sanitize_external_url
+
   def self.create_or_update(project, params)
     self.transaction(requires_new: true) do
       tracing_setting = self.for_project(project)
@@ -17,4 +19,10 @@ class ProjectTracingSetting < ActiveRecord::Base
   def self.for_project(project)
     self.where(project: project).first_or_initialize
   end
+
+  private
+
+  def sanitize_external_url
+    self.external_url = ActionController::Base.helpers.sanitize(self.external_url, tags: [])
+  end
 end
diff --git a/ee/app/views/layouts/nav/sidebar/_tracing_link.html.haml b/ee/app/views/layouts/nav/sidebar/_tracing_link.html.haml
index 1c0125c7605..be9a3dfd7d4 100644
--- a/ee/app/views/layouts/nav/sidebar/_tracing_link.html.haml
+++ b/ee/app/views/layouts/nav/sidebar/_tracing_link.html.haml
@@ -3,7 +3,7 @@
 - if project_nav_tab? :settings
   = nav_link(controller: :tracings, action: [:show]) do
     - if @project.tracing_external_url.present?
-      = link_to @project.tracing_external_url, target: "_blank", rel: 'noopener noreferrer' do
+      = link_to sanitize(@project.tracing_external_url, tags: []), target: "_blank", rel: 'noopener noreferrer' do
         %span
           = _('Tracing')
           %i.strong.ml-1.fa.fa-external-link
diff --git a/ee/app/views/projects/settings/operations/show.html.haml b/ee/app/views/projects/settings/operations/show.html.haml
index 74054e6e49d..1d1941cd342 100644
--- a/ee/app/views/projects/settings/operations/show.html.haml
+++ b/ee/app/views/projects/settings/operations/show.html.haml
@@ -8,12 +8,16 @@
   %h4
     = _("Jaeger tracing")
   %p
-    - tracing_url = has_jaeger_url ? @project.tracing_external_url : project_tracing_path(@project)
-    - meta = has_jaeger_url ? 'rel="noopener noreferrer" target="_blank"' : ''
-    - icon = has_jaeger_url ? sprite_icon('external-link', size: 16, css_class: 'ml-1 vertical-align-middle') : ''
-    - tracing_start_tag = "<a href='#{tracing_url}' #{meta}>".html_safe
-    - tracing_end_tag = "#{icon}</a>".html_safe
-    = _("To open Jaeger and easily view tracing from GitLab, link the %{start_tag}Tracing%{end_tag} page to your server").html_safe % { start_tag: tracing_start_tag, end_tag: tracing_end_tag }
+    - if has_jaeger_url
+      - tracing_link = link_to sanitize(@project.tracing_external_url, tags: []), target: "_blank", rel: 'noopener noreferrer' do
+        %span
+          = _('Tracing')
+          = sprite_icon('external-link', size: 16, css_class: 'ml-1 vertical-align-middle')
+    - else
+      - tracing_link = link_to project_tracing_path(@project) do
+        %span
+          = _('Tracing')
+    = _("To open Jaeger and easily view tracing from GitLab, link the %{link} page to your server").html_safe % { link: tracing_link }
   = form_for @tracing_settings, as: :tracing_settings, url: project_settings_operations_path(@project) do |f|
     = form_errors(@tracing_settings)
     .form-group
diff --git a/ee/changelogs/unreleased/security-357-tracing-stored-xss.yml b/ee/changelogs/unreleased/security-357-tracing-stored-xss.yml
new file mode 100644
index 00000000000..c735dc97335
--- /dev/null
+++ b/ee/changelogs/unreleased/security-357-tracing-stored-xss.yml
@@ -0,0 +1,5 @@
+---
+title: Sanitize tracing external_urls before saving to DB and when displaying the URL to prevent XSS issues
+merge_request:
+author:
+type: security
diff --git a/ee/db/post_migrate/20181116100917_sanitize_tracing_external_url.rb b/ee/db/post_migrate/20181116100917_sanitize_tracing_external_url.rb
new file mode 100644
index 00000000000..ad7365e2910
--- /dev/null
+++ b/ee/db/post_migrate/20181116100917_sanitize_tracing_external_url.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class SanitizeTracingExternalUrl < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  class ProjectTracingSetting < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'project_tracing_settings'
+
+    def sanitize_external_url
+      self.external_url = ActionController::Base.helpers.sanitize(self.external_url, tags: [])
+    end
+  end
+
+  def up
+    ProjectTracingSetting.each_batch(of: 50) do |batch|
+      batch.each do |rec|
+        rec.sanitize_external_url
+
+        rec.save! if rec.changed?
+      end
+    end
+  end
+
+  def down
+    # no-op
+  end
+end
diff --git a/ee/spec/factories/project_tracing_settings.rb b/ee/spec/factories/project_tracing_settings.rb
new file mode 100644
index 00000000000..03a81180404
--- /dev/null
+++ b/ee/spec/factories/project_tracing_settings.rb
@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :project_tracing_setting do
+    project
+    external_url 'https://example.com'
+  end
+end
diff --git a/ee/spec/migrations/sanitize_tracing_external_url_spec.rb b/ee/spec/migrations/sanitize_tracing_external_url_spec.rb
new file mode 100644
index 00000000000..486ee87270c
--- /dev/null
+++ b/ee/spec/migrations/sanitize_tracing_external_url_spec.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'post_migrate', '20181116100917_sanitize_tracing_external_url.rb')
+
+describe SanitizeTracingExternalUrl, :migration do
+  let(:migration) { described_class.new }
+
+  describe '#up' do
+    let(:projects) { table(:projects) }
+    let(:namespaces) { table(:namespaces) }
+    let(:project_tracing_settings) { table(:project_tracing_settings) }
+
+    let(:valid_url) { "https://replaceme.com/" }
+    let(:invalid_url) { "https://replaceme.com/'><script>alert(document.cookie)</script>" }
+    let(:cleaned_url) { "https://replaceme.com/'>" }
+
+    before do
+      namespaces.create(id: 1, name: 'gitlab-org', path: 'gitlab-org')
+      projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1', namespace_id: 1)
+      projects.create!(id: 124, name: 'gitlab2', path: 'gitlab2', namespace_id: 1)
+      project_tracing_settings.create!(id: 2234, external_url: invalid_url, project_id: 123)
+      project_tracing_settings.create!(id: 2235, external_url: valid_url, project_id: 124)
+    end
+
+    it 'correctly sanitizes project_tracing_settings external_url' do
+      migrate!
+
+      expect(project_tracing_settings.order(:id).pluck(:external_url)).to match_array([cleaned_url, valid_url])
+    end
+  end
+end
diff --git a/ee/spec/models/project_tracing_setting_spec.rb b/ee/spec/models/project_tracing_setting_spec.rb
index 7bdecffb44a..df340bbba2e 100644
--- a/ee/spec/models/project_tracing_setting_spec.rb
+++ b/ee/spec/models/project_tracing_setting_spec.rb
@@ -24,5 +24,11 @@ describe ProjectTracingSetting do
       tracing_setting.external_url = " "
       expect(tracing_setting).not_to be_valid
     end
+
+    it 'sanitizes the url' do
+      tracing_setting.external_url = "https://replaceme.com/'><script>alert(document.cookie)</script>"
+      expect(tracing_setting).to be_valid
+      expect(tracing_setting.external_url).to eq("https://replaceme.com/'>")
+    end
   end
 end
diff --git a/ee/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb b/ee/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
index e92d2e25aa4..f1abcddc3a9 100644
--- a/ee/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
+++ b/ee/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb
@@ -31,25 +31,43 @@ describe 'layouts/nav/sidebar/_project' do
 
     context 'with project.tracing_external_url' do
       let(:tracing_url) { 'https://tracing.url' }
+      let(:tracing_settings) { create(:project_tracing_setting, project: project, external_url: tracing_url) }
 
       before do
         allow(view).to receive(:can?).and_return(true)
-
-        allow(project).to receive(:tracing_external_url).and_return(tracing_url)
       end
 
       it 'links to project.tracing_external_url' do
+        expect(tracing_settings.external_url).to eq(tracing_url)
+        expect(project.tracing_external_url).to eq(tracing_url)
+
         render
 
         expect(rendered).to have_link('Tracing', href: tracing_url)
       end
+
+      context 'with malicious external_url' do
+        let(:malicious_tracing_url) { "https://replaceme.com/'><script>alert(document.cookie)</script>" }
+        let(:cleaned_url) { "https://replaceme.com/'>" }
+
+        before do
+          tracing_settings.update_column(:external_url, malicious_tracing_url)
+        end
+
+        it 'sanitizes external_url' do
+          expect(project.tracing_external_url).to eq(malicious_tracing_url)
+
+          render
+
+          expect(tracing_settings.external_url).to eq(malicious_tracing_url)
+          expect(rendered).to have_link('Tracing', href: cleaned_url)
+        end
+      end
     end
 
     context 'without project.tracing_external_url' do
       before do
         allow(view).to receive(:can?).and_return(true)
-
-        allow(project).to receive(:tracing_external_url).and_return(nil)
       end
 
       it 'links to Tracing page' do
diff --git a/ee/spec/views/projects/settings/operations/show.html.haml_spec.rb b/ee/spec/views/projects/settings/operations/show.html.haml_spec.rb
new file mode 100644
index 00000000000..e30e00c24b2
--- /dev/null
+++ b/ee/spec/views/projects/settings/operations/show.html.haml_spec.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'projects/settings/operations/show' do
+  let(:project) { create(:project, :repository) }
+
+  before do
+    assign(:project, project)
+    assign(:repository, project.repository)
+    allow(view).to receive(:current_ref).and_return('master')
+
+    stub_licensed_features(tracing: true)
+  end
+
+  describe 'Operations > Tracing' do
+    context 'with project.tracing_external_url' do
+      let(:tracing_url) { 'https://tracing.url' }
+      let(:tracing_settings) { create(:project_tracing_setting, project: project, external_url: tracing_url) }
+
+      before do
+        allow(view).to receive(:can?).and_return(true)
+
+        assign(:tracing_settings, tracing_settings)
+      end
+
+      it 'links to project.tracing_external_url' do
+        render
+
+        expect(rendered).to have_link('Tracing', href: tracing_url)
+      end
+
+      context 'with malicious external_url' do
+        let(:malicious_tracing_url) { "https://replaceme.com/'><script>alert(document.cookie)</script>" }
+        let(:cleaned_url) { "https://replaceme.com/'>" }
+
+        before do
+          tracing_settings.update_column(:external_url, malicious_tracing_url)
+        end
+
+        it 'sanitizes external_url' do
+          render
+
+          expect(tracing_settings.external_url).to eq(malicious_tracing_url)
+          expect(rendered).to have_link('Tracing', href: cleaned_url)
+        end
+      end
+    end
+
+    context 'without project.tracing_external_url' do
+      let(:tracing_settings) { build(:project_tracing_setting, project: project) }
+
+      before do
+        allow(view).to receive(:can?).and_return(true)
+
+        tracing_settings.external_url = nil
+
+        assign(:tracing_settings, tracing_settings)
+      end
+
+      it 'links to Tracing page' do
+        render
+
+        expect(rendered).to have_link('Tracing', href: project_tracing_path(project))
+      end
+    end
+  end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 6d3b265169d..1bb203a2b49 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -8701,7 +8701,7 @@ msgstr ""
 msgid "To only use CI/CD features for an external repository, choose <strong>CI/CD for external repo</strong>."
 msgstr ""
 
-msgid "To open Jaeger and easily view tracing from GitLab, link the %{start_tag}Tracing%{end_tag} page to your server"
+msgid "To open Jaeger and easily view tracing from GitLab, link the %{link} page to your server"
 msgstr ""
 
 msgid "To preserve performance only <strong>%{display_size} of ${real_size}</strong> files are displayed."