Commit ec40b369 authored by Filipa Lacerda's avatar Filipa Lacerda Committed by Jose Vargas

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 e271b302
<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}`;
},
},
......
<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"
>
......
......@@ -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
......
......@@ -82,7 +82,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)
......
......@@ -32,7 +32,7 @@ module Gitlab
end
def description
"<br> (#{failure_reason_message})"
"- (#{failure_reason_message})"
end
def failure_reason_message
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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',
);
});
});
......@@ -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');
});
});
});
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
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