diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-06-03 00:59:19 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-06-03 00:59:19 +0300 |
commit | 1385478346704d03ab9d3a9bf8ae3812cea0b6b5 (patch) | |
tree | c2b68728119200c48fbfe09bb09397d4e31659b7 | |
parent | 361d9dae8bafae8c830d68d16ea0f76482ba9343 (diff) |
Add latest changes from gitlab-org/security/gitlab@16-0-stable-ee
56 files changed, 1161 insertions, 296 deletions
diff --git a/app/assets/javascripts/single_file_diff.js b/app/assets/javascripts/single_file_diff.js index bab167bb7e4..11896a75798 100644 --- a/app/assets/javascripts/single_file_diff.js +++ b/app/assets/javascripts/single_file_diff.js @@ -24,7 +24,9 @@ export default class SingleFileDiff { this.content = $('.diff-content', this.file); this.$chevronRightIcon = $('.diff-toggle-caret .chevron-right', this.file); this.$chevronDownIcon = $('.diff-toggle-caret .chevron-down', this.file); - this.diffForPath = this.content.find('[data-diff-for-path]').data('diffForPath'); + this.diffForPath = this.content + .find('div:not(.note-text)[data-diff-for-path]') + .data('diffForPath'); this.isOpen = !this.diffForPath; if (this.diffForPath) { this.collapsedContent = this.content; diff --git a/app/controllers/abuse_reports_controller.rb b/app/controllers/abuse_reports_controller.rb index edeac57bc42..5a4f80fcb32 100644 --- a/app/controllers/abuse_reports_controller.rb +++ b/app/controllers/abuse_reports_controller.rb @@ -1,17 +1,10 @@ # frozen_string_literal: true class AbuseReportsController < ApplicationController - before_action :set_user, only: [:new, :add_category] + before_action :set_user, only: [:add_category] feature_category :insider_threat - def new - @abuse_report = AbuseReport.new( - user_id: @user.id, - reported_from_url: params.fetch(:ref_url, '') - ) - end - def add_category @abuse_report = AbuseReport.new( user_id: @user.id, diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 15901e13c1a..af1c85532c3 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -247,13 +247,13 @@ module MergeRequestsHelper end branch = if merge_request.for_fork? - _('%{fork_icon} %{source_project_path}:%{source_branch}').html_safe % { fork_icon: fork_icon.html_safe, source_project_path: merge_request.source_project_path.html_safe, source_branch: merge_request.source_branch.html_safe } + html_escape(_('%{fork_icon} %{source_project_path}:%{source_branch}')) % { fork_icon: fork_icon.html_safe, source_project_path: merge_request.source_project_path, source_branch: merge_request.source_branch } else merge_request.source_branch end branch_title = if merge_request.for_fork? - _('%{source_project_path}:%{source_branch}').html_safe % { source_project_path: merge_request.source_project_path.html_safe, source_branch: merge_request.source_branch.html_safe } + html_escape(_('%{source_project_path}:%{source_branch}')) % { source_project_path: merge_request.source_project_path, source_branch: merge_request.source_branch } else merge_request.source_branch end diff --git a/app/models/concerns/ci/artifactable.rb b/app/models/concerns/ci/artifactable.rb index 3fdbd6a8789..974f8213a29 100644 --- a/app/models/concerns/ci/artifactable.rb +++ b/app/models/concerns/ci/artifactable.rb @@ -9,6 +9,7 @@ module Ci STORE_COLUMN = :file_store NotSupportedAdapterError = Class.new(StandardError) + FILE_FORMAT_ADAPTERS = { # While zip is a streamable file format, performing streaming # reads requires that each entry in the zip has certain headers @@ -41,6 +42,9 @@ module Ci raise NotSupportedAdapterError, 'This file format requires a dedicated adapter' end + ::Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator + .new(file: file, file_format: file_format.to_sym).validate! + log_artifacts_filesize(file.model) file.open do |stream| diff --git a/app/models/concerns/exportable.rb b/app/models/concerns/exportable.rb index 066a44912be..c305d272a14 100644 --- a/app/models/concerns/exportable.rb +++ b/app/models/concerns/exportable.rb @@ -3,19 +3,6 @@ module Exportable extend ActiveSupport::Concern - def readable_records(association, current_user: nil) - association_records = try(association) - return unless association_records.present? - - if has_many_association?(association) - DeclarativePolicy.user_scope do - association_records.select { |record| readable_record?(record, current_user) } - end - else - readable_record?(association_records, current_user) ? association_records : nil - end - end - def exportable_association?(association, current_user: nil) return false unless respond_to?(association) return true if has_many_association?(association) @@ -30,8 +17,17 @@ module Exportable exportable_restricted_associations & keys end - def has_many_association?(association_name) - self.class.reflect_on_association(association_name)&.macro == :has_many + def to_authorized_json(keys_to_authorize, current_user, options) + modified_options = filtered_associations_opts(options, keys_to_authorize) + record_hash = as_json(modified_options).with_indifferent_access + + keys_to_authorize.each do |key| + next unless record_hash.key?(key) + + record_hash[key] = authorized_association_records(key, current_user, options) + end + + record_hash.to_json end private @@ -47,4 +43,47 @@ module Exportable record.readable_by?(user) end end + + def has_many_association?(association_name) + self.class.reflect_on_association(association_name)&.macro == :has_many + end + + def readable_records(association, current_user: nil) + association_records = try(association) + return unless association_records.present? + + if has_many_association?(association) + DeclarativePolicy.user_scope do + association_records.select { |record| readable_record?(record, current_user) } + end + else + readable_record?(association_records, current_user) ? association_records : nil + end + end + + def authorized_association_records(key, current_user, options) + records = readable_records(key, current_user: current_user) + empty_assoc = has_many_association?(key) ? [] : nil + return empty_assoc unless records.present? + + assoc_opts = association_options(key, options)&.dig(key) + records.as_json(assoc_opts) + end + + def filtered_associations_opts(options, associations) + options_copy = options.deep_dup + + associations.each do |key| + assoc_opts = association_options(key, options_copy) + next unless assoc_opts + + assoc_opts[key] = { only: [:id] } + end + + options_copy + end + + def association_options(key, options) + options[:include].find { |assoc| assoc.key?(key) } + end end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index b1ec6b8ba32..8926e805d8d 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -27,6 +27,7 @@ module Issuable include ClosedAtFilterable include VersionedDescription include SortableTitle + include Exportable TITLE_LENGTH_MAX = 255 TITLE_HTML_LENGTH_MAX = 800 @@ -230,6 +231,10 @@ module Issuable issuable_severity&.severity || IssuableSeverity::DEFAULT end + def exportable_restricted_associations + super + [:notes] + end + private def validate_description_length? diff --git a/app/models/issue.rb b/app/models/issue.rb index b7125617034..f214bc0f1af 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -25,7 +25,6 @@ class Issue < ApplicationRecord include FromUnion include EachBatch include PgFullTextSearchable - include Exportable extend ::Gitlab::Utils::Override diff --git a/app/models/label.rb b/app/models/label.rb index 32b399ac461..0831ba40536 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -14,6 +14,7 @@ class Label < ApplicationRecord cache_markdown_field :description, pipeline: :single_line DEFAULT_COLOR = ::Gitlab::Color.of('#6699cc') + DESCRIPTION_LENGTH_MAX = 512.kilobytes attribute :color, ::Gitlab::Database::Type::Color.new, default: DEFAULT_COLOR @@ -32,6 +33,10 @@ class Label < ApplicationRecord validates :title, uniqueness: { scope: [:group_id, :project_id] } validates :title, length: { maximum: 255 } + # we validate the description against DESCRIPTION_LENGTH_MAX only for labels being created and on updates if + # the description changes to avoid breaking the existing labels which may have their descriptions longer + validates :description, bytesize: { maximum: -> { DESCRIPTION_LENGTH_MAX } }, if: :validate_description_length? + default_scope { order(title: :asc) } # rubocop:disable Cop/DefaultScope scope :templates, -> { where(template: true, type: [Label.name, nil]) } @@ -282,6 +287,16 @@ class Label < ApplicationRecord private + def validate_description_length? + return false unless description_changed? + + previous_description = changes['description'].first + # previous_description will be nil for new records + return true if previous_description.blank? + + previous_description.bytesize <= DESCRIPTION_LENGTH_MAX || description.bytesize > previous_description.bytesize + end + def issues_count(user, params = {}) params.merge!(subject_foreign_key => subject.id, label_name: title, scope: 'all') IssuesFinder.new(user, params.with_indifferent_access).execute.count diff --git a/app/models/project_team.rb b/app/models/project_team.rb index dd200aec807..ca1064997af 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -117,12 +117,14 @@ class ProjectTeam owners.include?(user) end - def import(source_project, current_user = nil) + def import(source_project, current_user) target_project = project source_members = source_project.project_members.to_a target_user_ids = target_project.project_members.pluck_user_ids + importer_access_level = max_member_access(current_user.id) + source_members.reject! do |member| # Skip if user already present in team !member.invite? && target_user_ids.include?(member.user_id) @@ -132,6 +134,8 @@ class ProjectTeam new_member = member.dup new_member.id = nil new_member.source = target_project + # So that a maintainer cannot import a member with owner access + new_member.access_level = [new_member.access_level, importer_access_level].min new_member.created_by = current_user new_member end diff --git a/app/models/user.rb b/app/models/user.rb index dc70ff2e232..50da6f9e491 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1549,7 +1549,9 @@ class User < ApplicationRecord # rubocop: enable CodeReuse/ServiceClass def primary_email_verified? - confirmed? && !temp_oauth_email? + return false unless confirmed? && !temp_oauth_email? + + !email_changed? || new_record? end def accept_pending_invitations! diff --git a/config/routes.rb b/config/routes.rb index 9c8ad8fe047..1b5d8beb405 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -204,7 +204,7 @@ InitializerConnections.raise_if_new_database_connection do end # Spam reports - resources :abuse_reports, only: [:new, :create] do + resources :abuse_reports, only: [:create] do collection do post :add_category end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index c52d14f59fe..525e06eda91 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -15831,7 +15831,7 @@ four standard [pagination arguments](#connection-pagination-arguments): Represents vulnerable project counts for each grade. -Returns [`[VulnerableProjectsByGrade!]!`](#vulnerableprojectsbygrade). +Returns [`[VulnerableProjectsByGrade!]`](#vulnerableprojectsbygrade). ###### Arguments diff --git a/doc/api/projects.md b/doc/api/projects.md index 3105da44906..e0f49c26e69 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -2379,6 +2379,11 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git Import members from another project. +If the importing member's role in the target project is: + +- Maintainer, then members with the Owner role in the source project are imported with the Maintainer role. +- Owner, then members with the Owner role in the source project are imported with the Owner role. + ```plaintext POST /projects/:id/import_project_members/:project_id ``` diff --git a/doc/user/project/members/index.md b/doc/user/project/members/index.md index 6e20492db05..8b2cacf084e 100644 --- a/doc/user/project/members/index.md +++ b/doc/user/project/members/index.md @@ -200,6 +200,11 @@ Prerequisite: - You must have the Maintainer or Owner role. +If the importing member's role in the target project is: + +- Maintainer, then members with the Owner role in the source project are imported with the Maintainer role. +- Owner, then members with the Owner role in the source project are imported with the Owner role. + To import users: 1. On the top bar, select **Main menu > Projects** and find your project. diff --git a/lib/api/concerns/packages/npm_endpoints.rb b/lib/api/concerns/packages/npm_endpoints.rb index afbde296161..74ad3bb296f 100644 --- a/lib/api/concerns/packages/npm_endpoints.rb +++ b/lib/api/concerns/packages/npm_endpoints.rb @@ -27,6 +27,11 @@ module API end helpers do + params :package_name do + requires :package_name, type: String, file_path: true, desc: 'Package name', + documentation: { example: 'mypackage' } + end + def redirect_or_present_audit_report redirect_registry_request( forward_to_registry: true, @@ -172,7 +177,7 @@ module API tags %w[npm_packages] end params do - requires :package_name, type: String, desc: 'Package name' + use :package_name end route_setting :authentication, job_token_allowed: true, deploy_token_allowed: true get '*package_name', format: false, requirements: ::API::Helpers::Packages::Npm::NPM_ENDPOINT_REQUIREMENTS do diff --git a/lib/banzai/filter/dollar_math_post_filter.rb b/lib/banzai/filter/dollar_math_post_filter.rb index 94d1b4bcb48..76f69a66e8d 100644 --- a/lib/banzai/filter/dollar_math_post_filter.rb +++ b/lib/banzai/filter/dollar_math_post_filter.rb @@ -17,19 +17,21 @@ module Banzai # encoded and will therefore not interfere with the detection of the dollar syntax. # Corresponds to the "$...$" syntax - DOLLAR_INLINE_PATTERN = %r{ - (?<matched>\$(?<math>(?:\S[^$\n]*?\S|[^$\s]))\$)(?:[^\d]|$) - }x.freeze + DOLLAR_INLINE_UNTRUSTED = + '(?P<matched>\$(?P<math>(?:\S[^$\n]*?\S|[^$\s]))\$)(?:[^\d]|$)' + DOLLAR_INLINE_UNTRUSTED_REGEX = + Gitlab::UntrustedRegexp.new(DOLLAR_INLINE_UNTRUSTED, multiline: false) # Corresponds to the "$$...$$" syntax - DOLLAR_DISPLAY_INLINE_PATTERN = %r{ - (?<matched>\$\$\ *(?<math>[^$\n]+?)\ *\$\$) - }x.freeze + DOLLAR_DISPLAY_INLINE_UNTRUSTED = + '(?P<matched>\$\$\ *(?P<math>[^$\n]+?)\ *\$\$)' + DOLLAR_DISPLAY_INLINE_UNTRUSTED_REGEX = + Gitlab::UntrustedRegexp.new(DOLLAR_DISPLAY_INLINE_UNTRUSTED, multiline: false) # Order dependent. Handle the `$$` syntax before the `$` syntax DOLLAR_MATH_PIPELINE = [ - { pattern: DOLLAR_DISPLAY_INLINE_PATTERN, style: :display }, - { pattern: DOLLAR_INLINE_PATTERN, style: :inline } + { pattern: DOLLAR_DISPLAY_INLINE_UNTRUSTED_REGEX, style: :display }, + { pattern: DOLLAR_INLINE_UNTRUSTED_REGEX, style: :inline } ].freeze # Do not recognize math inside these tags @@ -46,16 +48,18 @@ module Banzai next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) node_html = node.to_html - next unless node_html.match?(DOLLAR_INLINE_PATTERN) || - node_html.match?(DOLLAR_DISPLAY_INLINE_PATTERN) + next unless DOLLAR_INLINE_UNTRUSTED_REGEX.match?(node_html) || + DOLLAR_DISPLAY_INLINE_UNTRUSTED_REGEX.match?(node_html) temp_doc = Nokogiri::HTML.fragment(node_html) DOLLAR_MATH_PIPELINE.each do |pipeline| temp_doc.xpath('child::text()').each do |temp_node| html = temp_node.to_html - temp_node.content.scan(pipeline[:pattern]).each do |matched, math| - html.sub!(matched, math_html(math: math, style: pipeline[:style])) + + pipeline[:pattern].scan(temp_node.content).each do |match| + math = pipeline[:pattern].extract_named_group(:math, match) + html.sub!(match.first, math_html(math: math, style: pipeline[:style])) end temp_node.replace(html) diff --git a/lib/banzai/filter/front_matter_filter.rb b/lib/banzai/filter/front_matter_filter.rb index c788137e122..53683ce07d9 100644 --- a/lib/banzai/filter/front_matter_filter.rb +++ b/lib/banzai/filter/front_matter_filter.rb @@ -6,13 +6,13 @@ module Banzai def call lang_mapping = Gitlab::FrontMatter::DELIM_LANG - html.sub(Gitlab::FrontMatter::PATTERN) do |_match| - lang = $~[:lang].presence || lang_mapping[$~[:delim]] + Gitlab::FrontMatter::PATTERN_UNTRUSTED_REGEX.replace_gsub(html) do |match| + lang = match[:lang].presence || lang_mapping[match[:delim]] - before = $~[:before] - before = "\n#{before}" if $~[:encoding].presence + before = match[:before] + before = "\n#{before}" if match[:encoding].presence - "#{before}```#{lang}:frontmatter\n#{$~[:front_matter]}```\n" + "#{before}```#{lang}:frontmatter\n#{match[:front_matter]}```\n" end end end diff --git a/lib/banzai/filter/inline_diff_filter.rb b/lib/banzai/filter/inline_diff_filter.rb index e47ff15e7b7..2a43540934c 100644 --- a/lib/banzai/filter/inline_diff_filter.rb +++ b/lib/banzai/filter/inline_diff_filter.rb @@ -6,6 +6,14 @@ module Banzai class InlineDiffFilter < HTML::Pipeline::Filter IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set + INLINE_DIFF_DELETION_UNTRUSTED = '(?:\[\-(.*?)\-\]|\{\-(.*?)\-\})' + INLINE_DIFF_DELETION_UNTRUSTED_REGEX = + Gitlab::UntrustedRegexp.new(INLINE_DIFF_DELETION_UNTRUSTED, multiline: false) + + INLINE_DIFF_ADDITION_UNTRUSTED = '(?:\[\+(.*?)\+\]|\{\+(.*?)\+\})' + INLINE_DIFF_ADDITION_UNTRUSTED_REGEX = + Gitlab::UntrustedRegexp.new(INLINE_DIFF_ADDITION_UNTRUSTED, multiline: false) + def call doc.xpath('descendant-or-self::text()').each do |node| next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) @@ -21,8 +29,13 @@ module Banzai end def inline_diff_filter(text) - html_content = text.gsub(/(?:\[\-(.*?)\-\]|\{\-(.*?)\-\})/, '<span class="idiff left right deletion">\1\2</span>') - html_content.gsub(/(?:\[\+(.*?)\+\]|\{\+(.*?)\+\})/, '<span class="idiff left right addition">\1\2</span>') + html_content = INLINE_DIFF_DELETION_UNTRUSTED_REGEX.replace_gsub(text) do |match| + %(<span class="idiff left right deletion">#{match[1]}#{match[2]}</span>) + end + + INLINE_DIFF_ADDITION_UNTRUSTED_REGEX.replace_gsub(html_content) do |match| + %(<span class="idiff left right addition">#{match[1]}#{match[2]}</span>) + end end end end diff --git a/lib/bitbucket_server/representation/pull_request.rb b/lib/bitbucket_server/representation/pull_request.rb index acbc44f6119..9a8eec67bd7 100644 --- a/lib/bitbucket_server/representation/pull_request.rb +++ b/lib/bitbucket_server/representation/pull_request.rb @@ -80,7 +80,6 @@ module BitbucketServer state: state, title: title, source_branch_name: source_branch_name, - source_branch_sha: source_branch_sha, target_branch_name: target_branch_name, target_branch_sha: target_branch_sha } diff --git a/lib/extracts_ref.rb b/lib/extracts_ref.rb index 49c9772f760..5f73b474956 100644 --- a/lib/extracts_ref.rb +++ b/lib/extracts_ref.rb @@ -157,11 +157,11 @@ module ExtractsRef end def ambiguous_ref?(project, ref) + return false unless ref return true if project.repository.ambiguous_ref?(ref) + return false unless ref.starts_with?(%r{(refs|heads|tags)/}) - return false unless ref&.starts_with?('refs/') - - unprefixed_ref = ref.sub(%r{^refs/(heads|tags)/}, '') + unprefixed_ref = ref.sub(%r{^(refs/)?(heads|tags)/}, '') project.repository.commit(unprefixed_ref).present? end end diff --git a/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.html.haml b/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.html.haml index d8f7466a1ca..53d64fdf488 100644 --- a/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.html.haml +++ b/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.html.haml @@ -3,16 +3,16 @@ Dear GitLab user, %p - As part of our commitment to keeping GitLab secure, we have identified and addressed a vulnerability in GitLab that allowed some users to bypass the email verification process in a #{link_to("recent security release", "https://about.gitlab.com/releases/2020/05/27/security-release-13-0-1-released", target: '_blank')}. + As part of our commitment to keeping GitLab secure, we have identified and addressed a vulnerability in GitLab that allowed some users to bypass the email verification process in a #{link_to('recent security release', 'https://about.gitlab.com/releases/2020/05/27/security-release-13-0-1-released', target: '_blank')}. %p As a precautionary measure, you will need to re-verify some of your account's email addresses before continuing to use GitLab. Sorry for the inconvenience! %p - We have already sent the re-verification email with a subject line of "Confirmation instructions" from #{@verification_from_mail}. Please feel free to contribute any questions or comments to #{link_to("this issue", "https://gitlab.com/gitlab-com/www-gitlab-com/-/issues/7942", target: '_blank')}. + We have already sent the re-verification email with a subject line of 'Confirmation instructions' from #{@verification_from_mail}. Please feel free to contribute any questions or comments to #{link_to('this issue', 'https://gitlab.com/gitlab-com/www-gitlab-com/-/issues/7942', target: '_blank')}. %p - If you are not "#{@user.username}", please #{link_to 'report this to our administrator', new_abuse_report_url(user_id: @user.id)} + If you are not "#{@user.username}", please report abuse from the user's #{link_to('profile page', user_url(@user.id), target: '_blank', rel: 'noopener noreferrer')}. #{link_to('Learn more.', help_page_url('user/report_abuse', anchor: 'report-abuse-from-the-users-profile-page', target: '_blank', rel: 'noopener noreferrer'))} %p Thank you for being a GitLab user! diff --git a/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.text.erb b/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.text.erb index d20af9b9803..81ef68e99f0 100644 --- a/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.text.erb +++ b/lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.text.erb @@ -9,6 +9,8 @@ As a precautionary measure, you will need to re-verify some of your account's em We have already sent the re-verification email with a subject line of "Confirmation instructions" from <%= @verification_from_mail %>. Please feel free to contribute any questions or comments to this issue: https://gitlab.com/gitlab-com/www-gitlab-com/-/issues/7942 -If you are not "<%= @user.username %>", please report this to our administrator. Report link: <%= new_abuse_report_url(user_id: @user.id) %> +If you are not "<%= @user.username %>", please report abuse from the user's profile page: <%= user_url(@user.id) %>. + +Learn more: <%= help_page_url('user/report_abuse', anchor: 'report-abuse-from-the-users-profile-page') %> Thank you for being a GitLab user! diff --git a/lib/gitlab/checks/tag_check.rb b/lib/gitlab/checks/tag_check.rb index 007a775eaf5..5c43ca946b5 100644 --- a/lib/gitlab/checks/tag_check.rb +++ b/lib/gitlab/checks/tag_check.rb @@ -10,7 +10,8 @@ module Gitlab 'Only a project maintainer or owner can delete a protected tag.', delete_protected_tag_non_web: 'You can only delete protected tags using the web interface.', create_protected_tag: 'You are not allowed to create this tag as it is protected.', - default_branch_collision: 'You cannot use default branch name to create a tag' + default_branch_collision: 'You cannot use default branch name to create a tag', + prohibited_tag_name: 'You cannot create a tag with a prohibited pattern.' }.freeze LOG_MESSAGES = { @@ -29,11 +30,20 @@ module Gitlab end default_branch_collision_check + prohibited_tag_checks protected_tag_checks end private + def prohibited_tag_checks + return if deletion? + + if tag_name.start_with?("refs/tags/") # rubocop: disable Style/GuardClause + raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name] + end + end + def protected_tag_checks logger.log_timed(LOG_MESSAGES[__method__]) do return unless ProtectedTag.protected?(project, tag_name) # rubocop:disable Cop/AvoidReturnFromBlocks diff --git a/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator.rb b/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator.rb new file mode 100644 index 00000000000..04930da5e30 --- /dev/null +++ b/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Artifacts + class DecompressedArtifactSizeValidator + DEFAULT_MAX_BYTES = 4.gigabytes.freeze + + FILE_FORMAT_VALIDATORS = { + gzip: ::Gitlab::Ci::DecompressedGzipSizeValidator + }.freeze + + FileDecompressionError = Class.new(StandardError) + + def initialize(file:, file_format:, max_bytes: DEFAULT_MAX_BYTES) + @file = file + @file_path = file&.path + @file_format = file_format + @max_bytes = max_bytes + end + + def validate! + validator_class = FILE_FORMAT_VALIDATORS[file_format.to_sym] + + return if file_path.nil? + return if validator_class.nil? + + if file.respond_to?(:object_store) && file.object_store == ObjectStorage::Store::REMOTE + return if valid_on_storage?(validator_class) + elsif validator_class.new(archive_path: file_path, max_bytes: max_bytes).valid? + return + end + + raise(FileDecompressionError, 'File decompression error') + end + + private + + attr_reader :file_path, :file, :file_format, :max_bytes + + def valid_on_storage?(validator_class) + temp_filename = "#{SecureRandom.uuid}.gz" + + is_valid = false + Tempfile.open(temp_filename, '/tmp') do |tempfile| + tempfile.binmode + ::Faraday.get(file.url) do |req| + req.options.on_data = proc { |chunk, _| tempfile.write(chunk) } + end + + is_valid = validator_class.new(archive_path: tempfile.path, max_bytes: max_bytes).valid? + tempfile.unlink + end + + is_valid + end + end + end + end +end diff --git a/lib/gitlab/ci/decompressed_gzip_size_validator.rb b/lib/gitlab/ci/decompressed_gzip_size_validator.rb new file mode 100644 index 00000000000..a92f3007671 --- /dev/null +++ b/lib/gitlab/ci/decompressed_gzip_size_validator.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + class DecompressedGzipSizeValidator + DEFAULT_MAX_BYTES = 4.gigabytes.freeze + TIMEOUT_LIMIT = 210.seconds + + ServiceError = Class.new(StandardError) + + def initialize(archive_path:, max_bytes: DEFAULT_MAX_BYTES) + @archive_path = archive_path + @max_bytes = max_bytes + end + + def valid? + validate + end + + private + + def validate + pgrps = nil + valid_archive = true + + validate_archive_path + + Timeout.timeout(TIMEOUT_LIMIT) do + stderr_r, stderr_w = IO.pipe + stdout, wait_threads = Open3.pipeline_r(*command, pgroup: true, err: stderr_w) + + # When validation is performed on a small archive (e.g. 100 bytes) + # `wait_thr` finishes before we can get process group id. Do not + # raise exception in this scenario. + pgrps = wait_threads.map do |wait_thr| + Process.getpgid(wait_thr[:pid]) + rescue Errno::ESRCH + nil + end + pgrps.compact! + + status = wait_threads.last.value + + if status.success? + result = stdout.readline + + valid_archive = false if result.to_i > max_bytes + else + valid_archive = false + end + + ensure + stdout.close + stderr_w.close + stderr_r.close + end + + valid_archive + rescue StandardError + pgrps.each { |pgrp| Process.kill(-1, pgrp) } if pgrps + + false + end + + def validate_archive_path + Gitlab::Utils.check_path_traversal!(archive_path) + + raise(ServiceError, 'Archive path is a symlink') if File.lstat(archive_path).symlink? + raise(ServiceError, 'Archive path is not a file') unless File.file?(archive_path) + end + + def command + [['gzip', '-dc', archive_path], ['wc', '-c']] + end + + attr_reader :archive_path, :max_bytes + end + end +end diff --git a/lib/gitlab/front_matter.rb b/lib/gitlab/front_matter.rb index 093501e860b..2a759434b81 100644 --- a/lib/gitlab/front_matter.rb +++ b/lib/gitlab/front_matter.rb @@ -8,15 +8,35 @@ module Gitlab ';;;' => 'json' }.freeze - DELIM = Regexp.union(DELIM_LANG.keys) + DELIM_UNTRUSTED = "(?:#{Gitlab::FrontMatter::DELIM_LANG.keys.map { |x| RE2::Regexp.escape(x) }.join('|')})".freeze - PATTERN = %r{ - \A(?<encoding>[^\r\n]*coding:[^\r\n]*\R)? # optional encoding line - (?<before>\s*) - ^(?<delim>#{DELIM})[ \t]*(?<lang>\S*)\R # opening front matter marker (optional language specifier) - (?<front_matter>.*?) # front matter block content (not greedy) - ^(\k<delim> | \.{3}) # closing front matter marker - [^\S\r\n]*(\R|\z) - }mx.freeze + # Original pattern: + # \A(?<encoding>[^\r\n]*coding:[^\r\n]*\R)? # optional encoding line + # (?<before>\s*) + # ^(?<delim>#{DELIM})[ \t]*(?<lang>\S*)\R # opening front matter marker (optional language specifier) + # (?<front_matter>.*?) # front matter block content (not greedy) + # ^(\k<delim> | \.{3}) # closing front matter marker + # [^\S\r\n]*(\R|\z) + # rubocop:disable Style/StringConcatenation + # rubocop:disable Style/LineEndConcatenation + PATTERN_UNTRUSTED = + # optional encoding line + "\\A(?P<encoding>[^\\r\\n]*coding:[^\\r\\n]*#{::Gitlab::UntrustedRegexp::BACKSLASH_R})?" + + '(?P<before>\s*)' + + + # opening front matter marker (optional language specifier) + "^(?P<delim>#{DELIM_UNTRUSTED})[ \\t]*(?P<lang>\\S*)#{::Gitlab::UntrustedRegexp::BACKSLASH_R}" + + + # front matter block content (not greedy) + '(?P<front_matter>(?:\n|.)*?)' + + + # closing front matter marker + "^((?P<delim_closing>#{DELIM_UNTRUSTED})|\\.{3})" + + "[^\\S\\r\\n]*(#{::Gitlab::UntrustedRegexp::BACKSLASH_R}|\\z)" + # rubocop:enable Style/LineEndConcatenation + # rubocop:enable Style/StringConcatenation + + PATTERN_UNTRUSTED_REGEX = + Gitlab::UntrustedRegexp.new(PATTERN_UNTRUSTED, multiline: true) end end diff --git a/lib/gitlab/import_export/json/streaming_serializer.rb b/lib/gitlab/import_export/json/streaming_serializer.rb index 9bb0770dc90..2c64ca53f76 100644 --- a/lib/gitlab/import_export/json/streaming_serializer.rb +++ b/lib/gitlab/import_export/json/streaming_serializer.rb @@ -133,29 +133,10 @@ module Gitlab def authorized_record_json(record, options) include_keys = options[:include].flat_map(&:keys) keys_to_authorize = record.try(:restricted_associations, include_keys) - return record.to_json(options) if keys_to_authorize.blank? - - record_hash = record.as_json(options).with_indifferent_access - filtered_record_hash(record, keys_to_authorize, record_hash).to_json(options) - end - - def filtered_record_hash(record, keys_to_authorize, record_hash) - keys_to_authorize.each do |key| - next unless record_hash[key].present? - - readable = record.try(:readable_records, key, current_user: current_user) - if record.has_many_association?(key) - readable_ids = readable.pluck(:id) - record_hash[key].keep_if do |association_record| - readable_ids.include?(association_record[:id]) - end - else - record_hash[key] = nil unless readable.present? - end - end + return record.to_json(options) if keys_to_authorize.blank? - record_hash + record.to_authorized_json(keys_to_authorize, current_user, options) end def batch(relation, key) diff --git a/lib/gitlab/untrusted_regexp.rb b/lib/gitlab/untrusted_regexp.rb index fe3377dae68..150f50e4da6 100644 --- a/lib/gitlab/untrusted_regexp.rb +++ b/lib/gitlab/untrusted_regexp.rb @@ -16,6 +16,10 @@ module Gitlab class UntrustedRegexp require_dependency 're2' + # recreate Ruby's \R metacharacter + # https://ruby-doc.org/3.2.2/Regexp.html#class-Regexp-label-Character+Classes + BACKSLASH_R = '(\n|\v|\f|\r|\x{0085}|\x{2028}|\x{2029}|\r\n)' + delegate :===, :source, to: :regexp def initialize(pattern, multiline: false) diff --git a/lib/gitlab/wiki_pages/front_matter_parser.rb b/lib/gitlab/wiki_pages/front_matter_parser.rb index 071b0dde619..dbe848f0acf 100644 --- a/lib/gitlab/wiki_pages/front_matter_parser.rb +++ b/lib/gitlab/wiki_pages/front_matter_parser.rb @@ -53,7 +53,7 @@ module Gitlab include Gitlab::Utils::StrongMemoize def initialize(delim = nil, lang = '', text = nil) - @lang = lang.downcase.presence || Gitlab::FrontMatter::DELIM_LANG[delim] + @lang = lang&.downcase.presence || Gitlab::FrontMatter::DELIM_LANG[delim] @text = text&.strip! end @@ -109,11 +109,17 @@ module Gitlab end def parse_front_matter_block - wiki_content.match(Gitlab::FrontMatter::PATTERN) { |m| Block.new(m[:delim], m[:lang], m[:front_matter]) } || Block.new + if match = Gitlab::FrontMatter::PATTERN_UNTRUSTED_REGEX.match(wiki_content) + Block.new(match[:delim], match[:lang], match[:front_matter]) + else + Block.new + end end def strip_front_matter_block - wiki_content.gsub(Gitlab::FrontMatter::PATTERN, '') + Gitlab::FrontMatter::PATTERN_UNTRUSTED_REGEX.replace_gsub(wiki_content) do + '' + end end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index b652aba1fff..577f10b961c 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -193,38 +193,79 @@ RSpec.describe ProjectsController, feature_category: :projects do end end - context 'when the default branch name can resolve to another ref' do - let!(:project_with_default_branch) do - create(:project, :public, :custom_repo, files: ['somefile']).tap do |p| - p.repository.create_branch("refs/heads/refs/heads/#{other_ref}", 'master') - p.change_head("refs/heads/#{other_ref}") - end.reload + context 'when the default branch name is ambiguous' do + let_it_be(:project_with_default_branch) do + create(:project, :public, :custom_repo, files: ['somefile']) end - let(:other_ref) { 'branch-name' } + shared_examples 'ambiguous ref redirects' do + let(:project) { project_with_default_branch } + let(:branch_ref) { "refs/heads/#{ref}" } + let(:repo) { project.repository } - context 'but there is no other ref' do - it 'responds with ok' do - get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch } - expect(response).to be_ok + before do + repo.create_branch(branch_ref, 'master') + repo.change_head(ref) end - end - context 'and that other ref exists' do - let(:tree_with_default_branch) do - branch = project_with_default_branch.repository.find_branch(project_with_default_branch.default_branch) - project_tree_path(project_with_default_branch, branch.target) + after do + repo.change_head('master') + repo.delete_branch(branch_ref) end - before do - project_with_default_branch.repository.create_branch(other_ref, 'master') + subject do + get( + :show, + params: { + namespace_id: project.namespace, + id: project + } + ) + end + + context 'when there is no conflicting ref' do + let(:other_ref) { 'non-existent-ref' } + + it { is_expected.to have_gitlab_http_status(:ok) } end - it 'redirects to tree view for the default branch' do - get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch } - expect(response).to redirect_to(tree_with_default_branch) + context 'and that other ref exists' do + let(:other_ref) { 'master' } + + let(:project_default_root_tree_path) do + sha = repo.find_branch(project.default_branch).target + project_tree_path(project, sha) + end + + it 'redirects to tree view for the default branch' do + is_expected.to redirect_to(project_default_root_tree_path) + end end end + + context 'when ref starts with ref/heads/' do + let(:ref) { "refs/heads/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end + + context 'when ref starts with ref/tags/' do + let(:ref) { "refs/tags/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end + + context 'when ref starts with heads/' do + let(:ref) { "heads/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end + + context 'when ref starts with tags/' do + let(:ref) { "tags/#{other_ref}" } + + include_examples 'ambiguous ref redirects' + end end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 351583b7ef6..9cf755b2842 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -20,6 +20,10 @@ FactoryBot.define do public_email { email } end + trait :notification_email do + notification_email { email } + end + trait :private_profile do private_profile { true } end diff --git a/spec/frontend/single_file_diff_spec.js b/spec/frontend/single_file_diff_spec.js index ff2a4e31e0b..572fd3810ab 100644 --- a/spec/frontend/single_file_diff_spec.js +++ b/spec/frontend/single_file_diff_spec.js @@ -67,6 +67,21 @@ describe('SingleFileDiff', () => { expect(mock.history.get.length).toBe(1); }); + it('ignores user-defined diff path attributes', () => { + setHTMLFixture(` + <div class="diff-file"> + <div class="diff-content"> + <div class="diff-viewer" data-type="simple"> + <div class="note-text"><a data-diff-for-path="test/note/path">Test note</a></div> + <div data-diff-for-path="${blobDiffPath}">MOCK CONTENT</div> + </div> + </div> + </div> +`); + const { diffForPath } = new SingleFileDiff(document.querySelector('.diff-file')); + expect(diffForPath).toEqual(blobDiffPath); + }); + it('does not load diffs via axios for already expanded diffs', async () => { setHTMLFixture(` <div class="diff-file"> diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb index 93df9d5f94b..277869e7bd3 100644 --- a/spec/helpers/merge_requests_helper_spec.rb +++ b/spec/helpers/merge_requests_helper_spec.rb @@ -183,10 +183,18 @@ RSpec.describe MergeRequestsHelper, feature_category: :code_review_workflow do end describe '#merge_request_source_branch' do - let_it_be(:project) { create(:project) } - let(:forked_project) { fork_project(project) } - let(:merge_request_forked) { create(:merge_request, source_project: forked_project, target_project: project) } + branch_name = 'name<script>test</script>' + let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:forked_project) { fork_project(project) } + let(:merge_request_forked) do + create( + :merge_request, + source_project: forked_project, + source_branch: branch_name, + target_project: project + ) + end context 'when merge request is a fork' do subject { merge_request_source_branch(merge_request_forked) } @@ -194,6 +202,10 @@ RSpec.describe MergeRequestsHelper, feature_category: :code_review_workflow do it 'does show the fork icon' do expect(subject).to match(/fork/) end + + it 'escapes properly' do + expect(subject).to include(html_escape(branch_name)) + end end context 'when merge request is not a fork' do diff --git a/spec/lib/banzai/filter/front_matter_filter_spec.rb b/spec/lib/banzai/filter/front_matter_filter_spec.rb index b15f3ad2cd6..9d25dafb727 100644 --- a/spec/lib/banzai/filter/front_matter_filter_spec.rb +++ b/spec/lib/banzai/filter/front_matter_filter_spec.rb @@ -189,19 +189,29 @@ RSpec.describe Banzai::Filter::FrontMatterFilter, feature_category: :team_planni end end - it 'fails fast for strings with many spaces' do - content = "coding:" + " " * 50_000 + ";" + describe 'protects against malicious backtracking' do + it 'fails fast for strings with many spaces' do + content = "coding:" + " " * 50_000 + ";" - expect do - Timeout.timeout(3.seconds) { filter(content) } - end.not_to raise_error - end + expect do + Timeout.timeout(3.seconds) { filter(content) } + end.not_to raise_error + end + + it 'fails fast for strings with many newlines' do + content = "coding:\n" + ";;;" + "\n" * 10_000 + "x" - it 'fails fast for strings with many newlines' do - content = "coding:\n" + ";;;" + "\n" * 10_000 + "x" + expect do + Timeout.timeout(3.seconds) { filter(content) } + end.not_to raise_error + end + + it 'fails fast for strings with many `coding:`' do + content = "coding:" * 120_000 + "\n" * 80_000 + ";" - expect do - Timeout.timeout(3.seconds) { filter(content) } - end.not_to raise_error + expect do + Timeout.timeout(3.seconds) { filter(content) } + end.not_to raise_error + end end end diff --git a/spec/lib/banzai/filter/inline_diff_filter_spec.rb b/spec/lib/banzai/filter/inline_diff_filter_spec.rb index 1ef00139db2..1388a9053d9 100644 --- a/spec/lib/banzai/filter/inline_diff_filter_spec.rb +++ b/spec/lib/banzai/filter/inline_diff_filter_spec.rb @@ -67,4 +67,12 @@ RSpec.describe Banzai::Filter::InlineDiffFilter do doc = "<tt>START {+something added+} END</tt>" expect(filter(doc).to_html).to eq(doc) end + + it 'protects against malicious backtracking' do + doc = '[-{-' * 250_000 + + expect do + Timeout.timeout(3.seconds) { filter(doc) } + end.not_to raise_error + end end diff --git a/spec/lib/banzai/filter/math_filter_spec.rb b/spec/lib/banzai/filter/math_filter_spec.rb index ded94dd6ce5..e4ebebc0fde 100644 --- a/spec/lib/banzai/filter/math_filter_spec.rb +++ b/spec/lib/banzai/filter/math_filter_spec.rb @@ -215,6 +215,14 @@ RSpec.describe Banzai::Filter::MathFilter, feature_category: :team_planning do expect(doc.search('.js-render-math').count).to eq(2) end + it 'protects against malicious backtracking' do + doc = pipeline_filter("$$#{' ' * 1_000_000}$") + + expect do + Timeout.timeout(3.seconds) { filter(doc) } + end.not_to raise_error + end + def pipeline_filter(text) context = { project: nil, no_sourcepos: true } diff --git a/spec/lib/bitbucket_server/representation/pull_request_spec.rb b/spec/lib/bitbucket_server/representation/pull_request_spec.rb index c5d0ee8908d..5312bc1d71b 100644 --- a/spec/lib/bitbucket_server/representation/pull_request_spec.rb +++ b/spec/lib/bitbucket_server/representation/pull_request_spec.rb @@ -115,7 +115,6 @@ RSpec.describe BitbucketServer::Representation::PullRequest, feature_category: : author: "root", description: "Test", source_branch_name: "refs/heads/root/CODE_OF_CONDUCTmd-1530600625006", - source_branch_sha: "074e2b4dddc5b99df1bf9d4a3f66cfc15481fdc8", target_branch_name: "refs/heads/master", target_branch_sha: "839fa9a2d434eb697815b8fcafaecc51accfdbbc", title: "Added a new line" diff --git a/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb b/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb index f430009989b..05bd020d4e2 100644 --- a/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb +++ b/spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb @@ -7,6 +7,6 @@ RSpec.describe Gitlab::BackgroundMigration::Mailers::UnconfirmMailer do let(:subject) { described_class.unconfirm_notification_email(user) } it 'contains abuse report url' do - expect(subject.body.encoded).to include(Rails.application.routes.url_helpers.new_abuse_report_url(user_id: user.id)) + expect(subject.body.encoded).to include(Gitlab::Routing.url_helpers.user_url(user.id)) end end diff --git a/spec/lib/gitlab/checks/tag_check_spec.rb b/spec/lib/gitlab/checks/tag_check_spec.rb index 50ffa5fad10..e75b0459337 100644 --- a/spec/lib/gitlab/checks/tag_check_spec.rb +++ b/spec/lib/gitlab/checks/tag_check_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Checks::TagCheck do +RSpec.describe Gitlab::Checks::TagCheck, feature_category: :source_code_management do include_context 'change access checks context' describe '#validate!' do @@ -14,6 +14,29 @@ RSpec.describe Gitlab::Checks::TagCheck do expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You are not allowed to change existing tags on this project.') end + context "prohibited tags check" do + it "prohibits tag names that include refs/tags/ at the head" do + allow(subject).to receive(:tag_name).and_return("refs/tags/foo") + + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a tag with a prohibited pattern.") + end + + it "doesn't prohibit a nested refs/tags/ string in a tag name" do + allow(subject).to receive(:tag_name).and_return("fix-for-refs/tags/foo") + + expect { subject.validate! }.not_to raise_error + end + + context "deleting a refs/tags headed tag" do + let(:newrev) { "0000000000000000000000000000000000000000" } + let(:ref) { "refs/tags/refs/tags/267208abfe40e546f5e847444276f7d43a39503e" } + + it "doesn't prohibit the deletion of a refs/tags/ tag name" do + expect { subject.validate! }.not_to raise_error + end + end + end + context 'with protected tag' do let!(:protected_tag) { create(:protected_tag, project: project, name: 'v*') } diff --git a/spec/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator_spec.rb b/spec/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator_spec.rb new file mode 100644 index 00000000000..ef39a431d63 --- /dev/null +++ b/spec/lib/gitlab/ci/artifacts/decompressed_artifact_size_validator_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator, feature_category: :build_artifacts do + include WorkhorseHelpers + + let_it_be(:file_path) { File.join(Dir.tmpdir, 'decompressed_archive_size_validator_spec.gz') } + let(:file) { File.open(file_path) } + let(:file_format) { :gzip } + let(:max_bytes) { 20 } + let(:gzip_valid?) { true } + let(:validator) { instance_double(::Gitlab::Ci::DecompressedGzipSizeValidator, valid?: gzip_valid?) } + + before(:all) do + Zlib::GzipWriter.open(file_path) do |gz| + gz.write('Hello World!') + end + end + + after(:all) do + FileUtils.rm(file_path) + end + + before do + allow(::Gitlab::Ci::DecompressedGzipSizeValidator) + .to receive(:new) + .and_return(validator) + end + + subject { described_class.new(file: file, file_format: file_format, max_bytes: max_bytes) } + + shared_examples 'when file does not exceed allowed compressed size' do + let(:gzip_valid?) { true } + + it 'passes validation' do + expect { subject.validate! }.not_to raise_error + end + end + + shared_examples 'when file exceeds allowed decompressed size' do + let(:gzip_valid?) { false } + + it 'raises an exception' do + expect { subject.validate! } + .to raise_error(Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator::FileDecompressionError) + end + end + + describe '#validate!' do + it_behaves_like 'when file does not exceed allowed compressed size' + + it_behaves_like 'when file exceeds allowed decompressed size' + end + + context 'when file is not provided' do + let(:file) { nil } + + it 'passes validation' do + expect { subject.validate! }.not_to raise_error + end + end + + context 'when the file is located in the cloud' do + let(:remote_path) { File.join(remote_store_path, remote_id) } + + let(:file_url) { "http://s3.amazonaws.com/#{remote_path}" } + let(:file) do + instance_double(JobArtifactUploader, + path: file_path, + url: file_url, + object_store: ObjectStorage::Store::REMOTE) + end + + let(:remote_id) { 'generated-remote-id-12345' } + let(:remote_store_path) { ObjectStorage::TMP_UPLOAD_PATH } + + before do + stub_request(:get, %r{s3.amazonaws.com/#{remote_path}}) + .to_return(status: 200, body: File.read('spec/fixtures/build.env.gz')) + end + + it_behaves_like 'when file does not exceed allowed compressed size' + + it_behaves_like 'when file exceeds allowed decompressed size' + end + + context 'when file_format is not on the list' do + let_it_be(:file_format) { 'rar' } + + it 'passes validation' do + expect { subject.validate! }.not_to raise_error + end + end +end diff --git a/spec/lib/gitlab/ci/decompressed_gzip_size_validator_spec.rb b/spec/lib/gitlab/ci/decompressed_gzip_size_validator_spec.rb new file mode 100644 index 00000000000..6ca3f4d415e --- /dev/null +++ b/spec/lib/gitlab/ci/decompressed_gzip_size_validator_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Ci::DecompressedGzipSizeValidator, feature_category: :importers do + let_it_be(:filepath) { File.join(Dir.tmpdir, 'decompressed_gzip_size_validator_spec.gz') } + + before(:all) do + create_compressed_file + end + + after(:all) do + FileUtils.rm(filepath) + end + + subject { described_class.new(archive_path: filepath, max_bytes: max_bytes) } + + describe '#valid?' do + let(:max_bytes) { 20 } + + context 'when file does not exceed allowed decompressed size' do + it 'returns true' do + expect(subject.valid?).to eq(true) + end + + context 'when the waiter thread no longer exists due to being terminated or crashing' do + it 'gracefully handles the absence of the waiter without raising exception' do + allow(Process).to receive(:getpgid).and_raise(Errno::ESRCH) + + expect(subject.valid?).to eq(true) + end + end + end + + context 'when file exceeds allowed decompressed size' do + let(:max_bytes) { 1 } + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when exception occurs during header readings' do + shared_examples 'raises exception and terminates validator process group' do + let(:std) { instance_double(IO, close: nil) } + let(:wait_thr) { double } + let(:wait_threads) { [wait_thr, wait_thr] } + + before do + allow(Process).to receive(:getpgid).and_return(2) + allow(Open3).to receive(:pipeline_r).and_return([std, wait_threads]) + allow(wait_thr).to receive(:[]).with(:pid).and_return(1) + allow(wait_thr).to receive(:value).and_raise(exception) + end + + it 'terminates validator process group' do + expect(Process).to receive(:kill).with(-1, 2).twice + expect(subject.valid?).to eq(false) + end + end + + context 'when timeout occurs' do + let(:exception) { Timeout::Error } + + include_examples 'raises exception and terminates validator process group' + end + + context 'when exception occurs' do + let(:error_message) { 'Error!' } + let(:exception) { StandardError.new(error_message) } + + include_examples 'raises exception and terminates validator process group' + end + end + + describe 'archive path validation' do + let(:filesize) { nil } + + context 'when archive path is traversed' do + let(:filepath) { '/foo/../bar' } + + it 'does not pass validation' do + expect(subject.valid?).to eq(false) + end + end + end + + context 'when archive path is not a string' do + let(:filepath) { 123 } + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when archive path is a symlink' do + let(:filepath) { File.join(Dir.tmpdir, 'symlink') } + + before do + FileUtils.ln_s(filepath, filepath, force: true) + end + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when archive path is not a file' do + let(:filepath) { Dir.mktmpdir } + let(:filesize) { File.size(filepath) } + + after do + FileUtils.rm_rf(filepath) + end + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + end + + def create_compressed_file + Zlib::GzipWriter.open(filepath) do |gz| + gz.write('Hello World!') + end + end +end diff --git a/spec/lib/gitlab/import_export/project/tree_saver_spec.rb b/spec/lib/gitlab/import_export/project/tree_saver_spec.rb index 4166eba4e8e..d8b8903c8ca 100644 --- a/spec/lib/gitlab/import_export/project/tree_saver_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_saver_spec.rb @@ -7,6 +7,8 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver, :with_license, feature_ let_it_be(:exportable_path) { 'project' } let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } + let_it_be(:private_project) { create(:project, :private, group: group) } + let_it_be(:private_mr) { create(:merge_request, source_project: private_project, project: private_project) } let_it_be(:project) { setup_project } shared_examples 'saves project tree successfully' do @@ -118,6 +120,13 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver, :with_license, feature_ expect(reviewer).not_to be_nil expect(reviewer['user_id']).to eq(user.id) end + + it 'has merge requests system notes' do + system_notes = subject.first['notes'].select { |note| note['system'] } + + expect(system_notes.size).to eq(1) + expect(system_notes.first['note']).to eq('merged') + end end context 'with snippets' do @@ -492,6 +501,9 @@ RSpec.describe Gitlab::ImportExport::Project::TreeSaver, :with_license, feature_ create(:milestone, project: project) discussion_note = create(:discussion_note, noteable: issue, project: project) mr_note = create(:note, noteable: merge_request, project: project) + create(:system_note, noteable: merge_request, project: project, author: user, note: 'merged') + private_system_note = "mentioned in merge request #{private_mr.to_reference(project)}" + create(:system_note, noteable: merge_request, project: project, author: user, note: private_system_note) create(:note, noteable: snippet, project: project) create(:note_on_commit, author: user, diff --git a/spec/models/concerns/ci/artifactable_spec.rb b/spec/models/concerns/ci/artifactable_spec.rb index 6af244a5a0f..f61fb74fa4e 100644 --- a/spec/models/concerns/ci/artifactable_spec.rb +++ b/spec/models/concerns/ci/artifactable_spec.rb @@ -36,6 +36,21 @@ RSpec.describe Ci::Artifactable do expect { |b| artifact.each_blob(&b) }.to yield_control.exactly(3).times end end + + context 'when decompressed artifact size validator fails' do + let(:artifact) { build(:ci_job_artifact, :junit) } + + before do + allow_next_instance_of(Gitlab::Ci::DecompressedGzipSizeValidator) do |instance| + allow(instance).to receive(:valid?).and_return(false) + end + end + + it 'fails on blob' do + expect { |b| artifact.each_blob(&b) } + .to raise_error(::Gitlab::Ci::Artifacts::DecompressedArtifactSizeValidator::FileDecompressionError) + end + end end context 'when file format is raw' do diff --git a/spec/models/concerns/exportable_spec.rb b/spec/models/concerns/exportable_spec.rb index 74709b06403..4f36fa52c6b 100644 --- a/spec/models/concerns/exportable_spec.rb +++ b/spec/models/concerns/exportable_spec.rb @@ -9,6 +9,7 @@ RSpec.describe Exportable, feature_category: :importers do let_it_be(:issue) { create(:issue, project: project, milestone: milestone) } let_it_be(:note1) { create(:system_note, project: project, noteable: issue) } let_it_be(:note2) { create(:system_note, project: project, noteable: issue) } + let_it_be(:options) { { include: [{ notes: { only: [:note] }, milestone: { only: :title } }] } } let_it_be(:model_klass) do Class.new(ApplicationRecord) do @@ -28,19 +29,27 @@ RSpec.describe Exportable, feature_category: :importers do subject { model_klass.new } - describe '.readable_records' do + describe '.to_authorized_json' do let_it_be(:model_record) { model_klass.new } - context 'when model does not respond to association name' do - it 'returns nil' do - expect(subject.readable_records(:foo, current_user: user)).to be_nil + context 'when key to authorize is not an association name' do + it 'returns string without given key' do + expect(subject.to_authorized_json([:foo], user, options)).not_to include('foo') end end - context 'when model does respond to association name' do + context 'when key to authorize is an association name' do + let(:key_to_authorize) { :notes } + + subject(:record_json) { model_record.to_authorized_json([key_to_authorize], user, options) } + context 'when there are no records' do - it 'returns nil' do - expect(model_record.readable_records(:notes, current_user: user)).to be_nil + before do + allow(model_record).to receive(:notes).and_return(Note.none) + end + + it 'returns string including the empty association' do + expect(record_json).to include("\"notes\":[]") end end @@ -57,8 +66,9 @@ RSpec.describe Exportable, feature_category: :importers do end end - it 'returns collection of readable records' do - expect(model_record.readable_records(:notes, current_user: user)).to contain_exactly(note1, note2) + it 'returns string containing all records' do + expect(record_json) + .to include("\"notes\":[{\"note\":\"#{note1.note}\"},{\"note\":\"#{note2.note}\"}]") end end @@ -70,8 +80,19 @@ RSpec.describe Exportable, feature_category: :importers do end end - it 'returns collection of readable records' do - expect(model_record.readable_records(:notes, current_user: user)).to eq([]) + it 'returns string including the empty association' do + expect(record_json).to include("\"notes\":[]") + end + end + + context 'when user can read some records' do + before do + allow(model_record).to receive(:readable_records).with(:notes, current_user: user) + .and_return([note1]) + end + + it 'returns string containing readable records only' do + expect(record_json).to include("\"notes\":[{\"note\":\"#{note1.note}\"}]") end end end @@ -87,13 +108,15 @@ RSpec.describe Exportable, feature_category: :importers do it 'calls #readable_by?' do expect(note1).to receive(:readable_by?).with(user) - model_record.readable_records(:notes, current_user: user) + record_json end end context 'with single relation' do + let(:key_to_authorize) { :milestone } + before do - allow(model_record).to receive(:try).with(:milestone).and_return(issue.milestone) + allow(model_record).to receive(:milestone).and_return(issue.milestone) end context 'when user can read the record' do @@ -101,8 +124,8 @@ RSpec.describe Exportable, feature_category: :importers do allow(milestone).to receive(:readable_by?).with(user).and_return(true) end - it 'returns collection of readable records' do - expect(model_record.readable_records(:milestone, current_user: user)).to eq(milestone) + it 'returns string including association' do + expect(record_json).to include("\"milestone\":{\"title\":\"#{milestone.title}\"}") end end @@ -111,8 +134,8 @@ RSpec.describe Exportable, feature_category: :importers do allow(milestone).to receive(:readable_by?).with(user).and_return(false) end - it 'returns collection of readable records' do - expect(model_record.readable_records(:milestone, current_user: user)).to be_nil + it 'returns string with null association' do + expect(record_json).to include("\"milestone\":null") end end end @@ -211,26 +234,4 @@ RSpec.describe Exportable, feature_category: :importers do end end end - - describe '.has_many_association?' do - let(:model_associations) { [:notes, :labels] } - - context 'when association type is `has_many`' do - it 'returns true' do - expect(subject.has_many_association?(:notes)).to eq(true) - end - end - - context 'when association type is `has_one`' do - it 'returns true' do - expect(subject.has_many_association?(:milestone)).to eq(false) - end - end - - context 'when association type is `belongs_to`' do - it 'returns true' do - expect(subject.has_many_association?(:project)).to eq(false) - end - end - end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index e11c7e98287..42b70cbb858 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -1057,4 +1057,22 @@ RSpec.describe Issuable do end end end + + context 'with exportable associations' do + let_it_be(:project) { create(:project, group: create(:group, :private)) } + + context 'for issues' do + let_it_be_with_reload(:resource) { create(:issue, project: project) } + + it_behaves_like 'an exportable' + end + + context 'for merge requests' do + let_it_be_with_reload(:resource) do + create(:merge_request, source_project: project, project: project) + end + + it_behaves_like 'an exportable' + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 26a1edcbcff..65e02da2b5d 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -44,6 +44,122 @@ RSpec.describe Label do is_expected.to allow_value("customer's request").for(:title) is_expected.to allow_value('s' * 255).for(:title) end + + describe 'description length' do + let(:invalid_description) { 'x' * (::Label::DESCRIPTION_LENGTH_MAX + 1) } + let(:valid_description) { 'short description' } + let(:label) { build(:label, project: project, description: description) } + + let(:error_message) do + format( + _('is too long (%{size}). The maximum size is %{max_size}.'), + size: ActiveSupport::NumberHelper.number_to_human_size(invalid_description.bytesize), + max_size: ActiveSupport::NumberHelper.number_to_human_size(::Label::DESCRIPTION_LENGTH_MAX) + ) + end + + subject(:validate) { label.validate } + + context 'when label is a new record' do + context 'when description exceeds the maximum size' do + let(:description) { invalid_description } + + it 'adds a description too long error' do + validate + + expect(label.errors[:description]).to contain_exactly(error_message) + end + end + + context 'when description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + end + + context 'when label is an existing record' do + before do + label.description = existing_description + label.save!(validate: false) + label.description = description + end + + context 'when record already had a valid description' do + let(:existing_description) { 'small difference so it triggers description_changed?' } + + context 'when new description exceeds the maximum size' do + let(:description) { invalid_description } + + it 'adds a description too long error' do + validate + + expect(label.errors[:description]).to contain_exactly(error_message) + end + end + + context 'when new description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + end + + context 'when record existed with an invalid description' do + let(:existing_description) { "#{invalid_description} small difference so it triggers description_changed?" } + + context 'when description is not changed' do + let(:description) { existing_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + + context 'when new description exceeds the maximum size' do + context 'when new description is shorter than existing description' do + let(:description) { invalid_description } + + it 'allows updating descriptions that already existed above the limit' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + + context 'when new description is longer than existing description' do + let(:description) { "#{existing_description}1" } + + it 'adds a description too long error' do + validate + + expect(label.errors[:description]).to contain_exactly(error_message) + end + end + end + + context 'when new description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(label.errors).not_to have_key(:description) + end + end + end + end + end end describe 'scopes' do diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 20dae056646..67af7a049cb 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -106,36 +106,6 @@ RSpec.describe ProjectMember do end end - describe '.import_team' do - before do - @project_1 = create(:project) - @project_2 = create(:project) - - @user_1 = create :user - @user_2 = create :user - - @project_1.add_developer(@user_1) - @project_2.add_reporter(@user_2) - - @status = @project_2.team.import(@project_1) - end - - it { expect(@status).to be_truthy } - - describe 'project 2 should get user 1 as developer. user_2 should not be changed' do - it { expect(@project_2.users).to include(@user_1) } - it { expect(@project_2.users).to include(@user_2) } - - it { expect(Ability.allowed?(@user_1, :create_project, @project_2)).to be_truthy } - it { expect(Ability.allowed?(@user_2, :read_project, @project_2)).to be_truthy } - end - - describe 'project 1 should not be changed' do - it { expect(@project_1.users).to include(@user_1) } - it { expect(@project_1.users).not_to include(@user_2) } - end - end - describe '.truncate_teams' do before do @project_1 = create(:project) diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index f4cf3130aa9..aca6f7d053f 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -164,6 +164,57 @@ RSpec.describe ProjectTeam, feature_category: :subgroups do end end + describe '#import_team' do + let_it_be(:source_project) { create(:project) } + let_it_be(:target_project) { create(:project) } + let_it_be(:source_project_owner) { source_project.first_owner } + let_it_be(:source_project_developer) { create(:user) { |user| source_project.add_developer(user) } } + let_it_be(:current_user) { create(:user) { |user| target_project.add_maintainer(user) } } + + subject(:import) { target_project.team.import(source_project, current_user) } + + it { is_expected.to be_truthy } + + it 'target project includes source member with the same access' do + import + + imported_member_access = target_project.members.find_by!(user: source_project_developer).access_level + expect(imported_member_access).to eq(Gitlab::Access::DEVELOPER) + end + + it 'does not change the source project members' do + import + + expect(source_project.users).to include(source_project_developer) + expect(source_project.users).not_to include(current_user) + end + + shared_examples 'imports source owners with correct access' do + specify do + import + + source_owner_access_in_target = target_project.members.find_by!(user: source_project_owner).access_level + expect(source_owner_access_in_target).to eq(target_access_level) + end + end + + context 'when importer is a maintainer in target project' do + it_behaves_like 'imports source owners with correct access' do + let(:target_access_level) { Gitlab::Access::MAINTAINER } + end + end + + context 'when importer is an owner in target project' do + before do + target_project.add_owner(current_user) + end + + it_behaves_like 'imports source owners with correct access' do + let(:target_access_level) { Gitlab::Access::OWNER } + end + end + end + describe '#find_member' do context 'personal project' do let(:project) do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c73dac7251e..a643dd0b4e5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -684,34 +684,116 @@ RSpec.describe User, feature_category: :user_profile do end end - describe '#commit_email=' do - subject(:user) { create(:user) } + shared_examples 'for user notification, public, and commit emails' do + context 'when confirmed primary email' do + let(:user) { create(:user) } + let(:email) { user.email } - it 'can be set to a confirmed email' do - confirmed = create(:email, :confirmed, user: user) - user.commit_email = confirmed.email + it 'can be set' do + set_email - expect(user).to be_valid + expect(user).to be_valid + end + + context 'when primary email is changed' do + before do + user.email = generate(:email) + end + + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + end + + context 'when confirmed secondary email' do + let(:email) { create(:email, :confirmed, user: user).email } + + it 'can be set' do + set_email + + expect(user).to be_valid + end + end + + context 'when unconfirmed secondary email' do + let(:email) { create(:email, user: user).email } + + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + end + + context 'when invalid confirmed secondary email' do + let(:email) { create(:email, :confirmed, :skip_validate, user: user, email: 'invalid') } + + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + end end - it 'can not be set to an unconfirmed email' do - unconfirmed = create(:email, user: user) - user.commit_email = unconfirmed.email + context 'when unconfirmed primary email ' do + let(:user) { create(:user, :unconfirmed) } + let(:email) { user.email } - expect(user).not_to be_valid + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end end - it 'can not be set to a non-existent email' do - user.commit_email = 'non-existent-email@nonexistent.nonexistent' + context 'when new record' do + let(:user) { build(:user, :unconfirmed) } + let(:email) { user.email } - expect(user).not_to be_valid + it 'can not be set' do + set_email + + expect(user).not_to be_valid + end + + context 'when skipping confirmation' do + before do + user.skip_confirmation = true + end + + it 'can be set' do + set_email + + expect(user).to be_valid + end + end end + end - it 'can not be set to an invalid email, even if confirmed' do - confirmed = create(:email, :confirmed, :skip_validate, user: user, email: 'invalid') - user.commit_email = confirmed.email + describe 'notification_email' do + include_examples 'for user notification, public, and commit emails' do + subject(:set_email) do + user.notification_email = email + end + end + end - expect(user).not_to be_valid + describe 'public_email' do + include_examples 'for user notification, public, and commit emails' do + subject(:set_email) do + user.public_email = email + end + end + end + + describe 'commit_email' do + include_examples 'for user notification, public, and commit emails' do + subject(:set_email) do + user.commit_email = email + end end end @@ -3532,15 +3614,40 @@ RSpec.describe User, feature_category: :user_profile do describe '#verified_emails' do let(:user) { create(:user) } + let!(:confirmed_email) { create(:email, :confirmed, user: user) } - it 'returns only confirmed emails' do - email_confirmed = create :email, user: user, confirmed_at: Time.current - create :email, user: user + before do + create(:email, user: user) + end + it 'returns only confirmed emails' do expect(user.verified_emails).to contain_exactly( user.email, user.private_commit_email, - email_confirmed.email + confirmed_email.email + ) + end + + it 'does not return primary email when primary email is changed' do + original_email = user.email + user.email = generate(:email) + + expect(user.verified_emails).to contain_exactly( + user.private_commit_email, + confirmed_email.email, + original_email + ) + end + + it 'does not return unsaved primary email even if skip_confirmation is enabled' do + original_email = user.email + user.skip_confirmation = true + user.email = generate(:email) + + expect(user.verified_emails).to contain_exactly( + user.private_commit_email, + confirmed_email.email, + original_email ) end end diff --git a/spec/requests/abuse_reports_controller_spec.rb b/spec/requests/abuse_reports_controller_spec.rb index 4b81394aea3..0b9cf24230d 100644 --- a/spec/requests/abuse_reports_controller_spec.rb +++ b/spec/requests/abuse_reports_controller_spec.rb @@ -19,43 +19,6 @@ RSpec.describe AbuseReportsController, feature_category: :insider_threat do sign_in(reporter) end - describe 'GET new' do - let(:ref_url) { 'http://example.com' } - - it 'sets the instance variables' do - get new_abuse_report_path(user_id: user.id, ref_url: ref_url) - - expect(assigns(:abuse_report)).to be_kind_of(AbuseReport) - expect(assigns(:abuse_report)).to have_attributes( - user_id: user.id, - reported_from_url: ref_url - ) - end - - context 'when the user has already been deleted' do - it 'redirects the reporter to root_path' do - user_id = user.id - user.destroy! - - get new_abuse_report_path(user_id: user_id) - - expect(response).to redirect_to root_path - expect(flash[:alert]).to eq(_('Cannot create the abuse report. The user has been deleted.')) - end - end - - context 'when the user has already been blocked' do - it 'redirects the reporter to the user\'s profile' do - user.block - - get new_abuse_report_path(user_id: user.id) - - expect(response).to redirect_to user - expect(flash[:alert]).to eq(_('Cannot create the abuse report. This user has been blocked.')) - end - end - end - describe 'POST add_category', :aggregate_failures do subject(:request) { post add_category_abuse_reports_path, params: request_params } diff --git a/spec/requests/api/npm_instance_packages_spec.rb b/spec/requests/api/npm_instance_packages_spec.rb index 591a8ee68dc..97de7fa9e52 100644 --- a/spec/requests/api/npm_instance_packages_spec.rb +++ b/spec/requests/api/npm_instance_packages_spec.rb @@ -13,11 +13,12 @@ RSpec.describe API::NpmInstancePackages, feature_category: :package_registry do describe 'GET /api/v4/packages/npm/*package_name' do let(:url) { api("/packages/npm/#{package_name}") } + subject { get(url) } + it_behaves_like 'handling get metadata requests', scope: :instance + it_behaves_like 'rejects invalid package names' context 'with a duplicate package name in another project' do - subject { get(url) } - let_it_be(:project2) { create(:project, :public, namespace: namespace) } let_it_be(:package2) do create(:npm_package, diff --git a/spec/requests/api/npm_project_packages_spec.rb b/spec/requests/api/npm_project_packages_spec.rb index 1f5ebc80824..d673645c51a 100644 --- a/spec/requests/api/npm_project_packages_spec.rb +++ b/spec/requests/api/npm_project_packages_spec.rb @@ -23,6 +23,9 @@ RSpec.describe API::NpmProjectPackages, feature_category: :package_registry do it_behaves_like 'handling get metadata requests', scope: :project it_behaves_like 'accept get request on private project with access to package registry for everyone' + it_behaves_like 'rejects invalid package names' do + subject { get(url) } + end end describe 'GET /api/v4/projects/:id/packages/npm/-/package/*package_name/dist-tags' do diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 2bd23340a88..a59ca5211ed 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -351,14 +351,6 @@ RSpec.describe InvitesController, 'routing' do end end -RSpec.describe AbuseReportsController, 'routing' do - let_it_be(:user) { create(:user) } - - it 'to #new' do - expect(get("/-/abuse_reports/new?user_id=#{user.id}")).to route_to('abuse_reports#new', user_id: user.id.to_s) - end -end - RSpec.describe SentNotificationsController, 'routing' do it 'to #unsubscribe' do expect(get("/-/sent_notifications/4bee17d4a63ed60cf5db53417e9aeb4c/unsubscribe")) diff --git a/spec/support/shared_examples/features/reportable_note_shared_examples.rb b/spec/support/shared_examples/features/reportable_note_shared_examples.rb index 133da230bed..264cc9c798b 100644 --- a/spec/support/shared_examples/features/reportable_note_shared_examples.rb +++ b/spec/support/shared_examples/features/reportable_note_shared_examples.rb @@ -6,7 +6,6 @@ RSpec.shared_examples 'reportable note' do |type| let(:comment) { find("##{ActionView::RecordIdentifier.dom_id(note)}") } let(:more_actions_selector) { '.more-actions.dropdown' } - let(:abuse_report_path) { new_abuse_report_path(user_id: note.author.id, ref_url: noteable_note_url(note)) } it 'has an edit button' do expect(comment).to have_selector('.js-note-edit') diff --git a/spec/support/shared_examples/models/exportable_shared_examples.rb b/spec/support/shared_examples/models/exportable_shared_examples.rb index 37c3e68fd5f..57e231e4a6e 100644 --- a/spec/support/shared_examples/models/exportable_shared_examples.rb +++ b/spec/support/shared_examples/models/exportable_shared_examples.rb @@ -1,27 +1,28 @@ # frozen_string_literal: true -RSpec.shared_examples 'resource with exportable associations' do - before do - stub_licensed_features(stubbed_features) if stubbed_features.any? - end +RSpec.shared_examples 'an exportable' do |restricted_association: :project| + let_it_be(:user) { create(:user) } describe '#exportable_association?' do - let(:association) { single_association } + let(:association) { restricted_association } subject { resource.exportable_association?(association, current_user: user) } it { is_expected.to be_falsey } - context 'when user can read resource' do + context 'when user can only read resource' do before do - group.add_developer(user) + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(user, :"read_#{resource.to_ability_name}", resource) + .and_return(true) end it { is_expected.to be_falsey } context "when user can read resource's association" do before do - other_group.add_developer(user) + allow(resource).to receive(:readable_record?).with(anything, user).and_return(true) end it { is_expected.to be_truthy } @@ -31,41 +32,48 @@ RSpec.shared_examples 'resource with exportable associations' do it { is_expected.to be_falsey } end - - context 'for an unauthenticated user' do - let(:user) { nil } - - it { is_expected.to be_falsey } - end end end end - describe '#readable_records' do - subject { resource.readable_records(association, current_user: user) } + describe '#to_authorized_json' do + let(:options) { { include: [{ notes: { only: [:id] } }] } } + + subject { resource.to_authorized_json(keys, user, options) } before do - group.add_developer(user) + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(user, :"read_#{resource.to_ability_name}", resource) + .and_return(true) end context 'when association not supported' do - let(:association) { :foo } + let(:keys) { [:foo] } - it { is_expected.to be_nil } + it { is_expected.not_to include('foo') } end context 'when association is `:notes`' do - let(:association) { :notes } + let_it_be(:readable_note) { create(:system_note, noteable: resource, project: project, note: 'readable') } + let_it_be(:restricted_note) { create(:system_note, noteable: resource, project: project, note: 'restricted') } - it { is_expected.to match_array([readable_note]) } + let(:restricted_note_access) { false } + let(:keys) { [:notes] } - context 'when user have access' do - before do - other_group.add_developer(user) - end + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :read_note, readable_note).and_return(true) + allow(Ability).to receive(:allowed?).with(user, :read_note, restricted_note).and_return(restricted_note_access) + end + + it { is_expected.to include("\"notes\":[{\"id\":#{readable_note.id}}]") } + + context 'when user have access to all notes' do + let(:restricted_note_access) { true } - it 'returns all records' do - is_expected.to match_array([readable_note, restricted_note]) + it 'string includes all notes' do + is_expected.to include("\"notes\":[{\"id\":#{readable_note.id}},{\"id\":#{restricted_note.id}}]") end end end diff --git a/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb index f53532d00d7..f430db61976 100644 --- a/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb @@ -849,3 +849,14 @@ RSpec.shared_examples 'handling different package names, visibilities and user r it_behaves_like example_name, status: status end end + +RSpec.shared_examples 'rejects invalid package names' do + let(:package_name) { "%0d%0ahttp:/%2fexample.com" } + + it do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(Gitlab::Json.parse(response.body)).to eq({ 'error' => 'package_name should be a valid file path' }) + end +end |