diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-03-29 23:48:15 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-03-29 23:48:15 +0000 |
commit | ef77d7f75069ca5f71261d80bc9caea59168cba2 (patch) | |
tree | b5d128c44de05edc90e0d3cb5fca398c55803628 | |
parent | b405157ce7809b3671155faa8f3c3395e3fc74ce (diff) | |
download | gitlab-ce-ef77d7f75069ca5f71261d80bc9caea59168cba2.tar.gz |
Add latest changes from gitlab-org/security/gitlab@15-9-stable-ee
-rw-r--r-- | app/finders/environments/environment_names_finder.rb | 11 | ||||
-rw-r--r-- | app/models/concerns/taskable.rb | 36 | ||||
-rw-r--r-- | app/models/project_feature.rb | 3 | ||||
-rw-r--r-- | app/views/explore/projects/page_out_of_bounds.html.haml | 2 | ||||
-rw-r--r-- | lib/gitlab/regex.rb | 58 | ||||
-rw-r--r-- | lib/gitlab/untrusted_regexp.rb | 11 | ||||
-rw-r--r-- | spec/finders/environments/environment_names_finder_spec.rb | 26 | ||||
-rw-r--r-- | spec/lib/gitlab/regex_spec.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/untrusted_regexp_spec.rb | 32 | ||||
-rw-r--r-- | spec/models/concerns/taskable_spec.rb | 6 | ||||
-rw-r--r-- | spec/policies/project_policy_spec.rb | 4 | ||||
-rw-r--r-- | spec/views/explore/projects/page_out_of_bounds.html.haml_spec.rb | 26 |
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 |