Commit 6d0c6141 authored by Cindy Pallares's avatar Cindy Pallares

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

[master] Stored XSS in Operation Page

See merge request gitlab/gitlab-ee!722
parents 6743ffd7 5cca659d
......@@ -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"
......
......@@ -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
......@@ -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
......
......@@ -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
......
---
title: Sanitize tracing external_urls before saving to DB and when displaying the URL to prevent XSS issues
merge_request:
author:
type: security
# 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
FactoryBot.define do
factory :project_tracing_setting do
project
external_url 'https://example.com'
end
end
# 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
......@@ -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
......@@ -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
......
# 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
......@@ -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."
......
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