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

12 files changed, 156 insertions, 69 deletions

diff --git a/app/finders/environments/environment_names_finder.rb b/app/finders/environments/environment_names_finder.rb
index d4928f0fc84..ffb689f45e2 100644
--- a/app/finders/environments/environment_names_finder.rb
+++ b/app/finders/environments/environment_names_finder.rb
@@ -32,18 +32,9 @@ module Environments
     end
 
     def namespace_environments
-      # We assume reporter access is needed for the :read_environment permission
-      # here. This expection is also present in
-      # IssuableFinder::Params#min_access_level, which is used for filtering out
-      # merge requests that don't have the right permissions.
-      #
-      # We use this approach so we don't need to load every project into memory
-      # just to verify if we can see their environments. Doing so would not be
-      # efficient, and possibly mess up pagination if certain projects are not
-      # meant to be visible.
       projects = project_or_group
         .all_projects
-        .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER)
+        .filter_by_feature_visibility(:environments, current_user)
 
       Environment.for_project(projects)
     end
diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb
index f9eba4cc2fe..dee1c820f23 100644
--- a/app/models/concerns/taskable.rb
+++ b/app/models/concerns/taskable.rb
@@ -24,25 +24,37 @@ module Taskable
     (\s.+)                        # followed by whitespace and some text.
   }x.freeze
 
+  ITEM_PATTERN_UNTRUSTED =
+    '^' \
+    '(?:(?:>\s{0,4})*)' \
+    '(?P<prefix>(?:\s*(?:[-+*]|(?:\d+\.)))+)' \
+    '\s+' \
+    '(?P<checkbox>' \
+    "#{COMPLETE_PATTERN.source}|#{INCOMPLETE_PATTERN.source}" \
+    ')' \
+    '(?P<label>\s.+)'.freeze
+
   # ignore tasks in code or html comment blocks.  HTML blocks
   # are ok as we allow tasks inside <detail> blocks
-  REGEX = %r{
-      #{::Gitlab::Regex.markdown_code_or_html_comments}
-    |
-      (?<task_item>
-        #{ITEM_PATTERN}
-      )
-  }mx.freeze
+  REGEX =
+    "#{::Gitlab::Regex.markdown_code_or_html_comments_untrusted}" \
+    "|" \
+    "(?P<task_item>" \
+    "#{ITEM_PATTERN_UNTRUSTED}" \
+    ")".freeze
 
   def self.get_tasks(content)
     items = []
 
-    content.to_s.scan(REGEX) do
-      next unless $~[:task_item]
+    regex = Gitlab::UntrustedRegexp.new(REGEX, multiline: true)
+    regex.scan(content.to_s).each do |match|
+      next unless regex.extract_named_group(:task_item, match)
+
+      prefix = regex.extract_named_group(:prefix, match)
+      checkbox = regex.extract_named_group(:checkbox, match)
+      label = regex.extract_named_group(:label, match)
 
-      $~[:task_item].scan(ITEM_PATTERN) do |prefix, checkbox, label|
-        items << TaskList::Item.new("#{prefix.strip} #{checkbox}", label.strip)
-      end
+      items << TaskList::Item.new("#{prefix.strip} #{checkbox}", label.strip)
     end
 
     items
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index 168646bbe41..23b0665cb74 100644
--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -36,7 +36,8 @@ class ProjectFeature < ApplicationRecord
     merge_requests: Gitlab::Access::REPORTER,
     metrics_dashboard: Gitlab::Access::REPORTER,
     container_registry: Gitlab::Access::REPORTER,
-    package_registry: Gitlab::Access::REPORTER
+    package_registry: Gitlab::Access::REPORTER,
+    environments: Gitlab::Access::REPORTER
   }.freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze
 
diff --git a/app/views/explore/projects/page_out_of_bounds.html.haml b/app/views/explore/projects/page_out_of_bounds.html.haml
index ef5ee2c679e..e13768a3ccb 100644
--- a/app/views/explore/projects/page_out_of_bounds.html.haml
+++ b/app/views/explore/projects/page_out_of_bounds.html.haml
@@ -18,5 +18,5 @@
       %h5= _("Maximum page reached")
       %p= _("Sorry, you have exceeded the maximum browsable page number. Please use the API to explore further.")
 
-      = render Pajamas::ButtonComponent.new(href: request.params.merge(page: @max_page_number)) do
+      = render Pajamas::ButtonComponent.new(href: safe_params.merge(page: @max_page_number)) do
         = _("Back to page %{number}") % { number: @max_page_number }
diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb
index 93d23add5eb..943218a9972 100644
--- a/lib/gitlab/regex.rb
+++ b/lib/gitlab/regex.rb
@@ -448,6 +448,17 @@ module Gitlab
       )
     }mx.freeze
 
+    # Code blocks:
+    # ```
+    # Anything, including `>>>` blocks which are ignored by this filter
+    # ```
+    MARKDOWN_CODE_BLOCK_REGEX_UNTRUSTED =
+      '(?P<code>' \
+        '^```\n' \
+        '(?:\n|.)*?' \
+        '\n```\ *$' \
+      ')'.freeze
+
     MARKDOWN_HTML_BLOCK_REGEX = %r{
       (?<html>
         # HTML block:
@@ -461,27 +472,19 @@ module Gitlab
       )
     }mx.freeze
 
-    MARKDOWN_HTML_COMMENT_LINE_REGEX = %r{
-      (?<html_comment_line>
-        # HTML comment line:
-        # <!-- some commented text -->
-
-        ^<!--\ .*?\ -->\ *$
-      )
-    }mx.freeze
-
-    MARKDOWN_HTML_COMMENT_BLOCK_REGEX = %r{
-      (?<html_comment_block>
-        # HTML comment block:
-        # <!-- some commented text
-        # additional text
-        # -->
+    # HTML comment line:
+    # <!-- some commented text -->
+    MARKDOWN_HTML_COMMENT_LINE_REGEX_UNTRUSTED =
+      '(?P<html_comment_line>' \
+        '^<!--\ .*?\ -->\ *$' \
+      ')'.freeze
 
-        ^<!--.*\n
-        .+?
-        \n-->\ *$
-      )
-    }mx.freeze
+    MARKDOWN_HTML_COMMENT_BLOCK_REGEX_UNTRUSTED =
+      '(?P<html_comment_block>' \
+        '^<!--.*?\n' \
+        '(?:\n|.)*?' \
+        '\n.*?-->\ *$' \
+      ')'.freeze
 
     def markdown_code_or_html_blocks
       @markdown_code_or_html_blocks ||= %r{
@@ -491,14 +494,13 @@ module Gitlab
       }mx.freeze
     end
 
-    def markdown_code_or_html_comments
-      @markdown_code_or_html_comments ||= %r{
-          #{MARKDOWN_CODE_BLOCK_REGEX}
-        |
-          #{MARKDOWN_HTML_COMMENT_LINE_REGEX}
-        |
-          #{MARKDOWN_HTML_COMMENT_BLOCK_REGEX}
-      }mx.freeze
+    def markdown_code_or_html_comments_untrusted
+      @markdown_code_or_html_comments_untrusted ||=
+        "#{MARKDOWN_CODE_BLOCK_REGEX_UNTRUSTED}" \
+        "|" \
+        "#{MARKDOWN_HTML_COMMENT_LINE_REGEX_UNTRUSTED}" \
+        "|" \
+        "#{MARKDOWN_HTML_COMMENT_BLOCK_REGEX_UNTRUSTED}"
     end
 
     # Based on Jira's project key format
diff --git a/lib/gitlab/untrusted_regexp.rb b/lib/gitlab/untrusted_regexp.rb
index 96e74f00c78..7c7bda3a8f9 100644
--- a/lib/gitlab/untrusted_regexp.rb
+++ b/lib/gitlab/untrusted_regexp.rb
@@ -47,6 +47,17 @@ module Gitlab
       RE2.Replace(text, regexp, rewrite)
     end
 
+    # #scan returns an array of the groups captured, rather than MatchData.
+    # Use this to give the capture group name and grab the proper value
+    def extract_named_group(name, match)
+      return unless match
+
+      match_position = regexp.named_capturing_groups[name.to_s]
+      raise RegexpError, "Invalid named capture group: #{name}" unless match_position
+
+      match[match_position - 1]
+    end
+
     def ==(other)
       self.source == other.source
     end
diff --git a/spec/finders/environments/environment_names_finder_spec.rb b/spec/finders/environments/environment_names_finder_spec.rb
index 438f9e9ea7c..c2336c59119 100644
--- a/spec/finders/environments/environment_names_finder_spec.rb
+++ b/spec/finders/environments/environment_names_finder_spec.rb
@@ -6,6 +6,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do
   describe '#execute' do
     let!(:group) { create(:group) }
     let!(:public_project) { create(:project, :public, namespace: group) }
+    let_it_be_with_reload(:public_project_with_private_environments) { create(:project, :public) }
     let!(:private_project) { create(:project, :private, namespace: group) }
     let!(:user) { create(:user) }
 
@@ -14,6 +15,11 @@ RSpec.describe Environments::EnvironmentNamesFinder do
       create(:environment, name: 'gprd', project: public_project)
       create(:environment, name: 'gprd', project: private_project)
       create(:environment, name: 'gcny', project: private_project)
+      create(:environment, name: 'gprivprd', project: public_project_with_private_environments)
+      create(:environment, name: 'gprivstg', project: public_project_with_private_environments)
+
+      public_project_with_private_environments.update!(namespace: group)
+      public_project_with_private_environments.project_feature.update!(environments_access_level: Featurable::PRIVATE)
     end
 
     context 'using a group' do
@@ -23,7 +29,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do
 
           names = described_class.new(group, user).execute
 
-          expect(names).to eq(%w[gcny gprd gstg])
+          expect(names).to eq(%w[gcny gprd gprivprd gprivstg gstg])
         end
       end
 
@@ -33,7 +39,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do
 
           names = described_class.new(group, user).execute
 
-          expect(names).to eq(%w[gcny gprd gstg])
+          expect(names).to eq(%w[gcny gprd gprivprd gprivstg gstg])
         end
       end
 
@@ -57,8 +63,18 @@ RSpec.describe Environments::EnvironmentNamesFinder do
         end
       end
 
+      context 'with a public project reporter which has private environments' do
+        it 'returns environment names for public projects' do
+          public_project_with_private_environments.add_reporter(user)
+
+          names = described_class.new(group, user).execute
+
+          expect(names).to eq(%w[gprd gprivprd gprivstg gstg])
+        end
+      end
+
       context 'with a group guest' do
-        it 'returns environment names for all public projects' do
+        it 'returns environment names for public projects' do
           group.add_guest(user)
 
           names = described_class.new(group, user).execute
@@ -68,7 +84,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do
       end
 
       context 'with a non-member' do
-        it 'returns environment names for all public projects' do
+        it 'returns environment names for only public projects with public environments' do
           names = described_class.new(group, user).execute
 
           expect(names).to eq(%w[gprd gstg])
@@ -76,7 +92,7 @@ RSpec.describe Environments::EnvironmentNamesFinder do
       end
 
       context 'without a user' do
-        it 'returns environment names for all public projects' do
+        it 'returns environment names for only public projects with public environments' do
           names = described_class.new(group).execute
 
           expect(names).to eq(%w[gprd gstg])
diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb
index 31de4068bc5..bc0f9e22d50 100644
--- a/spec/lib/gitlab/regex_spec.rb
+++ b/spec/lib/gitlab/regex_spec.rb
@@ -1140,7 +1140,7 @@ RSpec.describe Gitlab::Regex, feature_category: :tooling do
     end
 
     context 'HTML comment lines' do
-      subject { described_class::MARKDOWN_HTML_COMMENT_LINE_REGEX }
+      subject { Gitlab::UntrustedRegexp.new(described_class::MARKDOWN_HTML_COMMENT_LINE_REGEX_UNTRUSTED, multiline: true) }
 
       let(:expected) { [['<!-- an HTML comment -->'], ['<!-- another HTML comment -->']] }
       let(:markdown) do
@@ -1158,20 +1158,20 @@ RSpec.describe Gitlab::Regex, feature_category: :tooling do
       it { is_expected.to match(%(<!-- single line comment -->)) }
       it { is_expected.not_to match(%(<!--\nblock comment\n-->)) }
       it { is_expected.not_to match(%(must start in first column <!-- comment -->)) }
-      it { expect(markdown.scan(subject)).to eq expected }
+      it { expect(subject.scan(markdown)).to eq expected }
     end
 
     context 'HTML comment blocks' do
-      subject { described_class::MARKDOWN_HTML_COMMENT_BLOCK_REGEX }
+      subject { Gitlab::UntrustedRegexp.new(described_class::MARKDOWN_HTML_COMMENT_BLOCK_REGEX_UNTRUSTED, multiline: true) }
 
-      let(:expected) { %(<!-- the start of an HTML comment\n- [ ] list item commented out\n-->) }
+      let(:expected) { %(<!-- the start of an HTML comment\n- [ ] list item commented out\nmore text -->) }
       let(:markdown) do
         <<~MARKDOWN
         Regular text
 
         <!-- the start of an HTML comment
         - [ ] list item commented out
-        -->
+        more text -->
         MARKDOWN
       end
 
diff --git a/spec/lib/gitlab/untrusted_regexp_spec.rb b/spec/lib/gitlab/untrusted_regexp_spec.rb
index 270c4beec97..66675b20107 100644
--- a/spec/lib/gitlab/untrusted_regexp_spec.rb
+++ b/spec/lib/gitlab/untrusted_regexp_spec.rb
@@ -137,6 +137,38 @@ RSpec.describe Gitlab::UntrustedRegexp do
     end
   end
 
