From 56ff640a2f919e9d0e450964081381a8eccef5e4 Mon Sep 17 00:00:00 2001
From: GitLab Bot <gitlab-bot@gitlab.com>
Date: Wed, 29 Mar 2023 23:49:36 +0000
Subject: Add latest changes from gitlab-org/security/gitlab@15-10-stable-ee

---
 app/finders/notes_finder.rb                        |  8 +++
 app/models/concerns/taskable.rb                    | 36 +++++++++-----
 .../merge_requests/push_options_handler_service.rb | 10 +++-
 lib/gitlab/regex.rb                                | 58 +++++++++++-----------
 lib/gitlab/untrusted_regexp.rb                     | 11 ++++
 spec/finders/notes_finder_spec.rb                  | 20 ++++++++
 spec/lib/gitlab/regex_spec.rb                      | 10 ++--
 spec/lib/gitlab/untrusted_regexp_spec.rb           | 32 ++++++++++++
 spec/models/concerns/taskable_spec.rb              |  6 +--
 .../push_options_handler_service_spec.rb           | 15 ++++++
 10 files changed, 155 insertions(+), 51 deletions(-)

diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index c542ffbce7e..81017290f12 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -30,6 +30,7 @@ class NotesFinder
     notes = init_collection
     notes = since_fetch_at(notes)
     notes = notes.with_notes_filter(@params[:notes_filter]) if notes_filter?
+    notes = redact_internal(notes)
     sort(notes)
   end
 
@@ -181,6 +182,13 @@ class NotesFinder
 
     notes.order_by(sort)
   end
+
+  def redact_internal(notes)
+    subject = @project || target
+    return notes if Ability.allowed?(@current_user, :read_internal_note, subject)
+
+    notes.not_internal
+  end
 end
 
 NotesFinder.prepend_mod_with('NotesFinder')
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/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb
index 235dc6678df..e9abafceb13 100644
--- a/app/services/merge_requests/push_options_handler_service.rb
+++ b/app/services/merge_requests/push_options_handler_service.rb
@@ -54,7 +54,15 @@ module MergeRequests
     end
 
     def validate_service
-      errors << 'User is required' if current_user.nil?
+      if current_user.nil?
+        errors << 'User is required'
+        return
+      end
+
+      unless current_user&.can?(:read_code, target_project)
+        errors << 'User access was denied'
+        return
+      end
 
       unless target_project.merge_requests_enabled?
         errors << "Merge requests are not enabled for project #{target_project.full_path}"
diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb
index 5b235639ae8..de6eba9b9c9 100644
--- a/lib/gitlab/regex.rb
+++ b/lib/gitlab/regex.rb
@@ -453,6 +453,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:
@@ -466,27 +477,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{
@@ -496,14 +499,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/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index 792a14e3064..1255a882114 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -106,6 +106,26 @@ RSpec.describe NotesFinder do
       end
     end
 
+    context 'for notes on public issue in public project' do
+      let_it_be(:public_project) { create(:project, :public) }
+      let_it_be(:guest_member) { create(:user) }
+      let_it_be(:reporter_member) { create(:user) }
+      let_it_be(:guest_project_member) { create(:project_member, :guest, user: guest_member, project: public_project) }
+      let_it_be(:reporter_project_member) { create(:project_member, :reporter, user: reporter_member, project: public_project) }
+      let_it_be(:internal_note) { create(:note_on_issue, project: public_project, internal: true) }
+      let_it_be(:public_note) { create(:note_on_issue, project: public_project) }
+
+      it 'shows all notes when the current_user has reporter access' do
+        notes = described_class.new(reporter_member, project: public_project).execute
+        expect(notes).to contain_exactly internal_note, public_note
+      end
+
+      it 'shows only public notes when the current_user has guest access' do
+        notes = described_class.new(guest_member, project: public_project).execute
+        expect(notes).to contain_exactly public_note
+      end
+    end
+
     context 'for target type' do
       let(:project) { create(:project, :repository) }
       let!(:note1) { create :note_on_issue, project: project }
diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb
index d885051b93b..e51e62d5f0a 100644
--- a/spec/lib/gitlab/regex_spec.rb
+++ b/spec/lib/gitlab/regex_spec.rb
@@ -1171,7 +1171,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
@@ -1189,20 +1189,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/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb
index 7ca795ecd1a..49ec8b09939 100644
--- a/spec/services/merge_requests/push_options_handler_service_spec.rb
+++ b/spec/services/merge_requests/push_options_handler_service_spec.rb
@@ -861,6 +861,21 @@ RSpec.describe MergeRequests::PushOptionsHandlerService, feature_category: :sour
     end
   end
 
+  describe 'when user does not have access to target project' do
+    let(:push_options) { { create: true, target: 'my-branch' } }
+    let(:changes) { default_branch_changes }
+
+    before do
+      allow(user1).to receive(:can?).with(:read_code, project).and_return(false)
+    end
+
+    it 'records an error', :sidekiq_inline do
+      service.execute
+
+      expect(service.errors).to eq(["User access was denied"])
+    end
+  end
+
   describe 'when MRs are not enabled' do
     let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user1) } }
     let(:push_options) { { create: true } }
-- 
cgit v1.2.1