From f40b5860d76a8ea5d964260834a6e83516b0f1fd Mon Sep 17 00:00:00 2001 From: Reuben Pereira Date: Mon, 7 Jan 2019 17:55:21 +0000 Subject: Add table and model for error tracking settings --- .../project_error_tracking_setting.rb | 14 ++++++ app/models/project.rb | 1 + app/validators/url_validator.rb | 4 +- ...0181212171634_create_error_tracking_settings.rb | 17 ++++++++ db/schema.rb | 8 ++++ lib/gitlab/import_export/import_export.yml | 4 ++ lib/gitlab/import_export/relation_factory.rb | 1 + lib/gitlab/url_blocker.rb | 18 +++++++- spec/db/schema_spec.rb | 7 +++ spec/factories/project_error_tracking_settings.rb | 10 +++++ spec/lib/gitlab/import_export/all_models.yml | 3 ++ .../gitlab/import_export/safe_model_attributes.yml | 5 +++ .../project_error_tracking_setting_spec.rb | 36 +++++++++++++++ spec/models/project_spec.rb | 1 + spec/validators/url_validator_spec.rb | 51 ++++++++++++++++++++++ 15 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 app/models/error_tracking/project_error_tracking_setting.rb create mode 100644 db/migrate/20181212171634_create_error_tracking_settings.rb create mode 100644 spec/factories/project_error_tracking_settings.rb create mode 100644 spec/models/error_tracking/project_error_tracking_setting_spec.rb diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb new file mode 100644 index 00000000000..632c64c2f1c --- /dev/null +++ b/app/models/error_tracking/project_error_tracking_setting.rb @@ -0,0 +1,14 @@ +# 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 diff --git a/app/models/project.rb b/app/models/project.rb index e2f010a0432..9d0348d848a 100644 --- a/app/models/project.rb +++ b/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 diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb index 5feb0b0f05b..d3e32818dc7 100644 --- a/app/validators/url_validator.rb +++ b/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 diff --git a/db/migrate/20181212171634_create_error_tracking_settings.rb b/db/migrate/20181212171634_create_error_tracking_settings.rb new file mode 100644 index 00000000000..18c38bd2c47 --- /dev/null +++ b/db/migrate/20181212171634_create_error_tracking_settings.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 12e4ed6d627..87826881d58 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1573,6 +1573,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" @@ -2434,6 +2441,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 diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index a1a374cef4a..7987533978c 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -74,6 +74,7 @@ project_tree: - :prometheus_metrics - :project_badges - :ci_cd_settings + - :error_tracking_setting # Only include the following attributes for the models specified. included_attributes: @@ -162,6 +163,9 @@ excluded_attributes: - :token_encrypted services: - :template + error_tracking_setting: + - :encrypted_token + - :encrypted_token_iv methods: labels: diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index bce12103cce..099b488f68e 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -24,6 +24,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 diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 44c71f8431d..9b7b0db9525 100644 --- a/lib/gitlab/url_blocker.rb +++ b/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) diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 7c505ee0d43..897b4411055 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -64,6 +64,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 @@ -71,6 +72,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 diff --git a/spec/factories/project_error_tracking_settings.rb b/spec/factories/project_error_tracking_settings.rb new file mode 100644 index 00000000000..f044cbe8755 --- /dev/null +++ b/spec/factories/project_error_tracking_settings.rb @@ -0,0 +1,10 @@ +# 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 diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index d3cae137c3c..5afa9669b1a 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -314,6 +314,7 @@ project: - repository_languages - pool_repository - kubernetes_namespaces +- error_tracking_setting award_emoji: - awardable - user @@ -345,3 +346,5 @@ resource_label_events: - merge_request - epic - label +error_tracking_setting: +- project diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 2422868474e..fe2087e8fc3 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -600,3 +600,8 @@ ResourceLabelEvent: - label_id - user_id - created_at +ErrorTracking::ProjectErrorTrackingSetting: +- id +- api_url +- enabled +- project_id diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb new file mode 100644 index 00000000000..83f29718eda --- /dev/null +++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb @@ -0,0 +1,36 @@ +# 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/'>" + + expect(project_error_tracking_setting).not_to be_valid + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 65b59c7b21b..5e7345ca180 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -62,6 +62,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) } diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb index f3f3386382f..1bb42382e8a 100644 --- a/spec/validators/url_validator_spec.rb +++ b/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/'>" } + let(:safe_url) { 'https://replaceme.com/path/to/somewhere' } + + let(:unsafe_internal_url) do + Gitlab.config.gitlab.protocol + '://' + Gitlab.config.gitlab.host + + "/'>" + 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 -- cgit v1.2.1