Add latest changes from gitlab-org/security/gitlab@14-2-stable-ee

10 files changed, 166 insertions, 104 deletions

diff --git a/app/helpers/integrations_helper.rb b/app/helpers/integrations_helper.rb
index f15566a551a..904508867d3 100644
--- a/app/helpers/integrations_helper.rb
+++ b/app/helpers/integrations_helper.rb
@@ -137,7 +137,7 @@ module IntegrationsHelper
   def jira_issue_breadcrumb_link(issue_reference)
     link_to '', { class: 'gl-display-flex gl-align-items-center gl-white-space-nowrap' } do
       icon = image_tag image_path('illustrations/logos/jira.svg'), width: 15, height: 15, class: 'gl-mr-2'
-      [icon, issue_reference].join.html_safe
+      [icon, html_escape(issue_reference)].join.html_safe
     end
   end
 
diff --git a/app/models/design_management/design.rb b/app/models/design_management/design.rb
index 79f5a63bcb6..feb1bf5438c 100644
--- a/app/models/design_management/design.rb
+++ b/app/models/design_management/design.rb
@@ -169,7 +169,7 @@ module DesignManagement
       @link_reference_pattern ||= begin
         path_segment = %r{issues/#{Gitlab::Regex.issue}/designs}
         ext = Regexp.new(Regexp.union(SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT).source, Regexp::IGNORECASE)
-        valid_char = %r{[^/\s]} # any char that is not a forward slash or whitespace
+        valid_char = %r{[[:word:]\.\-\+]}
         filename_pattern = %r{
           (?<url_filename> #{valid_char}+ \. #{ext})
         }x
diff --git a/app/models/integrations/datadog.rb b/app/models/integrations/datadog.rb
index 5516e6bc2c0..6422f6bddab 100644
--- a/app/models/integrations/datadog.rb
+++ b/app/models/integrations/datadog.rb
@@ -8,7 +8,6 @@ module Integrations
 
     DEFAULT_DOMAIN = 'datadoghq.com'
     URL_TEMPLATE = 'https://webhooks-http-intake.logs.%{datadog_domain}/api/v2/webhook'
-    URL_TEMPLATE_API_KEYS = 'https://app.%{datadog_domain}/account/settings#api'
     URL_API_KEYS_DOCS = "https://docs.#{DEFAULT_DOMAIN}/account_management/api-app-keys/"
 
     SUPPORTED_EVENTS = %w[
@@ -90,7 +89,7 @@ module Integrations
           help: ERB::Util.html_escape(
             s_('DatadogIntegration|%{linkOpen}API key%{linkClose} used for authentication with Datadog.')
           ) % {
-            linkOpen: '<a href="%s" target="_blank" rel="noopener noreferrer">'.html_safe % api_keys_url,
+            linkOpen: %Q{<a href="#{URL_API_KEYS_DOCS}" target="_blank" rel="noopener noreferrer">}.html_safe,
             linkClose: '</a>'.html_safe
           },
           required: true
@@ -132,12 +131,6 @@ module Integrations
       url.to_s
     end
 
-    def api_keys_url
-      return URL_API_KEYS_DOCS unless datadog_site.presence
-
-      sprintf(URL_TEMPLATE_API_KEYS, datadog_domain: datadog_domain)
-    end
-
     def execute(data)
       object_kind = data[:object_kind]
       object_kind = 'job' if object_kind == 'build'
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index 08c4e0231e7..5ffa746e109 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -29,14 +29,7 @@ module Groups
 
       group.chat_team&.remove_mattermost_team(current_user)
 
-      # If any other groups are shared with the group that is being destroyed,
-      # we should specifically trigger update of all project authorizations
-      # for users that are the direct members of this group.
-      # If not, the project authorization records of these users to projects within the shared groups
-      # will never be removed, causing inconsistencies with access permissions.
-      if any_other_groups_are_shared_with_this_group?
-        user_ids_for_project_authorizations_refresh = group.users_ids_of_direct_members
-      end
+      user_ids_for_project_authorizations_refresh = obtain_user_ids_for_project_authorizations_refresh
 
       group.destroy
 
@@ -52,9 +45,33 @@ module Groups
 
     private
 
-    def any_other_groups_are_shared_with_this_group?
+    def any_groups_shared_with_this_group?
       group.shared_group_links.any?
     end
+
+    def any_projects_shared_with_this_group?
+      group.project_group_links.any?
+    end
+
+    # Destroying a group automatically destroys all project authorizations directly
+    # associated with the group and descendents. However, project authorizations
+    # for projects and groups this group is shared with are not. Without a manual
+    # refresh, the project authorization records of these users to shared projects
+    # and projects within the shared groups will never be removed, causing
+    # inconsistencies with access permissions.
+    #
+    # This method retrieves the user IDs that need to be refreshed. If only
+    # groups are shared with this group, only direct members need to be refreshed.
+    # If projects are also shared with the group, direct members *and* shared
+    # members of other groups need to be refreshed.
+    # `Group#user_ids_for_project_authorizations` returns both direct and shared
+    # members' user IDs.
+    def obtain_user_ids_for_project_authorizations_refresh
+      return unless any_projects_shared_with_this_group? || any_groups_shared_with_this_group?
+      return group.user_ids_for_project_authorizations if any_projects_shared_with_this_group?
+
+      group.users_ids_of_direct_members
+    end
   end
 end
 
diff --git a/lib/gitlab/markdown_cache.rb b/lib/gitlab/markdown_cache.rb
index db39904ea23..d6371732624 100644
--- a/lib/gitlab/markdown_cache.rb
+++ b/lib/gitlab/markdown_cache.rb
@@ -11,7 +11,7 @@ module Gitlab
     # this if the change to the renderer output is a new feature or a
     # minor bug fix.
     # See: https://gitlab.com/gitlab-org/gitlab/-/issues/330313
-    CACHE_COMMONMARK_VERSION        = 28
+    CACHE_COMMONMARK_VERSION        = 29
     CACHE_COMMONMARK_VERSION_START  = 10
 
     BaseError = Class.new(StandardError)
diff --git a/spec/helpers/integrations_helper_spec.rb b/spec/helpers/integrations_helper_spec.rb
index 8e652d2f150..3a7d4d12513 100644
--- a/spec/helpers/integrations_helper_spec.rb
+++ b/spec/helpers/integrations_helper_spec.rb
@@ -98,4 +98,19 @@ RSpec.describe IntegrationsHelper do
       end
     end
   end
+
+  describe '#jira_issue_breadcrumb_link' do
+    let(:issue_reference) { nil }
+
+    subject { helper.jira_issue_breadcrumb_link(issue_reference) }
+
+    context 'when issue_reference contains HTML' do
+      let(:issue_reference) { "<script>alert('XSS')</script>" }
+
+      it 'escapes issue reference' do
+        is_expected.not_to include(issue_reference)
+        is_expected.to include(html_escape(issue_reference))
+      end
+    end
+  end
 end
diff --git a/spec/lib/banzai/filter/references/design_reference_filter_spec.rb b/spec/lib/banzai/filter/references/design_reference_filter_spec.rb
index cb1f3d520a4..d616aabea45 100644
--- a/spec/lib/banzai/filter/references/design_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/references/design_reference_filter_spec.rb
@@ -90,11 +90,8 @@ RSpec.describe Banzai::Filter::References::DesignReferenceFilter do
       [
         ['simple.png'],
         ['SIMPLE.PNG'],
-        ['has spaces.png'],
         ['has-hyphen.jpg'],
-        ['snake_case.svg'],
-        ['has "quotes".svg'],
-        ['has <special> characters [o].svg']
+        ['snake_case.svg']
       ]
     end
 
@@ -138,40 +135,25 @@ RSpec.describe Banzai::Filter::References::DesignReferenceFilter do
     end
   end
 
-  context 'a design with a quoted filename' do
-    let(:filename) { %q{A "very" good file.png} }
-    let(:design) { create(:design, :with_versions, issue: issue, filename: filename) }
-
-    it 'links to the design' do
-      expect(doc.css('a').first.attr('href'))
-        .to eq url_for_design(design)
-    end
-  end
-
   context 'internal reference' do
     it_behaves_like 'a reference containing an element node'
 
-    context 'the reference is valid' do
-      it_behaves_like 'a good link reference'
+    it_behaves_like 'a good link reference'
 
-      context 'the filename needs to be escaped' do
-        where(:filename) do
-          [
-            ['with some spaces.png'],
-            ['with <script>console.log("pwded")<%2Fscript>.png']
-          ]
-        end
+    context 'the filename contains invalid characters' do
+      where(:filename) do
+        [
+          ['with some spaces.png'],
+          ['with <script>console.log("pwded")<%2Fscript>.png'],
+          ['foo"bar.png'],
+          ['A "very" good file.png']
+        ]
+      end
 
-        with_them do
-          let(:design) { create(:design, :with_versions, filename: filename, issue: issue) }
-          let(:link) { doc.css('a').first }
+      with_them do
+        let(:design) { create(:design, :with_versions, filename: filename, issue: issue) }
 
-          it 'replaces the content with the reference, but keeps the link', :aggregate_failures do
-            expect(doc.text).to eq(CGI.unescapeHTML("Added #{design.to_reference}"))
-            expect(link.attr('title')).to eq(design.filename)
-            expect(link.attr('href')).to eq(design_url)
-          end
-        end
+        it_behaves_like 'a no-op filter'
       end
     end
 
diff --git a/spec/models/design_management/design_spec.rb b/spec/models/design_management/design_spec.rb
index f2ce5e42eaf..b0601ea3f08 100644
--- a/spec/models/design_management/design_spec.rb
+++ b/spec/models/design_management/design_spec.rb
@@ -572,6 +572,12 @@ RSpec.describe DesignManagement::Design do
       expect(described_class.link_reference_pattern).not_to match(url_for_designs(issue))
     end
 
+    it 'intentionally ignores filenames with any special character' do
+      design = build(:design, issue: issue, filename: '"invalid')
+
+      expect(described_class.link_reference_pattern).not_to match(url_for_design(design))
+    end
+
     where(:ext) do
       (described_class::SAFE_IMAGE_EXT + described_class::DANGEROUS_IMAGE_EXT).flat_map do |ext|
         [[ext], [ext.upcase]]
@@ -593,14 +599,6 @@ RSpec.describe DesignManagement::Design do
         )
       end
 
-      context 'the file needs to be encoded' do
-        let(:filename) { "my file.#{ext}" }
-
-        it 'extracts the encoded filename' do
-          expect(captures).to include('url_filename' => 'my%20file.' + ext)
-        end
-      end
-
       context 'the file is all upper case' do
         let(:filename) { "file.#{ext}".upcase }
 
diff --git a/spec/models/integrations/datadog_spec.rb b/spec/models/integrations/datadog_spec.rb
index 677bd4c5e48..7049e64c2ce 100644
--- a/spec/models/integrations/datadog_spec.rb
+++ b/spec/models/integrations/datadog_spec.rb
@@ -127,18 +127,6 @@ RSpec.describe Integrations::Datadog do
     end
   end
 
-  describe '#api_keys_url' do
-    subject { instance.api_keys_url }
-
-    it { is_expected.to eq("https://app.#{dd_site}/account/settings#api") }
-
-    context 'with unset datadog_site' do
-      let(:dd_site) { '' }
-
-      it { is_expected.to eq("https://docs.datadoghq.com/account_management/api-app-keys/") }
-    end
-  end
-
   describe '#test' do
     subject(:result) { saved_instance.test(pipeline_data) }
 
diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
index c794acdf76d..5135be8fff5 100644
--- a/spec/services/groups/destroy_service_spec.rb
+++ b/spec/services/groups/destroy_service_spec.rb
@@ -152,56 +152,125 @@ RSpec.describe Groups::DestroyService do
     end
 
     context 'for shared groups within different hierarchies' do
-      let(:shared_with_group) { group }
-      let!(:shared_group) { create(:group, :private) }
-      let!(:shared_group_child) { create(:group, :private, parent: shared_group) }
-      let!(:shared_group_user) { create(:user) }
+      let(:group1) { create(:group, :private) }
+      let(:group2) { create(:group, :private) }
 
-      let!(:project) { create(:project, group: shared_group) }
-      let!(:project_child) { create(:project, group: shared_group_child) }
+      let(:group1_user) { create(:user) }
+      let(:group2_user) { create(:user) }
 
       before do
-        shared_group.add_user(shared_group_user, Gitlab::Access::OWNER)
-
-        create(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_group)
-        shared_with_group.refresh_members_authorized_projects
+        group1.add_user(group1_user, Gitlab::Access::OWNER)
+        group2.add_user(group2_user, Gitlab::Access::OWNER)
       end
 
-      context 'the shared group is deleted' do
-        it 'updates project authorization' do
-          expect(shared_group_user.can?(:read_project, project)).to eq(true)
-          expect(shared_group_user.can?(:read_project, project_child)).to eq(true)
+      context 'when a project is shared with a group' do
+        let!(:group1_project) { create(:project, :private, group: group1) }
+
+        before do
+          create(:project_group_link, project: group1_project, group: group2)
+        end
+
+        context 'and the shared group is deleted' do
+          it 'updates project authorizations so group2 users no longer have access', :aggregate_failures do
+            expect(group1_user.can?(:read_project, group1_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group1_project)).to eq(true)
 
-          destroy_group(shared_group, shared_group_user, false)
+            destroy_group(group2, group2_user, false)
 
-          expect(shared_group_user.can?(:read_project, project)).to eq(false)
-          expect(shared_group_user.can?(:read_project, project_child)).to eq(false)
+            expect(group1_user.can?(:read_project, group1_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group1_project)).to eq(false)
+          end
+
+          it 'calls the service to update project authorizations only with necessary user ids' do
+            expect(UserProjectAccessChangedService)
+              .to receive(:new).with(array_including(group2_user.id)).and_call_original
+
+            destroy_group(group2, group2_user, false)
+          end
         end
 
-        it 'does not make use of specific service to update project_authorizations records' do
-          expect(UserProjectAccessChangedService)
-            .not_to receive(:new).with(shared_group.user_ids_for_project_authorizations).and_call_original
+        context 'and the group is shared with another group' do
+          let(:group3) { create(:group, :private) }
+          let(:group3_user) { create(:user) }
+
+          before do
+            group3.add_user(group3_user, Gitlab::Access::OWNER)
+
+            create(:group_group_link, shared_group: group2, shared_with_group: group3)
+            group3.refresh_members_authorized_projects
+          end
+
+          it 'updates project authorizations so group2 and group3 users no longer have access', :aggregate_failures do
+            expect(group1_user.can?(:read_project, group1_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group1_project)).to eq(true)
+            expect(group3_user.can?(:read_project, group1_project)).to eq(true)
+
+            destroy_group(group2, group2_user, false)
+
+            expect(group1_user.can?(:read_project, group1_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group1_project)).to eq(false)
+            expect(group3_user.can?(:read_project, group1_project)).to eq(false)
+          end
 
-          destroy_group(shared_group, shared_group_user, false)
+          it 'calls the service to update project authorizations only with necessary user ids' do
+            expect(UserProjectAccessChangedService)
+              .to receive(:new).with(array_including(group2_user.id, group3_user.id)).and_call_original
+
+            destroy_group(group2, group2_user, false)
+          end
         end
       end
 
-      context 'the shared_with group is deleted' do
-        it 'updates project authorization' do
-          expect(user.can?(:read_project, project)).to eq(true)
-          expect(user.can?(:read_project, project_child)).to eq(true)
+      context 'when a group is shared with a group' do
+        let!(:group2_project) { create(:project, :private, group: group2) }
 
-          destroy_group(shared_with_group, user, false)
+        before do
+          create(:group_group_link, shared_group: group2, shared_with_group: group1)
+          group1.refresh_members_authorized_projects
+        end
+
+        context 'and the shared group is deleted' do
+          it 'updates project authorizations since the project has been deleted with the group', :aggregate_failures do
+            expect(group1_user.can?(:read_project, group2_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group2_project)).to eq(true)
+
+            destroy_group(group2, group2_user, false)
 
-          expect(user.can?(:read_project, project)).to eq(false)
-          expect(user.can?(:read_project, project_child)).to eq(false)
+            expect(group1_user.can?(:read_project, group2_project)).to eq(false)
+            expect(group2_user.can?(:read_project, group2_project)).to eq(false)
+          end
+
+          it 'does not call the service to update project authorizations' do
+            expect(UserProjectAccessChangedService).not_to receive(:new)
+
+            destroy_group(group2, group2_user, false)
+          end
         end
 
-        it 'makes use of a specific service to update project_authorizations records' do
-          expect(UserProjectAccessChangedService)
-            .to receive(:new).with(shared_with_group.user_ids_for_project_authorizations).and_call_original
+        context 'the shared_with group is deleted' do
+          let!(:group2_subgroup) { create(:group, :private, parent: group2)}
+          let!(:group2_subgroup_project) { create(:project, :private, group: group2_subgroup) }
 
-          destroy_group(shared_with_group, user, false)
+          it 'updates project authorizations so users of both groups lose access', :aggregate_failures do
+            expect(group1_user.can?(:read_project, group2_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group2_project)).to eq(true)
+            expect(group1_user.can?(:read_project, group2_subgroup_project)).to eq(true)
+            expect(group2_user.can?(:read_project, group2_subgroup_project)).to eq(true)
+
+            destroy_group(group1, group1_user, false)
+
+            expect(group1_user.can?(:read_project, group2_project)).to eq(false)
+            expect(group2_user.can?(:read_project, group2_project)).to eq(true)
+            expect(group1_user.can?(:read_project, group2_subgroup_project)).to eq(false)
+            expect(group2_user.can?(:read_project, group2_subgroup_project)).to eq(true)
+          end
+
+          it 'calls the service to update project authorizations only with necessary user ids' do
+            expect(UserProjectAccessChangedService)
+              .to receive(:new).with([group1_user.id]).and_call_original
+
+            destroy_group(group1, group1_user, false)
+          end
         end
       end
     end