diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-01-09 10:40:54 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-01-09 10:40:54 +0000 |
commit | 6f1b4cb8e5c4389e2c4d432e39bb9cf0a31d5da6 (patch) | |
tree | bddf757eda9fca323adbccd2716a950c1dcdd579 | |
parent | b8a1c5346855a50b711e26def061d90fae10ab79 (diff) | |
download | gitlab-ce-6f1b4cb8e5c4389e2c4d432e39bb9cf0a31d5da6.tar.gz |
Add latest changes from gitlab-org/security/gitlab@15-5-stable-ee
-rw-r--r-- | app/controllers/concerns/page_limiter.rb | 2 | ||||
-rw-r--r-- | app/models/active_session.rb | 2 | ||||
-rw-r--r-- | app/models/hooks/web_hook.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/safe_device_detector.rb | 16 | ||||
-rw-r--r-- | spec/factories/project_hooks.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/safe_device_detector_spec.rb | 20 | ||||
-rw-r--r-- | spec/models/hooks/web_hook_spec.rb | 28 | ||||
-rw-r--r-- | spec/services/web_hook_service_spec.rb | 5 |
8 files changed, 78 insertions, 4 deletions
diff --git a/app/controllers/concerns/page_limiter.rb b/app/controllers/concerns/page_limiter.rb index 362b02e5856..3e75c428fdd 100644 --- a/app/controllers/concerns/page_limiter.rb +++ b/app/controllers/concerns/page_limiter.rb @@ -57,7 +57,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/models/active_session.rb b/app/models/active_session.rb index 7dbc95c251b..0dcd8105b72 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 9f55d81b470..f761193a3ab 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -42,6 +42,7 @@ class WebHook < ApplicationRecord after_initialize :initialize_url_variables before_validation :reset_token + before_validation :reset_url_variables, unless: ->(hook) { hook.is_a?(ServiceHook) } scope :executable, -> do next all unless Feature.enabled?(:web_hooks_disable_failed) @@ -205,6 +206,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 web_hooks_disable_failed? self.class.web_hooks_disable_failed?(self) 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/factories/project_hooks.rb b/spec/factories/project_hooks.rb index dbb5c357acb..946b3925ee9 100644 --- a/spec/factories/project_hooks.rb +++ b/spec/factories/project_hooks.rb @@ -6,6 +6,10 @@ FactoryBot.define do enable_ssl_verification { false } project + trait :url_variables do + url_variables { { 'abc' => 'supers3cret' } } + end + trait :token do token { generate(:token) } end 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 5fbde9eafa0..2e8e63e20ec 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) } @@ -161,6 +161,32 @@ RSpec.describe WebHook do expect(hook.url).to eq('https://webhook.example.com/new-hook') 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 end describe 'encrypted attributes' do diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 551c3dbcc82..3fb4f938e62 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 |