diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 558ca8b6e8..e218910f9e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -614,7 +614,8 @@ docs lint: # Build HTML from Markdown - bundle exec nanoc # Check the internal links - - bundle exec nanoc check internal_links + # Disabled until https://gitlab.com/gitlab-com/gitlab-docs/issues/305 is resolved + # - bundle exec nanoc check internal_links downtime_check: <<: *rake-exec diff --git a/CHANGELOG.md b/CHANGELOG.md index 9de77431eb..4eb40fc0af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,33 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.5.6 (2018-12-28) + +### Security (17 changes) + +- Escape label and milestone titles to prevent XSS in GFM autocomplete. !2741 +- Validate LFS hrefs before downloading them. +- Ensure that build token is only used when running. +- Add subresources removal to member destroy service. +- Prevent a path traversal attack on global file templates. +- Allow changing group CI/CD settings only for owners. +- Authorize before reading job information via API. +- Prevent leaking protected variables for ambiguous refs. +- Escape html entities in LabelReferenceFilter when no label found. +- Prevent private snippets from being embeddable. +- Issuable no longer is visible to users when project can't be viewed. +- Don't expose cross project repositories through diffs when creating merge reqeusts. +- Fix SSRF with import_url and remote mirror url. +- Fix persistent symlink in project import. +- Set URL rel attribute for broken URLs. +- Project guests no longer are able to see refs page. +- Delete confidential todos for user when downgraded to Guest. + +### Other (1 change) + +- Fix due date test. !23845 + + ## 11.5.5 (2018-12-20) ### Security (1 change) diff --git a/VERSION b/VERSION index b0525db05f..252c408627 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -11.5.5 +11.5.6 diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index 6f8b73564d..835206b210 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -244,7 +244,7 @@ class GfmAutoComplete { displayTpl(value) { let tmpl = GfmAutoComplete.Loading.template; if (value.title != null) { - tmpl = GfmAutoComplete.Milestones.template; + tmpl = GfmAutoComplete.Milestones.templateFunction(value.title); } return tmpl; }, @@ -311,7 +311,7 @@ class GfmAutoComplete { searchKey: 'search', data: GfmAutoComplete.defaultLoadingData, displayTpl(value) { - let tmpl = GfmAutoComplete.Labels.template; + let tmpl = GfmAutoComplete.Labels.templateFunction(value.color, value.title); if (GfmAutoComplete.isLoading(value)) { tmpl = GfmAutoComplete.Loading.template; } @@ -576,9 +576,11 @@ GfmAutoComplete.Members = { }, }; GfmAutoComplete.Labels = { - template: - // eslint-disable-next-line no-template-curly-in-string - '
  • ${title}
    • ', + templateFunction(color, title) { + return `
  • ${_.escape(title)}
    • `; + }, }; // Issues, MergeRequests and Snippets GfmAutoComplete.Issues = { @@ -588,8 +590,9 @@ GfmAutoComplete.Issues = { }; // Milestones GfmAutoComplete.Milestones = { - // eslint-disable-next-line no-template-curly-in-string - template: '
  • ${title}
    • ', + templateFunction(title) { + return `
  • ${_.escape(title)}
    • `; + }, }; GfmAutoComplete.Loading = { template: diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index ca713192c9..6402e01ddc 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -35,7 +35,9 @@ module MembershipActions respond_to do |format| format.html do - message = "User was successfully removed from #{source_type}." + source = source_type == 'group' ? 'group and any subresources' : source_type + + message = "User was successfully removed from #{source}." redirect_to members_page_url, notice: message end diff --git a/app/controllers/groups/settings/ci_cd_controller.rb b/app/controllers/groups/settings/ci_cd_controller.rb index c1dcc463de..f476f428fd 100644 --- a/app/controllers/groups/settings/ci_cd_controller.rb +++ b/app/controllers/groups/settings/ci_cd_controller.rb @@ -4,7 +4,7 @@ module Groups module Settings class CiCdController < Groups::ApplicationController skip_cross_project_access_check :show - before_action :authorize_admin_pipeline! + before_action :authorize_admin_group! def show define_ci_variables @@ -26,8 +26,8 @@ module Groups .map { |variable| variable.present(current_user: current_user) } end - def authorize_admin_pipeline! - return render_404 unless can?(current_user, :admin_pipeline, group) + def authorize_admin_group! + return render_404 unless can?(current_user, :admin_group, group) end end end diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index a44acb12bd..255f1f3569 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -75,7 +75,14 @@ class Projects::SnippetsController < Projects::ApplicationController format.json do render_blob_json(blob) end - format.js { render 'shared/snippets/show'} + + format.js do + if @snippet.embeddable? + render 'shared/snippets/show' + else + head :not_found + end + end end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8bf93bfd68..878816475b 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -19,6 +19,7 @@ class ProjectsController < Projects::ApplicationController before_action :lfs_blob_ids, only: [:show], if: [:repo_exists?, :project_view_files?] before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export] before_action :present_project, only: [:edit] + before_action :authorize_download_code!, only: [:refs] # Authorize before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export] diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index dd9bf17cf0..8ea5450b4e 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -80,7 +80,13 @@ class SnippetsController < ApplicationController render_blob_json(blob) end - format.js { render 'shared/snippets/show' } + format.js do + if @snippet.embeddable? + render 'shared/snippets/show' + else + head :not_found + end + end end end diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index 5a21403bc5..75106e62cc 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -18,12 +18,13 @@ module MembersHelper "remove #{member.user.name} from" end - "#{text} #{action} the #{member.source.human_name} #{member.real_source_type.humanize(capitalize: false)}?" + "#{text} #{action} the #{member.source.human_name} #{source_text(member)}?" end def remove_member_title(member) action = member.request? ? 'Deny access request' : 'Remove user' - "#{action} from #{member.real_source_type.humanize(capitalize: false)}" + + "#{action} from #{source_text(member)}" end def leave_confirmation_message(member_source) @@ -35,4 +36,14 @@ module MembersHelper options = params.slice(:search, :sort).merge(options) "#{request.path}?#{options.to_param}" end + + private + + def source_text(member) + type = member.real_source_type.humanize(capitalize: false) + + return type if member.request? || member.invite? || type != 'group' + + 'group and any subresources' + end end diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index c7d31f3469..a20c47ed91 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -130,12 +130,4 @@ module SnippetsHelper link_to external_snippet_icon('download'), download_url, class: 'btn', target: '_blank', title: 'Download', rel: 'noopener noreferrer' end - - def public_snippet? - if @snippet.project_id? - can?(nil, :read_project_snippet, @snippet) - else - can?(nil, :read_personal_snippet, @snippet) - end - end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 889f8ce27a..4b943486af 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -10,6 +10,7 @@ module Ci include Importable include Gitlab::Utils::StrongMemoize include Deployable + include HasRef belongs_to :project, inverse_of: :builds belongs_to :runner @@ -152,6 +153,10 @@ module Ci .execute(build) # rubocop: enable CodeReuse/ServiceClass end + + def find_running_by_token(token) + running.find_by_token(token) + end end state_machine :status do @@ -638,11 +643,11 @@ module Ci def secret_group_variables return [] unless project.group - project.group.ci_variables_for(ref, project) + project.group.ci_variables_for(git_ref, project) end def secret_project_variables(environment: persisted_environment) - project.ci_variables_for(ref: ref, environment: environment) + project.ci_variables_for(ref: git_ref, environment: environment) end def steps diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 56010e899a..9941baa323 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -11,6 +11,7 @@ module Ci include Gitlab::Utils::StrongMemoize include AtomicInternalId include EnumWithNil + include HasRef belongs_to :project, inverse_of: :pipelines belongs_to :user @@ -374,10 +375,6 @@ module Ci @commit ||= Commit.lazy(project, sha) end - def branch? - !tag? - end - def stuck? pending_builds.any?(&:stuck?) end @@ -577,7 +574,7 @@ module Ci end def protected_ref? - strong_memoize(:protected_ref) { project.protected_for?(ref) } + strong_memoize(:protected_ref) { project.protected_for?(git_ref) } end def legacy_trigger @@ -697,16 +694,6 @@ module Ci end end - def git_ref - if branch? - Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s - elsif tag? - Gitlab::Git::TAG_REF_PREFIX + ref.to_s - else - raise ArgumentError, 'Invalid pipeline type!' - end - end - def latest_builds_status return 'failed' unless yaml_errors.blank? diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb new file mode 100644 index 0000000000..d7089294ef --- /dev/null +++ b/app/models/concerns/has_ref.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module HasRef + extend ActiveSupport::Concern + + def branch? + !tag? + end + + def git_ref + if branch? + Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s + elsif tag? + Gitlab::Git::TAG_REF_PREFIX + ref.to_s + end + end +end diff --git a/app/models/member.rb b/app/models/member.rb index 0696ea46c8..99da80b5f9 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -76,6 +76,7 @@ class Member < ActiveRecord::Base scope :owners, -> { active.where(access_level: OWNER) } scope :owners_and_maintainers, -> { active.where(access_level: [OWNER, MAINTAINER]) } scope :owners_and_masters, -> { owners_and_maintainers } # @deprecated + scope :with_user, -> (user) { where(user: user) } scope :order_name_asc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'ASC')) } scope :order_name_desc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'DESC')) } diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index fc49ee7ac8..2c9e1ba1d8 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -12,6 +12,8 @@ class GroupMember < Member validates :source_type, format: { with: /\ANamespace\z/ } default_scope { where(source_type: SOURCE_TYPE) } + scope :in_groups, ->(groups) { where(source_id: groups.select(:id)) } + after_create :update_two_factor_requirement, unless: :invite? after_destroy :update_two_factor_requirement, unless: :invite? diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index 537f2a3a23..5fa38ee74d 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -14,6 +14,10 @@ class ProjectMember < Member default_scope { where(source_type: SOURCE_TYPE) } scope :in_project, ->(project) { where(source_id: project.id) } + scope :in_namespaces, ->(groups) do + joins('INNER JOIN projects ON projects.id = members.source_id') + .where('projects.namespace_id in (?)', groups.select(:id)) + end class << self # Add users to projects with passed access option diff --git a/app/models/project.rb b/app/models/project.rb index 10792753ad..8b060f9fa1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -300,10 +300,9 @@ class Project < ActiveRecord::Base validates :namespace, presence: true validates :name, uniqueness: { scope: :namespace_id } - validates :import_url, url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS }, - ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS }, - allow_localhost: false, - enforce_user: true }, if: [:external_import?, :import_url_changed?] + validates :import_url, public_url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS }, + ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS }, + enforce_user: true }, if: [:external_import?, :import_url_changed?] validates :star_count, numericality: { greater_than_or_equal_to: 0 } validate :check_limit, on: :create validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? } @@ -1818,10 +1817,21 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - if repository.branch_exists?(ref) - ProtectedBranch.protected?(self, ref) - elsif repository.tag_exists?(ref) - ProtectedTag.protected?(self, ref) + raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) + + resolved_ref = repository.expand_ref(ref) || ref + return false unless Gitlab::Git.tag_ref?(resolved_ref) || Gitlab::Git.branch_ref?(resolved_ref) + + ref_name = if resolved_ref == ref + Gitlab::Git.ref_name(resolved_ref) + else + ref + end + + if Gitlab::Git.branch_ref?(resolved_ref) + ProtectedBranch.protected?(self, ref_name) + elsif Gitlab::Git.tag_ref?(resolved_ref) + ProtectedTag.protected?(self, ref_name) end end diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index c1f53b5da4..b2d04d198c 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -18,7 +18,7 @@ class RemoteMirror < ActiveRecord::Base belongs_to :project, inverse_of: :remote_mirrors - validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true } + validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true } before_save :set_new_remote_name, if: :mirror_url_changed? diff --git a/app/models/repository.rb b/app/models/repository.rb index 6e179f61a7..cf04308983 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -26,6 +26,7 @@ class Repository delegate :bundle_to_disk, to: :raw_repository CreateTreeError = Class.new(StandardError) + AmbiguousRefError = Class.new(StandardError) # Methods that cache data from the Git repository. # @@ -176,6 +177,18 @@ class Repository tags.find { |tag| tag.name == name } end + def ambiguous_ref?(ref) + tag_exists?(ref) && branch_exists?(ref) + end + + def expand_ref(ref) + if tag_exists?(ref) + Gitlab::Git::TAG_REF_PREFIX + ref + elsif branch_exists?(ref) + Gitlab::Git::BRANCH_REF_PREFIX + ref + end + end + def add_branch(user, branch_name, ref) branch = raw_repository.add_branch(branch_name, user: user, target: ref) diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 11856b5590..f9b23bbbf6 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -175,6 +175,12 @@ class Snippet < ActiveRecord::Base :visibility_level end + def embeddable? + ability = project_id? ? :read_project_snippet : :read_personal_snippet + + Ability.allowed?(nil, ability, self) + end + def notes_with_associations notes.includes(:author) end diff --git a/app/models/todo.rb b/app/models/todo.rb index 7b64615f69..d9b86d941b 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -4,6 +4,11 @@ class Todo < ActiveRecord::Base include Sortable include FromUnion + # Time to wait for todos being removed when not visible for user anymore. + # Prevents TODOs being removed by mistake, for example, removing access from a user + # and giving it back again. + WAIT_FOR_DELETE = 1.hour + ASSIGNED = 1 MENTIONED = 2 BUILD_FAILED = 3 diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 6d8b575102..ecb2797d1d 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -11,7 +11,7 @@ class IssuablePolicy < BasePolicy @user && @subject.assignee_or_author?(@user) end - rule { assignee_or_author }.policy do + rule { can?(:guest_access) & assignee_or_author }.policy do enable :read_issue enable :update_issue enable :reopen_issue diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 0bf0e967dc..83ffc3dc8c 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -31,7 +31,7 @@ module Groups def after_update if group.previous_changes.include?(:visibility_level) && group.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id) + TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id) end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index fba252b0ba..67b246a6bd 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -38,7 +38,7 @@ module Issues if issue.previous_changes.include?('confidential') # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::ConfidentialIssueWorker.perform_in(1.hour, issue.id) if issue.confidential? + TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, issue.id) if issue.confidential? create_confidentiality_note(issue) end diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb index d734571f83..e78affff79 100644 --- a/app/services/members/base_service.rb +++ b/app/services/members/base_service.rb @@ -47,5 +47,11 @@ module Members raise "Unknown action '#{action}' on #{member}!" end end + + def enqueue_delete_todos(member) + type = member.is_a?(GroupMember) ? 'Group' : 'Project' + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::EntityLeaveWorker.perform_in(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) + end end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index c186a5971d..828871125a 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -2,9 +2,11 @@ module Members class DestroyService < Members::BaseService - def execute(member, skip_authorization: false) + def execute(member, skip_authorization: false, skip_subresources: false) raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member) + @skip_auth = skip_authorization + return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user) member.destroy @@ -15,7 +17,8 @@ module Members notification_service.decline_access_request(member) end - enqeue_delete_todos(member) + delete_subresources(member) unless skip_subresources + enqueue_delete_todos(member) after_execute(member: member) @@ -24,7 +27,30 @@ module Members private - def enqeue_delete_todos(member) + def delete_subresources(member) + return unless member.is_a?(GroupMember) && member.user && member.group + + delete_project_members(member) + delete_subgroup_members(member) if Group.supports_nested_groups? + end + + def delete_project_members(member) + groups = member.group.self_and_descendants + + ProjectMember.in_namespaces(groups).with_user(member.user).each do |project_member| + self.class.new(current_user).execute(project_member, skip_authorization: @skip_auth) + end + end + + def delete_subgroup_members(member) + groups = member.group.descendants + + GroupMember.in_groups(groups).with_user(member.user).each do |group_member| + self.class.new(current_user).execute(group_member, skip_authorization: @skip_auth, skip_subresources: true) + end + end + + def enqueue_delete_todos(member) type = member.is_a?(GroupMember) ? 'Group' : 'Project' # don't enqueue immediately to prevent todos removal in case of a mistake TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type) diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index 1f5618dae5..ff8d5c1d8c 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -10,9 +10,18 @@ module Members if member.update(params) after_execute(action: permission, old_access_level: old_access_level, member: member) + + # Deletes only confidential issues todos for guests + enqueue_delete_todos(member) if downgrading_to_guest? end member end + + private + + def downgrading_to_guest? + params[:access_level] == Gitlab::Access::GUEST + end end end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 6c69452e2a..fe29df2286 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -17,7 +17,7 @@ module MergeRequests merge_request.source_project = find_source_project merge_request.target_project = find_target_project merge_request.target_branch = find_target_branch - merge_request.can_be_created = branches_valid? + merge_request.can_be_created = projects_and_branches_valid? # compare branches only if branches are valid, otherwise # compare_branches may raise an error @@ -48,15 +48,19 @@ module MergeRequests to: :merge_request def find_source_project - return source_project if source_project.present? && can?(current_user, :read_project, source_project) + return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) project end def find_target_project - return target_project if target_project.present? && can?(current_user, :read_project, target_project) + return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) - project.default_merge_request_target + target_project = project.default_merge_request_target + + return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) + + project end def find_target_branch @@ -71,10 +75,11 @@ module MergeRequests params[:target_branch].present? end - def branches_valid? + def projects_and_branches_valid? + return false if source_project.nil? || target_project.nil? return false unless source_branch_specified? || target_branch_specified? - validate_branches + validate_projects_and_branches errors.blank? end @@ -93,7 +98,12 @@ module MergeRequests end end - def validate_branches + def validate_projects_and_branches + merge_request.validate_target_project + merge_request.validate_fork + + return if errors.any? + add_error('You must select source and target branch') unless branches_present? add_error('You must select different branches') if same_source_and_target? add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists? diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index f9b9781ad5..b512844343 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -12,28 +12,43 @@ module Projects return if LfsObject.exists?(oid: oid) - sanitized_uri = Gitlab::UrlSanitizer.new(url) - Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS) + sanitized_uri = sanitize_url!(url) with_tmp_file(oid) do |file| - size = download_and_save_file(file, sanitized_uri) - lfs_object = LfsObject.new(oid: oid, size: size, file: file) + download_and_save_file(file, sanitized_uri) + lfs_object = LfsObject.new(oid: oid, size: file.size, file: file) project.all_lfs_objects << lfs_object end + rescue Gitlab::UrlBlocker::BlockedUrlError => e + Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}") rescue StandardError => e - Rails.logger.error("LFS file with oid #{oid} could't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}") + Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}") end # rubocop: enable CodeReuse/ActiveRecord private + def sanitize_url!(url) + Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri| + # Just validate that HTTP/HTTPS protocols are used. The + # subsequent Gitlab::HTTP.get call will do network checks + # based on the settings. + Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, + protocols: VALID_PROTOCOLS) + end + end + def download_and_save_file(file, sanitized_uri) - IO.copy_stream(open(sanitized_uri.sanitized_url, headers(sanitized_uri)), file) # rubocop:disable Security/Open + response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment| + file.write(fragment) + end + + raise StandardError, "Received error code #{response.code}" unless response.success? end def headers(sanitized_uri) - {}.tap do |headers| + query_options.tap do |headers| credentials = sanitized_uri.credentials if credentials[:user].present? || credentials[:password].present? @@ -43,10 +58,14 @@ module Projects end end + def query_options + { stream_body: true } + end + def with_tmp_file(oid) create_tmp_storage_dir - File.open(File.join(tmp_storage_dir, oid), 'w') { |file| yield file } + File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file } end def create_tmp_storage_dir diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 93e48fc019..dd1b9680ec 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -61,9 +61,9 @@ module Projects if project.previous_changes.include?(:visibility_level) && project.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id) + TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) elsif (project_changed_feature_keys & todos_features_changes).present? - TodosDestroyer::PrivateFeaturesWorker.perform_in(1.hour, project.id) + TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) end if project.previous_changes.include?('path') diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index 10bfc30492..a43296aa80 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -30,7 +30,7 @@ - if @snippet.updated_at != @snippet.created_at = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true) - - if public_snippet? + - if @snippet.embeddable? .embed-snippet .input-group .input-group-prepend diff --git a/debian/changelog b/debian/changelog index 5e467e4d74..5d084c8889 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,14 @@ +gitlab (11.5.6+dfsg-1) unstable; urgency=high + + * New upstream version 11.5.6+dfsg (Closes: #918086) (Fixes: CVE-2018-20488, + CVE-2018-20489, CVE-2018-20490, CVE-2018-20491, CVE-2018-20492, + CVE-2018-20493, CVE-2018-20494, CVE-2018-20495, CVE-2018-20496, + CVE-2018-20497, CVE-2018-20498, CVE-2018-20499, CVE-2018-20500, + CVE-2018-20501, CVE-2018-20507) + * Bump Standards-Version to 4.3.0 + + -- Sruthi Chandran Thu, 03 Jan 2019 12:56:20 +0530 + gitlab (11.5.5+dfsg-1~bpo9+1) stretch-backports; urgency=medium * Rebuild for stretch-backports. diff --git a/debian/control b/debian/control index f5bc9cd777..8ffe3f1dfa 100644 --- a/debian/control +++ b/debian/control @@ -7,7 +7,7 @@ Uploaders: Cédric Boutillier , Balasankar C , Sruthi Chandran Build-Depends: debhelper (>= 10~), gem2deb, bc -Standards-Version: 4.2.1 +Standards-Version: 4.3.0 Vcs-Git: https://salsa.debian.org/ruby-team/gitlab.git Vcs-Browser: https://salsa.debian.org/ruby-team/gitlab Homepage: https://about.gitlab.com/ diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index f94d592d0d..830f17aa7f 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -35,6 +35,9 @@ A Todo appears in your Todos dashboard when: - the author, or - have set it to automatically merge once pipeline succeeds. +NOTE: **Note:** +When an user no longer has access to a resource related to a Todo like an issue, merge request, project or group the related Todos, for security reasons, gets deleted within the next hour. The delete is delayed to prevent data loss in case user got their access revoked by mistake. + ### Directly addressed Todos > [Introduced][ce-7926] in GitLab 9.0. diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5572e86985..d8bbcb4acc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1346,7 +1346,17 @@ module API end class Dependency < Grape::Entity - expose :id, :name, :token + expose :id, :name + expose :token do |dependency, options| + # overrides the job's dependency authorization token + # with the token of the job that is being run + # this way we use the parent job auth token + # + # ideally we would change the runner implementation to + # use different token, but this would require upgrade of + # all runners which is impossible + options[:auth_token] + end expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts? } end @@ -1374,7 +1384,10 @@ module API expose :artifacts, using: Artifacts expose :cache, using: Cache expose :credentials, using: Credentials - expose :dependencies, using: Dependency + expose :dependencies do |model| + Dependency.represent(model.dependencies, + options.merge(auth_token: model.token)) + end expose :features end end diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 45d0343bc8..1a296c8ddb 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -36,26 +36,32 @@ module API def validate_job!(job) not_found! unless job - yield if block_given? - project = job.project - forbidden!('Project has been deleted!') if project.nil? || project.pending_delete? - forbidden!('Job has been erased!') if job.erased? + job_forbidden!(job, 'Project has been deleted!') if project.nil? || project.pending_delete? + job_forbidden!(job, 'Job has been erased!') if job.erased? + job_forbidden!(job, 'Not running!') unless job.running? end - def authenticate_job! - job = Ci::Build.find_by_id(params[:id]) - - validate_job!(job) do - forbidden! unless job_token_valid?(job) - end - - job - end - - def job_token_valid?(job) + def authenticate_job_by_token! token = (params[JOB_TOKEN_PARAM] || env[JOB_TOKEN_HEADER]).to_s - token && job.valid_token?(token) + + Ci::Build.find_by_token(token).tap do |job| + validate_job!(job) + end + end + + # we look for a job that has ID and token matching + def authenticate_job! + authenticate_job_by_token!.tap do |job| + job_forbidden!(job, 'Invalid Job ID!') unless job.id == params[:id] + end + end + + # we look for a job that has been shared via pipeline using the ID + def authenticate_pipeline_job! + job = authenticate_job_by_token! + + job.pipeline.builds.find(params[:id]) end def max_artifacts_size diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 697555c960..4cd46516f1 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -38,6 +38,8 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ':id/jobs' do + authorize_read_builds! + builds = user_project.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) @@ -56,7 +58,10 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ':id/pipelines/:pipeline_id/jobs' do + authorize!(:read_pipeline, user_project) pipeline = user_project.pipelines.find(params[:pipeline_id]) + authorize!(:read_build, pipeline) + builds = pipeline.builds builds = filter_builds(builds, params[:scope]) builds = builds.preload(:job_artifacts_archive, :job_artifacts, project: [:namespace]) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 2f15f3a7d7..3be2c28197 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -146,7 +146,6 @@ module API end put '/:id' do job = authenticate_job! - job_forbidden!(job, 'Job is not running') unless job.running? job.trace.set(params[:trace]) if params[:trace] @@ -174,7 +173,6 @@ module API end patch '/:id/trace' do job = authenticate_job! - job_forbidden!(job, 'Job is not running') unless job.running? error!('400 Missing header Content-Range', 400) unless request.headers.key?('Content-Range') content_range = request.headers['Content-Range'] @@ -217,8 +215,7 @@ module API require_gitlab_workhorse! Gitlab::Workhorse.verify_api_request!(headers) - job = authenticate_job! - forbidden!('Job is not running') unless job.running? + authenticate_job! if params[:filesize] file_size = params[:filesize].to_i @@ -261,7 +258,6 @@ module API require_gitlab_workhorse! job = authenticate_job! - forbidden!('Job is not running!') unless job.running? artifacts = UploadedFile.from_params(params, :file, JobArtifactUploader.workhorse_local_upload_path) metadata = UploadedFile.from_params(params, :metadata, JobArtifactUploader.workhorse_local_upload_path) @@ -308,7 +304,7 @@ module API optional :direct_download, default: false, type: Boolean, desc: %q(Perform direct download from remote storage instead of proxying artifacts) end get '/:id/artifacts' do - job = authenticate_job! + job = authenticate_pipeline_job! present_carrierwave_file!(job.artifacts_file, supports_direct_download: params[:direct_download]) end diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 2e6d742de2..4f60b6f84c 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -9,11 +9,10 @@ module Banzai def call links.each do |node| uri = uri(node['href'].to_s) - next unless uri - node.set_attribute('href', uri.to_s) + node.set_attribute('href', uri.to_s) if uri - if SCHEMES.include?(uri.scheme) && external_url?(uri) + if SCHEMES.include?(uri&.scheme) && !internal_url?(uri) node.set_attribute('rel', 'nofollow noreferrer noopener') node.set_attribute('target', '_blank') end @@ -35,11 +34,12 @@ module Banzai doc.xpath(query) end - def external_url?(uri) + def internal_url?(uri) + return false if uri.nil? # Relative URLs miss a hostname - return false unless uri.hostname + return true unless uri.hostname - uri.hostname != internal_url.hostname + uri.hostname == internal_url.hostname end def internal_url diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index 04ec38209c..f90a35952e 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -29,7 +29,7 @@ module Banzai if label yield match, label.id, project, namespace, $~ else - match + escape_html_entities(match) end end end @@ -102,6 +102,10 @@ module Banzai CGI.unescapeHTML(text.to_s) end + def escape_html_entities(text) + CGI.escapeHTML(text.to_s) + end + def object_link_title(object, matches) # use title of wrapped element instead nil diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 6eb5f9e230..f52af8364d 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -296,7 +296,7 @@ module Gitlab private def find_build_by_token(token) - ::Ci::Build.running.find_by_token(token) + ::Ci::Build.find_running_by_token(token) end end end diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 05978804d9..0635afed11 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -54,7 +54,13 @@ module Gitlab def protected_ref? strong_memoize(:protected_ref) do - project.protected_for?(ref) + project.protected_for?(origin_ref) + end + end + + def ambiguous_ref? + strong_memoize(:ambiguous_ref) do + project.repository.ambiguous_ref?(origin_ref) end end end diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index d88851d824..9c6c2bc8e2 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -16,6 +16,10 @@ module Gitlab unless @command.sha return error('Commit not found') end + + if @command.ambiguous_ref? + return error('Ref is ambiguous') + end end def break? diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index c4aac228b2..44a62586a2 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref.start_with?(TAG_REF_PREFIX) + ref =~ /^#{TAG_REF_PREFIX}.+/ end def branch_ref?(ref) - ref.start_with?(BRANCH_REF_PREFIX) + ref =~ /^#{BRANCH_REF_PREFIX}.+/ end def blank_ref?(ref) diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 362d5cc451..0188238365 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -118,7 +118,7 @@ describe Groups::GroupMembersController do it '[HTML] removes user from members' do delete :destroy, group_id: group, id: member - expect(response).to set_flash.to 'User was successfully removed from group.' + expect(response).to set_flash.to 'User was successfully removed from group and any subresources.' expect(response).to redirect_to(group_group_members_path(group)) expect(group.members).not_to include member end diff --git a/spec/controllers/groups/settings/ci_cd_controller_spec.rb b/spec/controllers/groups/settings/ci_cd_controller_spec.rb index 06ccace824..1bcc30915a 100644 --- a/spec/controllers/groups/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/groups/settings/ci_cd_controller_spec.rb @@ -5,30 +5,65 @@ describe Groups::Settings::CiCdController do let(:user) { create(:user) } before do - group.add_maintainer(user) sign_in(user) end describe 'GET #show' do - it 'renders show with 200 status code' do - get :show, group_id: group + context 'when user is owner' do + before do + group.add_owner(user) + end - expect(response).to have_gitlab_http_status(200) - expect(response).to render_template(:show) + it 'renders show with 200 status code' do + get :show, group_id: group + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:show) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :show, group_id: group + + expect(response).to have_gitlab_http_status(404) + end end end describe 'PUT #reset_registration_token' do subject { put :reset_registration_token, group_id: group } - it 'resets runner registration token' do - expect { subject }.to change { group.reload.runners_token } + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'resets runner registration token' do + expect { subject }.to change { group.reload.runners_token } + end + + it 'redirects the user to admin runners page' do + subject + + expect(response).to redirect_to(group_settings_ci_cd_path) + end end - it 'redirects the user to admin runners page' do - subject + context 'when user is not owner' do + before do + group.add_maintainer(user) + end - expect(response).to redirect_to(group_settings_ci_cd_path) + it 'renders a 404' do + subject + + expect(response).to have_gitlab_http_status(404) + end end end end diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 9c383bd762..70bf182cde 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -371,6 +371,46 @@ describe Projects::SnippetsController do end end + describe "GET #show for embeddable content" do + let(:project_snippet) { create(:project_snippet, snippet_permission, project: project, author: user) } + + before do + sign_in(user) + + get :show, namespace_id: project.namespace, project_id: project, id: project_snippet.to_param, format: :js + end + + context 'when snippet is private' do + let(:snippet_permission) { :private } + + it 'responds with status 404' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when snippet is public' do + let(:snippet_permission) { :public } + + it 'responds with status 200' do + expect(assigns(:snippet)).to eq(project_snippet) + expect(response).to have_gitlab_http_status(200) + end + end + + context 'when the project is private' do + let(:project) { create(:project_empty_repo, :private) } + + context 'when snippet is public' do + let(:project_snippet) { create(:project_snippet, :public, project: project, author: user) } + + it 'responds with status 404' do + expect(assigns(:snippet)).to eq(project_snippet) + expect(response).to have_gitlab_http_status(404) + end + end + end + end + describe 'GET #raw' do let(:project_snippet) do create( diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 7849bec476..febbced605 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -590,10 +590,10 @@ describe ProjectsController do end describe "GET refs" do - let(:public_project) { create(:project, :public, :repository) } + let(:project) { create(:project, :public, :repository) } it 'gets a list of branches and tags' do - get :refs, namespace_id: public_project.namespace, id: public_project, sort: 'updated_desc' + get :refs, namespace_id: project.namespace, id: project, sort: 'updated_desc' parsed_body = JSON.parse(response.body) expect(parsed_body['Branches']).to include('master') @@ -603,7 +603,7 @@ describe ProjectsController do end it "gets a list of branches, tags and commits" do - get :refs, namespace_id: public_project.namespace, id: public_project, ref: "123456" + get :refs, namespace_id: project.namespace, id: project, ref: "123456" parsed_body = JSON.parse(response.body) expect(parsed_body["Branches"]).to include("master") @@ -618,7 +618,7 @@ describe ProjectsController do end it "gets a list of branches, tags and commits" do - get :refs, namespace_id: public_project.namespace, id: public_project, ref: "123456" + get :refs, namespace_id: project.namespace, id: project, ref: "123456" parsed_body = JSON.parse(response.body) expect(parsed_body["Branches"]).to include("master") @@ -626,6 +626,22 @@ describe ProjectsController do expect(parsed_body["Commits"]).to include("123456") end end + + context 'when private project' do + let(:project) { create(:project, :repository) } + + context 'as a guest' do + it 'renders forbidden' do + user = create(:user) + project.add_guest(user) + + sign_in(user) + get :refs, namespace_id: project.namespace, id: project + + expect(response).to have_gitlab_http_status(404) + end + end + end end describe 'POST #preview_markdown' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 9effe47ab0..f3d6493786 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -80,6 +80,12 @@ describe SnippetsController do expect(assigns(:snippet)).to eq(personal_snippet) expect(response).to have_gitlab_http_status(200) end + + it 'responds with status 404 when embeddable content is requested' do + get :show, id: personal_snippet.to_param, format: :js + + expect(response).to have_gitlab_http_status(404) + end end end @@ -106,6 +112,12 @@ describe SnippetsController do expect(assigns(:snippet)).to eq(personal_snippet) expect(response).to have_gitlab_http_status(200) end + + it 'responds with status 404 when embeddable content is requested' do + get :show, id: personal_snippet.to_param, format: :js + + expect(response).to have_gitlab_http_status(404) + end end context 'when not signed in' do @@ -131,6 +143,13 @@ describe SnippetsController do expect(assigns(:snippet)).to eq(personal_snippet) expect(response).to have_gitlab_http_status(200) end + + it 'responds with status 200 when embeddable content is requested' do + get :show, id: personal_snippet.to_param, format: :js + + expect(assigns(:snippet)).to eq(personal_snippet) + expect(response).to have_gitlab_http_status(200) + end end context 'when not signed in' do diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 89e0cdd8ed..57e3ddfb39 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -7,7 +7,7 @@ describe 'Group variables', :js do let(:page_path) { group_settings_ci_cd_path(group) } before do - group.add_maintainer(user) + group.add_owner(user) gitlab_sign_in(user) visit page_path diff --git a/spec/features/issues/gfm_autocomplete_spec.rb b/spec/features/issues/gfm_autocomplete_spec.rb index 7c591dacce..b8d7e48510 100644 --- a/spec/features/issues/gfm_autocomplete_spec.rb +++ b/spec/features/issues/gfm_autocomplete_spec.rb @@ -3,6 +3,8 @@ require 'rails_helper' describe 'GFM autocomplete', :js do let(:issue_xss_title) { 'This will execute alert Tries to access private repo of public project' do + let(:current_user) { create(:user) } + let(:private_project) do + create(:project, :public, :repository, + path: 'nothing-to-see-here', + name: 'nothing to see here', + repository_access_level: ProjectFeature::PRIVATE) + end + let(:owned_project) do + create(:project, :public, :repository, + namespace: current_user.namespace, + creator: current_user) + end + + context 'when the user enters the querystring info for the other project' do + let(:mr_path) do + project_new_merge_request_diffs_path( + owned_project, + merge_request: { + source_project_id: private_project.id, + source_branch: 'feature' + } + ) + end + + before do + sign_in current_user + visit mr_path + end + + it "does not mention the project the user can't see the repo of" do + expect(page).not_to have_content('nothing-to-see-here') + end + end +end diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index cb7a912946..09de983f66 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -259,8 +259,9 @@ describe 'Runners' do context 'group runners in group settings' do let(:group) { create(:group) } + before do - group.add_maintainer(user) + group.add_owner(user) end context 'group with no runners' do diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index 4590904c93..908e8960f3 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/spec/helpers/members_helper_spec.rb @@ -16,7 +16,7 @@ describe MembersHelper do it { expect(remove_member_message(project_member_invite)).to eq "Are you sure you want to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.full_name} project?" } it { expect(remove_member_message(project_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{project.full_name} project?" } it { expect(remove_member_message(project_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{project.full_name} project?" } - it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group?" } + it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group and any subresources?" } it { expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group?" } it { expect(remove_member_message(group_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{group.name} group?" } it { expect(remove_member_message(group_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{group.name} group?" } @@ -33,7 +33,7 @@ describe MembersHelper do it { expect(remove_member_title(project_member)).to eq 'Remove user from project' } it { expect(remove_member_title(project_member_request)).to eq 'Deny access request from project' } - it { expect(remove_member_title(group_member)).to eq 'Remove user from group' } + it { expect(remove_member_title(group_member)).to eq 'Remove user from group and any subresources' } it { expect(remove_member_title(group_member_request)).to eq 'Deny access request from group' } end diff --git a/spec/javascripts/boards/components/issue_due_date_spec.js b/spec/javascripts/boards/components/issue_due_date_spec.js index 9e49330c05..054cf8c5b7 100644 --- a/spec/javascripts/boards/components/issue_due_date_spec.js +++ b/spec/javascripts/boards/components/issue_due_date_spec.js @@ -49,10 +49,11 @@ describe('Issue Due Date component', () => { it('should render month and day for other dates', () => { date.setDate(date.getDate() + 17); vm = createComponent(date); + const today = new Date(); + const isDueInCurrentYear = today.getFullYear() === date.getFullYear(); + const format = isDueInCurrentYear ? 'mmm d' : 'mmm d, yyyy'; - expect(vm.$el.querySelector('time').textContent.trim()).toEqual( - dateFormat(date, 'mmm d', true), - ); + expect(vm.$el.querySelector('time').textContent.trim()).toEqual(dateFormat(date, format, true)); }); it('should contain the correct `.text-danger` css class for overdue issue', () => { diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index 2a3c0cd78b..e6dae8d538 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -49,16 +49,16 @@ describe Banzai::Filter::ExternalLinkFilter do end context 'for invalid urls' do - it 'skips broken hrefs' do + it 'adds rel and target attributes to broken hrefs' do doc = filter %q(

    Google

    ) - expected = %q(

    Google

    ) + expected = %q(

    Google

    ) expect(doc.to_html).to eq(expected) end - it 'skips improperly formatted mailtos' do + it 'adds rel and target to improperly formatted mailtos' do doc = filter %q(

    Email

    ) - expected = %q(

    Email

    ) + expected = %q(

    Email

    ) expect(doc.to_html).to eq(expected) end diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 00257ed790..9cfdb9e53a 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -236,6 +236,24 @@ describe Banzai::Filter::LabelReferenceFilter do end end + context 'References with html entities' do + let!(:label) { create(:label, name: '<html>', project: project) } + + it 'links to a valid reference' do + doc = reference_filter('See ~"<html>"') + + expect(doc.css('a').first.attr('href')).to eq urls + .project_issues_url(project, label_name: label.name) + expect(doc.text).to eq 'See ' + end + + it 'ignores invalid label names and escapes entities' do + act = %(Label #{Label.reference_prefix}"<non valid>") + + expect(reference_filter(act).to_html).to eq act + end + end + describe 'consecutive references' do let(:bug) { create(:label, name: 'bug', project: project) } let(:feature_proposal) { create(:label, name: 'feature proposal', project: project) } diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 75a177d2d1..6aa802ce6f 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -182,4 +182,24 @@ describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(false) } end end + + describe '#ambiguous_ref' do + let(:project) { create(:project, :repository) } + let(:command) { described_class.new(project: project, origin_ref: 'ref') } + + subject { command.ambiguous_ref? } + + context 'when ref is not ambiguous' do + it { is_expected. to eq(false) } + end + + context 'when ref is ambiguous' do + before do + project.repository.add_tag(project.creator, 'ref', 'master') + project.repository.add_branch(project.creator, 'ref', 'master') + end + + it { is_expected. to eq(true) } + end + end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index 284aed91e2..1b014ecfaa 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -14,6 +14,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do Gitlab::Ci::Pipeline::Chain::Command.new( project: project, current_user: user, + origin_ref: 'master', seeds_block: nil) end @@ -106,6 +107,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do Gitlab::Ci::Pipeline::Chain::Command.new( project: project, current_user: user, + origin_ref: 'master', seeds_block: seeds_block) end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb index fb1b53fc55..a7cad423d0 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb @@ -42,6 +42,27 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do end end + context 'when ref is ambiguous' do + let(:project) do + create(:project, :repository).tap do |proj| + proj.repository.add_tag(user, 'master', 'master') + end + end + let(:command) do + Gitlab::Ci::Pipeline::Chain::Command.new( + project: project, current_user: user, origin_ref: 'master') + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'adds an error about missing ref' do + expect(pipeline.errors.to_a) + .to include 'Ref is ambiguous' + end + end + context 'when does not have existing SHA set' do let(:command) do Gitlab::Ci::Pipeline::Chain::Command.new( diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index fffa727c2e..2cf812b26d 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Seed::Build do - let(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:attributes) do { name: 'rspec', diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb index 05ce3412fd..82f741845d 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Seed::Stage do - let(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:attributes) do { name: 'test', diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 6849bc6db7..1874ff1782 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2275,6 +2275,8 @@ describe Ci::Build do end context 'when protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2287,7 +2289,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2295,7 +2297,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2320,6 +2322,8 @@ describe Ci::Build do end context 'when group protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2332,7 +2336,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2340,7 +2344,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2615,7 +2619,7 @@ describe Ci::Build do allow_any_instance_of(Project) .to receive(:ci_variables_for) - .with(ref: 'master', environment: nil) do + .with(ref: 'refs/heads/master', environment: nil) do [create(:ci_variable, key: 'secret', value: 'value')] end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 9e6146b8a4..4f8b5b7682 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -227,6 +227,10 @@ describe Ci::Pipeline, :mailer do end describe '#protected_ref?' do + before do + pipeline.project = create(:project, :repository) + end + it 'delegates method to project' do expect(pipeline).not_to be_protected_ref end diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb new file mode 100644 index 0000000000..8aed72d77a --- /dev/null +++ b/spec/models/concerns/has_ref_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HasRef do + describe '#branch?' do + let(:build) { create(:ci_build) } + + subject { build.branch? } + + context 'is not a tag' do + before do + build.tag = false + end + + it 'return true when tag is set to false' do + is_expected.to be_truthy + end + end + + context 'is not a tag' do + before do + build.tag = true + end + + it 'return false when tag is set to true' do + is_expected.to be_falsey + end + end + end + + describe '#git_ref' do + subject { build.git_ref } + + context 'when tag is true' do + let(:build) { create(:ci_build, tag: true) } + + it 'returns a tag ref' do + is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX) + end + end + + context 'when tag is false' do + let(:build) { create(:ci_build, tag: false) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when tag is nil' do + let(:build) { create(:ci_build, tag: nil) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + end +end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 8174868152..a64720f187 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -243,6 +243,20 @@ describe Event do expect(event.visible_to_user?(admin)).to eq true end end + + context 'private project' do + let(:project) { create(:project, :private) } + let(:target) { note_on_issue } + + it do + expect(event.visible_to_user?(non_member)).to eq false + expect(event.visible_to_user?(author)).to eq false + expect(event.visible_to_user?(assignee)).to eq false + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq true + expect(event.visible_to_user?(admin)).to eq true + end + end end context 'merge request diff note event' do @@ -265,8 +279,8 @@ describe Event do it do expect(event.visible_to_user?(non_member)).to eq false - expect(event.visible_to_user?(author)).to eq true - expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(author)).to eq false + expect(event.visible_to_user?(assignee)).to eq false expect(event.visible_to_user?(member)).to eq true expect(event.visible_to_user?(guest)).to eq false expect(event.visible_to_user?(admin)).to eq true diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index eaa852b008..b6ea32ec42 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -268,6 +268,13 @@ describe Project do expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed') end + it 'does not allow import_url pointing to the local network' do + project = build(:project, import_url: 'https://192.168.1.1') + + expect(project).to be_invalid + expect(project.errors[:import_url].first).to include('Requests to the local network are not allowed') + end + it "does not allow import_url with invalid ports for new projects" do project = build(:project, import_url: 'http://github.com:25/t.git') @@ -2475,6 +2482,10 @@ describe Project do end context 'when the ref is not protected' do + before do + allow(project).to receive(:protected_for?).with('ref').and_return(false) + end + it 'contains only the CI variables' do is_expected.to contain_exactly(ci_variable) end @@ -2514,42 +2525,139 @@ describe Project do end describe '#protected_for?' do - let(:project) { create(:project) } + let(:project) { create(:project, :repository) } - subject { project.protected_for?('ref') } + subject { project.protected_for?(ref) } - context 'when the ref is not protected' do + shared_examples 'ref is not protected' do before do stub_application_setting( default_branch_protection: Gitlab::Access::PROTECTION_NONE) end it 'returns false' do - is_expected.to be_falsey + is_expected.to be false end end - context 'when the ref is a protected branch' do + shared_examples 'ref is protected branch' do before do - allow(project).to receive(:repository).and_call_original - allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(true) - create(:protected_branch, name: 'ref', project: project) + create(:protected_branch, name: 'master', project: project) end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true end end - context 'when the ref is a protected tag' do + shared_examples 'ref is protected tag' do before do - allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(false) - allow(project).to receive_message_chain(:repository, :tag_exists?).and_return(true) - create(:protected_tag, name: 'ref', project: project) + create(:protected_tag, name: 'v1.0.0', project: project) end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true + end + end + + context 'when ref is nil' do + let(:ref) { nil } + + it 'returns false' do + is_expected.to be false + end + end + + context 'when ref is ref name' do + context 'when ref is ambiguous' do + let(:ref) { 'ref' } + + before do + project.repository.add_branch(project.creator, 'ref', 'master') + project.repository.add_tag(project.creator, 'ref', 'master') + end + + it 'raises an error' do + expect { subject }.to raise_error(Repository::AmbiguousRefError) + end + end + + context 'when the ref is not protected' do + let(:ref) { 'master' } + + it_behaves_like 'ref is not protected' + end + + context 'when the ref is a protected branch' do + let(:ref) { 'master' } + + it_behaves_like 'ref is protected branch' + end + + context 'when the ref is a protected tag' do + let(:ref) { 'v1.0.0' } + + it_behaves_like 'ref is protected tag' + end + + context 'when ref does not exist' do + let(:ref) { 'something' } + + it 'returns false' do + is_expected.to be false + end + end + end + + context 'when ref is full ref' do + context 'when the ref is not protected' do + let(:ref) { 'refs/heads/master' } + + it_behaves_like 'ref is not protected' + end + + context 'when the ref is a protected branch' do + let(:ref) { 'refs/heads/master' } + + it_behaves_like 'ref is protected branch' + end + + context 'when the ref is a protected tag' do + let(:ref) { 'refs/tags/v1.0.0' } + + it_behaves_like 'ref is protected tag' + end + + context 'when branch ref name is a full tag ref' do + let(:ref) { 'refs/tags/something' } + + before do + project.repository.add_branch(project.creator, ref, 'master') + end + + context 'when ref is not protected' do + it 'returns false' do + is_expected.to be false + end + end + + context 'when ref is a protected branch' do + before do + create(:protected_branch, name: 'refs/tags/something', project: project) + end + + it 'returns true' do + is_expected.to be true + end + end + end + + context 'when ref does not exist' do + let(:ref) { 'refs/heads/something' } + + it 'returns false' do + is_expected.to be false + end end end end @@ -2758,7 +2866,7 @@ describe Project do it 'shows full error updating an invalid MR' do error_message = 'Failed to replace merge_requests because one or more of the new records could not be saved.'\ - ' Validate fork Source project is not a fork of the target project' + ' Validate fork Source project is not a fork of the target project' expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) } .to raise_error(ActiveRecord::RecordNotSaved, error_message) diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 3d316fb3c5..4374421ccc 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -24,6 +24,20 @@ describe RemoteMirror do expect(remote_mirror).to be_invalid expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character') end + + it 'does not allow url pointing to localhost' do + remote_mirror = build(:remote_mirror, url: 'http://127.0.0.2/t.git') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to loopback addresses are not allowed') + end + + it 'does not allow url pointing to the local network' do + remote_mirror = build(:remote_mirror, url: 'https://192.168.1.1') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to the local network are not allowed') + end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 799a60ac62..fbf8f330d6 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1066,6 +1066,67 @@ describe Repository do end end + describe '#ambiguous_ref?' do + let(:ref) { 'ref' } + + subject { repository.ambiguous_ref?(ref) } + + context 'when ref is ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + repository.add_branch(project.creator, ref, 'master') + end + + it 'should be true' do + is_expected.to eq(true) + end + end + + context 'when ref is not ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + end + + it 'should be false' do + is_expected.to eq(false) + end + end + end + + describe '#expand_ref' do + let(:ref) { 'ref' } + + subject { repository.expand_ref(ref) } + + context 'when ref is not tag or branch name' do + let(:ref) { 'refs/heads/master' } + + it 'returns nil' do + is_expected.to eq(nil) + end + end + + context 'when ref is tag name' do + before do + repository.add_tag(project.creator, ref, 'master') + end + + it 'returns the tag ref' do + is_expected.to eq("refs/tags/#{ref}") + end + end + + context 'when ref is branch name' do + before do + repository.add_branch(project.creator, ref, 'master') + end + + it 'returns the branch ref' do + is_expected.to eq("refs/heads/#{ref}") + end + end + end + describe '#add_branch' do let(:branch_name) { 'new_feature' } let(:target) { 'master' } diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 7a7272ccb6..664dc3fa14 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -423,4 +423,41 @@ describe Snippet do expect(blob.data).to eq(snippet.content) end end + + describe '#embeddable?' do + context 'project snippet' do + [ + { project: :public, snippet: :public, embeddable: true }, + { project: :internal, snippet: :public, embeddable: false }, + { project: :private, snippet: :public, embeddable: false }, + { project: :public, snippet: :internal, embeddable: false }, + { project: :internal, snippet: :internal, embeddable: false }, + { project: :private, snippet: :internal, embeddable: false }, + { project: :public, snippet: :private, embeddable: false }, + { project: :internal, snippet: :private, embeddable: false }, + { project: :private, snippet: :private, embeddable: false } + ].each do |combination| + it 'only returns true when both project and snippet are public' do + project = create(:project, combination[:project]) + snippet = create(:project_snippet, combination[:snippet], project: project) + + expect(snippet.embeddable?).to eq(combination[:embeddable]) + end + end + end + + context 'personal snippet' do + [ + { snippet: :public, embeddable: true }, + { snippet: :internal, embeddable: false }, + { snippet: :private, embeddable: false } + ].each do |combination| + it 'only returns true when snippet is public' do + snippet = create(:personal_snippet, combination[:snippet]) + + expect(snippet.embeddable?).to eq(combination[:embeddable]) + end + end + end + end end diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index d1bf98995e..db3df76047 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do let(:policies) { described_class.new(user, issue) } describe '#rules' do + context 'when user is author of issuable' do + let(:merge_request) { create(:merge_request, source_project: project, author: user) } + let(:policies) { described_class.new(user, merge_request) } + + context 'when user is able to read project' do + it 'enables user to read and update issuables' do + expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + + context 'when project is private' do + let(:project) { create(:project, :private) } + + context 'when user belongs to the projects team' do + it 'enables user to read and update issuables' do + project.add_maintainer(user) + + expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + + it 'disallows user from reading and updating issuables from that project' do + expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + end + context 'when discussion is locked for the issuable' do let(:issue) { create(:issue, project: project, discussion_locked: true) } diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 8770365c89..402031075e 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -142,10 +142,20 @@ describe API::Jobs do end context 'unauthorized user' do - let(:api_user) { nil } + context 'when user is not logged in' do + let(:api_user) { nil } - it 'does not return project jobs' do - expect(response).to have_gitlab_http_status(401) + it 'does not return project jobs' do + expect(response).to have_gitlab_http_status(401) + end + end + + context 'when user is guest' do + let(:api_user) { guest } + + it 'does not return project jobs' do + expect(response).to have_gitlab_http_status(403) + end end end @@ -241,10 +251,20 @@ describe API::Jobs do end context 'unauthorized user' do - let(:api_user) { nil } + context 'when user is not logged in' do + let(:api_user) { nil } - it 'does not return jobs' do - expect(response).to have_gitlab_http_status(401) + it 'does not return jobs' do + expect(response).to have_gitlab_http_status(401) + end + end + + context 'when user is guest' do + let(:api_user) { guest } + + it 'does not return jobs' do + expect(response).to have_gitlab_http_status(403) + end end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 909703a8d4..e5d3cc8de2 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -441,9 +441,11 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'picks a job' do request_job info: { platform: :darwin } + runner.reload + expect(response).to have_gitlab_http_status(201) expect(response.headers).not_to have_key('X-GitLab-Last-Update') - expect(runner.reload.platform).to eq('darwin') + expect(runner.platform).to eq('darwin') expect(json_response['id']).to eq(job.id) expect(json_response['token']).to eq(job.token) expect(json_response['job_info']).to eq(expected_job_info) @@ -537,8 +539,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(2) expect(json_response['dependencies']).to include( - { 'id' => job.id, 'name' => job.name, 'token' => job.token }, - { 'id' => job2.id, 'name' => job2.name, 'token' => job2.token }) + { 'id' => job.id, 'name' => job.name, 'token' => test_job.token }, + { 'id' => job2.id, 'name' => job2.name, 'token' => test_job.token }) end end @@ -557,7 +559,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(1) expect(json_response['dependencies']).to include( - { 'id' => job.id, 'name' => job.name, 'token' => job.token, + { 'id' => job.id, 'name' => job.name, 'token' => test_job.token, 'artifacts_file' => { 'filename' => 'ci_build_artifacts.zip', 'size' => 106365 } }) end end @@ -582,7 +584,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(response).to have_gitlab_http_status(201) expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(1) - expect(json_response['dependencies'][0]).to include('id' => job2.id, 'name' => job2.name, 'token' => job2.token) + expect(json_response['dependencies'][0]).to include( + 'id' => job2.id, 'name' => job2.name, 'token' => test_job.token) end end @@ -983,7 +986,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do patch_the_trace end - it 'returns Forbidden ' do + it 'returns Forbidden' do expect(response.status).to eq(403) end end @@ -1024,11 +1027,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when the job is canceled' do before do - job.cancel + job.cancel! patch_the_trace end - it 'receives status in header' do + it 'responds with forbidden and status in header' do + expect(response).to have_gitlab_http_status(403) expect(response.header['Job-Status']).to eq 'canceled' end end @@ -1199,7 +1203,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'fails to authorize artifacts posting' do authorize_artifacts(token: job.project.runners_token) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end @@ -1212,10 +1216,10 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'authorization token is invalid' do - it 'responds with forbidden' do + it 'responds with not found' do authorize_artifacts(token: 'invalid', filesize: 100 ) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end @@ -1248,12 +1252,24 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end it 'responds with forbidden' do - upload_artifacts(file_upload, headers_with_token) - expect(response).to have_gitlab_http_status(403) end end + context 'when job has been canceled' do + let(:job) { create(:ci_build) } + + before do + job.cancel! + upload_artifacts(file_upload, headers_with_token) + end + + it 'responds with forbidden' do + expect(response).to have_gitlab_http_status(403) + expect(response.header['Job-Status']).to eq('canceled') + end + end + context 'when job is running' do shared_examples 'successful artifacts upload' do it 'updates successfully' do @@ -1303,10 +1319,10 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'when using runners token' do - it 'responds with forbidden' do + it 'responds with not found' do upload_artifacts(file_upload, headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.project.runners_token)) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end end @@ -1526,10 +1542,13 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end describe 'GET /api/v4/jobs/:id/artifacts' do - let(:token) { job.token } + let(:project) { create(:project) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } + let(:running_job) { create(:ci_build, :running, pipeline: pipeline) } + let(:token) { running_job.token } context 'when job has artifacts' do - let(:job) { create(:ci_build) } + let(:job) { create(:ci_build, pipeline: pipeline) } let(:store) { JobArtifactUploader::Store::LOCAL } before do @@ -1555,7 +1574,6 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when artifacts are stored remotely' do let(:store) { JobArtifactUploader::Store::REMOTE } - let!(:job) { create(:ci_build) } context 'when proxy download is being used' do before do @@ -1582,6 +1600,30 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end + context 'when using running token from another pipeline' do + let(:running_job) { create(:ci_build, :running, project: project) } + + before do + download_artifact + end + + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when using running token from another project' do + let(:running_job) { create(:ci_build, :running) } + + before do + download_artifact + end + + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) + end + end + context 'when using runnners token' do let(:token) { job.project.runners_token } @@ -1589,8 +1631,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do download_artifact end - it 'responds with forbidden' do - expect(response).to have_gitlab_http_status(403) + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) end end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 84cfa53ea0..d87a7dd234 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -56,7 +56,7 @@ describe Groups::UpdateService do create(:project, :private, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) - .with(1.hour, internal_group.id) + .with(Todo::WAIT_FOR_DELETE, internal_group.id) end it "changes permission level to private" do diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index f0b0f7956c..ca366cdf1d 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do expect(project.issues.opened).to be_empty expect(project.issues.closed).not_to be_empty end + + context 'when issue for a different project is created' do + let(:private_project) { create(:project, :private) } + let(:issue) { create(:issue, project: private_project, author: user) } + + context 'when user has access to the project' do + it 'closes all issues passed' do + private_project.add_maintainer(user) + + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).not_to be_empty + end + end + + context 'when user does not have access to project' do + it 'only closes all issues that the user has access to' do + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).to be_empty + end + end + end end describe 'reopen issues' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index bd519e7f07..ce20bf2bef 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -77,7 +77,7 @@ describe Issues::UpdateService, :mailer do end it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do - expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, issue.id) update_issue(confidential: true) end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 0a5220c7c6..e872a53776 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -22,7 +22,7 @@ describe Members::DestroyService do shared_examples 'a service destroying a member' do before do type = member.is_a?(GroupMember) ? 'Group' : 'Project' - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(1.hour, member.user_id, member.source_id, type) + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) end it 'destroys the member' do @@ -69,14 +69,14 @@ describe Members::DestroyService do it 'calls Member#after_decline_request' do expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) - described_class.new(current_user).execute(member) + described_class.new(current_user).execute(member, opts) end context 'when current user is the member' do it 'does not call Member#after_decline_request' do expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - described_class.new(member_user).execute(member) + described_class.new(member_user).execute(member, opts) end end end @@ -159,7 +159,7 @@ describe Members::DestroyService do end it_behaves_like 'a service destroying a member' do - let(:opts) { { skip_authorization: true } } + let(:opts) { { skip_authorization: true, skip_subresources: true } } let(:member) { group_project.requesters.find_by(user_id: member_user.id) } end @@ -168,12 +168,14 @@ describe Members::DestroyService do end it_behaves_like 'a service destroying a member' do - let(:opts) { { skip_authorization: true } } + let(:opts) { { skip_authorization: true, skip_subresources: true } } let(:member) { group.requesters.find_by(user_id: member_user.id) } end end context 'when current user can destroy the given access requester' do + let(:opts) { { skip_subresources: true } } + before do group_project.add_maintainer(current_user) group.add_owner(current_user) @@ -229,4 +231,54 @@ describe Members::DestroyService do end end end + + context 'subresources' do + let(:user) { create(:user) } + let(:member_user) { create(:user) } + let(:opts) { {} } + + let(:group) { create(:group, :public) } + let(:subgroup) { create(:group, parent: group) } + let(:subsubgroup) { create(:group, parent: subgroup) } + let(:subsubproject) { create(:project, group: subsubgroup) } + + let(:group_project) { create(:project, :public, group: group) } + let(:control_project) { create(:project, group: subsubgroup) } + + before do + create(:group_member, :developer, group: subsubgroup, user: member_user) + + subsubproject.add_developer(member_user) + control_project.add_maintainer(user) + group.add_owner(user) + + group_member = create(:group_member, :developer, group: group, user: member_user) + + described_class.new(user).execute(group_member, opts) + end + + it 'removes the project membership' do + expect(group_project.members.map(&:user)).not_to include(member_user) + end + + it 'removes the group membership' do + expect(group.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subgroup membership', :postgresql do + expect(subgroup.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subsubgroup membership', :postgresql do + expect(subsubgroup.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subsubproject membership', :postgresql do + expect(subsubproject.members.map(&:user)).not_to include(member_user) + end + + it 'does not remove the user from the control project' do + expect(control_project.members.map(&:user)).to include(user) + end + end end diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 6d19a95ffe..599ed39ca3 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -20,11 +20,28 @@ describe Members::UpdateService do shared_examples 'a service updating a member' do it 'updates the member' do + expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + updated_member = described_class.new(current_user, params).execute(member, permission: permission) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER) end + + context 'when member is downgraded to guest' do + let(:params) do + { access_level: Gitlab::Access::GUEST } + end + + it 'schedules to delete confidential todos' do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end + end end before do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index c9a668994e..c969db7e99 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MergeRequests::BuildService do using RSpec::Parameterized::TableSyntax include RepoHelpers + include ProjectForksHelper let(:project) { create(:project, :repository) } let(:source_project) { nil } @@ -44,7 +45,7 @@ describe MergeRequests::BuildService do describe '#execute' do it 'calls the compare service with the correct arguments' do - allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true) + allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true) expect(CompareService).to receive(:new) .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch) .and_call_original @@ -375,11 +376,27 @@ describe MergeRequests::BuildService do end end + context 'target_project is set but repo is not accessible by current_user' do + let(:target_project) do + create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE) + end + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'source_project is set and accessible by current_user' do let(:source_project) { create(:project, :public, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + before do + # To create merge requests _from_ a project the user needs at least + # developer access + source_project.add_developer(user) + end + + it 'sets source project correctly' do expect(merge_request.source_project).to eq(source_project) end end @@ -388,11 +405,43 @@ describe MergeRequests::BuildService do let(:source_project) { create(:project, :private, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + it 'sets source project correctly' do expect(merge_request.source_project).to eq(project) end end + context 'source_project is set but the user cannot create merge requests from the project' do + let(:source_project) do + create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it 'sets the source_project correctly' do + expect(merge_request.source_project).to eq(project) + end + end + + context 'target_project is not in the fork network of source_project' do + let(:target_project) { create(:project, :public, :repository) } + + it 'adds an error to the merge request' do + expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project') + end + end + + context 'target_project is in the fork network of source project but no longer accessible' do + let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) } + let(:source_project) { project } + let(:target_project) { create(:project, :public, :repository) } + + before do + target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'sets the target_project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'when specifying target branch in the description' do let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" } diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index d7d7f1874e..95c9b6e63b 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do let(:project) { create(:project) } let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } let(:download_link) { "http://gitlab.com/#{oid}" } - let(:lfs_content) do - <<~HEREDOC - whatever - HEREDOC - end + let(:lfs_content) { SecureRandom.random_bytes(10) } subject { described_class.new(project) } before do allow(project).to receive(:lfs_enabled?).and_return(true) WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) end describe '#execute' do @@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do it 'stores the content' do subject.execute(oid, download_link) - expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content + expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end end @@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when localhost requests are allowed' do + let(:download_link) { 'http://192.168.2.120' } + + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + end + + it 'downloads the file' do + expect(subject).to receive(:download_and_save_file).and_call_original + + expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1) + end + end + context 'when a bad URL is used' do - where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2']) + where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) with_them do it 'does not download the file' do - expect(subject).not_to receive(:download_and_save_file) - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } end end end + context 'when the URL points to a redirected URL' do + context 'that is blocked' do + where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) + + with_them do + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + end + + it 'does not follow the redirection' do + expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/) + + expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + end + end + end + + context 'that is valid' do + let(:redirect_link) { "http://example.com/"} + + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content) + end + + it 'follows the redirection' do + expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) + end + end + end + context 'when an lfs object with the same oid already exists' do before do create(:lfs_object, oid: 'oid') diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index d58ff2cedc..8adfc63222 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -41,7 +41,7 @@ describe Projects::UpdateService do end it 'updates the project to private' do - expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id) + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) @@ -191,7 +191,7 @@ describe Projects::UpdateService do context 'when changing feature visibility to private' do it 'updates the visibility correctly' do expect(TodosDestroyer::PrivateFeaturesWorker) - .to receive(:perform_in).with(1.hour, project.id) + .to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, project_feature_attributes: { issues_access_level: ProjectFeature::PRIVATE } diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index c52515aefd..253f2e44d1 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -19,6 +19,7 @@ describe TodoService do before do project.add_guest(guest) project.add_developer(author) + project.add_developer(assignee) project.add_developer(member) project.add_developer(john_doe) project.add_developer(skipped)