Commit 5e8f11e5 authored by Filipa Lacerda's avatar Filipa Lacerda

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
parent 68082d35
<script> <script>
import $ from 'jquery'; import $ from 'jquery';
import _ from 'underscore';
import JobNameComponent from './job_name_component.vue'; import JobNameComponent from './job_name_component.vue';
import JobComponent from './job_component.vue'; import JobComponent from './job_component.vue';
import tooltip from '../../../vue_shared/directives/tooltip'; import tooltip from '../../../vue_shared/directives/tooltip';
...@@ -47,7 +46,7 @@ export default { ...@@ -47,7 +46,7 @@ export default {
computed: { computed: {
tooltipText() { tooltipText() {
return _.escape(`${this.job.name} - ${this.job.status.label}`); return `${this.job.name} - ${this.job.status.label}`;
}, },
}, },
......
<script> <script>
import _ from 'underscore';
import ActionComponent from './action_component.vue'; import ActionComponent from './action_component.vue';
import JobNameComponent from './job_name_component.vue'; import JobNameComponent from './job_name_component.vue';
import tooltip from '../../../vue_shared/directives/tooltip'; import tooltip from '../../../vue_shared/directives/tooltip';
...@@ -62,7 +61,7 @@ export default { ...@@ -62,7 +61,7 @@ export default {
const textBuilder = []; const textBuilder = [];
if (this.job.name) { if (this.job.name) {
textBuilder.push(_.escape(this.job.name)); textBuilder.push(this.job.name);
} }
if (this.job.name && this.status.tooltip) { if (this.job.name && this.status.tooltip) {
...@@ -106,7 +105,6 @@ export default { ...@@ -106,7 +105,6 @@ export default {
:class="cssClassJobName" :class="cssClassJobName"
:data-boundary="tooltipBoundary" :data-boundary="tooltipBoundary"
data-container="body" data-container="body"
data-html="true"
class="js-pipeline-graph-job-link" class="js-pipeline-graph-job-link"
> >
...@@ -122,7 +120,6 @@ export default { ...@@ -122,7 +120,6 @@ export default {
:title="tooltipText" :title="tooltipText"
:class="cssClassJobName" :class="cssClassJobName"
class="js-job-component-tooltip non-details-job-component" class="js-job-component-tooltip non-details-job-component"
data-html="true"
data-container="body" data-container="body"
> >
......
...@@ -6,12 +6,12 @@ ...@@ -6,12 +6,12 @@
- tooltip = "#{subject.name} - #{status.status_tooltip}" - tooltip = "#{subject.name} - #{status.status_tooltip}"
- if status.has_details? - 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{ class: klass }= sprite_icon(status.icon)
%span.ci-build-text= subject.name %span.ci-build-text= subject.name
- else - 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{ class: klass }= sprite_icon(status.icon)
%span.ci-build-text= subject.name %span.ci-build-text= subject.name
......
...@@ -87,7 +87,7 @@ ...@@ -87,7 +87,7 @@
- builds.select{|build| build.status == build_status}.each do |build| - builds.select{|build| build.status == build_status}.each do |build|
.build-job{ class: sidebar_build_class(build, @build), data: { stage: build.stage } } .build-job{ class: sidebar_build_class(build, @build), data: { stage: build.stage } }
- tooltip = sanitize(build.tooltip_message.dup) - 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') = sprite_icon('arrow-right', size:16, css_class: 'icon-arrow-right')
%span{ class: "ci-status-icon-#{build.status}" } %span{ class: "ci-status-icon-#{build.status}" }
= ci_icon_for_status(build.status) = ci_icon_for_status(build.status)
......
...@@ -32,7 +32,7 @@ module Gitlab ...@@ -32,7 +32,7 @@ module Gitlab
end end
def description def description
"<br> (#{failure_reason_message})" "- (#{failure_reason_message})"
end end
def failure_reason_message def failure_reason_message
......
...@@ -40,7 +40,7 @@ describe 'User browses a job', :js do ...@@ -40,7 +40,7 @@ describe 'User browses a job', :js do
it 'displays the failure reason' do it 'displays the failure reason' do
within('.builds-container') do within('.builds-container') do
build_link = first('.build-job > a') 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 end
end end
...@@ -51,7 +51,7 @@ describe 'User browses a job', :js do ...@@ -51,7 +51,7 @@ describe 'User browses a job', :js do
it 'displays the failure reason and retried label' do it 'displays the failure reason and retried label' do
within('.builds-container') do within('.builds-container') do
build_link = first('.build-job > a') 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 end
end end
......
...@@ -36,7 +36,7 @@ describe 'User browses jobs' do ...@@ -36,7 +36,7 @@ describe 'User browses jobs' do
it 'displays a tooltip with the failure reason' do it 'displays a tooltip with the failure reason' do
page.within('.ci-table') do page.within('.ci-table') do
failed_job_link = page.find('.ci-failed') 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 end
end end
......
...@@ -126,7 +126,7 @@ describe 'Pipeline', :js do ...@@ -126,7 +126,7 @@ describe 'Pipeline', :js do
it 'should include the failure reason' do it 'should include the failure reason' do
page.within('#ci-badge-test') do page.within('#ci-badge-test') do
build_link = page.find('.js-pipeline-graph-job-link') 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 end
end end
...@@ -319,7 +319,7 @@ describe 'Pipeline', :js do ...@@ -319,7 +319,7 @@ describe 'Pipeline', :js do
it 'displays a tooltip with the failure reason' do it 'displays a tooltip with the failure reason' do
page.within('.ci-table') do page.within('.ci-table') do
failed_job_link = page.find('.ci-failed') 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 end
end end
......
...@@ -406,7 +406,7 @@ describe 'Pipelines', :js do ...@@ -406,7 +406,7 @@ describe 'Pipelines', :js do
within('.js-builds-dropdown-list') do within('.js-builds-dropdown-list') do
build_element = page.find('.mini-pipeline-graph-dropdown-item') 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 end
end end
......
...@@ -82,12 +82,4 @@ describe('dropdown job component', () => { ...@@ -82,12 +82,4 @@ describe('dropdown job component', () => {
it('renders dropdown with jobs', () => { it('renders dropdown with jobs', () => {
expect(vm.$el.querySelectorAll('.scrollable-menu>ul>li').length).toEqual(mock.jobs.length); 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',
);
});
}); });
...@@ -161,24 +161,4 @@ describe('pipeline graph job component', () => { ...@@ -161,24 +161,4 @@ describe('pipeline graph job component', () => {
expect(component.$el.querySelector(tooltipBoundary)).toBeNull(); 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');
});
});
}); });
...@@ -88,7 +88,7 @@ describe Gitlab::Ci::Status::Build::Factory do ...@@ -88,7 +88,7 @@ describe Gitlab::Ci::Status::Build::Factory do
expect(status.icon).to eq 'status_failed' expect(status.icon).to eq 'status_failed'
expect(status.favicon).to eq 'favicon_status_failed' expect(status.favicon).to eq 'favicon_status_failed'
expect(status.label).to eq '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_details
expect(status).to have_action expect(status).to have_action
end end
......
...@@ -76,7 +76,7 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do ...@@ -76,7 +76,7 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do
let(:status) { described_class.new(build_status) } let(:status) { described_class.new(build_status) }
it 'does override badge_tooltip' do 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
end end
...@@ -87,7 +87,7 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do ...@@ -87,7 +87,7 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do
let(:status) { described_class.new(build_status) } let(:status) { described_class.new(build_status) }
it 'does override status_tooltip' do 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
end end
......
...@@ -52,7 +52,7 @@ describe Gitlab::Ci::Status::Build::Failed do ...@@ -52,7 +52,7 @@ describe Gitlab::Ci::Status::Build::Failed do
let(:status) { Gitlab::Ci::Status::Failed.new(build, user) } let(:status) { Gitlab::Ci::Status::Failed.new(build, user) }
it 'does override badge_tooltip' do 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
end end
...@@ -61,7 +61,7 @@ describe Gitlab::Ci::Status::Build::Failed do ...@@ -61,7 +61,7 @@ describe Gitlab::Ci::Status::Build::Failed do
let(:status) { Gitlab::Ci::Status::Failed.new(build, user) } let(:status) { Gitlab::Ci::Status::Failed.new(build, user) }
it 'does override status_tooltip' do 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
end end
......
...@@ -66,7 +66,7 @@ describe Gitlab::Ci::Status::Build::Retried do ...@@ -66,7 +66,7 @@ describe Gitlab::Ci::Status::Build::Retried do
let(:status) { Gitlab::Ci::Status::Build::Failed.new(failed_status) } let(:status) { Gitlab::Ci::Status::Build::Failed.new(failed_status) }
it 'does override status_tooltip' do 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
end end
......
...@@ -78,7 +78,7 @@ describe Ci::BuildPresenter do ...@@ -78,7 +78,7 @@ describe Ci::BuildPresenter do
it 'returns the reason of failure' do it 'returns the reason of failure' do
status_title = presenter.status_title status_title = presenter.status_title
expect(status_title).to eq('Failed <br> (unknown failure)') expect(status_title).to eq('Failed - (unknown failure)')
end end
end end
...@@ -89,7 +89,7 @@ describe Ci::BuildPresenter do ...@@ -89,7 +89,7 @@ describe Ci::BuildPresenter do
status_title = presenter.status_title status_title = presenter.status_title
expect(status_title).not_to include('(retried)') 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
end end
...@@ -99,7 +99,7 @@ describe Ci::BuildPresenter do ...@@ -99,7 +99,7 @@ describe Ci::BuildPresenter do
it 'returns the reason of failure' do it 'returns the reason of failure' do
status_title = presenter.status_title status_title = presenter.status_title
expect(status_title).to eq('Failed <br> (unknown failure)') expect(status_title).to eq('Failed - (unknown failure)')
end end
end end
...@@ -173,7 +173,7 @@ describe Ci::BuildPresenter do ...@@ -173,7 +173,7 @@ describe Ci::BuildPresenter do
it 'returns the reason of failure' do it 'returns the reason of failure' do
tooltip = subject.tooltip_message 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
end end
...@@ -183,7 +183,7 @@ describe Ci::BuildPresenter do ...@@ -183,7 +183,7 @@ describe Ci::BuildPresenter do
it 'should include the reason of failure and the retried title' do it 'should include the reason of failure and the retried title' do
tooltip = subject.tooltip_message 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
end end
...@@ -193,7 +193,7 @@ describe Ci::BuildPresenter do ...@@ -193,7 +193,7 @@ describe Ci::BuildPresenter do
it 'should include the reason of failure' do it 'should include the reason of failure' do
tooltip = subject.tooltip_message 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
end end
......
...@@ -37,7 +37,7 @@ describe BuildSerializer do ...@@ -37,7 +37,7 @@ describe BuildSerializer do
it 'serializes only status' do it 'serializes only status' do
expect(subject[:text]).to eq(status.text) expect(subject[:text]).to eq(status.text)
expect(subject[:label]).to eq('failed') 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[:icon]).to eq(status.icon)
expect(subject[:favicon]).to match_asset_path("/assets/ci_favicons/#{status.favicon}.png") expect(subject[:favicon]).to match_asset_path("/assets/ci_favicons/#{status.favicon}.png")
end end
......
...@@ -142,7 +142,7 @@ describe JobEntity do ...@@ -142,7 +142,7 @@ describe JobEntity do
end end
it 'should indicate the failure reason on tooltip' do 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 end
it 'should include a callout message with a verbose output' do it 'should include a callout message with a verbose output' do
...@@ -166,7 +166,7 @@ describe JobEntity do ...@@ -166,7 +166,7 @@ describe JobEntity do
end end
it 'should indicate the failure reason on tooltip' do 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 end
it 'should include a callout message with a verbose output' do it 'should include a callout message with a verbose output' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment