Automatic merge of gitlab-org/gitlab-ce master

app/models/error_tracking/project_error_tracking_setting.rb

+# frozen_string_literal: true
+
+module ErrorTracking
+  class ProjectErrorTrackingSetting < ActiveRecord::Base
+    belongs_to :project
+
+    validates :api_url, length: { maximum: 255 }, public_url: true, url: { enforce_sanitization: true }
+
+    attr_encrypted :token,
+      mode: :per_attribute_iv,
+      key: Settings.attr_encrypted_db_key_base_truncated,
+      algorithm: 'aes-256-gcm'
+  end
+end

app/models/project.rb

@@ -187,6 +187,7 @@ class Project < ActiveRecord::Base
   has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project
   has_one :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_one :project_repository, inverse_of: :project
+  has_one :error_tracking_setting, inverse_of: :project, class_name: 'ErrorTracking::ProjectErrorTrackingSetting'
 
   # Merge Requests for target project should be removed with it
   has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project

app/validators/url_validator.rb

@@ -26,6 +26,7 @@
 # - allow_local_network: Allow urls pointing to private network addresses. Default: true
 # - ports: Allowed ports. Default: all.
 # - enforce_user: Validate user format. Default: false
+# - enforce_sanitization: Validate that there are no html/css/js tags. Default: false
 #
 # Example:
 #   class User < ActiveRecord::Base
@@ -70,7 +71,8 @@ class UrlValidator < ActiveModel::EachValidator
       allow_localhost: true,
       allow_local_network: true,
       ascii_only: false,
-      enforce_user: false
+      enforce_user: false,
+      enforce_sanitization: false
     }
   end
 

db/migrate/20181212171634_create_error_tracking_settings.rb

+# frozen_string_literal: true
+
+class CreateErrorTrackingSettings < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :project_error_tracking_settings, id: :int, primary_key: :project_id, default: nil do |t|
+      t.boolean :enabled, null: false, default: true
+      t.string :api_url, null: false
+      t.string :encrypted_token
+      t.string :encrypted_token_iv
+      t.foreign_key :projects, column: :project_id, on_delete: :cascade
+    end
+  end
+end

db/schema.rb

@@ -2199,6 +2199,13 @@ ActiveRecord::Schema.define(version: 20190103140724) do
     t.index ["project_id", "deploy_token_id"], name: "index_project_deploy_tokens_on_project_id_and_deploy_token_id", unique: true, using: :btree
   end
 
+  create_table "project_error_tracking_settings", primary_key: "project_id", id: :integer, force: :cascade do |t|
+    t.boolean "enabled", default: true, null: false
+    t.string "api_url", null: false
+    t.string "encrypted_token"
+    t.string "encrypted_token_iv"
+  end
+
   create_table "project_features", force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "merge_requests_access_level"
@@ -3439,6 +3446,7 @@ ActiveRecord::Schema.define(version: 20190103140724) do
   add_foreign_key "project_custom_attributes", "projects", on_delete: :cascade
   add_foreign_key "project_deploy_tokens", "deploy_tokens", on_delete: :cascade
   add_foreign_key "project_deploy_tokens", "projects", on_delete: :cascade
+  add_foreign_key "project_error_tracking_settings", "projects", on_delete: :cascade
   add_foreign_key "project_features", "projects", name: "fk_18513d9b92", on_delete: :cascade
   add_foreign_key "project_group_links", "projects", name: "fk_daa8cee94c", on_delete: :cascade
   add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade

lib/gitlab/import_export/import_export.yml

@@ -77,6 +77,7 @@ project_tree:
   - :prometheus_metrics
   - :project_badges
   - :ci_cd_settings
+  - :error_tracking_setting
 
 # Only include the following attributes for the models specified.
 included_attributes:
@@ -175,6 +176,9 @@ excluded_attributes:
     - :token_encrypted
   services:
     - :template
+  error_tracking_setting:
+    - :encrypted_token
+    - :encrypted_token_iv
 
 methods:
   labels:

lib/gitlab/import_export/relation_factory.rb

@@ -26,6 +26,7 @@ module Gitlab
                     project_badges: 'Badge',
                     metrics: 'MergeRequest::Metrics',
                     ci_cd_settings: 'ProjectCiCdSetting',
+                    error_tracking_setting: 'ErrorTracking::ProjectErrorTrackingSetting',
                     links: 'Releases::Link' }.freeze
 
       USER_REFERENCES = %w[author_id assignee_id updated_by_id merged_by_id latest_closed_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id closed_by_id].freeze

lib/gitlab/url_blocker.rb

@@ -8,16 +8,18 @@ module Gitlab
     BlockedUrlError = Class.new(StandardError)
 
     class << self
-      def validate!(url, ports: [], protocols: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false)
+      def validate!(url, ports: [], protocols: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false)
         return true if url.nil?
 
         # Param url can be a string, URI or Addressable::URI
         uri = parse_url(url)
 
+        validate_html_tags!(uri) if enforce_sanitization
+
         # Allow imports from the GitLab instance itself but only from the configured ports
         return true if internal?(uri)
 
-        port = uri.port || uri.default_port
+        port = get_port(uri)
         validate_protocol!(uri.scheme, protocols)
         validate_port!(port, ports) if ports.any?
         validate_user!(uri.user) if enforce_user
@@ -50,6 +52,18 @@ module Gitlab
 
       private
 
+      def get_port(uri)
+        uri.port || uri.default_port
+      end
+
+      def validate_html_tags!(uri)
+        uri_str = uri.to_s
+        sanitized_uri = ActionController::Base.helpers.sanitize(uri_str, tags: [])
+        if sanitized_uri != uri_str
+          raise BlockedUrlError, 'HTML/CSS/JS tags are not allowed'
+        end
+      end
+
       def parse_url(url)
         raise Addressable::URI::InvalidURIError if multiline?(url)
 

spec/db/schema_spec.rb

@@ -68,6 +68,7 @@ describe 'Database schema' do
         let(:indexes) { connection.indexes(table) }
         let(:columns) { connection.columns(table) }
         let(:foreign_keys) { connection.foreign_keys(table) }
+        let(:primary_key_column) { connection.primary_key(table) }
 
         context 'all foreign keys' do
           # for index to be effective, the FK constraint has to be at first place
@@ -75,6 +76,12 @@ describe 'Database schema' do
             first_indexed_column = indexes.map(&:columns).map(&:first)
             foreign_keys_columns = foreign_keys.map(&:column)
 
+            # Add the primary key column to the list of indexed columns because
+            # postgres and mysql both automatically create an index on the primary
+            # key. Also, the rails connection.indexes() method does not return
+            # automatically generated indexes (like the primary key index).
+            first_indexed_column = first_indexed_column.push(primary_key_column)
+
             expect(first_indexed_column.uniq).to include(*foreign_keys_columns)
           end
         end

spec/factories/project_error_tracking_settings.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :project_error_tracking_setting, class: ErrorTracking::ProjectErrorTrackingSetting do
+    project
+    api_url 'https://gitlab.com'
+    enabled true
+    token 'access_token_123'
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -313,6 +313,7 @@ project:
 - repository_languages
 - pool_repository
 - kubernetes_namespaces
+- error_tracking_setting
 award_emoji:
 - awardable
 - user
@@ -342,3 +343,5 @@ resource_label_events:
 - merge_request
 - epic
 - label
+error_tracking_setting:
+- project

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -648,3 +648,8 @@ ResourceLabelEvent:
 - label_id
 - user_id
 - created_at
+ErrorTracking::ProjectErrorTrackingSetting:
+- id
+- api_url
+- enabled
+- project_id

spec/models/error_tracking/project_error_tracking_setting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ErrorTracking::ProjectErrorTrackingSetting do
+  set(:project) { create(:project) }
+
+  describe 'Associations' do
+    it { is_expected.to belong_to(:project) }
+  end
+
+  describe 'Validations' do
+    subject { create(:project_error_tracking_setting, project: project) }
+
+    context 'when api_url is over 255 chars' do
+      before do
+        subject.api_url = 'https://' + 'a' * 250
+      end
+
+      it 'fails validation' do
+        expect(subject).not_to be_valid
+        expect(subject.errors.messages[:api_url]).to include('is too long (maximum is 255 characters)')
+      end
+    end
+
+    context 'With unsafe url' do
+      let(:project_error_tracking_setting) { create(:project_error_tracking_setting, project: project) }
+
+      it 'fails validation' do
+        project_error_tracking_setting.api_url = "https://replaceme.com/'><script>alert(document.cookie)</script>"
+
+        expect(project_error_tracking_setting).not_to be_valid
+      end
+    end
+  end
+end

spec/models/project_spec.rb

@@ -63,6 +63,7 @@ describe Project do
     it { is_expected.to have_one(:last_event).class_name('Event') }
     it { is_expected.to have_one(:forked_from_project).through(:fork_network_member) }
     it { is_expected.to have_one(:auto_devops).class_name('ProjectAutoDevops') }
+    it { is_expected.to have_one(:error_tracking_setting).class_name('ErrorTracking::ProjectErrorTrackingSetting') }
     it { is_expected.to have_many(:commit_statuses) }
     it { is_expected.to have_many(:ci_pipelines) }
     it { is_expected.to have_many(:builds) }

spec/validators/url_validator_spec.rb

@@ -172,4 +172,55 @@ describe UrlValidator do
       end
     end
   end
+
+  context 'when enforce_sanitization is' do
+    let(:validator) { described_class.new(attributes: [:link_url], enforce_sanitization: enforce_sanitization) }
+    let(:unsafe_url) { "https://replaceme.com/'><script>alert(document.cookie)</script>" }
+    let(:safe_url) { 'https://replaceme.com/path/to/somewhere' }
+
+    let(:unsafe_internal_url) do
+      Gitlab.config.gitlab.protocol + '://' + Gitlab.config.gitlab.host +
+        "/'><script>alert(document.cookie)</script>"
+    end
+
+    context 'true' do
+      let(:enforce_sanitization) { true }
+
+      it 'prevents unsafe urls' do
+        badge.link_url = unsafe_url
+
+        subject
+
+        expect(badge.errors.empty?).to be false
+      end
+
+      it 'prevents unsafe internal urls' do
+        badge.link_url = unsafe_internal_url
+
+        subject
+
+        expect(badge.errors.empty?).to be false
+      end
+
+      it 'allows safe urls' do
+        badge.link_url = safe_url
+
+        subject
+
+        expect(badge.errors.empty?).to be true
+      end
+    end
+
+    context 'false' do
+      let(:enforce_sanitization) { false }
+
+      it 'allows unsafe urls' do
+        badge.link_url = unsafe_url
+
+        subject
+
+        expect(badge.errors.empty?).to be true
+      end
+    end
+  end
 end