From 5e8f11e5fdb792f17d86cf9321537c5c56801a17 Mon Sep 17 00:00:00 2001
From: Filipa Lacerda <filipa@gitlab.com>
Date: Thu, 9 Aug 2018 12:05:13 +0100
Subject: Removes <br> sent from backend on tooltips in jobs

When backend sends HTML it requires frontend to append it to the DOM causing
XSS vulnerabilities. By removing the `<br>` we avoid those vulnerabilities
---
 .../components/graph/dropdown_job_component.vue      |  3 +--
 .../pipelines/components/graph/job_component.vue     |  5 +----
 app/views/ci/status/_dropdown_graph_badge.html.haml  |  4 ++--
 app/views/projects/jobs/_sidebar.html.haml           |  2 +-
 lib/gitlab/ci/status/build/failed.rb                 |  2 +-
 spec/features/projects/jobs/user_browses_job_spec.rb |  4 ++--
 .../features/projects/jobs/user_browses_jobs_spec.rb |  2 +-
 spec/features/projects/pipelines/pipeline_spec.rb    |  4 ++--
 spec/features/projects/pipelines/pipelines_spec.rb   |  2 +-
 .../pipelines/graph/dropdown_job_component_spec.js   |  8 --------
 .../pipelines/graph/job_component_spec.js            | 20 --------------------
 spec/lib/gitlab/ci/status/build/factory_spec.rb      |  2 +-
 .../gitlab/ci/status/build/failed_allowed_spec.rb    |  4 ++--
 spec/lib/gitlab/ci/status/build/failed_spec.rb       |  4 ++--
 spec/lib/gitlab/ci/status/build/retried_spec.rb      |  2 +-
 spec/presenters/ci/build_presenter_spec.rb           | 12 ++++++------
 spec/serializers/build_serializer_spec.rb            |  2 +-
 spec/serializers/job_entity_spec.rb                  |  4 ++--
 18 files changed, 27 insertions(+), 59 deletions(-)

diff --git a/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue b/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue
index 8487c8036ee..2ad66f4fe86 100644
--- a/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue
+++ b/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue
@@ -1,6 +1,5 @@
 <script>
 import $ from 'jquery';
-import _ from 'underscore';
 import JobNameComponent from './job_name_component.vue';
 import JobComponent from './job_component.vue';
 import tooltip from '../../../vue_shared/directives/tooltip';
