From 66f47187da83f122b48b21ff1a8096e0d9f9e7fd Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 9 Jan 2023 10:41:08 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-7-stable-ee --- app/controllers/concerns/page_limiter.rb | 2 +- app/helpers/submodule_helper.rb | 78 ++++++++++++---------- app/models/active_session.rb | 2 +- app/models/hooks/web_hook.rb | 5 ++ .../error_tracking/list_projects_service.rb | 16 ++++- lib/gitlab/safe_device_detector.rb | 16 +++++ spec/helpers/submodule_helper_spec.rb | 2 +- spec/lib/gitlab/safe_device_detector_spec.rb | 20 ++++++ spec/models/hooks/web_hook_spec.rb | 28 +++++++- .../error_tracking/list_projects_service_spec.rb | 30 +++++++-- spec/services/web_hook_service_spec.rb | 5 +- 11 files changed, 156 insertions(+), 48 deletions(-) create mode 100644 lib/gitlab/safe_device_detector.rb create mode 100644 spec/lib/gitlab/safe_device_detector_spec.rb diff --git a/app/controllers/concerns/page_limiter.rb b/app/controllers/concerns/page_limiter.rb index 1d044a41899..97df540d31b 100644 --- a/app/controllers/concerns/page_limiter.rb +++ b/app/controllers/concerns/page_limiter.rb @@ -58,7 +58,7 @@ module PageLimiter # Record the page limit being hit in Prometheus def record_page_limit_interception - dd = DeviceDetector.new(request.user_agent) + dd = Gitlab::SafeDeviceDetector.new(request.user_agent) Gitlab::Metrics.counter(:gitlab_page_out_of_bounds, controller: params[:controller], diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb index e3e2f423da3..c38d69df8e4 100644 --- a/app/helpers/submodule_helper.rb +++ b/app/helpers/submodule_helper.rb @@ -17,40 +17,26 @@ module SubmoduleHelper url = File.join(Gitlab.config.gitlab.url, repository.project.full_path) end - if url =~ %r{([^/:]+)/([^/]+(?:\.git)?)\Z} - namespace = Regexp.last_match(1) - project = Regexp.last_match(2) - gitlab_hosts = [Gitlab.config.gitlab.url, - Gitlab.config.gitlab_shell.ssh_path_prefix] - - gitlab_hosts.each do |host| - if url.start_with?(host) - namespace, _, project = url.sub(host, '').rpartition('/') - break - end - end - - namespace.delete_prefix!('/') - project.rstrip! - project.delete_suffix!('.git') - - if self_url?(url, namespace, project) - [ - url_helpers.namespace_project_path(namespace, project), - url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id), - (url_helpers.namespace_project_compare_path(namespace, project, to: submodule_item_id, from: old_submodule_item_id) if old_submodule_item_id) - ] - elsif relative_self_url?(url) - relative_self_links(url, submodule_item_id, old_submodule_item_id, repository.project) - elsif gist_github_dot_com_url?(url) - gist_github_com_tree_links(namespace, project, submodule_item_id) - elsif github_dot_com_url?(url) - github_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) - elsif gitlab_dot_com_url?(url) - gitlab_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) - else - [sanitize_submodule_url(url), nil, nil] - end + namespace, project = extract_namespace_project(url) + + if namespace.blank? || project.blank? + return [sanitize_submodule_url(url), nil, nil] + end + + if self_url?(url, namespace, project) + [ + url_helpers.namespace_project_path(namespace, project), + url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id), + (url_helpers.namespace_project_compare_path(namespace, project, to: submodule_item_id, from: old_submodule_item_id) if old_submodule_item_id) + ] + elsif relative_self_url?(url) + relative_self_links(url, submodule_item_id, old_submodule_item_id, repository.project) + elsif gist_github_dot_com_url?(url) + gist_github_com_tree_links(namespace, project, submodule_item_id) + elsif github_dot_com_url?(url) + github_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) + elsif gitlab_dot_com_url?(url) + gitlab_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) else [sanitize_submodule_url(url), nil, nil] end @@ -58,6 +44,30 @@ module SubmoduleHelper protected + def extract_namespace_project(url) + namespace_fragment, _, project = url.rpartition('/') + namespace = namespace_fragment.rpartition(%r{[:/]}).last + + return [nil, nil] unless project.present? && namespace.present? + + gitlab_hosts = [Gitlab.config.gitlab.url, + Gitlab.config.gitlab_shell.ssh_path_prefix] + + matching_host = gitlab_hosts.find do |host| + url.start_with?(host) + end + + if matching_host + namespace, _, project = url.delete_prefix(matching_host).rpartition('/') + end + + namespace.delete_prefix!('/') + project.rstrip! + project.delete_suffix!('.git') + + [namespace, project] + end + def gist_github_dot_com_url?(url) url =~ %r{gist\.github\.com[/:][^/]+/[^/]+\Z} end diff --git a/app/models/active_session.rb b/app/models/active_session.rb index b16c4a2b353..2d1dec1977d 100644 --- a/app/models/active_session.rb +++ b/app/models/active_session.rb @@ -67,7 +67,7 @@ class ActiveSession def self.set(user, request) Gitlab::Redis::Sessions.with do |redis| session_private_id = request.session.id.private_id - client = DeviceDetector.new(request.user_agent) + client = Gitlab::SafeDeviceDetector.new(request.user_agent) timestamp = Time.current expiry = Settings.gitlab['session_expire_delay'] * 60 diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 189291a38ec..49418cda3ac 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -41,6 +41,7 @@ class WebHook < ApplicationRecord after_initialize :initialize_url_variables before_validation :reset_token + before_validation :reset_url_variables, unless: ->(hook) { hook.is_a?(ServiceHook) } before_validation :set_branch_filter_nil, if: :branch_filter_strategy_all_branches? validates :push_events_branch_filter, untrusted_regexp: true, if: :branch_filter_strategy_regex? validates :push_events_branch_filter, "web_hooks/wildcard_branch_filter": true, if: :branch_filter_strategy_wildcard? @@ -213,6 +214,10 @@ class WebHook < ApplicationRecord self.token = nil if url_changed? && !encrypted_token_changed? end + def reset_url_variables + self.url_variables = {} if url_changed? && !encrypted_url_variables_changed? + end + def next_failure_count recent_failures.succ.clamp(1, MAX_FAILURES) end diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb index 2f23d47029c..d52306ef805 100644 --- a/app/services/error_tracking/list_projects_service.rb +++ b/app/services/error_tracking/list_projects_service.rb @@ -2,6 +2,8 @@ module ErrorTracking class ListProjectsService < ErrorTracking::BaseService + MASKED_TOKEN_REGEX = /\A\*+\z/.freeze + private def perform @@ -20,23 +22,31 @@ module ErrorTracking def project_error_tracking_setting (super || project.build_error_tracking_setting).tap do |setting| + url_changed = !setting.api_url&.start_with?(params[:api_host]) + setting.api_url = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from( api_host: params[:api_host], organization_slug: 'org', project_slug: 'proj' ) - setting.token = token(setting) + setting.token = token(setting, url_changed) setting.enabled = true end end strong_memoize_attr :project_error_tracking_setting - def token(setting) + def token(setting, url_changed) + return if url_changed && masked_token? + # Use param token if not masked, otherwise use database token - return params[:token] unless /\A\*+\z/.match?(params[:token]) + return params[:token] unless masked_token? setting.token end + + def masked_token? + MASKED_TOKEN_REGEX.match?(params[:token]) + end end end diff --git a/lib/gitlab/safe_device_detector.rb b/lib/gitlab/safe_device_detector.rb new file mode 100644 index 00000000000..ba1010ae5e4 --- /dev/null +++ b/lib/gitlab/safe_device_detector.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +# rubocop:disable Gitlab/NamespacedClass +require 'device_detector' + +module Gitlab + class SafeDeviceDetector < ::DeviceDetector + USER_AGENT_MAX_SIZE = 1024 + + def initialize(user_agent) + super(user_agent) + @user_agent = user_agent && user_agent[0..USER_AGENT_MAX_SIZE] + end + end +end + +# rubocop:enable Gitlab/NamespacedClass diff --git a/spec/helpers/submodule_helper_spec.rb b/spec/helpers/submodule_helper_spec.rb index a419b6b9c84..2e8304e8b49 100644 --- a/spec/helpers/submodule_helper_spec.rb +++ b/spec/helpers/submodule_helper_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe SubmoduleHelper do +RSpec.describe SubmoduleHelper, feature_category: :source_code_management do include RepoHelpers let(:submodule_item) { double(id: 'hash', path: 'rack') } diff --git a/spec/lib/gitlab/safe_device_detector_spec.rb b/spec/lib/gitlab/safe_device_detector_spec.rb new file mode 100644 index 00000000000..c37dc1e1c7e --- /dev/null +++ b/spec/lib/gitlab/safe_device_detector_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'device_detector' +require_relative '../../../lib/gitlab/safe_device_detector' + +RSpec.describe Gitlab::SafeDeviceDetector, feature_category: :authentication_and_authorization do + it 'retains the behavior for normal user agents' do + chrome_user_agent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 \ + (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36" + + expect(described_class.new(chrome_user_agent).user_agent).to be_eql(chrome_user_agent) + expect(described_class.new(chrome_user_agent).name).to be_eql('Chrome') + end + + it 'truncates big user agents' do + big_user_agent = "chrome #{'abc' * 1024}" + expect(described_class.new(big_user_agent).user_agent).not_to be_eql(big_user_agent) + end +end diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 994d5688808..75ff917c036 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe WebHook do +RSpec.describe WebHook, feature_category: :integrations do include AfterNextHelpers let_it_be(:project) { create(:project) } @@ -225,6 +225,32 @@ RSpec.describe WebHook do end end + describe 'before_validation :reset_url_variables' do + subject(:hook) { build_stubbed(:project_hook, :url_variables, project: project, url: 'http://example.com/{abc}') } + + it 'resets url variables if url changed' do + hook.url = 'http://example.com/new-hook' + + expect(hook).to be_valid + expect(hook.url_variables).to eq({}) + end + + it 'resets url variables if url is changed but url variables stayed the same' do + hook.url = 'http://test.example.com/{abc}' + + expect(hook).not_to be_valid + expect(hook.url_variables).to eq({}) + end + + it 'does not reset url variables if both url and url variables are changed' do + hook.url = 'http://example.com/{one}/{two}' + hook.url_variables = { 'one' => 'foo', 'two' => 'bar' } + + expect(hook).to be_valid + expect(hook.url_variables).to eq({ 'one' => 'foo', 'two' => 'bar' }) + end + end + it "only consider these branch filter strategies are valid" do expected_valid_types = %w[all_branches regex wildcard] expect(described_class.branch_filter_strategies.keys).to contain_exactly(*expected_valid_types) diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index ce391bd1ca0..8408adcc21d 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::ListProjectsService do +RSpec.describe ErrorTracking::ListProjectsService, feature_category: :integrations do let_it_be(:user) { create(:user) } let_it_be(:project, reload: true) { create(:project) } @@ -51,15 +51,33 @@ RSpec.describe ErrorTracking::ListProjectsService do end context 'masked param token' do - let(:params) { ActionController::Parameters.new(token: "*********", api_host: new_api_host) } + let(:params) { ActionController::Parameters.new(token: "*********", api_host: api_host) } - before do - expect(error_tracking_setting).to receive(:list_sentry_projects) + context 'with the current api host' do + let(:api_host) { 'https://sentrytest.gitlab.com' } + + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) .and_return({ projects: [] }) + end + + it 'uses database token' do + expect { subject.execute }.not_to change { error_tracking_setting.token } + end end - it 'uses database token' do - expect { subject.execute }.not_to change { error_tracking_setting.token } + context 'with a new api host' do + let(:api_host) { new_api_host } + + it 'returns an error' do + expect(result[:message]).to start_with('Token is a required field') + expect(error_tracking_setting).not_to be_valid + expect(error_tracking_setting).not_to receive(:list_sentry_projects) + end + + it 'resets the token' do + expect { subject.execute }.to change { error_tracking_setting.token }.from(token).to(nil) + end end end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index c081b20d95f..4b925a058e7 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -129,7 +129,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state context 'there is userinfo' do before do - project_hook.update!(url: 'http://{one}:{two}@example.com') + project_hook.update!( + url: 'http://{one}:{two}@example.com', + url_variables: { 'one' => 'a', 'two' => 'b' } + ) stub_full_request('http://example.com', method: :post) end -- cgit v1.2.3