Read about our upcoming Code of Conduct on this issue

Commit 2e2cfd19 authored by Reuben Pereira's avatar Reuben Pereira Committed by Sean McGivern
Browse files

Add table and model for error tracking settings

parent 7b4d8ab08c5f
# 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
...@@ -187,6 +187,7 @@ class Project < ActiveRecord::Base ...@@ -187,6 +187,7 @@ class Project < ActiveRecord::Base
has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project 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 :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_one :project_repository, inverse_of: :project 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 # Merge Requests for target project should be removed with it
has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project
......
...@@ -26,6 +26,7 @@ ...@@ -26,6 +26,7 @@
# - allow_local_network: Allow urls pointing to private network addresses. Default: true # - allow_local_network: Allow urls pointing to private network addresses. Default: true
# - ports: Allowed ports. Default: all. # - ports: Allowed ports. Default: all.
# - enforce_user: Validate user format. Default: false # - enforce_user: Validate user format. Default: false
# - enforce_sanitization: Validate that there are no html/css/js tags. Default: false
# #
# Example: # Example:
# class User < ActiveRecord::Base # class User < ActiveRecord::Base
...@@ -70,7 +71,8 @@ def default_options ...@@ -70,7 +71,8 @@ def default_options
allow_localhost: true, allow_localhost: true,
allow_local_network: true, allow_local_network: true,
ascii_only: false, ascii_only: false,
enforce_user: false enforce_user: false,
enforce_sanitization: false
} }
end end
......
# 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
...@@ -1573,6 +1573,13 @@ ...@@ -1573,6 +1573,13 @@
t.index ["project_id", "deploy_token_id"], name: "index_project_deploy_tokens_on_project_id_and_deploy_token_id", unique: true, using: :btree t.index ["project_id", "deploy_token_id"], name: "index_project_deploy_tokens_on_project_id_and_deploy_token_id", unique: true, using: :btree
end 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| create_table "project_features", force: :cascade do |t|
t.integer "project_id", null: false t.integer "project_id", null: false
t.integer "merge_requests_access_level" t.integer "merge_requests_access_level"
...@@ -2434,6 +2441,7 @@ ...@@ -2434,6 +2441,7 @@
add_foreign_key "project_custom_attributes", "projects", on_delete: :cascade 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", "deploy_tokens", on_delete: :cascade
add_foreign_key "project_deploy_tokens", "projects", 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_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_group_links", "projects", name: "fk_daa8cee94c", on_delete: :cascade
add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade
......
...@@ -74,6 +74,7 @@ project_tree: ...@@ -74,6 +74,7 @@ project_tree:
- :prometheus_metrics - :prometheus_metrics
- :project_badges - :project_badges
- :ci_cd_settings - :ci_cd_settings
- :error_tracking_setting
# Only include the following attributes for the models specified. # Only include the following attributes for the models specified.
included_attributes: included_attributes:
...@@ -162,6 +163,9 @@ excluded_attributes: ...@@ -162,6 +163,9 @@ excluded_attributes:
- :token_encrypted - :token_encrypted
services: services:
- :template - :template
error_tracking_setting:
- :encrypted_token
- :encrypted_token_iv
methods: methods:
labels: labels:
......
...@@ -24,6 +24,7 @@ class RelationFactory ...@@ -24,6 +24,7 @@ class RelationFactory
project_badges: 'Badge', project_badges: 'Badge',
metrics: 'MergeRequest::Metrics', metrics: 'MergeRequest::Metrics',
ci_cd_settings: 'ProjectCiCdSetting', ci_cd_settings: 'ProjectCiCdSetting',
error_tracking_setting: 'ErrorTracking::ProjectErrorTrackingSetting',
links: 'Releases::Link' }.freeze 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 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
......
...@@ -8,16 +8,18 @@ class UrlBlocker ...@@ -8,16 +8,18 @@ class UrlBlocker
BlockedUrlError = Class.new(StandardError) BlockedUrlError = Class.new(StandardError)
class << self 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? return true if url.nil?
# Param url can be a string, URI or Addressable::URI # Param url can be a string, URI or Addressable::URI
uri = parse_url(url) uri = parse_url(url)
validate_html_tags!(uri) if enforce_sanitization
# Allow imports from the GitLab instance itself but only from the configured ports # Allow imports from the GitLab instance itself but only from the configured ports
return true if internal?(uri) return true if internal?(uri)
port = uri.port || uri.default_port port = get_port(uri)
validate_protocol!(uri.scheme, protocols) validate_protocol!(uri.scheme, protocols)
validate_port!(port, ports) if ports.any? validate_port!(port, ports) if ports.any?
validate_user!(uri.user) if enforce_user validate_user!(uri.user) if enforce_user
...@@ -50,6 +52,18 @@ def blocked_url?(*args) ...@@ -50,6 +52,18 @@ def blocked_url?(*args)
private 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) def parse_url(url)
raise Addressable::URI::InvalidURIError if multiline?(url) raise Addressable::URI::InvalidURIError if multiline?(url)
......
...@@ -64,6 +64,7 @@ ...@@ -64,6 +64,7 @@
let(:indexes) { connection.indexes(table) } let(:indexes) { connection.indexes(table) }
let(:columns) { connection.columns(table) } let(:columns) { connection.columns(table) }
let(:foreign_keys) { connection.foreign_keys(table) } let(:foreign_keys) { connection.foreign_keys(table) }
let(:primary_key_column) { connection.primary_key(table) }
context 'all foreign keys' do context 'all foreign keys' do
# for index to be effective, the FK constraint has to be at first place # for index to be effective, the FK constraint has to be at first place
...@@ -71,6 +72,12 @@ ...@@ -71,6 +72,12 @@
first_indexed_column = indexes.map(&:columns).map(&:first) first_indexed_column = indexes.map(&:columns).map(&:first)
foreign_keys_columns = foreign_keys.map(&:column) 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) expect(first_indexed_column.uniq).to include(*foreign_keys_columns)
end end
end end
......
# 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
...@@ -314,6 +314,7 @@ project: ...@@ -314,6 +314,7 @@ project:
- repository_languages - repository_languages
- pool_repository - pool_repository
- kubernetes_namespaces - kubernetes_namespaces
- error_tracking_setting
award_emoji: award_emoji:
- awardable - awardable
- user - user
...@@ -345,3 +346,5 @@ resource_label_events: ...@@ -345,3 +346,5 @@ resource_label_events:
- merge_request - merge_request
- epic - epic
- label - label
error_tracking_setting:
- project
...@@ -600,3 +600,8 @@ ResourceLabelEvent: ...@@ -600,3 +600,8 @@ ResourceLabelEvent:
- label_id - label_id
- user_id - user_id
- created_at - created_at
ErrorTracking::ProjectErrorTrackingSetting:
- id
- api_url
- enabled
- project_id
# 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
...@@ -62,6 +62,7 @@ ...@@ -62,6 +62,7 @@
it { is_expected.to have_one(:last_event).class_name('Event') } 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(:forked_from_project).through(:fork_network_member) }
it { is_expected.to have_one(:auto_devops).class_name('ProjectAutoDevops') } 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(:commit_statuses) }
it { is_expected.to have_many(:ci_pipelines) } it { is_expected.to have_many(:ci_pipelines) }
it { is_expected.to have_many(:builds) } it { is_expected.to have_many(:builds) }
......
...@@ -172,4 +172,55 @@ ...@@ -172,4 +172,55 @@
end end
end 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 end
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