Add latest changes from gitlab-org/security/gitlab@15-7-stable-ee

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