+  describe '#extract_named_group' do
+    let(:re) { described_class.new('(?P<name>\w+) (?P<age>\d+)|(?P<name_only>\w+)') }
+    let(:text) { 'Bob 40' }
+
+    it 'returns values for both named groups' do
+      matched = re.scan(text).first
+
+      expect(re.extract_named_group(:name, matched)).to eq 'Bob'
+      expect(re.extract_named_group(:age, matched)).to eq '40'
+    end
+
+    it 'returns nil if there was no match for group' do
+      matched = re.scan('Bob').first
+
+      expect(re.extract_named_group(:name, matched)).to be_nil
+      expect(re.extract_named_group(:age, matched)).to be_nil
+      expect(re.extract_named_group(:name_only, matched)).to eq 'Bob'
+    end
+
+    it 'returns nil if match is nil' do
+      matched = '(?P<age>\d+)'.scan(text).first
+
+      expect(re.extract_named_group(:age, matched)).to be_nil
+    end
+
+    it 'raises if name is not a capture group' do
+      matched = re.scan(text).first
+
+      expect { re.extract_named_group(:foo, matched) }.to raise_error('Invalid named capture group: foo')
+    end
+  end
+
   describe '#match' do
     context 'when there are matches' do
       it 'returns a match object' do
diff --git a/spec/models/concerns/taskable_spec.rb b/spec/models/concerns/taskable_spec.rb
index 14f346f353b..20de8995d13 100644
--- a/spec/models/concerns/taskable_spec.rb
+++ b/spec/models/concerns/taskable_spec.rb
@@ -35,11 +35,7 @@ RSpec.describe Taskable, feature_category: :team_planning do
         TaskList::Item.new('- [ ]', 'First item'),
         TaskList::Item.new('- [x]', 'Second item'),
         TaskList::Item.new('* [x]', 'First item'),
-        TaskList::Item.new('* [ ]', 'Second item'),
-        TaskList::Item.new('+ [ ]', 'No-break space (U+00A0)'),
-        TaskList::Item.new('+ [ ]', 'Figure space (U+2007)'),
-        TaskList::Item.new('+ [ ]', 'Narrow no-break space (U+202F)'),
-        TaskList::Item.new('+ [ ]', 'Thin space (U+2009)')
+        TaskList::Item.new('* [ ]', 'Second item')
       ]
     end
 
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index b2fb310aca3..c29446c1f38 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -2016,7 +2016,7 @@ RSpec.describe ProjectPolicy, feature_category: :authentication_and_authorizatio
       :public   | ProjectFeature::ENABLED   | :anonymous  | true
       :public   | ProjectFeature::PRIVATE   | :maintainer | true
       :public   | ProjectFeature::PRIVATE   | :developer  | true
-      :public   | ProjectFeature::PRIVATE   | :guest      | true
+      :public   | ProjectFeature::PRIVATE   | :guest      | false
       :public   | ProjectFeature::PRIVATE   | :anonymous  | false
       :public   | ProjectFeature::DISABLED  | :maintainer | false
       :public   | ProjectFeature::DISABLED  | :developer  | false
@@ -2028,7 +2028,7 @@ RSpec.describe ProjectPolicy, feature_category: :authentication_and_authorizatio
       :internal | ProjectFeature::ENABLED   | :anonymous  | false
       :internal | ProjectFeature::PRIVATE   | :maintainer | true
       :internal | ProjectFeature::PRIVATE   | :developer  | true
-      :internal | ProjectFeature::PRIVATE   | :guest      | true
+      :internal | ProjectFeature::PRIVATE   | :guest      | false
       :internal | ProjectFeature::PRIVATE   | :anonymous  | false
       :internal | ProjectFeature::DISABLED  | :maintainer | false
       :internal | ProjectFeature::DISABLED  | :developer  | false
diff --git a/spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb b/spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb
new file mode 100644
index 00000000000..1ace28be5b4
--- /dev/null
+++ b/spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'explore/projects/page_out_of_bounds.html.haml', feature_category: :projects do
+  let(:page_limit) { 10 }
+  let(:unsafe_param) { 'hacked_using_unsafe_param!' }
+
+  before do
+    assign(:max_page_number, page_limit)
+
+    controller.params[:action] = 'index'
+    controller.params[:host] = unsafe_param
+    controller.params[:protocol] = unsafe_param
+    controller.params[:sort] = 'name_asc'
+  end
+
+  it 'removes unsafe params from the link' do
+    render
+
+    href = "/explore/projects?page=#{page_limit}&sort=name_asc"
+    button_text = format(_("Back to page %{number}"), number: page_limit)
+    expect(rendered).to have_link(button_text, href: href)
+    expect(rendered).not_to include(unsafe_param)
+  end
+end