summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/concerns/page_limiter.rb2
-rw-r--r--app/helpers/submodule_helper.rb78
-rw-r--r--app/models/active_session.rb2
-rw-r--r--app/models/hooks/web_hook.rb5
-rw-r--r--app/services/error_tracking/list_projects_service.rb16
-rw-r--r--lib/gitlab/safe_device_detector.rb16
-rw-r--r--spec/helpers/submodule_helper_spec.rb2
-rw-r--r--spec/lib/gitlab/safe_device_detector_spec.rb20
-rw-r--r--spec/models/hooks/web_hook_spec.rb28
-rw-r--r--spec/services/error_tracking/list_projects_service_spec.rb30
-rw-r--r--spec/services/web_hook_service_spec.rb5
11 files changed, 156 insertions, 48 deletions
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