@@ -47,7 +46,7 @@ export default {
 
   computed: {
     tooltipText() {
-      return _.escape(`${this.job.name} - ${this.job.status.label}`);
+      return `${this.job.name} - ${this.job.status.label}`;
     },
   },
 
diff --git a/app/assets/javascripts/pipelines/components/graph/job_component.vue b/app/assets/javascripts/pipelines/components/graph/job_component.vue
index 66f95147193..9ac16b7e541 100644
--- a/app/assets/javascripts/pipelines/components/graph/job_component.vue
+++ b/app/assets/javascripts/pipelines/components/graph/job_component.vue
@@ -1,5 +1,4 @@
 <script>
-import _ from 'underscore';
 import ActionComponent from './action_component.vue';
 import JobNameComponent from './job_name_component.vue';
 import tooltip from '../../../vue_shared/directives/tooltip';
@@ -62,7 +61,7 @@ export default {
       const textBuilder = [];
 
       if (this.job.name) {
-        textBuilder.push(_.escape(this.job.name));
+        textBuilder.push(this.job.name);
       }
 
       if (this.job.name && this.status.tooltip) {
@@ -106,7 +105,6 @@ export default {
       :class="cssClassJobName"
       :data-boundary="tooltipBoundary"
       data-container="body"
-      data-html="true"
       class="js-pipeline-graph-job-link"
     >
 
@@ -122,7 +120,6 @@ export default {
       :title="tooltipText"
       :class="cssClassJobName"
       class="js-job-component-tooltip non-details-job-component"
-      data-html="true"
       data-container="body"
     >
 
diff --git a/app/views/ci/status/_dropdown_graph_badge.html.haml b/app/views/ci/status/_dropdown_graph_badge.html.haml
index 8b0463db000..9de9143e8b1 100644
--- a/app/views/ci/status/_dropdown_graph_badge.html.haml
+++ b/app/views/ci/status/_dropdown_graph_badge.html.haml
@@ -6,12 +6,12 @@
 - tooltip = "#{subject.name} - #{status.status_tooltip}"
 
 - if status.has_details?
-  = link_to status.details_path, class: 'mini-pipeline-graph-dropdown-item', data: { toggle: 'tooltip', title: tooltip, html: 'true', container: 'body' }  do
+  = link_to status.details_path, class: 'mini-pipeline-graph-dropdown-item', data: { toggle: 'tooltip', title: tooltip, container: 'body' }  do
     %span{ class: klass }= sprite_icon(status.icon)
     %span.ci-build-text= subject.name
 
 - else
-  .menu-item.mini-pipeline-graph-dropdown-item{ data: { toggle: 'tooltip', html: 'true', title: tooltip, container: 'body' } }
+  .menu-item.mini-pipeline-graph-dropdown-item{ data: { toggle: 'tooltip', title: tooltip, container: 'body' } }
     %span{ class: klass }= sprite_icon(status.icon)
     %span.ci-build-text= subject.name
 
diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml
index 759efd4e9d4..74f88486738 100644
--- a/app/views/projects/jobs/_sidebar.html.haml
+++ b/app/views/projects/jobs/_sidebar.html.haml
@@ -87,7 +87,7 @@
         - builds.select{|build| build.status == build_status}.each do |build|
           .build-job{ class: sidebar_build_class(build, @build), data: { stage: build.stage } }
             - tooltip = sanitize(build.tooltip_message.dup)
-            = link_to(project_job_path(@project, build), data: { toggle: 'tooltip', html: 'true', title: tooltip, container: 'body' }) do
+            = link_to(project_job_path(@project, build), data: { toggle: 'tooltip', title: tooltip, container: 'body' }) do
               = sprite_icon('arrow-right', size:16, css_class: 'icon-arrow-right')
               %span{ class: "ci-status-icon-#{build.status}" }
                 = ci_icon_for_status(build.status)
diff --git a/lib/gitlab/ci/status/build/failed.rb b/lib/gitlab/ci/status/build/failed.rb
index 703f0b9217b..508b4814631 100644
--- a/lib/gitlab/ci/status/build/failed.rb
+++ b/lib/gitlab/ci/status/build/failed.rb
@@ -32,7 +32,7 @@ module Gitlab
           end
 
           def description
-            "<br> (#{failure_reason_message})"
+            "- (#{failure_reason_message})"
           end
 
           def failure_reason_message
diff --git a/spec/features/projects/jobs/user_browses_job_spec.rb b/spec/features/projects/jobs/user_browses_job_spec.rb
index 50e957bf12b..2d791947ee9 100644
--- a/spec/features/projects/jobs/user_browses_job_spec.rb
+++ b/spec/features/projects/jobs/user_browses_job_spec.rb
@@ -40,7 +40,7 @@ describe 'User browses a job', :js do
     it 'displays the failure reason' do
       within('.builds-container') do
         build_link = first('.build-job > a')
-        expect(build_link['data-title']).to eq('test - failed <br> (unknown failure)')
+        expect(build_link['data-title']).to eq('test - failed - (unknown failure)')
       end
     end
   end
@@ -51,7 +51,7 @@ describe 'User browses a job', :js do
     it 'displays the failure reason and retried label' do
       within('.builds-container') do
         build_link = first('.build-job > a')
-        expect(build_link['data-title']).to eq('test - failed <br> (unknown failure) (retried)')
+        expect(build_link['data-title']).to eq('test - failed - (unknown failure) (retried)')
       end
     end
   end
diff --git a/spec/features/projects/jobs/user_browses_jobs_spec.rb b/spec/features/projects/jobs/user_browses_jobs_spec.rb
index 08786fe1630..ebc20d15d67 100644
--- a/spec/features/projects/jobs/user_browses_jobs_spec.rb
+++ b/spec/features/projects/jobs/user_browses_jobs_spec.rb
@@ -36,7 +36,7 @@ describe 'User browses jobs' do
     it 'displays a tooltip with the failure reason' do
       page.within('.ci-table') do
         failed_job_link = page.find('.ci-failed')
-        expect(failed_job_link[:title]).to eq('Failed <br> (unknown failure)')
+        expect(failed_job_link[:title]).to eq('Failed - (unknown failure)')
       end
     end
   end
diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb
index a84492ea5f1..603503a531c 100644
--- a/spec/features/projects/pipelines/pipeline_spec.rb
+++ b/spec/features/projects/pipelines/pipeline_spec.rb
@@ -126,7 +126,7 @@ describe 'Pipeline', :js do
         it 'should include the failure reason' do
           page.within('#ci-badge-test') do
             build_link = page.find('.js-pipeline-graph-job-link')
-            expect(build_link['data-original-title']).to eq('test - failed <br> (unknown failure)')
+            expect(build_link['data-original-title']).to eq('test - failed - (unknown failure)')
           end
         end
       end
@@ -319,7 +319,7 @@ describe 'Pipeline', :js do
       it 'displays a tooltip with the failure reason' do
         page.within('.ci-table') do
           failed_job_link = page.find('.ci-failed')
-          expect(failed_job_link[:title]).to eq('Failed <br> (unknown failure)')
+          expect(failed_job_link[:title]).to eq('Failed - (unknown failure)')
         end
       end
     end
diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb
index 4a83bcc3efb..26a92f14787 100644
--- a/spec/features/projects/pipelines/pipelines_spec.rb
+++ b/spec/features/projects/pipelines/pipelines_spec.rb
@@ -406,7 +406,7 @@ describe 'Pipelines', :js do
 
             within('.js-builds-dropdown-list') do
               build_element = page.find('.mini-pipeline-graph-dropdown-item')
-              expect(build_element['data-original-title']).to eq('build - failed <br> (unknown failure)')
+              expect(build_element['data-original-title']).to eq('build - failed - (unknown failure)')
             end
           end
         end
diff --git a/spec/javascripts/pipelines/graph/dropdown_job_component_spec.js b/spec/javascripts/pipelines/graph/dropdown_job_component_spec.js
index ff584396d61..2b47ca236b2 100644
--- a/spec/javascripts/pipelines/graph/dropdown_job_component_spec.js
+++ b/spec/javascripts/pipelines/graph/dropdown_job_component_spec.js
@@ -82,12 +82,4 @@ describe('dropdown job component', () => {
   it('renders dropdown with jobs', () => {
     expect(vm.$el.querySelectorAll('.scrollable-menu>ul>li').length).toEqual(mock.jobs.length);
   });
-
-  it('escapes tooltip title', () => {
-    expect(
-      vm.$el.querySelector('.js-pipeline-graph-job-link').getAttribute('data-original-title'),
-    ).toEqual(
-      '&lt;img src=x onerror=alert(document.domain)&gt; - passed',
-    );
-  });
 });
diff --git a/spec/javascripts/pipelines/graph/job_component_spec.js b/spec/javascripts/pipelines/graph/job_component_spec.js
index 215ce1e81b5..0ae448f2ea8 100644
--- a/spec/javascripts/pipelines/graph/job_component_spec.js
+++ b/spec/javascripts/pipelines/graph/job_component_spec.js
@@ -161,24 +161,4 @@ describe('pipeline graph job component', () => {
       expect(component.$el.querySelector(tooltipBoundary)).toBeNull();
     });
   });
-
-  describe('tooltipText', () => {
-    it('escapes job name', () => {
-      component = mountComponent(JobComponent, {
-        job: {
-          id: 4259,
-          name: '<img src=x onerror=alert(document.domain)>',
-          status: {
-            icon: 'status_success',
-            label: 'success',
-            tooltip: 'failed',
-          },
-        },
-      });
-
-      expect(
-        component.$el.querySelector('.js-job-component-tooltip').getAttribute('data-original-title'),
-      ).toEqual('&lt;img src=x onerror=alert(document.domain)&gt; - failed');
-    });
-  });
 });
diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb
index d53a7d468e3..8b92088902b 100644
--- a/spec/lib/gitlab/ci/status/build/factory_spec.rb
+++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb
@@ -88,7 +88,7 @@ describe Gitlab::Ci::Status::Build::Factory do
         expect(status.icon).to eq 'status_failed'
         expect(status.favicon).to eq 'favicon_status_failed'
         expect(status.label).to eq 'failed'
-        expect(status.status_tooltip).to eq 'failed <br> (unknown failure)'
+        expect(status.status_tooltip).to eq 'failed - (unknown failure)'
         expect(status).to have_details
         expect(status).to have_action
       end
diff --git a/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb
index bfaa508785e..af03d5a1308 100644
--- a/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb
+++ b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb
@@ -76,7 +76,7 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do
     let(:status) { described_class.new(build_status) }
 
     it 'does override badge_tooltip' do
-      expect(status.badge_tooltip).to eq('failed <br> (unknown failure)')
+      expect(status.badge_tooltip).to eq('failed - (unknown failure)')
     end
   end
 
@@ -87,7 +87,7 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do
     let(:status) { described_class.new(build_status) }
 
     it 'does override status_tooltip' do
-      expect(status.status_tooltip).to eq 'failed <br> (unknown failure) (allowed to fail)'
+      expect(status.status_tooltip).to eq 'failed - (unknown failure) (allowed to fail)'
     end
   end
 
diff --git a/spec/lib/gitlab/ci/status/build/failed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_spec.rb
index b6676b40fd3..e424270f7c5 100644
--- a/spec/lib/gitlab/ci/status/build/failed_spec.rb
+++ b/spec/lib/gitlab/ci/status/build/failed_spec.rb
@@ -52,7 +52,7 @@ describe Gitlab::Ci::Status::Build::Failed do
     let(:status) { Gitlab::Ci::Status::Failed.new(build, user) }
 
     it 'does override badge_tooltip' do
-      expect(subject.badge_tooltip).to eq 'failed <br> (script failure)'
+      expect(subject.badge_tooltip).to eq 'failed - (script failure)'
     end
   end
 
@@ -61,7 +61,7 @@ describe Gitlab::Ci::Status::Build::Failed do
     let(:status) { Gitlab::Ci::Status::Failed.new(build, user) }
 
     it 'does override status_tooltip' do
-      expect(subject.status_tooltip).to eq 'failed <br> (script failure)'
+      expect(subject.status_tooltip).to eq 'failed - (script failure)'
     end
   end
 
diff --git a/spec/lib/gitlab/ci/status/build/retried_spec.rb b/spec/lib/gitlab/ci/status/build/retried_spec.rb
index ee9acaf1c21..76c2fb01e3f 100644
--- a/spec/lib/gitlab/ci/status/build/retried_spec.rb
+++ b/spec/lib/gitlab/ci/status/build/retried_spec.rb
@@ -66,7 +66,7 @@ describe Gitlab::Ci::Status::Build::Retried do
       let(:status) { Gitlab::Ci::Status::Build::Failed.new(failed_status) }
 
       it 'does override status_tooltip' do
-        expect(subject.status_tooltip).to eq 'failed <br> (unknown failure) (retried)'
+        expect(subject.status_tooltip).to eq 'failed - (unknown failure) (retried)'
       end
     end
 
diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb
index 6dfaa3b72f7..547d95e0908 100644
--- a/spec/presenters/ci/build_presenter_spec.rb
+++ b/spec/presenters/ci/build_presenter_spec.rb
@@ -78,7 +78,7 @@ describe Ci::BuildPresenter do
       it 'returns the reason of failure' do
         status_title = presenter.status_title
 
-        expect(status_title).to eq('Failed <br> (unknown failure)')
+        expect(status_title).to eq('Failed - (unknown failure)')
       end
     end
 
@@ -89,7 +89,7 @@ describe Ci::BuildPresenter do
         status_title = presenter.status_title
 
         expect(status_title).not_to include('(retried)')
-        expect(status_title).to eq('Failed <br> (unknown failure)')
+        expect(status_title).to eq('Failed - (unknown failure)')
       end
     end
 
@@ -99,7 +99,7 @@ describe Ci::BuildPresenter do
       it 'returns the reason of failure' do
         status_title = presenter.status_title
 
-        expect(status_title).to eq('Failed <br> (unknown failure)')
+        expect(status_title).to eq('Failed - (unknown failure)')
       end
     end
 
@@ -173,7 +173,7 @@ describe Ci::BuildPresenter do
       it 'returns the reason of failure' do
         tooltip = subject.tooltip_message
 
-        expect(tooltip).to eq("#{build.name} - failed <br> (script failure)")
+        expect(tooltip).to eq("#{build.name} - failed - (script failure)")
       end
     end
 
@@ -183,7 +183,7 @@ describe Ci::BuildPresenter do
       it 'should include the reason of failure and the retried title' do
         tooltip = subject.tooltip_message
 
-        expect(tooltip).to eq("#{build.name} - failed <br> (script failure) (retried)")
+        expect(tooltip).to eq("#{build.name} - failed - (script failure) (retried)")
       end
     end
 
@@ -193,7 +193,7 @@ describe Ci::BuildPresenter do
       it 'should include the reason of failure' do
         tooltip = subject.tooltip_message
 
-        expect(tooltip).to eq("#{build.name} - failed <br> (script failure) (allowed to fail)")
+        expect(tooltip).to eq("#{build.name} - failed - (script failure) (allowed to fail)")
       end
     end
 
diff --git a/spec/serializers/build_serializer_spec.rb b/spec/serializers/build_serializer_spec.rb
index 52459cd369d..302ef147eb2 100644
--- a/spec/serializers/build_serializer_spec.rb
+++ b/spec/serializers/build_serializer_spec.rb
@@ -37,7 +37,7 @@ describe BuildSerializer do
       it 'serializes only status' do
         expect(subject[:text]).to eq(status.text)
         expect(subject[:label]).to eq('failed')
-        expect(subject[:tooltip]).to eq('failed <br> (unknown failure)')
+        expect(subject[:tooltip]).to eq('failed - (unknown failure)')
         expect(subject[:icon]).to eq(status.icon)
         expect(subject[:favicon]).to match_asset_path("/assets/ci_favicons/#{status.favicon}.png")
       end
diff --git a/spec/serializers/job_entity_spec.rb b/spec/serializers/job_entity_spec.rb
index a5581a34517..8e1ca3f308d 100644
--- a/spec/serializers/job_entity_spec.rb
+++ b/spec/serializers/job_entity_spec.rb
@@ -142,7 +142,7 @@ describe JobEntity do
     end
 
     it 'should indicate the failure reason on tooltip' do
-      expect(subject[:status][:tooltip]).to eq('failed <br> (API failure)')
+      expect(subject[:status][:tooltip]).to eq('failed - (API failure)')
     end
 
     it 'should include a callout message with a verbose output' do
@@ -166,7 +166,7 @@ describe JobEntity do
     end
 
     it 'should indicate the failure reason on tooltip' do
-      expect(subject[:status][:tooltip]).to eq('failed <br> (API failure) (allowed to fail)')
+      expect(subject[:status][:tooltip]).to eq('failed - (API failure) (allowed to fail)')
     end
 
     it 'should include a callout message with a verbose output' do
-- 
cgit v1.2.1